| Message ID | 7c58868cd26d2fc4bd82d0d8b0dfb55636380110.1634808714.git.viresh.kumar@linaro.org |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | i2c: virtio: Add support for zero-length requests | expand |
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 Deng<jie.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 Deng<jie.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 Deng<jie.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).
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
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(-)