[PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Jie Deng]

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 <conghui.chen@intel.com>
Signed-off-by: Conghui Chen <conghui.chen@intel.com>
Signed-off-by: Jie Deng <jie.deng@intel.com>
---
 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

[PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Jie Deng]

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 <conghui.chen@intel.com>
Signed-off-by: Conghui Chen <conghui.chen@intel.com>
Signed-off-by: Jie Deng <jie.deng@intel.com>
---
 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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Viresh Kumar]

On 01-03-21, 14:41, Jie Deng wrote:
> +/**
> + * 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;
> +};

I am not able to appreciate the use of write/read bufs here as we
aren't trying to read/write data in the same transaction. Why do we
have two bufs here as well as in specs ?

What about this on top of your patch ?

---
 drivers/i2c/busses/i2c-virtio.c | 31 +++++++++++--------------------
 include/uapi/linux/virtio_i2c.h |  3 +--
 2 files changed, 12 insertions(+), 22 deletions(-)

diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
index 8c8bc9545418..e71ab1d2c83f 100644
--- a/drivers/i2c/busses/i2c-virtio.c
+++ b/drivers/i2c/busses/i2c-virtio.c
@@ -67,14 +67,13 @@ static int virtio_i2c_send_reqs(struct virtqueue *vq,
 		if (!buf)
 			break;
 
+		reqs[i].buf = buf;
+		sg_init_one(&msg_buf, reqs[i].buf, msgs[i].len);
+
 		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);
+			memcpy(reqs[i].buf, msgs[i].buf, msgs[i].len);
 			sgs[outcnt++] = &msg_buf;
 		}
 
@@ -84,13 +83,8 @@ static int virtio_i2c_send_reqs(struct virtqueue *vq,
 		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;
-			}
+			kfree(reqs[i].buf);
+			reqs[i].buf = NULL;
 			break;
 		}
 	}
@@ -118,14 +112,11 @@ static int virtio_i2c_complete_reqs(struct virtqueue *vq,
 			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;
-		}
+		if (msgs[i].flags & I2C_M_RD)
+			memcpy(msgs[i].buf, req->buf, msgs[i].len);
+
+		kfree(req->buf);
+		req->buf = NULL;
 	}
 
 	return i;
diff --git a/include/uapi/linux/virtio_i2c.h b/include/uapi/linux/virtio_i2c.h
index 92febf0c527e..61f0086ac75b 100644
--- a/include/uapi/linux/virtio_i2c.h
+++ b/include/uapi/linux/virtio_i2c.h
@@ -48,8 +48,7 @@ struct virtio_i2c_in_hdr {
  */
 struct virtio_i2c_req {
 	struct virtio_i2c_out_hdr out_hdr;
-	u8 *write_buf;
-	u8 *read_buf;
+	u8 *buf;
 	struct virtio_i2c_in_hdr in_hdr;
 };
 
-- 
viresh

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Andy Shevchenko]

On Mon, Mar 01, 2021 at 02:41:35PM +0800, Jie Deng wrote:
> 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.

...

> +		buf = kzalloc(msgs[i].len, GFP_KERNEL);
> +		if (!buf)
> +			break;
> +
> +		if (msgs[i].flags & I2C_M_RD) {

kzalloc()

> +			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);

kmemdup() ?

> +			sg_init_one(&msg_buf, reqs[i].write_buf, msgs[i].len);
> +			sgs[outcnt++] = &msg_buf;
> +		}

...

> +
> +

One blank line is enough.

...


> +	ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
> +	if (ret == 0)
> +		goto err_unlock_free;

> +	else

Redundant.

> +		nr = ret;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Andy Shevchenko]

On Mon, Mar 01, 2021 at 02:41:35PM +0800, Jie Deng wrote:
> 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.

...

> +		buf = kzalloc(msgs[i].len, GFP_KERNEL);
> +		if (!buf)
> +			break;
> +
> +		if (msgs[i].flags & I2C_M_RD) {

kzalloc()

> +			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);

kmemdup() ?

> +			sg_init_one(&msg_buf, reqs[i].write_buf, msgs[i].len);
> +			sgs[outcnt++] = &msg_buf;
> +		}

...

> +
> +

One blank line is enough.

...


> +	ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
> +	if (ret == 0)
> +		goto err_unlock_free;

> +	else

Redundant.

> +		nr = ret;

-- 
With Best Regards,
Andy Shevchenko


_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Andy Shevchenko]

