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 <[email protected]>
---
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
--
2.31.1.272.g89b43f80a514
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 <[email protected]>
> ---
> 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<[email protected]> 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.
--
viresh
On Fri, Oct 22, 2021 at 8:58 AM Viresh Kumar <[email protected]> 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 <[email protected]>
> > ---
> > 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<[email protected]> 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<[email protected]> # 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 <[email protected]>
> ---
> 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).
--
viresh