Add an I2C bus driver for virtio para-virtualization.
The controller can be emulated by the backend driver in
any device model software by following the virtio protocol.
The device specification can be found on
https://lists.oasis-open.org/archives/virtio-comment/202101/msg00008.html.
By following the specification, people may implement different
backend drivers to emulate different controllers according to
their needs.
Co-developed-by: Conghui Chen <[email protected]>
Signed-off-by: Conghui Chen <[email protected]>
Signed-off-by: Jie Deng <[email protected]>
---
drivers/i2c/busses/Kconfig | 11 ++
drivers/i2c/busses/Makefile | 3 +
drivers/i2c/busses/i2c-virtio.c | 281 ++++++++++++++++++++++++++++++++++++++++
include/uapi/linux/virtio_i2c.h | 56 ++++++++
include/uapi/linux/virtio_ids.h | 1 +
5 files changed, 352 insertions(+)
create mode 100644 drivers/i2c/busses/i2c-virtio.c
create mode 100644 include/uapi/linux/virtio_i2c.h
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 05ebf75..0860395 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -21,6 +21,17 @@ config I2C_ALI1535
This driver can also be built as a module. If so, the module
will be called i2c-ali1535.
+config I2C_VIRTIO
+ tristate "Virtio I2C Adapter"
+ depends on VIRTIO
+ help
+ If you say yes to this option, support will be included for the virtio
+ I2C adapter driver. The hardware can be emulated by any device model
+ software according to the virtio protocol.
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-virtio.
+
config I2C_ALI1563
tristate "ALI 1563"
depends on PCI
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 615f35e..b88da08 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -6,6 +6,9 @@
# ACPI drivers
obj-$(CONFIG_I2C_SCMI) += i2c-scmi.o
+# VIRTIO I2C host controller driver
+obj-$(CONFIG_I2C_VIRTIO) += i2c-virtio.o
+
# PC SMBus host controller drivers
obj-$(CONFIG_I2C_ALI1535) += i2c-ali1535.o
obj-$(CONFIG_I2C_ALI1563) += i2c-ali1563.o
diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
new file mode 100644
index 0000000..8c8bc95
--- /dev/null
+++ b/drivers/i2c/busses/i2c-virtio.c
@@ -0,0 +1,281 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Virtio I2C Bus Driver
+ *
+ * Copyright (c) 2021 Intel Corporation. All rights reserved.
+ */
+
+#include <linux/acpi.h>
+#include <linux/completion.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/io.h>
+#include <linux/jiffies.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/wait.h>
+
+#include <linux/virtio.h>
+#include <linux/virtio_i2c.h>
+
+/**
+ * struct virtio_i2c - virtio I2C data
+ * @vdev: virtio device for this controller
+ * @completion: completion of virtio I2C message
+ * @adap: I2C adapter for this controller
+ * @i2c_lock: lock for virtqueue processing
+ * @vq: the virtio virtqueue for communication
+ */
+struct virtio_i2c {
+ struct virtio_device *vdev;
+ struct completion completion;
+ struct i2c_adapter adap;
+ struct mutex i2c_lock;
+ struct virtqueue *vq;
+};
+
+static void virtio_i2c_msg_done(struct virtqueue *vq)
+{
+ struct virtio_i2c *vi = vq->vdev->priv;
+
+ complete(&vi->completion);
+}
+
+static int virtio_i2c_send_reqs(struct virtqueue *vq,
+ struct virtio_i2c_req *reqs,
+ struct i2c_msg *msgs, int nr)
+{
+ struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr;
+ int i, outcnt, incnt, err = 0;
+ u8 *buf;
+
+ for (i = 0; i < nr; i++) {
+ if (!msgs[i].len)
+ break;
+
+ reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1);
+
+ if (i != nr - 1)
+ reqs[i].out_hdr.flags |= VIRTIO_I2C_FLAGS_FAIL_NEXT;
+
+ outcnt = incnt = 0;
+ sg_init_one(&out_hdr, &reqs[i].out_hdr, sizeof(reqs[i].out_hdr));
+ sgs[outcnt++] = &out_hdr;
+
+ buf = kzalloc(msgs[i].len, GFP_KERNEL);
+ if (!buf)
+ break;
+
+ if (msgs[i].flags & I2C_M_RD) {
+ reqs[i].read_buf = buf;
+ sg_init_one(&msg_buf, reqs[i].read_buf, msgs[i].len);
+ sgs[outcnt + incnt++] = &msg_buf;
+ } else {
+ reqs[i].write_buf = buf;
+ memcpy(reqs[i].write_buf, msgs[i].buf, msgs[i].len);
+ sg_init_one(&msg_buf, reqs[i].write_buf, msgs[i].len);
+ sgs[outcnt++] = &msg_buf;
+ }
+
+ sg_init_one(&in_hdr, &reqs[i].in_hdr, sizeof(reqs[i].in_hdr));
+ sgs[outcnt + incnt++] = &in_hdr;
+
+ err = virtqueue_add_sgs(vq, sgs, outcnt, incnt, &reqs[i], GFP_KERNEL);
+ if (err < 0) {
+ pr_err("failed to add msg[%d] to virtqueue.\n", i);
+ if (msgs[i].flags & I2C_M_RD) {
+ kfree(reqs[i].read_buf);
+ reqs[i].read_buf = NULL;
+ } else {
+ kfree(reqs[i].write_buf);
+ reqs[i].write_buf = NULL;
+ }
+ break;
+ }
+ }
+
+ return i;
+}
+
+static int virtio_i2c_complete_reqs(struct virtqueue *vq,
+ struct virtio_i2c_req *reqs,
+ struct i2c_msg *msgs, int nr)
+{
+ struct virtio_i2c_req *req;
+ unsigned int len;
+ int i;
+
+ for (i = 0; i < nr; i++) {
+ req = (struct virtio_i2c_req *)virtqueue_get_buf(vq, &len);
+ if (!(req && req == &reqs[i])) {
+ pr_err("msg[%d]: addr=0x%x virtqueue error.\n", i, msgs[i].addr);
+ break;
+ }
+
+ if (req->in_hdr.status != VIRTIO_I2C_MSG_OK) {
+ pr_err("msg[%d]: addr=0x%x backend error.\n", i, msgs[i].addr);
+ break;
+ }
+
+ if (msgs[i].flags & I2C_M_RD) {
+ memcpy(msgs[i].buf, req->read_buf, msgs[i].len);
+ kfree(req->read_buf);
+ req->read_buf = NULL;
+ } else {
+ kfree(req->write_buf);
+ req->write_buf = NULL;
+ }
+ }
+
+ return i;
+}
+
+
+static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
+{
+ struct virtio_i2c *vi = i2c_get_adapdata(adap);
+ struct virtqueue *vq = vi->vq;
+ struct virtio_i2c_req *reqs;
+ unsigned long time_left;
+ int ret, nr;
+
+ reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL);
+ if (!reqs)
+ return -ENOMEM;
+
+ mutex_lock(&vi->i2c_lock);
+
+ ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
+ if (ret == 0)
+ goto err_unlock_free;
+ else
+ nr = ret;
+
+ virtqueue_kick(vq);
+
+ time_left = wait_for_completion_timeout(&vi->completion, adap->timeout);
+ if (!time_left) {
+ dev_err(&adap->dev, "virtio i2c backend timeout.\n");
+ ret = -ETIMEDOUT;
+ goto err_unlock_free;
+ }
+
+ ret = virtio_i2c_complete_reqs(vq, reqs, msgs, nr);
+
+ reinit_completion(&vi->completion);
+
+err_unlock_free:
+ mutex_unlock(&vi->i2c_lock);
+ kfree(reqs);
+ return ret;
+}
+
+static void virtio_i2c_del_vqs(struct virtio_device *vdev)
+{
+ vdev->config->reset(vdev);
+ vdev->config->del_vqs(vdev);
+}
+
+static int virtio_i2c_setup_vqs(struct virtio_i2c *vi)
+{
+ struct virtio_device *vdev = vi->vdev;
+
+ vi->vq = virtio_find_single_vq(vdev, virtio_i2c_msg_done, "msg");
+ return PTR_ERR_OR_ZERO(vi->vq);
+}
+
+static u32 virtio_i2c_func(struct i2c_adapter *adap)
+{
+ return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static struct i2c_algorithm virtio_algorithm = {
+ .master_xfer = virtio_i2c_xfer,
+ .functionality = virtio_i2c_func,
+};
+
+static struct i2c_adapter virtio_adapter = {
+ .owner = THIS_MODULE,
+ .name = "Virtio I2C Adapter",
+ .class = I2C_CLASS_DEPRECATED,
+ .algo = &virtio_algorithm,
+};
+
+static int virtio_i2c_probe(struct virtio_device *vdev)
+{
+ struct device *pdev = vdev->dev.parent;
+ struct virtio_i2c *vi;
+ int ret;
+
+ vi = devm_kzalloc(&vdev->dev, sizeof(*vi), GFP_KERNEL);
+ if (!vi)
+ return -ENOMEM;
+
+ vdev->priv = vi;
+ vi->vdev = vdev;
+
+ mutex_init(&vi->i2c_lock);
+ init_completion(&vi->completion);
+
+ ret = virtio_i2c_setup_vqs(vi);
+ if (ret)
+ return ret;
+
+ vi->adap = virtio_adapter;
+ i2c_set_adapdata(&vi->adap, vi);
+ vi->adap.dev.parent = &vdev->dev;
+ /* Setup ACPI node for controlled devices which will be probed through ACPI */
+ ACPI_COMPANION_SET(&vi->adap.dev, ACPI_COMPANION(pdev));
+ vi->adap.timeout = HZ / 10;
+
+ ret = i2c_add_adapter(&vi->adap);
+ if (ret) {
+ virtio_i2c_del_vqs(vdev);
+ dev_err(&vdev->dev, "failed to add virtio-i2c adapter.\n");
+ }
+
+ return ret;
+}
+
+static void virtio_i2c_remove(struct virtio_device *vdev)
+{
+ struct virtio_i2c *vi = vdev->priv;
+
+ i2c_del_adapter(&vi->adap);
+ virtio_i2c_del_vqs(vdev);
+}
+
+static struct virtio_device_id id_table[] = {
+ { VIRTIO_ID_I2C_ADPTER, VIRTIO_DEV_ANY_ID },
+ {}
+};
+MODULE_DEVICE_TABLE(virtio, id_table);
+
+static int __maybe_unused virtio_i2c_freeze(struct virtio_device *vdev)
+{
+ virtio_i2c_del_vqs(vdev);
+ return 0;
+}
+
+static int __maybe_unused virtio_i2c_restore(struct virtio_device *vdev)
+{
+ return virtio_i2c_setup_vqs(vdev->priv);
+}
+
+static struct virtio_driver virtio_i2c_driver = {
+ .id_table = id_table,
+ .probe = virtio_i2c_probe,
+ .remove = virtio_i2c_remove,
+ .driver = {
+ .name = "i2c_virtio",
+ },
+#ifdef CONFIG_PM_SLEEP
+ .freeze = virtio_i2c_freeze,
+ .restore = virtio_i2c_restore,
+#endif
+};
+module_virtio_driver(virtio_i2c_driver);
+
+MODULE_DESCRIPTION("Virtio i2c bus driver");
+MODULE_LICENSE("GPL");
diff --git a/include/uapi/linux/virtio_i2c.h b/include/uapi/linux/virtio_i2c.h
new file mode 100644
index 0000000..92febf0
--- /dev/null
+++ b/include/uapi/linux/virtio_i2c.h
@@ -0,0 +1,56 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later WITH Linux-syscall-note */
+/*
+ * Definitions for virtio I2C Adpter
+ *
+ * Copyright (c) 2021 Intel Corporation. All rights reserved.
+ */
+
+#ifndef _UAPI_LINUX_VIRTIO_I2C_H
+#define _UAPI_LINUX_VIRTIO_I2C_H
+
+#include <linux/types.h>
+#include <linux/virtio_ids.h>
+#include <linux/virtio_config.h>
+
+/**
+ * struct virtio_i2c_out_hdr - the virtio I2C message OUT header
+ * @addr: the controlled device address
+ * @padding: used to pad to full dword
+ * @flags: used for feature extensibility
+ */
+struct virtio_i2c_out_hdr {
+ __le16 addr;
+ __le16 padding;
+ __le32 flags;
+};
+
+/* The bit 0 of the @virtio_i2c_out_hdr.@flags, used to group the requests */
+#define VIRTIO_I2C_FLAGS_FAIL_NEXT 0x00000001
+
+/**
+ * struct virtio_i2c_in_hdr - the virtio I2C message IN header
+ * @status: the processing result from the backend
+ */
+struct virtio_i2c_in_hdr {
+ u8 status;
+};
+
+/* The final status written by the device */
+#define VIRTIO_I2C_MSG_OK 0
+#define VIRTIO_I2C_MSG_ERR 1
+
+/**
+ * struct virtio_i2c_req - the virtio I2C request structure
+ * @out_hdr: the OUT header of the virtio I2C message
+ * @write_buf: contains one I2C segment being written to the device
+ * @read_buf: contains one I2C segment being read from the device
+ * @in_hdr: the IN header of the virtio I2C message
+ */
+struct virtio_i2c_req {
+ struct virtio_i2c_out_hdr out_hdr;
+ u8 *write_buf;
+ u8 *read_buf;
+ struct virtio_i2c_in_hdr in_hdr;
+};
+
+#endif /* _UAPI_LINUX_VIRTIO_I2C_H */
diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
index bc1c062..6ae32db 100644
--- a/include/uapi/linux/virtio_ids.h
+++ b/include/uapi/linux/virtio_ids.h
@@ -54,5 +54,6 @@
#define VIRTIO_ID_FS 26 /* virtio filesystem */
#define VIRTIO_ID_PMEM 27 /* virtio pmem */
#define VIRTIO_ID_MAC80211_HWSIM 29 /* virtio mac80211-hwsim */
+#define VIRTIO_ID_I2C_ADPTER 34 /* virtio i2c adpter */
#endif /* _LINUX_VIRTIO_IDS_H */
--
2.7.4
On Mon, Mar 1, 2021 at 7:41 AM Jie Deng <[email protected]> wrote:
> --- /dev/null
> +++ b/include/uapi/linux/virtio_i2c.h
> @@ -0,0 +1,56 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later WITH Linux-syscall-note */
> +/*
> + * Definitions for virtio I2C Adpter
> + *
> + * Copyright (c) 2021 Intel Corporation. All rights reserved.
> + */
> +
> +#ifndef _UAPI_LINUX_VIRTIO_I2C_H
> +#define _UAPI_LINUX_VIRTIO_I2C_H
Why is this a uapi header? Can't this all be moved into the driver
itself?
> +/**
> + * struct virtio_i2c_req - the virtio I2C request structure
> + * @out_hdr: the OUT header of the virtio I2C message
> + * @write_buf: contains one I2C segment being written to the device
> + * @read_buf: contains one I2C segment being read from the device
> + * @in_hdr: the IN header of the virtio I2C message
> + */
> +struct virtio_i2c_req {
> + struct virtio_i2c_out_hdr out_hdr;
> + u8 *write_buf;
> + u8 *read_buf;
> + struct virtio_i2c_in_hdr in_hdr;
> +};
In particular, this structure looks like it is only ever usable between
the transfer functions in the driver itself, it is shared with neither
user space nor the virtio host side.
Arnd
On 01-03-21, 16:19, Arnd Bergmann wrote:
> On Mon, Mar 1, 2021 at 7:41 AM Jie Deng <[email protected]> wrote:
>
> > --- /dev/null
> > +++ b/include/uapi/linux/virtio_i2c.h
> > @@ -0,0 +1,56 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later WITH Linux-syscall-note */
> > +/*
> > + * Definitions for virtio I2C Adpter
> > + *
> > + * Copyright (c) 2021 Intel Corporation. All rights reserved.
> > + */
> > +
> > +#ifndef _UAPI_LINUX_VIRTIO_I2C_H
> > +#define _UAPI_LINUX_VIRTIO_I2C_H
>
> Why is this a uapi header? Can't this all be moved into the driver
> itself?
>
> > +/**
> > + * struct virtio_i2c_req - the virtio I2C request structure
> > + * @out_hdr: the OUT header of the virtio I2C message
> > + * @write_buf: contains one I2C segment being written to the device
> > + * @read_buf: contains one I2C segment being read from the device
> > + * @in_hdr: the IN header of the virtio I2C message
> > + */
> > +struct virtio_i2c_req {
> > + struct virtio_i2c_out_hdr out_hdr;
> > + u8 *write_buf;
> > + u8 *read_buf;
> > + struct virtio_i2c_in_hdr in_hdr;
> > +};
>
> In particular, this structure looks like it is only ever usable between
> the transfer functions in the driver itself, it is shared with neither
> user space nor the virtio host side.
Why is it so ? Won't you expect hypervisors or userspace apps to use
these ?
--
viresh
On 01-03-21, 14:41, Jie Deng wrote:
> diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
> +static int virtio_i2c_send_reqs(struct virtqueue *vq,
> + struct virtio_i2c_req *reqs,
> + struct i2c_msg *msgs, int nr)
> +{
> + struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr;
> + int i, outcnt, incnt, err = 0;
> + u8 *buf;
> +
> + for (i = 0; i < nr; i++) {
> + if (!msgs[i].len)
> + break;
> +
> + reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1);
> +
> + if (i != nr - 1)
> + reqs[i].out_hdr.flags |= VIRTIO_I2C_FLAGS_FAIL_NEXT;
> +
> + outcnt = incnt = 0;
> + sg_init_one(&out_hdr, &reqs[i].out_hdr, sizeof(reqs[i].out_hdr));
> + sgs[outcnt++] = &out_hdr;
> +
> + buf = kzalloc(msgs[i].len, GFP_KERNEL);
> + if (!buf)
> + break;
> +
> + if (msgs[i].flags & I2C_M_RD) {
> + reqs[i].read_buf = buf;
> + sg_init_one(&msg_buf, reqs[i].read_buf, msgs[i].len);
> + sgs[outcnt + incnt++] = &msg_buf;
> + } else {
> + reqs[i].write_buf = buf;
> + memcpy(reqs[i].write_buf, msgs[i].buf, msgs[i].len);
> + sg_init_one(&msg_buf, reqs[i].write_buf, msgs[i].len);
> + sgs[outcnt++] = &msg_buf;
> + }
> +
> + sg_init_one(&in_hdr, &reqs[i].in_hdr, sizeof(reqs[i].in_hdr));
> + sgs[outcnt + incnt++] = &in_hdr;
> +
> + err = virtqueue_add_sgs(vq, sgs, outcnt, incnt, &reqs[i], GFP_KERNEL);
> + if (err < 0) {
> + pr_err("failed to add msg[%d] to virtqueue.\n", i);
> + if (msgs[i].flags & I2C_M_RD) {
> + kfree(reqs[i].read_buf);
> + reqs[i].read_buf = NULL;
> + } else {
> + kfree(reqs[i].write_buf);
> + reqs[i].write_buf = NULL;
> + }
> + break;
> + }
> + }
> +
> + return i;
> +}
> diff --git a/include/uapi/linux/virtio_i2c.h b/include/uapi/linux/virtio_i2c.h
> +/**
> + * struct virtio_i2c_out_hdr - the virtio I2C message OUT header
> + * @addr: the controlled device address
> + * @padding: used to pad to full dword
> + * @flags: used for feature extensibility
> + */
> +struct virtio_i2c_out_hdr {
> + __le16 addr;
> + __le16 padding;
> + __le32 flags;
> +};
Both this code and the virtio spec (which is already merged) are
missing msgs[i].flags and they are never sent to backend. The only
flags available here are the ones defined by virtio spec and these are
not i2c flags.
I also looked at your i2c backend for acrn and it mistakenly copies
the hdr.flag, which is the virtio flag and not i2c flag.
https://github.com/projectacrn/acrn-hypervisor/blob/master/devicemodel/hw/pci/virtio/virtio_i2c.c#L539
I will send a fix for the specs if you agree that there is a problem
here.
what am I missing here ? This should have been caught in your testing
and so I feel I must be missing something.
--
viresh
On 03-03-21, 16:46, Jie Deng wrote:
> This is not a problem. My original proposal was to mirror the struct
> i2c_msg.
> The code you looked at was based on that.
> However, the virtio TC prefer not to mirror it. They have some concerns.
> For example, there is a bit I2C_M_RD in i2c_msg.flag which has the same
> meaning with
> the R/W in virtio descriptor. This is a repetition which may cause problems.
> So the virtio_i2c_out_hdr.flags is used to instead of i2c_msg.flags for
> extension.
So by default we don't support any of the existing flags except
I2C_M_RD?
#define I2C_M_TEN 0x0010 /* this is a ten bit chip address */
#define I2C_M_RD 0x0001 /* read data, from slave to master */
#define I2C_M_STOP 0x8000 /* if I2C_FUNC_PROTOCOL_MANGLING */
#define I2C_M_NOSTART 0x4000 /* if I2C_FUNC_NOSTART */
#define I2C_M_REV_DIR_ADDR 0x2000 /* if I2C_FUNC_PROTOCOL_MANGLING */
#define I2C_M_IGNORE_NAK 0x1000 /* if I2C_FUNC_PROTOCOL_MANGLING */
#define I2C_M_NO_RD_ACK 0x0800 /* if I2C_FUNC_PROTOCOL_MANGLING */
#define I2C_M_RECV_LEN 0x0400 /* length will be first received byte */
How do we work with clients who want to use such flags now ?
--
viresh
On 2021/3/3 15:54, Viresh Kumar wrote:
> On 01-03-21, 14:41, Jie Deng wrote:
>> diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
>> +static int virtio_i2c_send_reqs(struct virtqueue *vq,
>> + struct virtio_i2c_req *reqs,
>> + struct i2c_msg *msgs, int nr)
>> +{
>> + struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr;
>> + int i, outcnt, incnt, err = 0;
>> + u8 *buf;
>> +
>> + for (i = 0; i < nr; i++) {
>> + if (!msgs[i].len)
>> + break;
>> +
>> + reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1);
>> +
>> + if (i != nr - 1)
>> + reqs[i].out_hdr.flags |= VIRTIO_I2C_FLAGS_FAIL_NEXT;
>> +
>> + outcnt = incnt = 0;
>> + sg_init_one(&out_hdr, &reqs[i].out_hdr, sizeof(reqs[i].out_hdr));
>> + sgs[outcnt++] = &out_hdr;
>> +
>> + buf = kzalloc(msgs[i].len, GFP_KERNEL);
>> + if (!buf)
>> + break;
>> +
>> + if (msgs[i].flags & I2C_M_RD) {
>> + reqs[i].read_buf = buf;
>> + sg_init_one(&msg_buf, reqs[i].read_buf, msgs[i].len);
>> + sgs[outcnt + incnt++] = &msg_buf;
>> + } else {
>> + reqs[i].write_buf = buf;
>> + memcpy(reqs[i].write_buf, msgs[i].buf, msgs[i].len);
>> + sg_init_one(&msg_buf, reqs[i].write_buf, msgs[i].len);
>> + sgs[outcnt++] = &msg_buf;
>> + }
>> +
>> + sg_init_one(&in_hdr, &reqs[i].in_hdr, sizeof(reqs[i].in_hdr));
>> + sgs[outcnt + incnt++] = &in_hdr;
>> +
>> + err = virtqueue_add_sgs(vq, sgs, outcnt, incnt, &reqs[i], GFP_KERNEL);
>> + if (err < 0) {
>> + pr_err("failed to add msg[%d] to virtqueue.\n", i);
>> + if (msgs[i].flags & I2C_M_RD) {
>> + kfree(reqs[i].read_buf);
>> + reqs[i].read_buf = NULL;
>> + } else {
>> + kfree(reqs[i].write_buf);
>> + reqs[i].write_buf = NULL;
>> + }
>> + break;
>> + }
>> + }
>> +
>> + return i;
>> +}
>> diff --git a/include/uapi/linux/virtio_i2c.h b/include/uapi/linux/virtio_i2c.h
>> +/**
>> + * struct virtio_i2c_out_hdr - the virtio I2C message OUT header
>> + * @addr: the controlled device address
>> + * @padding: used to pad to full dword
>> + * @flags: used for feature extensibility
>> + */
>> +struct virtio_i2c_out_hdr {
>> + __le16 addr;
>> + __le16 padding;
>> + __le32 flags;
>> +};
> Both this code and the virtio spec (which is already merged) are
> missing msgs[i].flags and they are never sent to backend. The only
> flags available here are the ones defined by virtio spec and these are
> not i2c flags.
>
> I also looked at your i2c backend for acrn and it mistakenly copies
> the hdr.flag, which is the virtio flag and not i2c flag.
>
> https://github.com/projectacrn/acrn-hypervisor/blob/master/devicemodel/hw/pci/virtio/virtio_i2c.c#L539
>
> I will send a fix for the specs if you agree that there is a problem
> here.
>
> what am I missing here ? This should have been caught in your testing
> and so I feel I must be missing something.
This is not a problem. My original proposal was to mirror the struct
i2c_msg.
The code you looked at was based on that.
However, the virtio TC prefer not to mirror it. They have some concerns.
For example, there is a bit I2C_M_RD in i2c_msg.flag which has the same
meaning with
the R/W in virtio descriptor. This is a repetition which may cause problems.
So the virtio_i2c_out_hdr.flags is used to instead of i2c_msg.flags for
extension.
You can check this link
https://github.com/oasis-tcs/virtio-spec/issues/88 to learn the whole story.
There are discussions about the spec (v1 ~ v7).
I'm updating these drivers step by step to make sure they finally follow
the spec.
On 2021/3/3 17:38, Viresh Kumar wrote:
> On 03-03-21, 16:46, Jie Deng wrote:
>> This is not a problem. My original proposal was to mirror the struct
>> i2c_msg.
>> The code you looked at was based on that.
>> However, the virtio TC prefer not to mirror it. They have some concerns.
>> For example, there is a bit I2C_M_RD in i2c_msg.flag which has the same
>> meaning with
>> the R/W in virtio descriptor. This is a repetition which may cause problems.
>> So the virtio_i2c_out_hdr.flags is used to instead of i2c_msg.flags for
>> extension.
> So by default we don't support any of the existing flags except
> I2C_M_RD?
Yes. That's the current status.
> #define I2C_M_TEN 0x0010 /* this is a ten bit chip address */
> #define I2C_M_RD 0x0001 /* read data, from slave to master */
> #define I2C_M_STOP 0x8000 /* if I2C_FUNC_PROTOCOL_MANGLING */
> #define I2C_M_NOSTART 0x4000 /* if I2C_FUNC_NOSTART */
> #define I2C_M_REV_DIR_ADDR 0x2000 /* if I2C_FUNC_PROTOCOL_MANGLING */
> #define I2C_M_IGNORE_NAK 0x1000 /* if I2C_FUNC_PROTOCOL_MANGLING */
> #define I2C_M_NO_RD_ACK 0x0800 /* if I2C_FUNC_PROTOCOL_MANGLING */
> #define I2C_M_RECV_LEN 0x0400 /* length will be first received byte */
>
> How do we work with clients who want to use such flags now ?
My plan is to have a minimum driver get merged. Then we have a base and
we can
update virtio_i2c_out_hdr.flags for the feature extensibility. Then, If
you want to help to develop
this stuff, you can just follow the same flow. First, you can update the
Spec by sending
comments to [email protected]. Once your Spec patch is
acked by the
virtio TC, you can then send patches to update the corresponding drivers.
Thanks.