On Mon, Mar 01, 2021 at 05:24:41PM +0530, Viresh Kumar wrote:
> On 01-03-21, 14:41, Jie Deng wrote:
> > +/**
> > + * 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;
> > +};
> 
> I am not able to appreciate the use of write/read bufs here as we
> aren't trying to read/write data in the same transaction. Why do we
> have two bufs here as well as in specs ?

I²C and SMBus support bidirectional transfers as well. I think two buffers is
the right thing to do.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Andy Shevchenko]

On Mon, Mar 01, 2021 at 05:24:41PM +0530, Viresh Kumar wrote:
> On 01-03-21, 14:41, Jie Deng wrote:
> > +/**
> > + * 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;
> > +};
> 
> I am not able to appreciate the use of write/read bufs here as we
> aren't trying to read/write data in the same transaction. Why do we
> have two bufs here as well as in specs ?

I²C and SMBus support bidirectional transfers as well. I think two buffers is
the right thing to do.

-- 
With Best Regards,
Andy Shevchenko


_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Andy Shevchenko]

On Mon, Mar 01, 2021 at 02:09:25PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 01, 2021 at 05:24:41PM +0530, Viresh Kumar wrote:
> > On 01-03-21, 14:41, Jie Deng wrote:
> > > +/**
> > > + * 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;
> > > +};
> > 
> > I am not able to appreciate the use of write/read bufs here as we
> > aren't trying to read/write data in the same transaction. Why do we
> > have two bufs here as well as in specs ?
> 
> I²C and SMBus support bidirectional transfers as well. I think two buffers is
> the right thing to do.

Strictly speaking "half duplex".

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Andy Shevchenko]

On Mon, Mar 01, 2021 at 02:09:25PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 01, 2021 at 05:24:41PM +0530, Viresh Kumar wrote:
> > On 01-03-21, 14:41, Jie Deng wrote:
> > > +/**
> > > + * 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;
> > > +};
> > 
> > I am not able to appreciate the use of write/read bufs here as we
> > aren't trying to read/write data in the same transaction. Why do we
> > have two bufs here as well as in specs ?
> 
> I²C and SMBus support bidirectional transfers as well. I think two buffers is
> the right thing to do.

Strictly speaking "half duplex".

-- 
With Best Regards,
Andy Shevchenko


_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Arnd Bergmann]

On Mon, Mar 1, 2021 at 7:41 AM Jie Deng <jie.deng@intel.com> 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

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Arnd Bergmann]

On Mon, Mar 1, 2021 at 7:41 AM Jie Deng <jie.deng@intel.com> 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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Arnd Bergmann]

On Mon, Mar 1, 2021 at 1:10 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Mon, Mar 01, 2021 at 02:09:25PM +0200, Andy Shevchenko wrote:
> > On Mon, Mar 01, 2021 at 05:24:41PM +0530, Viresh Kumar wrote:
> > > On 01-03-21, 14:41, Jie Deng wrote:
> > > > +/**
> > > > + * 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;
> > > > +};
> > >
> > > I am not able to appreciate the use of write/read bufs here as we
> > > aren't trying to read/write data in the same transaction. Why do we
> > > have two bufs here as well as in specs ?
> >
> > I涎 and SMBus support bidirectional transfers as well. I think two buffers is
> > the right thing to do.
>
> Strictly speaking "half duplex".

But the driver does not support this at all: the sglist always has three
members as Viresh says: outhdr, msgbuf and inhdr. It then uses a
bounce buffer for the actual data transfer, and this always goes either
one way or the other.

I think the more important question is: does this driver actually need
the bounce buffer? It doesn't have to worry about adjacent stack
data being clobbered by cache maintenance because virtio is always
cache coherent and, so I suspect the bounce buffer can be left out.

It might actually be simpler to just have a fixed-length array of
headers on the stack and at most the corresponding number of
transfers for one virtqueue_kick().

Is there a reasonable limit on how many transfers we would
expect to handle at once? I see that most callers of i2c_transfer()
hardcode the number to '1' or '2', rarely '3' or '4', while the proposed
implementation seems to be optimized for much larger numbers.

       Arnd

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Arnd Bergmann]

On Mon, Mar 1, 2021 at 1:10 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Mon, Mar 01, 2021 at 02:09:25PM +0200, Andy Shevchenko wrote:
> > On Mon, Mar 01, 2021 at 05:24:41PM +0530, Viresh Kumar wrote:
> > > On 01-03-21, 14:41, Jie Deng wrote:
> > > > +/**
> > > > + * 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;
> > > > +};
> > >
> > > I am not able to appreciate the use of write/read bufs here as we
> > > aren't trying to read/write data in the same transaction. Why do we
> > > have two bufs here as well as in specs ?
> >
> > I涎 and SMBus support bidirectional transfers as well. I think two buffers is
> > the right thing to do.
>
> Strictly speaking "half duplex".

But the driver does not support this at all: the sglist always has three
members as Viresh says: outhdr, msgbuf and inhdr. It then uses a
bounce buffer for the actual data transfer, and this always goes either
one way or the other.

I think the more important question is: does this driver actually need
the bounce buffer? It doesn't have to worry about adjacent stack
data being clobbered by cache maintenance because virtio is always
cache coherent and, so I suspect the bounce buffer can be left out.

It might actually be simpler to just have a fixed-length array of
headers on the stack and at most the corresponding number of
transfers for one virtqueue_kick().

Is there a reasonable limit on how many transfers we would
expect to handle at once? I see that most callers of i2c_transfer()
hardcode the number to '1' or '2', rarely '3' or '4', while the proposed
implementation seems to be optimized for much larger numbers.

       Arnd
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Jie Deng]

On 2021/3/1 19:54, Viresh Kumar wrote:
> On 01-03-21, 14:41, Jie Deng wrote:
>> +/**
>> + * 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;
>> +};
> I am not able to appreciate the use of write/read bufs here as we
> aren't trying to read/write data in the same transaction. Why do we
> have two bufs here as well as in specs ?
>
> What about this on top of your patch ?
>
> ---
>   drivers/i2c/busses/i2c-virtio.c | 31 +++++++++++--------------------
>   include/uapi/linux/virtio_i2c.h |  3 +--
>   2 files changed, 12 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
> index 8c8bc9545418..e71ab1d2c83f 100644
> --- a/drivers/i2c/busses/i2c-virtio.c
> +++ b/drivers/i2c/busses/i2c-virtio.c
> @@ -67,14 +67,13 @@ static int virtio_i2c_send_reqs(struct virtqueue *vq,
>   		if (!buf)
>   			break;
>   
> +		reqs[i].buf = buf;
> +		sg_init_one(&msg_buf, reqs[i].buf, msgs[i].len);
> +
>   		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);
> +			memcpy(reqs[i].buf, msgs[i].buf, msgs[i].len);
>   			sgs[outcnt++] = &msg_buf;
>   		}
>   
> @@ -84,13 +83,8 @@ static int virtio_i2c_send_reqs(struct virtqueue *vq,
>   		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;
> -			}
> +			kfree(reqs[i].buf);
> +			reqs[i].buf = NULL;
>   			break;
>   		}
>   	}
> @@ -118,14 +112,11 @@ static int virtio_i2c_complete_reqs(struct virtqueue *vq,
>   			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;
> -		}
> +		if (msgs[i].flags & I2C_M_RD)
> +			memcpy(msgs[i].buf, req->buf, msgs[i].len);
> +
> +		kfree(req->buf);
> +		req->buf = NULL;
>   	}
>   
>   	return i;
> diff --git a/include/uapi/linux/virtio_i2c.h b/include/uapi/linux/virtio_i2c.h
> index 92febf0c527e..61f0086ac75b 100644
> --- a/include/uapi/linux/virtio_i2c.h
> +++ b/include/uapi/linux/virtio_i2c.h
> @@ -48,8 +48,7 @@ struct virtio_i2c_in_hdr {
>    */
>   struct virtio_i2c_req {
>   	struct virtio_i2c_out_hdr out_hdr;
> -	u8 *write_buf;
> -	u8 *read_buf;
> +	u8 *buf;
>   	struct virtio_i2c_in_hdr in_hdr;
>   };
>   

That's my original proposal. I used to mirror this interface with 
"struct i2c_msg".

But the design philosophy of virtio TC is that VIRTIO devices are not 
specific to Linux
so the specs design should avoid the limitations of the current Linux 
driver behavior.

We had some discussion about this. You may check these links to learn 
the story.
https://lists.oasis-open.org/archives/virtio-comment/202010/msg00016.html
https://lists.oasis-open.org/archives/virtio-comment/202010/msg00033.html
https://lists.oasis-open.org/archives/virtio-comment/202011/msg00025.html

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Jie Deng]

