The virtio specification received a new mandatory feature (VIRTIO_I2C_F_ZERO_LENGTH_REQUEST) for zero length requests. Fail if the feature isn't offered by the device.
For each read-request, set the VIRTIO_I2C_FLAGS_M_RD flag, as required by the VIRTIO_I2C_F_ZERO_LENGTH_REQUEST feature.
This allows us to support zero length requests, like SMBUS Quick, where the buffer need not be sent anymore.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- Hi Wolfram,
Please do not apply this until the spec changes [1] are merged, sending it early to get review done. I will ping you later once the spec is merged.
[1] https://lists.oasis-open.org/archives/virtio-dev/202110/msg00109.html
drivers/i2c/busses/i2c-virtio.c | 56 ++++++++++++++++++--------------- include/uapi/linux/virtio_i2c.h | 6 ++++ 2 files changed, 36 insertions(+), 26 deletions(-)
diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c index f10a603b13fb..1ed4daa918a0 100644 --- a/drivers/i2c/busses/i2c-virtio.c +++ b/drivers/i2c/busses/i2c-virtio.c @@ -62,35 +62,33 @@ static int virtio_i2c_prepare_reqs(struct virtqueue *vq, for (i = 0; i < num; i++) { int outcnt = 0, incnt = 0;
- /* - * We don't support 0 length messages and so filter out - * 0 length transfers by using i2c_adapter_quirks. - */ - if (!msgs[i].len) - break; - /* * Only 7-bit mode supported for this moment. For the address * format, Please check the Virtio I2C Specification. */ reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1);
+ if (msgs[i].flags & I2C_M_RD) + reqs[i].out_hdr.flags |= cpu_to_le32(VIRTIO_I2C_FLAGS_M_RD); + if (i != num - 1) - reqs[i].out_hdr.flags = cpu_to_le32(VIRTIO_I2C_FLAGS_FAIL_NEXT); + reqs[i].out_hdr.flags |= cpu_to_le32(VIRTIO_I2C_FLAGS_FAIL_NEXT);
sg_init_one(&out_hdr, &reqs[i].out_hdr, sizeof(reqs[i].out_hdr)); sgs[outcnt++] = &out_hdr;
- reqs[i].buf = i2c_get_dma_safe_msg_buf(&msgs[i], 1); - if (!reqs[i].buf) - break; + if (msgs[i].len) { + reqs[i].buf = i2c_get_dma_safe_msg_buf(&msgs[i], 1); + if (!reqs[i].buf) + break;
- sg_init_one(&msg_buf, reqs[i].buf, msgs[i].len); + sg_init_one(&msg_buf, reqs[i].buf, msgs[i].len);
- if (msgs[i].flags & I2C_M_RD) - sgs[outcnt + incnt++] = &msg_buf; - else - sgs[outcnt++] = &msg_buf; + if (msgs[i].flags & I2C_M_RD) + sgs[outcnt + incnt++] = &msg_buf; + else + sgs[outcnt++] = &msg_buf; + }
sg_init_one(&in_hdr, &reqs[i].in_hdr, sizeof(reqs[i].in_hdr)); sgs[outcnt + incnt++] = &in_hdr; @@ -191,7 +189,7 @@ static int virtio_i2c_setup_vqs(struct virtio_i2c *vi)
static u32 virtio_i2c_func(struct i2c_adapter *adap) { - return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK); + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL; }
static struct i2c_algorithm virtio_algorithm = { @@ -199,15 +197,16 @@ static struct i2c_algorithm virtio_algorithm = { .functionality = virtio_i2c_func, };
-static const struct i2c_adapter_quirks virtio_i2c_quirks = { - .flags = I2C_AQ_NO_ZERO_LEN, -}; - static int virtio_i2c_probe(struct virtio_device *vdev) { struct virtio_i2c *vi; int ret;
+ if (!virtio_has_feature(vdev, VIRTIO_I2C_F_ZERO_LENGTH_REQUEST)) { + dev_err(&vdev->dev, "Zero-length request feature is mandatory\n"); + return -EINVAL; + } + vi = devm_kzalloc(&vdev->dev, sizeof(*vi), GFP_KERNEL); if (!vi) return -ENOMEM; @@ -225,7 +224,6 @@ static int virtio_i2c_probe(struct virtio_device *vdev) snprintf(vi->adap.name, sizeof(vi->adap.name), "i2c_virtio at virtio bus %d", vdev->index); vi->adap.algo = &virtio_algorithm; - vi->adap.quirks = &virtio_i2c_quirks; vi->adap.dev.parent = &vdev->dev; vi->adap.dev.of_node = vdev->dev.of_node; i2c_set_adapdata(&vi->adap, vi); @@ -270,11 +268,17 @@ static int virtio_i2c_restore(struct virtio_device *vdev) } #endif
+static const unsigned int features[] = { + VIRTIO_I2C_F_ZERO_LENGTH_REQUEST, +}; + static struct virtio_driver virtio_i2c_driver = { - .id_table = id_table, - .probe = virtio_i2c_probe, - .remove = virtio_i2c_remove, - .driver = { + .feature_table = features, + .feature_table_size = ARRAY_SIZE(features), + .id_table = id_table, + .probe = virtio_i2c_probe, + .remove = virtio_i2c_remove, + .driver = { .name = "i2c_virtio", }, #ifdef CONFIG_PM_SLEEP diff --git a/include/uapi/linux/virtio_i2c.h b/include/uapi/linux/virtio_i2c.h index 7c6a6fc01ad6..acf3b6069136 100644 --- a/include/uapi/linux/virtio_i2c.h +++ b/include/uapi/linux/virtio_i2c.h @@ -11,9 +11,15 @@ #include <linux/const.h> #include <linux/types.h>
+/* Virtio I2C Feature bits */ +#define VIRTIO_I2C_F_ZERO_LENGTH_REQUEST 0 + /* The bit 0 of the @virtio_i2c_out_hdr.@flags, used to group the requests */ #define VIRTIO_I2C_FLAGS_FAIL_NEXT _BITUL(0)
+/* The bit 1 of the @virtio_i2c_out_hdr.@flags, used to mark a buffer as read */ +#define VIRTIO_I2C_FLAGS_M_RD _BITUL(1) + /** * struct virtio_i2c_out_hdr - the virtio I2C message OUT header * @addr: the controlled device address
On 2021/10/21 17:47, Viresh Kumar wrote:
The virtio specification received a new mandatory feature (VIRTIO_I2C_F_ZERO_LENGTH_REQUEST) for zero length requests. Fail if the feature isn't offered by the device.
For each read-request, set the VIRTIO_I2C_FLAGS_M_RD flag, as required by the VIRTIO_I2C_F_ZERO_LENGTH_REQUEST feature.
This allows us to support zero length requests, like SMBUS Quick, where the buffer need not be sent anymore.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Hi Wolfram,
Please do not apply this until the spec changes [1] are merged, sending it early to get review done. I will ping you later once the spec is merged.
[1] https://lists.oasis-open.org/archives/virtio-dev/202110/msg00109.html
drivers/i2c/busses/i2c-virtio.c | 56 ++++++++++++++++++--------------- include/uapi/linux/virtio_i2c.h | 6 ++++ 2 files changed, 36 insertions(+), 26 deletions(-)
Acked-by: Jie Dengjie.deng@intel.com once the spec is merged.
- if (!virtio_has_feature(vdev, VIRTIO_I2C_F_ZERO_LENGTH_REQUEST)) {
dev_err(&vdev->dev, "Zero-length request feature is mandatory\n");return -EINVAL;
It might be better to return -EOPNOTSUPP ?
On 22-10-21, 14:51, Jie Deng wrote:
- if (!virtio_has_feature(vdev, VIRTIO_I2C_F_ZERO_LENGTH_REQUEST)) {
dev_err(&vdev->dev, "Zero-length request feature is mandatory\n");return -EINVAL;It might be better to return -EOPNOTSUPP ?
Maybe that or one of these:
#define EBADE 52 /* Invalid exchange */ #define EPROTO 71 /* Protocol error */ #define EPFNOSUPPORT 96 /* Protocol family not supported */ #define ECONNREFUSED 111 /* Connection refused */
Arnd, any suggestions ? This is about the mandatory feature not being offered by the device.
On Fri, Oct 22, 2021 at 8:58 AM Viresh Kumar viresh.kumar@linaro.org wrote:
On 22-10-21, 14:51, Jie Deng wrote:
- if (!virtio_has_feature(vdev, VIRTIO_I2C_F_ZERO_LENGTH_REQUEST)) {
dev_err(&vdev->dev, "Zero-length request feature is mandatory\n");return -EINVAL;It might be better to return -EOPNOTSUPP ?
Maybe that or one of these:
#define EBADE 52 /* Invalid exchange */ #define EPROTO 71 /* Protocol error */ #define EPFNOSUPPORT 96 /* Protocol family not supported */ #define ECONNREFUSED 111 /* Connection refused */
Arnd, any suggestions ? This is about the mandatory feature not being offered by the device.
These are mostly used for network operations, I'd stick with either EINVAL or ENXIO in this case.
Arnd
On Fri, Oct 22, 2021 at 02:51:10PM +0800, Jie Deng wrote:
On 2021/10/21 17:47, Viresh Kumar wrote:
The virtio specification received a new mandatory feature (VIRTIO_I2C_F_ZERO_LENGTH_REQUEST) for zero length requests. Fail if the feature isn't offered by the device.
For each read-request, set the VIRTIO_I2C_FLAGS_M_RD flag, as required by the VIRTIO_I2C_F_ZERO_LENGTH_REQUEST feature.
This allows us to support zero length requests, like SMBUS Quick, where the buffer need not be sent anymore.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Hi Wolfram,
Please do not apply this until the spec changes [1] are merged, sending it early to get review done. I will ping you later once the spec is merged.
[1] https://lists.oasis-open.org/archives/virtio-dev/202110/msg00109.html
drivers/i2c/busses/i2c-virtio.c | 56 ++++++++++++++++++--------------- include/uapi/linux/virtio_i2c.h | 6 ++++ 2 files changed, 36 insertions(+), 26 deletions(-)
Acked-by: Jie Dengjie.deng@intel.com once the spec is merged.
There's supposed to be space before < btw. and one puts # before any comments this way tools can process the ack automatically:
Acked-by: Jie Dengjie.deng@intel.com # once the spec is merged.
- if (!virtio_has_feature(vdev, VIRTIO_I2C_F_ZERO_LENGTH_REQUEST)) {
dev_err(&vdev->dev, "Zero-length request feature is mandatory\n");return -EINVAL;It might be better to return -EOPNOTSUPP ?
On 21-10-21, 15:17, Viresh Kumar wrote:
The virtio specification received a new mandatory feature (VIRTIO_I2C_F_ZERO_LENGTH_REQUEST) for zero length requests. Fail if the feature isn't offered by the device.
For each read-request, set the VIRTIO_I2C_FLAGS_M_RD flag, as required by the VIRTIO_I2C_F_ZERO_LENGTH_REQUEST feature.
This allows us to support zero length requests, like SMBUS Quick, where the buffer need not be sent anymore.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Hi Wolfram,
Please do not apply this until the spec changes [1] are merged, sending it early to get review done. I will ping you later once the spec is merged.
[1] https://lists.oasis-open.org/archives/virtio-dev/202110/msg00109.html
Michael,
Can this be merged as well based on the current voting at the ballot ?
https://www.oasis-open.org/committees/ballot.php?id=3659
Wolfram,
I am asking as this patch should be considered as a fix, which needs to be applied to the 5.15 kernel itself if possible (now or via stable), as we are implementing a new mandatory feature, which will make the currently merged version of the driver unusable going forward (since this won't be backwards compatible).
stratos-dev@op-lists.linaro.org
-
Arnd Bergmann -
Jie Deng -
Michael S. Tsirkin -
Viresh Kumar