On 2021/3/1 19:54, Viresh Kumar wrote:
> On 01-03-21, 14:41, Jie Deng wrote:
>> +/**
>> + * 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;
>> +};
> I am not able to appreciate the use of write/read bufs here as we
> aren't trying to read/write data in the same transaction. Why do we
> have two bufs here as well as in specs ?
>
> What about this on top of your patch ?
>
> ---
>   drivers/i2c/busses/i2c-virtio.c | 31 +++++++++++--------------------
>   include/uapi/linux/virtio_i2c.h |  3 +--
>   2 files changed, 12 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
> index 8c8bc9545418..e71ab1d2c83f 100644
> --- a/drivers/i2c/busses/i2c-virtio.c
> +++ b/drivers/i2c/busses/i2c-virtio.c
> @@ -67,14 +67,13 @@ static int virtio_i2c_send_reqs(struct virtqueue *vq,
>   		if (!buf)
>   			break;
>   
> +		reqs[i].buf = buf;
> +		sg_init_one(&msg_buf, reqs[i].buf, msgs[i].len);
> +
>   		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);
> +			memcpy(reqs[i].buf, msgs[i].buf, msgs[i].len);
>   			sgs[outcnt++] = &msg_buf;
>   		}
>   
> @@ -84,13 +83,8 @@ static int virtio_i2c_send_reqs(struct virtqueue *vq,
>   		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;
> -			}
> +			kfree(reqs[i].buf);
> +			reqs[i].buf = NULL;
>   			break;
>   		}
>   	}
> @@ -118,14 +112,11 @@ static int virtio_i2c_complete_reqs(struct virtqueue *vq,
>   			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;
> -		}
> +		if (msgs[i].flags & I2C_M_RD)
> +			memcpy(msgs[i].buf, req->buf, msgs[i].len);
> +
> +		kfree(req->buf);
> +		req->buf = NULL;
>   	}
>   
>   	return i;
> diff --git a/include/uapi/linux/virtio_i2c.h b/include/uapi/linux/virtio_i2c.h
> index 92febf0c527e..61f0086ac75b 100644
> --- a/include/uapi/linux/virtio_i2c.h
> +++ b/include/uapi/linux/virtio_i2c.h
> @@ -48,8 +48,7 @@ struct virtio_i2c_in_hdr {
>    */
>   struct virtio_i2c_req {
>   	struct virtio_i2c_out_hdr out_hdr;
> -	u8 *write_buf;
> -	u8 *read_buf;
> +	u8 *buf;
>   	struct virtio_i2c_in_hdr in_hdr;
>   };
>   

That's my original proposal. I used to mirror this interface with 
"struct i2c_msg".

But the design philosophy of virtio TC is that VIRTIO devices are not 
specific to Linux
so the specs design should avoid the limitations of the current Linux 
driver behavior.

We had some discussion about this. You may check these links to learn 
the story.
https://lists.oasis-open.org/archives/virtio-comment/202010/msg00016.html
https://lists.oasis-open.org/archives/virtio-comment/202010/msg00033.html
https://lists.oasis-open.org/archives/virtio-comment/202011/msg00025.html
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Jie Deng]

[-- Attachment #1.1: Type: text/plain, Size: 3480 bytes --]


On 2021/3/1 23:19, Arnd Bergmann wrote:
> On Mon, Mar 1, 2021 at 7:41 AM Jie Deng <jie.deng@intel.com> 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

Please check this link.

https://lists.linuxfoundation.org/pipermail/virtualization/2020-October/050222.html

Jason/Stefan, could you please double confirm about the following ?

**************************************************************************

>>>>>/I think following definition in uAPI for the status is enough. />>>>>/There is no need to provide a "u8" status in the structure. />>>>>//>>>>>//* The final status written by the device */ />>>>>/#define VIRTIO_I2C_MSG_OK    0 />>>>>/#define VIRTIO_I2C_MSG_ERR    1 />>>>>//>>>>>/You can see an example in virtio_blk. />>>>>//>>>>>/In the spec: />>>>>//>>>>>/struct virtio_blk_req { />>>>>/le32 type; />>>>>/le32 reserved; />>>>>/le64 sector; />>>>>/u8 data[]; />>>>>/u8 status; />>>>>/}; />>>>>//>>>>>/In virtio_blk.h, there is only following definitions. />>>>>//>>>>>/#define VIRTIO_BLK_S_OK        0 />>>>>/#define VIRTIO_BLK_S_IOERR    1 />>>>>/#define VIRTIO_BLK_S_UNSUPP    2 />>>>>//>>>>//>>>>/virtio-blk is a bad example, it's just too late to fix. For any new />>>>/introduced uAPI it should be a complete one. />>>>//>>>>/Thanks />>>>//>>>/I checked a relatively new device "virtio_fs". />>>/I found following definition in spec but not in uAPI also. />>>//>>>/struct virtio_fs_req { />>>/// Device -readable part />>>/struct fuse_in_header in; />>>/u8 datain[]; />>>/// Device -writable part />>>/struct fuse_out_header out; />>>/u8 dataout[]; />>>/}; />>>//>>>/So is this also a bad example which has not been fixed yet. />>//>>//>>/Cc Stefan for the answer. />>//>>//>>>/Or what's your mean about "complete" here ? Is there any definition />>>/about "complete uAPI" ? />>//>>//>>/My understanding it should contain all the fields defined in the />>/virtio spec. />>//>>/Thanks />>//>/OK. I noticed this isn't strictly implemented in the current virtio />/codes. />/I'm not sure if this is already a consensus. I will follow it if this />/is the opinion of the majority. /

Please do that, this forces us to maintain uABI compatibility which is
what Linux try to maintain for ever.

******************************************************************************


[-- Attachment #1.2: Type: text/html, Size: 5932 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Viresh Kumar]

On 02-03-21, 10:21, Jie Deng wrote:
> On 2021/3/1 19:54, Viresh Kumar wrote:
> That's my original proposal. I used to mirror this interface with "struct
> i2c_msg".
> 
> But the design philosophy of virtio TC is that VIRTIO devices are not
> specific to Linux
> so the specs design should avoid the limitations of the current Linux driver
> behavior.

Right, I understand that.

> We had some discussion about this. You may check these links to learn the
> story.
> https://lists.oasis-open.org/archives/virtio-comment/202010/msg00016.html
> https://lists.oasis-open.org/archives/virtio-comment/202010/msg00033.html
> https://lists.oasis-open.org/archives/virtio-comment/202011/msg00025.html

So the thing is that we want to support full duplex mode, right ?

How will that work protocol wise ? I mean how would we know if we are
expecting both tx and rx buffers in a transfer ?

-- 
viresh

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Viresh Kumar]

On 01-03-21, 14:10, Andy Shevchenko wrote:
> On Mon, Mar 01, 2021 at 02:09:25PM +0200, Andy Shevchenko wrote:
> > On Mon, Mar 01, 2021 at 05:24:41PM +0530, Viresh Kumar wrote:
> > > On 01-03-21, 14:41, Jie Deng wrote:
> > > > +/**
> > > > + * 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;
> > > > +};
> > > 
> > > I am not able to appreciate the use of write/read bufs here as we
> > > aren't trying to read/write data in the same transaction. Why do we
> > > have two bufs here as well as in specs ?
> > 
> > I²C and SMBus support bidirectional transfers as well. I think two buffers is
> > the right thing to do.
> 
> Strictly speaking "half duplex".

Half duplex is what this driver implemented, i.e. only one side can
send a msg at once, we don't need two buffers for that for sure.

Though we need two buffers if we are ever going to support full
duplex.

-- 
viresh

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Viresh Kumar]

On 01-03-21, 16:46, Arnd Bergmann wrote:
> But the driver does not support this at all: the sglist always has three
> members as Viresh says: outhdr, msgbuf and inhdr. It then uses a
> bounce buffer for the actual data transfer, and this always goes either
> one way or the other.

Yes and if the driver doesn't support the specification fully then it
should say so in a comment at least and also fail in case someone
tries a full duplex transfer..

-- 
viresh

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Viresh Kumar]

On 01-03-21, 16:19, Arnd Bergmann wrote:
> On Mon, Mar 1, 2021 at 7:41 AM Jie Deng <jie.deng@intel.com> 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

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Viresh Kumar]

On 02-03-21, 09:31, Viresh Kumar wrote:
> On 01-03-21, 16:19, Arnd Bergmann wrote:
> > On Mon, Mar 1, 2021 at 7:41 AM Jie Deng <jie.deng@intel.com> 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 ?

This comment applies only for the first two structures as the third
one is never exchanged over virtio.

-- 
viresh

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Viresh Kumar]

On 01-03-21, 14:41, Jie Deng wrote:
> +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);

And this won't work for 10 bit addressing right? Don't we support that
in kernel ?

From Spec:

\begin{tabular}{ |l||l|l|l|l|l|l|l|l|l|l|l|l|l|l|l|l| }
\hline
Bits           & 15 & 14 & 13 & 12 & 11 & 10 & 9  & 8  & 7  & 6  & 5  & 4  & 3  & 2  & 1  & 0 \\
\hline
7-bit address  & 0  & 0  & 0  & 0  & 0  & 0  & 0  & 0  & A6 & A5 & A4 & A3 & A2 & A1 & A0 & 0 \\
\hline
10-bit address & A7 & A6 & A5 & A4 & A3 & A2 & A1 & A0 & 1  & 1  & 1  & 1  & 0  & A9 & A8 & 0 \\
\hline
\end{tabular}

-- 
viresh

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Jie Deng]

On 2021/3/2 12:22, Viresh Kumar wrote:
> On 02-03-21, 09:31, Viresh Kumar wrote:
>> On 01-03-21, 16:19, Arnd Bergmann wrote:
>>> On Mon, Mar 1, 2021 at 7:41 AM Jie Deng <jie.deng@intel.com> 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 ?
> This comment applies only for the first two structures as the third
> one is never exchanged over virtio.
Yeah. Actually, the backend only needs "struct virtio_i2c_out_hdr out_hdr"
and "struct virtio_i2c_in_hdr in_hdr" for communication. So we only need 
to keep
the first two in uapi and move "struct virtio_i2c_req" into the driver.

But Jason wanted to include "struct virtio_i2c_req" in uapi. He 
explained in this link
https://lists.linuxfoundation.org/pipermail/virtualization/2020-October/050222.html.
Do you agree with that explanation ?

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Jie Deng]

On 2021/3/2 12:22, Viresh Kumar wrote:
> On 02-03-21, 09:31, Viresh Kumar wrote:
>> On 01-03-21, 16:19, Arnd Bergmann wrote:
>>> On Mon, Mar 1, 2021 at 7:41 AM Jie Deng <jie.deng@intel.com> 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 ?
> This comment applies only for the first two structures as the third
> one is never exchanged over virtio.
Yeah. Actually, the backend only needs "struct virtio_i2c_out_hdr out_hdr"
and "struct virtio_i2c_in_hdr in_hdr" for communication. So we only need 
to keep
the first two in uapi and move "struct virtio_i2c_req" into the driver.

But Jason wanted to include "struct virtio_i2c_req" in uapi. He 
explained in this link
https://lists.linuxfoundation.org/pipermail/virtualization/2020-October/050222.html.
Do you agree with that explanation ?
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Viresh Kumar]

On 02-03-21, 13:06, Jie Deng wrote:
> Yeah. Actually, the backend only needs "struct virtio_i2c_out_hdr out_hdr"
> and "struct virtio_i2c_in_hdr in_hdr" for communication. So we only need to
> keep
> the first two in uapi and move "struct virtio_i2c_req" into the driver.
> 
> But Jason wanted to include "struct virtio_i2c_req" in uapi. He explained in
> this link
> https://lists.linuxfoundation.org/pipermail/virtualization/2020-October/050222.html.
> Do you agree with that explanation ?

I am not sure I understood his reasoning well, but it doesn't make any
sense to keep this in uapi header if this is never going to get
transferred over the wire.

Moreover, the struct virtio_i2c_req in spec is misleading to me and
rather creates unnecessary confusion. There is no structure like this
which ever get passed here, but rather there are multiple vq
transactions which take place, one with just the out header, then one
with buffer and finally one with in header.

I am not sure what's the right way of documenting it or if this is a
standard virtio world follows.

-- 
viresh

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Jie Deng]

On 2021/3/2 12:42, Viresh Kumar wrote:
> On 01-03-21, 14:41, Jie Deng wrote:
>> +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);
> And this won't work for 10 bit addressing right? Don't we support that
> in kernel ?
>
>  From Spec:
>
> \begin{tabular}{ |l||l|l|l|l|l|l|l|l|l|l|l|l|l|l|l|l| }
> \hline
> Bits           & 15 & 14 & 13 & 12 & 11 & 10 & 9  & 8  & 7  & 6  & 5  & 4  & 3  & 2  & 1  & 0 \\
> \hline
> 7-bit address  & 0  & 0  & 0  & 0  & 0  & 0  & 0  & 0  & A6 & A5 & A4 & A3 & A2 & A1 & A0 & 0 \\
> \hline
> 10-bit address & A7 & A6 & A5 & A4 & A3 & A2 & A1 & A0 & 1  & 1  & 1  & 1  & 0  & A9 & A8 & 0 \\
> \hline
> \end{tabular}
Currently, to make things simple, this driver only supports 7 bit mode.
It doesn't declare "I2C_FUNC_10BIT_ADDR" in the functionality.
We may add in the future according to the need.

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Jie Deng]

On 2021/3/2 12:42, Viresh Kumar wrote:
> On 01-03-21, 14:41, Jie Deng wrote:
>> +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);
> And this won't work for 10 bit addressing right? Don't we support that
> in kernel ?
>
>  From Spec:
>
> \begin{tabular}{ |l||l|l|l|l|l|l|l|l|l|l|l|l|l|l|l|l| }
> \hline
> Bits           & 15 & 14 & 13 & 12 & 11 & 10 & 9  & 8  & 7  & 6  & 5  & 4  & 3  & 2  & 1  & 0 \\
> \hline
> 7-bit address  & 0  & 0  & 0  & 0  & 0  & 0  & 0  & 0  & A6 & A5 & A4 & A3 & A2 & A1 & A0 & 0 \\
> \hline
> 10-bit address & A7 & A6 & A5 & A4 & A3 & A2 & A1 & A0 & 1  & 1  & 1  & 1  & 0  & A9 & A8 & 0 \\
> \hline
> \end{tabular}
Currently, to make things simple, this driver only supports 7 bit mode.
It doesn't declare "I2C_FUNC_10BIT_ADDR" in the functionality.
We may add in the future according to the need.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Viresh Kumar]

On 02-03-21, 13:21, Jie Deng wrote:
> 
> On 2021/3/2 12:42, Viresh Kumar wrote:
> > On 01-03-21, 14:41, Jie Deng wrote:
> > > +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);
> > And this won't work for 10 bit addressing right? Don't we support that
> > in kernel ?
> > 
> >  From Spec:
> > 
> > \begin{tabular}{ |l||l|l|l|l|l|l|l|l|l|l|l|l|l|l|l|l| }
> > \hline
> > Bits           & 15 & 14 & 13 & 12 & 11 & 10 & 9  & 8  & 7  & 6  & 5  & 4  & 3  & 2  & 1  & 0 \\
> > \hline
> > 7-bit address  & 0  & 0  & 0  & 0  & 0  & 0  & 0  & 0  & A6 & A5 & A4 & A3 & A2 & A1 & A0 & 0 \\
> > \hline
> > 10-bit address & A7 & A6 & A5 & A4 & A3 & A2 & A1 & A0 & 1  & 1  & 1  & 1  & 0  & A9 & A8 & 0 \\
> > \hline
> > \end{tabular}
> Currently, to make things simple, this driver only supports 7 bit mode.
> It doesn't declare "I2C_FUNC_10BIT_ADDR" in the functionality.
> We may add in the future according to the need.

Okay, fair enough. Please add such details somewhere in the code so
people can understand it. You can very well add these at the top of
the file as well, in the general header.

-- 
viresh

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Jason Wang]

On 2021/3/2 1:16 下午, Viresh Kumar wrote:
> On 02-03-21, 13:06, Jie Deng wrote:
>> Yeah. Actually, the backend only needs "struct virtio_i2c_out_hdr out_hdr"
>> and "struct virtio_i2c_in_hdr in_hdr" for communication. So we only need to
>> keep
>> the first two in uapi and move "struct virtio_i2c_req" into the driver.
>>
>> But Jason wanted to include "struct virtio_i2c_req" in uapi. He explained in
>> this link
>> https://lists.linuxfoundation.org/pipermail/virtualization/2020-October/050222.html.
>> Do you agree with that explanation ?
> I am not sure I understood his reasoning well, but it doesn't make any
> sense to keep this in uapi header if this is never going to get
> transferred over the wire.


I think I was wrong. It should be sufficient have in_hdr and out_hdr in 
uAPI.

Thanks


>
> Moreover, the struct virtio_i2c_req in spec is misleading to me and
> rather creates unnecessary confusion. There is no structure like this
> which ever get passed here, but rather there are multiple vq
> transactions which take place, one with just the out header, then one
> with buffer and finally one with in header.
>
> I am not sure what's the right way of documenting it or if this is a
> standard virtio world follows.
>

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Jason Wang]

On 2021/3/2 1:16 下午, Viresh Kumar wrote:
> On 02-03-21, 13:06, Jie Deng wrote:
>> Yeah. Actually, the backend only needs "struct virtio_i2c_out_hdr out_hdr"
>> and "struct virtio_i2c_in_hdr in_hdr" for communication. So we only need to
>> keep
>> the first two in uapi and move "struct virtio_i2c_req" into the driver.
>>
>> But Jason wanted to include "struct virtio_i2c_req" in uapi. He explained in
>> this link
>> https://lists.linuxfoundation.org/pipermail/virtualization/2020-October/050222.html.
>> Do you agree with that explanation ?
> I am not sure I understood his reasoning well, but it doesn't make any
> sense to keep this in uapi header if this is never going to get
> transferred over the wire.


I think I was wrong. It should be sufficient have in_hdr and out_hdr in 
uAPI.

Thanks


>
> Moreover, the struct virtio_i2c_req in spec is misleading to me and
> rather creates unnecessary confusion. There is no structure like this
> which ever get passed here, but rather there are multiple vq
> transactions which take place, one with just the out header, then one
> with buffer and finally one with in header.
>
> I am not sure what's the right way of documenting it or if this is a
> standard virtio world follows.
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Jie Deng]

[-- Attachment #1.1: Type: text/plain, Size: 2014 bytes --]


On 2021/3/2 11:43, Viresh Kumar wrote:
> On 02-03-21, 10:21, Jie Deng wrote:
>> On 2021/3/1 19:54, Viresh Kumar wrote:
>> That's my original proposal. I used to mirror this interface with "struct
>> i2c_msg".
>>
>> But the design philosophy of virtio TC is that VIRTIO devices are not
>> specific to Linux
>> so the specs design should avoid the limitations of the current Linux driver
>> behavior.
> Right, I understand that.
>
>> We had some discussion about this. You may check these links to learn the
>> story.
>> https://lists.oasis-open.org/archives/virtio-comment/202010/msg00016.html
>> https://lists.oasis-open.org/archives/virtio-comment/202010/msg00033.html
>> https://lists.oasis-open.org/archives/virtio-comment/202011/msg00025.html
> So the thing is that we want to support full duplex mode, right ?
>
> How will that work protocol wise ? I mean how would we know if we are
> expecting both tx and rx buffers in a transfer ?
Not for the full duplex. As Paolo explained in those links.
We defined a combined request called "write-read request"

"
This is when a write is followed by a read: the master
starts off the transmission with a write, then sends a second START,
then continues with a read from the same address.

In theory there's no difference between one multi-segment transaction
and many single-segment transactions _in a single-master scenario_.

However, it is a plausible configuration to have multiple guests sharing
an I2C host device as if they were multiple master.

So the spec should provide a way at least to support for transactions with
1 write and 1 read segment in one request to the same address.

"

 From the perspective of specification design, it hopes to provide more 
choices
while from the perspective of specific implementation, we can choose 
what we need
as long as it does not violate the specification.

Since the current Linux driver doesn't use this mechanism. I'm 
considering to move
the "struct virtio_i2c_req" into the driver and use one "buf" instead.

[-- Attachment #1.2: Type: text/html, Size: 3731 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Jie Deng]

On 2021/3/2 11:43, Viresh Kumar wrote:
> On 02-03-21, 10:21, Jie Deng wrote:
>> On 2021/3/1 19:54, Viresh Kumar wrote:
>> That's my original proposal. I used to mirror this interface with "struct
>> i2c_msg".
>>
>> But the design philosophy of virtio TC is that VIRTIO devices are not
>> specific to Linux
>> so the specs design should avoid the limitations of the current Linux driver
>> behavior.
> Right, I understand that.
>
>> We had some discussion about this. You may check these links to learn the
>> story.
>> https://lists.oasis-open.org/archives/virtio-comment/202010/msg00016.html
>> https://lists.oasis-open.org/archives/virtio-comment/202010/msg00033.html
>> https://lists.oasis-open.org/archives/virtio-comment/202011/msg00025.html
> So the thing is that we want to support full duplex mode, right ?
>
> How will that work protocol wise ? I mean how would we know if we are
> expecting both tx and rx buffers in a transfer ?

Not for the full duplex. As Paolo explained in those links.
We defined a combined request called "write-read request"

"
This is when a write is followed by a read: the master
starts off the transmission with a write, then sends a second START,
then continues with a read from the same address.

In theory there's no difference between one multi-segment transaction
and many single-segment transactions _in a single-master scenario_.

However, it is a plausible configuration to have multiple guests sharing
an I2C host device as if they were multiple master.

So the spec should provide a way at least to support for transactions with
1 write and 1 read segment in one request to the same address.

"

 From the perspective of specification design, it hopes to provide more 
choices
while from the perspective of specific implementation, we can choose 
what we need
as long as it does not violate the specification.

Since the current Linux driver doesn't use this mechanism. I'm 
considering to move
the "struct virtio_i2c_req" into the driver and use one "buf" instead.

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Jie Deng]

On 2021/3/2 11:43, Viresh Kumar wrote:
> On 02-03-21, 10:21, Jie Deng wrote:
>> On 2021/3/1 19:54, Viresh Kumar wrote:
>> That's my original proposal. I used to mirror this interface with "struct
>> i2c_msg".
>>
>> But the design philosophy of virtio TC is that VIRTIO devices are not
>> specific to Linux
>> so the specs design should avoid the limitations of the current Linux driver
>> behavior.
> Right, I understand that.
>
>> We had some discussion about this. You may check these links to learn the
>> story.
>> https://lists.oasis-open.org/archives/virtio-comment/202010/msg00016.html
>> https://lists.oasis-open.org/archives/virtio-comment/202010/msg00033.html
>> https://lists.oasis-open.org/archives/virtio-comment/202011/msg00025.html
> So the thing is that we want to support full duplex mode, right ?
>
> How will that work protocol wise ? I mean how would we know if we are
> expecting both tx and rx buffers in a transfer ?

Not for the full duplex. As Paolo explained in those links.
We defined a combined request called "write-read request"

"
This is when a write is followed by a read: the master
starts off the transmission with a write, then sends a second START,
then continues with a read from the same address.

In theory there's no difference between one multi-segment transaction
and many single-segment transactions _in a single-master scenario_.

However, it is a plausible configuration to have multiple guests sharing
an I2C host device as if they were multiple master.

So the spec should provide a way at least to support for transactions with
1 write and 1 read segment in one request to the same address.

"

 From the perspective of specification design, it hopes to provide more 
choices
while from the perspective of specific implementation, we can choose 
what we need
as long as it does not violate the specification.

Since the current Linux driver doesn't use this mechanism. I'm 
considering to move
the "struct virtio_i2c_req" into the driver and use one "buf" instead.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Jie Deng]

On 2021/3/1 20:07, Andy Shevchenko wrote:
> On Mon, Mar 01, 2021 at 02:41:35PM +0800, Jie Deng wrote:
>> 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.
> ...
>
>> +		buf = kzalloc(msgs[i].len, GFP_KERNEL);
>> +		if (!buf)
>> +			break;
>> +
>> +		if (msgs[i].flags & I2C_M_RD) {
> kzalloc()
>
>> +			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);
> kmemdup() ?
Do you mean using "kzalloc" in the if condition and "kmemdup" in the 
else condition ?
Then we have to check the NULL twice which is also not good.
>> +			sg_init_one(&msg_buf, reqs[i].write_buf, msgs[i].len);
>> +			sgs[outcnt++] = &msg_buf;
>> +		}
> ...
>
>> +
>> +
> One blank line is enough.
Will fix it. Thank you.
> ...
>
>
>> +	ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
>> +	if (ret == 0)
>> +		goto err_unlock_free;
>> +	else
> Redundant.
Good catch !
>> +		nr = ret;

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Jie Deng]

On 2021/3/1 20:07, Andy Shevchenko wrote:
> On Mon, Mar 01, 2021 at 02:41:35PM +0800, Jie Deng wrote:
>> 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.
> ...
>
>> +		buf = kzalloc(msgs[i].len, GFP_KERNEL);
>> +		if (!buf)
>> +			break;
>> +
>> +		if (msgs[i].flags & I2C_M_RD) {
> kzalloc()
>
>> +			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);
> kmemdup() ?
Do you mean using "kzalloc" in the if condition and "kmemdup" in the 
else condition ?
Then we have to check the NULL twice which is also not good.
>> +			sg_init_one(&msg_buf, reqs[i].write_buf, msgs[i].len);
>> +			sgs[outcnt++] = &msg_buf;
>> +		}
> ...
>
>> +
>> +
> One blank line is enough.
Will fix it. Thank you.
> ...
>
>
>> +	ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
>> +	if (ret == 0)
>> +		goto err_unlock_free;
>> +	else
> Redundant.
Good catch !
>> +		nr = ret;
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Viresh Kumar]

On 02-03-21, 14:24, Jie Deng wrote:
> Not for the full duplex. As Paolo explained in those links.
> We defined a combined request called "write-read request"
> 
> "
> This is when a write is followed by a read: the master
> starts off the transmission with a write, then sends a second START,
> then continues with a read from the same address.

I think above talks about the real I2C protocol at bus level ?

> In theory there's no difference between one multi-segment transaction
> and many single-segment transactions _in a single-master scenario_.
> 
> However, it is a plausible configuration to have multiple guests sharing
> an I2C host device as if they were multiple master.
> 
> So the spec should provide a way at least to support for transactions with
> 1 write and 1 read segment in one request to the same address.
> "

> From the perspective of specification design, it hopes to provide more
> choices
> while from the perspective of specific implementation, we can choose what we
> need
> as long as it does not violate the specification.

That is fine, but what I was not able to understand was how do we get
to know if we should expect both write and read bufs after the out
header or only one of them ?

I think I have understood that part now, we need to look at incnt and
outcnt and make out what kind of transfer we need to do.

- If outcnt == 1 and incnt == 2, then it is read operation.
- If outcnt == 2 and incnt == 1, then it is write operation.
- If outcnt == 2 and incnt == 2, then it is read-write operation.

Anything else is invalid. Is my understanding correct here ?

> Since the current Linux driver doesn't use this mechanism. I'm considering
> to move
> the "struct virtio_i2c_req" into the driver and use one "buf" instead.

Linux can very much have its own definition of the structure and so I
am not in favor of any such structure in the spec as well, it confuses
people (like me) :).

-- 
viresh

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Jie Deng]

On 2021/3/2 15:24, Viresh Kumar wrote:
> On 02-03-21, 14:24, Jie Deng wrote:
>> Not for the full duplex. As Paolo explained in those links.
>> We defined a combined request called "write-read request"
>>
>> "
>> This is when a write is followed by a read: the master
>> starts off the transmission with a write, then sends a second START,
>> then continues with a read from the same address.
> I think above talks about the real I2C protocol at bus level ?
>
>> In theory there's no difference between one multi-segment transaction
>> and many single-segment transactions _in a single-master scenario_.
>>
>> However, it is a plausible configuration to have multiple guests sharing
>> an I2C host device as if they were multiple master.
>>
>> So the spec should provide a way at least to support for transactions with
>> 1 write and 1 read segment in one request to the same address.
>> "
>>  From the perspective of specification design, it hopes to provide more
>> choices
>> while from the perspective of specific implementation, we can choose what we
>> need
>> as long as it does not violate the specification.
> That is fine, but what I was not able to understand was how do we get
> to know if we should expect both write and read bufs after the out
> header or only one of them ?
>
> I think I have understood that part now, we need to look at incnt and
> outcnt and make out what kind of transfer we need to do.
>
> - If outcnt == 1 and incnt == 2, then it is read operation.
> - If outcnt == 2 and incnt == 1, then it is write operation.
> - If outcnt == 2 and incnt == 2, then it is read-write operation.
>
> Anything else is invalid. Is my understanding correct here ?
Correct.  By checking the sequences of descriptor's R/W in the virtqueue,
You can know the type of request. A simple state machine can be used to
classify in this case.

O I  I   : read request.

O O I  : write request.

O O I I : read-write request.

But if only using the first two types like in this driver, the backend 
will be much easier to
implement since the request is fixed to 3 descriptors and we only need 
to check the second
descriptor to know the type.
>
>> Since the current Linux driver doesn't use this mechanism. I'm considering
>> to move
>> the "struct virtio_i2c_req" into the driver and use one "buf" instead.
> Linux can very much have its own definition of the structure and so I
> am not in favor of any such structure in the spec as well, it confuses
> people (like me) :).
>

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Jie Deng]

On 2021/3/2 15:24, Viresh Kumar wrote:
> On 02-03-21, 14:24, Jie Deng wrote:
>> Not for the full duplex. As Paolo explained in those links.
>> We defined a combined request called "write-read request"
>>
>> "
>> This is when a write is followed by a read: the master
>> starts off the transmission with a write, then sends a second START,
>> then continues with a read from the same address.
> I think above talks about the real I2C protocol at bus level ?
>
>> In theory there's no difference between one multi-segment transaction
>> and many single-segment transactions _in a single-master scenario_.
>>
>> However, it is a plausible configuration to have multiple guests sharing
>> an I2C host device as if they were multiple master.
>>
>> So the spec should provide a way at least to support for transactions with
>> 1 write and 1 read segment in one request to the same address.
>> "
>>  From the perspective of specification design, it hopes to provide more
>> choices
>> while from the perspective of specific implementation, we can choose what we
>> need
>> as long as it does not violate the specification.
> That is fine, but what I was not able to understand was how do we get
> to know if we should expect both write and read bufs after the out
> header or only one of them ?
>
> I think I have understood that part now, we need to look at incnt and
> outcnt and make out what kind of transfer we need to do.
>
> - If outcnt == 1 and incnt == 2, then it is read operation.
> - If outcnt == 2 and incnt == 1, then it is write operation.
> - If outcnt == 2 and incnt == 2, then it is read-write operation.
>
> Anything else is invalid. Is my understanding correct here ?
Correct.  By checking the sequences of descriptor's R/W in the virtqueue,
You can know the type of request. A simple state machine can be used to
classify in this case.

O I  I   : read request.

O O I  : write request.

O O I I : read-write request.

But if only using the first two types like in this driver, the backend 
will be much easier to
implement since the request is fixed to 3 descriptors and we only need 
to check the second
descriptor to know the type.
>
>> Since the current Linux driver doesn't use this mechanism. I'm considering
>> to move
>> the "struct virtio_i2c_req" into the driver and use one "buf" instead.
> Linux can very much have its own definition of the structure and so I
> am not in favor of any such structure in the spec as well, it confuses
> people (like me) :).
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2441 bytes --]

On Tue, Mar 02, 2021 at 10:42:06AM +0800, Jie Deng wrote:
> 
> On 2021/3/1 23:19, Arnd Bergmann wrote:
> > On Mon, Mar 1, 2021 at 7:41 AM Jie Deng <jie.deng@intel.com> 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 */

The uapi VIRTIO header files are BSD licensed so they can be easily used
by other projects (including other operating systems and hypervisors
that don't use Linux). Please see the license headers in
<linux/virtio_net.h> or <linux/virtio_blk.h>.

> > > +/*
> > > + * 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?

Linux VIRTIO drivers provide a uapi header with structs and constants
that describe the device interface. This allows other software like
QEMU, other operating systems, etc to reuse these headers instead of
redefining everything.

These files should contain:
1. Device-specific feature bits (VIRTIO_<device>_F_<feature>)
2. VIRTIO Configuration Space layout (struct virtio_<device>_config)
3. Virtqueue request layout (struct virtio_<device>_<req/resp>)

For examples, see <linux/virtio_net.h> and <linux/virtio_blk.h>.

> > > +/**
> > > + * 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.

I agree. This struct is not part of the device interface. It's part of
the Linux driver implementation. This belongs inside the driver code and
not in include/uapi/ where public headers are located.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Stefan Hajnoczi]

[-- Attachment #1.1: Type: text/plain, Size: 2441 bytes --]

On Tue, Mar 02, 2021 at 10:42:06AM +0800, Jie Deng wrote:
> 
> On 2021/3/1 23:19, Arnd Bergmann wrote:
> > On Mon, Mar 1, 2021 at 7:41 AM Jie Deng <jie.deng@intel.com> 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 */

The uapi VIRTIO header files are BSD licensed so they can be easily used
by other projects (including other operating systems and hypervisors
that don't use Linux). Please see the license headers in
<linux/virtio_net.h> or <linux/virtio_blk.h>.

> > > +/*
> > > + * 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?

Linux VIRTIO drivers provide a uapi header with structs and constants
that describe the device interface. This allows other software like
QEMU, other operating systems, etc to reuse these headers instead of
redefining everything.

These files should contain:
1. Device-specific feature bits (VIRTIO_<device>_F_<feature>)
2. VIRTIO Configuration Space layout (struct virtio_<device>_config)
3. Virtqueue request layout (struct virtio_<device>_<req/resp>)

For examples, see <linux/virtio_net.h> and <linux/virtio_blk.h>.

> > > +/**
> > > + * 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.

I agree. This struct is not part of the device interface. It's part of
the Linux driver implementation. This belongs inside the driver code and
not in include/uapi/ where public headers are located.

Stefan

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Arnd Bergmann]

On Tue, Mar 2, 2021 at 10:51 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Tue, Mar 02, 2021 at 10:42:06AM +0800, Jie Deng wrote:
> > > > +/*
> > > > + * 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?
>
> Linux VIRTIO drivers provide a uapi header with structs and constants
> that describe the device interface. This allows other software like
> QEMU, other operating systems, etc to reuse these headers instead of
> redefining everything.
>
> These files should contain:
> 1. Device-specific feature bits (VIRTIO_<device>_F_<feature>)
> 2. VIRTIO Configuration Space layout (struct virtio_<device>_config)
> 3. Virtqueue request layout (struct virtio_<device>_<req/resp>)
>
> For examples, see <linux/virtio_net.h> and <linux/virtio_blk.h>.

Ok, makes sense. So it's not strictly uapi but just helpful for
virtio applications to reference these. I suppose it is slightly harder
when building qemu on other operating systems though, how does
it get these headers on BSD or Windows?

       Arnd

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Arnd Bergmann]

On Tue, Mar 2, 2021 at 10:51 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Tue, Mar 02, 2021 at 10:42:06AM +0800, Jie Deng wrote:
> > > > +/*
> > > > + * 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?
>
> Linux VIRTIO drivers provide a uapi header with structs and constants
> that describe the device interface. This allows other software like
> QEMU, other operating systems, etc to reuse these headers instead of
> redefining everything.
>
> These files should contain:
> 1. Device-specific feature bits (VIRTIO_<device>_F_<feature>)
> 2. VIRTIO Configuration Space layout (struct virtio_<device>_config)
> 3. Virtqueue request layout (struct virtio_<device>_<req/resp>)
>
> For examples, see <linux/virtio_net.h> and <linux/virtio_blk.h>.

Ok, makes sense. So it's not strictly uapi but just helpful for
virtio applications to reference these. I suppose it is slightly harder
when building qemu on other operating systems though, how does
it get these headers on BSD or Windows?

       Arnd
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Viresh Kumar]

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

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Jie Deng]

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.

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Jie Deng]

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.

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Viresh Kumar]

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

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1600 bytes --]

On Tue, Mar 02, 2021 at 11:54:02AM +0100, Arnd Bergmann wrote:
> On Tue, Mar 2, 2021 at 10:51 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > On Tue, Mar 02, 2021 at 10:42:06AM +0800, Jie Deng wrote:
> > > > > +/*
> > > > > + * 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?
> >
> > Linux VIRTIO drivers provide a uapi header with structs and constants
> > that describe the device interface. This allows other software like
> > QEMU, other operating systems, etc to reuse these headers instead of
> > redefining everything.
> >
> > These files should contain:
> > 1. Device-specific feature bits (VIRTIO_<device>_F_<feature>)
> > 2. VIRTIO Configuration Space layout (struct virtio_<device>_config)
> > 3. Virtqueue request layout (struct virtio_<device>_<req/resp>)
> >
> > For examples, see <linux/virtio_net.h> and <linux/virtio_blk.h>.
> 
> Ok, makes sense. So it's not strictly uapi but just helpful for
> virtio applications to reference these. I suppose it is slightly harder
> when building qemu on other operating systems though, how does
> it get these headers on BSD or Windows?

qemu.git imports Linux headers from time to time and can use them
instead of system headers:

  https://gitlab.com/qemu-project/qemu/-/blob/master/scripts/update-linux-headers.sh

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Stefan Hajnoczi]

[-- Attachment #1.1: Type: text/plain, Size: 1600 bytes --]

On Tue, Mar 02, 2021 at 11:54:02AM +0100, Arnd Bergmann wrote:
> On Tue, Mar 2, 2021 at 10:51 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > On Tue, Mar 02, 2021 at 10:42:06AM +0800, Jie Deng wrote:
> > > > > +/*
> > > > > + * 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?
> >
> > Linux VIRTIO drivers provide a uapi header with structs and constants
> > that describe the device interface. This allows other software like
> > QEMU, other operating systems, etc to reuse these headers instead of
> > redefining everything.
> >
> > These files should contain:
> > 1. Device-specific feature bits (VIRTIO_<device>_F_<feature>)
> > 2. VIRTIO Configuration Space layout (struct virtio_<device>_config)
> > 3. Virtqueue request layout (struct virtio_<device>_<req/resp>)
> >
> > For examples, see <linux/virtio_net.h> and <linux/virtio_blk.h>.
> 
> Ok, makes sense. So it's not strictly uapi but just helpful for
> virtio applications to reference these. I suppose it is slightly harder
> when building qemu on other operating systems though, how does
> it get these headers on BSD or Windows?

qemu.git imports Linux headers from time to time and can use them
instead of system headers:

  https://gitlab.com/qemu-project/qemu/-/blob/master/scripts/update-linux-headers.sh

Stefan

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Jie Deng]

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 virtio-comment@lists.oasis-open.org. Once your Spec patch is 
acked by the
virtio TC, you can then send patches to update the corresponding drivers.

Thanks.

Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Jie Deng]

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 virtio-comment@lists.oasis-open.org. Once your Spec patch is 
acked by the
virtio TC, you can then send patches to update the corresponding drivers.

Thanks.

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization