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

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 <redacted>
Signed-off-by: Conghui Chen <redacted>
Signed-off-by: Jie Deng <redacted>
---
Changes v11 -> v12
	- Do not sent msg_buf for zero-length request.
	- Send requests to host only if all the number of transfers requested prepared successfully.
	- Remove the line #include <linux/bits.h> in virtio_i2c.h

Changes v10 -> v11
	- Remove vi->adap.class = I2C_CLASS_DEPRECATED.
	- Use #ifdef CONFIG_PM_SLEEP to replace the "__maybe_unused".
	- Remove "struct mutex lock" in "struct virtio_i2c".
	- Support zero-length request.
	- Remove unnecessary logs.
	- Remove vi->adap.timeout = HZ / 10, just use the default value.
	- Use BIT(0) to define VIRTIO_I2C_FLAGS_FAIL_NEXT.
	- Add the virtio_device index to adapter's naming mechanism.

 drivers/i2c/busses/Kconfig      |  11 ++
 drivers/i2c/busses/Makefile     |   3 +
 drivers/i2c/busses/i2c-virtio.c | 269 ++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/virtio_i2c.h |  40 ++++++
 include/uapi/linux/virtio_ids.h |   1 +
 5 files changed, 324 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 10acece..e47616a 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"
+	select 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 69e9963..9843756 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile

@@ -147,4 +147,7 @@ obj-$(CONFIG_I2C_XGENE_SLIMPRO) += i2c-xgene-slimpro.o
 obj-$(CONFIG_SCx200_ACB)	+= scx200_acb.o
 obj-$(CONFIG_I2C_FSI)		+= i2c-fsi.o
 
+# VIRTIO I2C host controller driver
+obj-$(CONFIG_I2C_VIRTIO)	+= i2c-virtio.o
+
 ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG

diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
new file mode 100644
index 0000000..45746b5
--- /dev/null
+++ b/drivers/i2c/busses/i2c-virtio.c

@@ -0,0 +1,269 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Virtio I2C Bus Driver
+ *
+ * The Virtio I2C Specification:
+ * https://raw.githubusercontent.com/oasis-tcs/virtio-spec/master/virtio-i2c.tex
+ *
+ * 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/kernel.h>
+#include <linux/module.h>
+#include <linux/virtio.h>
+#include <linux/virtio_ids.h>
+#include <linux/virtio_config.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
+ * @vq: the virtio virtqueue for communication
+ */
+struct virtio_i2c {
+	struct virtio_device *vdev;
+	struct completion completion;
+	struct i2c_adapter adap;
+	struct virtqueue *vq;
+};
+
+/**
+ * struct virtio_i2c_req - the virtio I2C request structure
+ * @out_hdr: the OUT header of the virtio I2C message
+ * @buf: the buffer into which data is read, or from which it's written
+ * @in_hdr: the IN header of the virtio I2C message
+ */
+struct virtio_i2c_req {
+	struct virtio_i2c_out_hdr out_hdr	____cacheline_aligned;
+	uint8_t *buf				____cacheline_aligned;
+	struct virtio_i2c_in_hdr in_hdr		____cacheline_aligned;
+};
+
+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;
+
+	for (i = 0; i < nr; i++) {
+		/*
+		 * Only 7-bit mode supported for this moment. For the address format,
+		 * Please check the Virtio I2C Specification.
+		 */
+		reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1);
+
+		if (i != nr - 1)
+			reqs[i].out_hdr.flags = cpu_to_le32(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;
+
+		if (msgs[i].len) {
+			reqs[i].buf = i2c_get_dma_safe_msg_buf(&msgs[i], 1);
+			if (!reqs[i].buf)
+				break;
+
+			sg_init_one(&msg_buf, reqs[i].buf, msgs[i].len);
+
+			if (msgs[i].flags & I2C_M_RD)
+				sgs[outcnt + incnt++] = &msg_buf;
+			else
+				sgs[outcnt++] = &msg_buf;
+		}
+
+		sg_init_one(&in_hdr, &reqs[i].in_hdr, sizeof(reqs[i].in_hdr));
+		sgs[outcnt + incnt++] = &in_hdr;
+
+		err = virtqueue_add_sgs(vq, sgs, outcnt, incnt, &reqs[i], GFP_KERNEL);
+		if (err < 0) {
+			i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], false);
+			break;
+		}
+	}
+
+	return i;
+}
+
+static int virtio_i2c_complete_reqs(struct virtqueue *vq,
+				    struct virtio_i2c_req *reqs,
+				    struct i2c_msg *msgs, int nr,
+				    bool fail)
+{
+	struct virtio_i2c_req *req;
+	bool failed = fail;
+	unsigned int len;
+	int i, j = 0;
+
+	for (i = 0; i < nr; i++) {
+		/* Detach the ith request from the vq */
+		req = virtqueue_get_buf(vq, &len);
+
+		/*
+		 * Condition (req && req == &reqs[i]) should always meet since
+		 * we have total nr requests in the vq.
+		 */
+		if (!failed && (WARN_ON(!(req && req == &reqs[i])) ||
+		    (req->in_hdr.status != VIRTIO_I2C_MSG_OK)))
+			failed = true;
+
+		i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], !failed);
+		if (!failed)
+			++j;
+	}
+
+	return (fail ? -ETIMEDOUT : j);
+}
+
+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;
+
+	reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL);
+	if (!reqs)
+		return -ENOMEM;
+
+	ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
+	if (ret != num) {
+		virtio_i2c_complete_reqs(vq, reqs, msgs, ret, true);
+		ret = 0;
+		goto err_free;
+	}
+
+	reinit_completion(&vi->completion);
+	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 = virtio_i2c_complete_reqs(vq, reqs, msgs, num, !time_left);
+
+err_free:
+	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 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;
+
+	init_completion(&vi->completion);
+
+	ret = virtio_i2c_setup_vqs(vi);
+	if (ret)
+		return ret;
+
+	vi->adap.owner = THIS_MODULE;
+	snprintf(vi->adap.name, sizeof(vi->adap.name),
+		 "i2c_virtio at virtio bus %d", vdev->index);
+	vi->adap.algo = &virtio_algorithm;
+	vi->adap.dev.parent = &vdev->dev;
+	i2c_set_adapdata(&vi->adap, vi);
+
+	/* Setup ACPI node for controlled devices which will be probed through ACPI */
+	ACPI_COMPANION_SET(&vi->adap.dev, ACPI_COMPANION(pdev));
+
+	ret = i2c_add_adapter(&vi->adap);
+	if (ret)
+		virtio_i2c_del_vqs(vdev);
+
+	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_ADAPTER, VIRTIO_DEV_ANY_ID },
+	{}
+};
+MODULE_DEVICE_TABLE(virtio, id_table);
+
+#ifdef CONFIG_PM_SLEEP
+static int virtio_i2c_freeze(struct virtio_device *vdev)
+{
+	virtio_i2c_del_vqs(vdev);
+	return 0;
+}
+
+static int virtio_i2c_restore(struct virtio_device *vdev)
+{
+	return virtio_i2c_setup_vqs(vdev->priv);
+}
+#endif
+
+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_AUTHOR("Jie Deng <jie.deng@intel.com>");
+MODULE_AUTHOR("Conghui Chen <conghui.chen@intel.com>");
+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..1b1754e
--- /dev/null
+++ b/include/uapi/linux/virtio_i2c.h

@@ -0,0 +1,40 @@
+/* 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>
+
+/* The bit 0 of the @virtio_i2c_out_hdr.@flags, used to group the requests */
+#define VIRTIO_I2C_FLAGS_FAIL_NEXT	BIT(0)
+
+/**
+ * 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;
+};
+
+/**
+ * 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
+
+#endif /* _UAPI_LINUX_VIRTIO_I2C_H */

diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
index 4fe842c..3b5f05a 100644
--- a/include/uapi/linux/virtio_ids.h
+++ b/include/uapi/linux/virtio_ids.h

@@ -55,6 +55,7 @@
 #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_ADAPTER		34 /* virtio i2c adapter */
 #define VIRTIO_ID_BT			40 /* virtio bluetooth */
 
 #endif /* _LINUX_VIRTIO_IDS_H */

-- 
2.7.4

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

On Fri, Jul 02, 2021 at 04:46:47PM +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.

...

+static int virtio_i2c_complete_reqs(struct virtqueue *vq,
+				    struct virtio_i2c_req *reqs,
+				    struct i2c_msg *msgs, int nr,
+				    bool fail)
+{
+	struct virtio_i2c_req *req;
+	bool failed = fail;
+	unsigned int len;
+	int i, j = 0;
+
+	for (i = 0; i < nr; i++) {
+		/* Detach the ith request from the vq */
+		req = virtqueue_get_buf(vq, &len);
+
+		/*
+		 * Condition (req && req == &reqs[i]) should always meet since
+		 * we have total nr requests in the vq.
+		 */
+		if (!failed && (WARN_ON(!(req && req == &reqs[i])) ||
+		    (req->in_hdr.status != VIRTIO_I2C_MSG_OK)))
+			failed = true;

...and after failed is true, we are continuing the loop, why?

+		i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], !failed);
+		if (!failed)

+			++j;

Besides better to read j++ the j itself can be renamed to something more
verbose.

+	}

+	return (fail ? -ETIMEDOUT : j);

Redundant parentheses.

+}

...

+	ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
+	if (ret != num) {
+		virtio_i2c_complete_reqs(vq, reqs, msgs, ret, true);

Below you check the returned code, here is not.

+		ret = 0;
+		goto err_free;
+	}
+
+	reinit_completion(&vi->completion);
+	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 = virtio_i2c_complete_reqs(vq, reqs, msgs, num, !time_left);
+
+err_free:
+	kfree(reqs);
+	return ret;

+#include <linux/types.h>
+
+/* The bit 0 of the @virtio_i2c_out_hdr.@flags, used to group the requests */
+#define VIRTIO_I2C_FLAGS_FAIL_NEXT	BIT(0)

It's _BITUL() or so from linux/const.h.
https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/const.h#L28
You may not use internal definitions in UAPI headers.

-- 
With Best Regards,
Andy Shevchenko


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

I think we missed the first deadline for 5.14-rc1 as Wolfram should be out of
office now. Anyway lets make sure we fix all the pending bits before he is back
and see if we can still pull it off by rc2.

On 02-07-21, 16:46, Jie Deng wrote:

It would be better to rename this to virtio_i2c_prepare_reqs instead, as this
doesn't send anything.

+				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;
+
+	for (i = 0; i < nr; i++) {
+		/*
+		 * Only 7-bit mode supported for this moment. For the address format,
+		 * Please check the Virtio I2C Specification.
+		 */
+		reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1);
+
+		if (i != nr - 1)
+			reqs[i].out_hdr.flags = cpu_to_le32(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;
+
+		if (msgs[i].len) {
+			reqs[i].buf = i2c_get_dma_safe_msg_buf(&msgs[i], 1);
+			if (!reqs[i].buf)
+				break;
+
+			sg_init_one(&msg_buf, reqs[i].buf, msgs[i].len);
+
+			if (msgs[i].flags & I2C_M_RD)
+				sgs[outcnt + incnt++] = &msg_buf;
+			else
+				sgs[outcnt++] = &msg_buf;
+		}
+
+		sg_init_one(&in_hdr, &reqs[i].in_hdr, sizeof(reqs[i].in_hdr));
+		sgs[outcnt + incnt++] = &in_hdr;
+
+		err = virtqueue_add_sgs(vq, sgs, outcnt, incnt, &reqs[i], GFP_KERNEL);
+		if (err < 0) {
+			i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], false);
+			break;
+		}
+	}
+
+	return i;
+}

The right way of doing this is is making this function return - Error on failure
and 0 on success. There is no point returning number of successful additions
here.

Moreover, on failures this needs to clean up (free the dmabufs) itself, just
like you did i2c_put_dma_safe_msg_buf() at the end. The caller shouldn't be
required to handle the error cases by freeing up resources.

+static int virtio_i2c_complete_reqs(struct virtqueue *vq,
+				    struct virtio_i2c_req *reqs,
+				    struct i2c_msg *msgs, int nr,
+				    bool fail)
+{
+	struct virtio_i2c_req *req;
+	bool failed = fail;
+	unsigned int len;
+	int i, j = 0;
+
+	for (i = 0; i < nr; i++) {
+		/* Detach the ith request from the vq */
+		req = virtqueue_get_buf(vq, &len);
+
+		/*
+		 * Condition (req && req == &reqs[i]) should always meet since
+		 * we have total nr requests in the vq.
+		 */
+		if (!failed && (WARN_ON(!(req && req == &reqs[i])) ||
+		    (req->in_hdr.status != VIRTIO_I2C_MSG_OK)))

What about writing this as:

		if (!failed && (WARN_ON(req != &reqs[i]) ||
		    (req->in_hdr.status != VIRTIO_I2C_MSG_OK)))

We don't need to check req here since if req is NULL, we will not do req->in_hdr
at all.

+			failed = true;
+
+		i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], !failed);
+		if (!failed)
+			++j;
+	}
+
+	return (fail ? -ETIMEDOUT : j);
+}
+

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

On 02-07-21, 12:58, Andy Shevchenko wrote:

On Fri, Jul 02, 2021 at 04:46:47PM +0800, Jie Deng wrote:

quoted

+static int virtio_i2c_complete_reqs(struct virtqueue *vq,
+				    struct virtio_i2c_req *reqs,
+				    struct i2c_msg *msgs, int nr,
+				    bool fail)
+{
+	struct virtio_i2c_req *req;
+	bool failed = fail;

Jie, you can actually get rid of this variable too. Jut rename fail to failed
and everything shall work as you want.

quoted

+	unsigned int len;
+	int i, j = 0;
+
+	for (i = 0; i < nr; i++) {
+		/* Detach the ith request from the vq */
+		req = virtqueue_get_buf(vq, &len);
+
+		/*
+		 * Condition (req && req == &reqs[i]) should always meet since
+		 * we have total nr requests in the vq.
+		 */
+		if (!failed && (WARN_ON(!(req && req == &reqs[i])) ||
+		    (req->in_hdr.status != VIRTIO_I2C_MSG_OK)))
+			failed = true;

...and after failed is true, we are continuing the loop, why?

Actually this function can be called with fail set to true. We proceed as we
need to call i2c_put_dma_safe_msg_buf() for all buffers we allocated earlier.

quoted

+		i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], !failed);
+		if (!failed)

quoted

+			++j;

Besides better to read j++ the j itself can be renamed to something more
verbose.

quoted

+	}

quoted

+	return (fail ? -ETIMEDOUT : j);

Redundant parentheses.

quoted

+}

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

On 2021/7/5 10:43, Viresh Kumar wrote:

On 02-07-21, 12:58, Andy Shevchenko wrote:

quoted

On Fri, Jul 02, 2021 at 04:46:47PM +0800, Jie Deng wrote:

quoted

+static int virtio_i2c_complete_reqs(struct virtqueue *vq,
+				    struct virtio_i2c_req *reqs,
+				    struct i2c_msg *msgs, int nr,
+				    bool fail)
+{
+	struct virtio_i2c_req *req;
+	bool failed = fail;

Jie, you can actually get rid of this variable too. Jut rename fail to failed
and everything shall work as you want.

Sure.

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

On 2021/7/5 10:40, Viresh Kumar wrote:

I think we missed the first deadline for 5.14-rc1 as Wolfram should be out of
office now. Anyway lets make sure we fix all the pending bits before he is back
and see if we can still pull it off by rc2.

On 02-07-21, 16:46, Jie Deng wrote:

quoted

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,

It would be better to rename this to virtio_i2c_prepare_reqs instead, as this
doesn't send anything.

That's a better name. I'm OK with that.

quoted

+				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;
+
+	for (i = 0; i < nr; i++) {
+		/*
+		 * Only 7-bit mode supported for this moment. For the address format,
+		 * Please check the Virtio I2C Specification.
+		 */
+		reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1);
+
+		if (i != nr - 1)
+			reqs[i].out_hdr.flags = cpu_to_le32(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;
+
+		if (msgs[i].len) {
+			reqs[i].buf = i2c_get_dma_safe_msg_buf(&msgs[i], 1);
+			if (!reqs[i].buf)
+				break;
+
+			sg_init_one(&msg_buf, reqs[i].buf, msgs[i].len);
+
+			if (msgs[i].flags & I2C_M_RD)
+				sgs[outcnt + incnt++] = &msg_buf;
+			else
+				sgs[outcnt++] = &msg_buf;
+		}
+
+		sg_init_one(&in_hdr, &reqs[i].in_hdr, sizeof(reqs[i].in_hdr));
+		sgs[outcnt + incnt++] = &in_hdr;
+
+		err = virtqueue_add_sgs(vq, sgs, outcnt, incnt, &reqs[i], GFP_KERNEL);
+		if (err < 0) {
+			i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], false);
+			break;
+		}
+	}
+
+	return i;
+}

The right way of doing this is is making this function return - Error on failure
and 0 on success. There is no point returning number of successful additions
here.

We need the number for virtio_i2c_complete_reqs to do cleanup. We don't 
have to

do cleanup "num" times every time. Just do it as needed.

Moreover, on failures this needs to clean up (free the dmabufs) itself, just
like you did i2c_put_dma_safe_msg_buf() at the end. The caller shouldn't be
required to handle the error cases by freeing up resources.

This function will return the number of requests being successfully 
prepared and make sure

resources of the failed request being freed. And 
virtio_i2c_complete_reqs will free the

resources of those successful request.

quoted

+static int virtio_i2c_complete_reqs(struct virtqueue *vq,
+				    struct virtio_i2c_req *reqs,
+				    struct i2c_msg *msgs, int nr,
+				    bool fail)
+{
+	struct virtio_i2c_req *req;
+	bool failed = fail;
+	unsigned int len;
+	int i, j = 0;
+
+	for (i = 0; i < nr; i++) {
+		/* Detach the ith request from the vq */
+		req = virtqueue_get_buf(vq, &len);
+
+		/*
+		 * Condition (req && req == &reqs[i]) should always meet since
+		 * we have total nr requests in the vq.
+		 */
+		if (!failed && (WARN_ON(!(req && req == &reqs[i])) ||
+		    (req->in_hdr.status != VIRTIO_I2C_MSG_OK)))

What about writing this as:

		if (!failed && (WARN_ON(req != &reqs[i]) ||
		    (req->in_hdr.status != VIRTIO_I2C_MSG_OK)))

We don't need to check req here since if req is NULL, we will not do req->in_hdr
at all.

It's right here just because the &reqs[i] will never be NULL in our 
case. But if you see

"virtio_i2c_complete_reqs" as an independent function, you need to check the

req. From the perspective of the callee, you can't ask the caller always 
give you

the non-NULL parameters. And some tools may give warnings.

quoted

+			failed = true;
+
+		i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], !failed);
+		if (!failed)
+			++j;
+	}
+
+	return (fail ? -ETIMEDOUT : j);
+}
+

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

On 2021/7/2 17:58, Andy Shevchenko wrote:

On Fri, Jul 02, 2021 at 04:46:47PM +0800, Jie Deng wrote:

quoted

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.

...

quoted

+static int virtio_i2c_complete_reqs(struct virtqueue *vq,
+				    struct virtio_i2c_req *reqs,
+				    struct i2c_msg *msgs, int nr,
+				    bool fail)
+{
+	struct virtio_i2c_req *req;
+	bool failed = fail;
+	unsigned int len;
+	int i, j = 0;
+
+	for (i = 0; i < nr; i++) {
+		/* Detach the ith request from the vq */
+		req = virtqueue_get_buf(vq, &len);
+
+		/*
+		 * Condition (req && req == &reqs[i]) should always meet since
+		 * we have total nr requests in the vq.
+		 */
+		if (!failed && (WARN_ON(!(req && req == &reqs[i])) ||
+		    (req->in_hdr.status != VIRTIO_I2C_MSG_OK)))
+			failed = true;

...and after failed is true, we are continuing the loop, why?

Still need to continue to do some cleanup.

quoted

+		i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], !failed);
+		if (!failed)
+			++j;

Besides better to read j++ the j itself can be renamed to something more
verbose.

Will change it to j++.

quoted

+	}
+	return (fail ? -ETIMEDOUT : j);

Redundant parentheses.

Will remove the parentheses.

quoted

+}

...

quoted

+	ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
+	if (ret != num) {
+		virtio_i2c_complete_reqs(vq, reqs, msgs, ret, true);

Below you check the returned code, here is not.

I will do some optimization here.

quoted

+		ret = 0;
+		goto err_free;
+	}
+
+	reinit_completion(&vi->completion);
+	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 = virtio_i2c_complete_reqs(vq, reqs, msgs, num, !time_left);
+
+err_free:
+	kfree(reqs);
+	return ret;

+++ b/include/uapi/linux/virtio_i2c.h
+#include <linux/types.h>
+
+/* The bit 0 of the @virtio_i2c_out_hdr.@flags, used to group the requests */
+#define VIRTIO_I2C_FLAGS_FAIL_NEXT	BIT(0)

It's _BITUL() or so from linux/const.h.
https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/const.h#L28
You may not use internal definitions in UAPI headers.

Let's use this _BITUL() from linux/const.h instead. Thank you !


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

On 05-07-21, 11:45, Jie Deng wrote:

On 2021/7/5 10:40, Viresh Kumar wrote:

quoted

On 02-07-21, 16:46, Jie Deng wrote:
The right way of doing this is is making this function return - Error on failure
and 0 on success. There is no point returning number of successful additions
here.

We need the number for virtio_i2c_complete_reqs to do cleanup. We don't have
to

do cleanup "num" times every time. Just do it as needed.

If you do full cleanup here, then you won't required that at the caller site.

quoted

Moreover, on failures this needs to clean up (free the dmabufs) itself, just
like you did i2c_put_dma_safe_msg_buf() at the end. The caller shouldn't be
required to handle the error cases by freeing up resources.

This function will return the number of requests being successfully prepared
and make sure

resources of the failed request being freed. And virtio_i2c_complete_reqs
will free the

resources of those successful request.

It just looks cleaner to give such responsibility to each and every function,
i.e. if they fail, they should clean stuff up instead of the caller. That's the
normal philosophy you will find across kernel in most of the cases.
 

quoted

quoted

+static int virtio_i2c_complete_reqs(struct virtqueue *vq,
+				    struct virtio_i2c_req *reqs,
+				    struct i2c_msg *msgs, int nr,
+				    bool fail)
+{
+	struct virtio_i2c_req *req;
+	bool failed = fail;
+	unsigned int len;
+	int i, j = 0;
+
+	for (i = 0; i < nr; i++) {
+		/* Detach the ith request from the vq */
+		req = virtqueue_get_buf(vq, &len);
+
+		/*
+		 * Condition (req && req == &reqs[i]) should always meet since
+		 * we have total nr requests in the vq.
+		 */
+		if (!failed && (WARN_ON(!(req && req == &reqs[i])) ||
+		    (req->in_hdr.status != VIRTIO_I2C_MSG_OK)))

What about writing this as:

		if (!failed && (WARN_ON(req != &reqs[i]) ||
		    (req->in_hdr.status != VIRTIO_I2C_MSG_OK)))

We don't need to check req here since if req is NULL, we will not do req->in_hdr
at all.

It's right here just because the &reqs[i] will never be NULL in our case.
But if you see

"virtio_i2c_complete_reqs" as an independent function, you need to check the

req. From the perspective of the callee, you can't ask the caller always
give you

the non-NULL parameters.

We need to keep this driver optimized in its current form. If you see your own
argument here, then why don't you test vq or msgs for a valid pointer ? And even
reqs.

If we know for certain that this will never happen, then it should be optimized.
But if you see a case where reqs[i] can be NULL here, then it would be fine.

And some tools may give warnings.

I don't see why a tool will raise an error here and if it does, then the tool is
buggy and not the driver. And we don't need to take care of that.

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

On 2021/7/5 10:43, Viresh Kumar wrote:

On 02-07-21, 12:58, Andy Shevchenko wrote:

quoted

On Fri, Jul 02, 2021 at 04:46:47PM +0800, Jie Deng wrote:

quoted

+static int virtio_i2c_complete_reqs(struct virtqueue *vq,
+				    struct virtio_i2c_req *reqs,
+				    struct i2c_msg *msgs, int nr,
+				    bool fail)
+{
+	struct virtio_i2c_req *req;
+	bool failed = fail;

Jie, you can actually get rid of this variable too. Jut rename fail to failed
and everything shall work as you want.

Oh, You are not right. I just found we can't remove this variable. The 
"fail" and "failed" have different

meanings for this function. We need fail to return the result.

quoted

quoted

+	unsigned int len;
+	int i, j = 0;
+
+	for (i = 0; i < nr; i++) {
+		/* Detach the ith request from the vq */
+		req = virtqueue_get_buf(vq, &len);
+
+		/*
+		 * Condition (req && req == &reqs[i]) should always meet since
+		 * we have total nr requests in the vq.
+		 */
+		if (!failed && (WARN_ON(!(req && req == &reqs[i])) ||
+		    (req->in_hdr.status != VIRTIO_I2C_MSG_OK)))
+			failed = true;

...and after failed is true, we are continuing the loop, why?

Actually this function can be called with fail set to true. We proceed as we
need to call i2c_put_dma_safe_msg_buf() for all buffers we allocated earlier.

quoted

quoted

+		i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], !failed);
+		if (!failed)
+			++j;

Besides better to read j++ the j itself can be renamed to something more
verbose.

quoted

+	}
+	return (fail ? -ETIMEDOUT : j);

Redundant parentheses.

quoted

+}

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

On 2021/7/5 12:38, Viresh Kumar wrote:

On 05-07-21, 11:45, Jie Deng wrote:

quoted

On 2021/7/5 10:40, Viresh Kumar wrote:

quoted

On 02-07-21, 16:46, Jie Deng wrote:
The right way of doing this is is making this function return - Error on failure
and 0 on success. There is no point returning number of successful additions
here.

We need the number for virtio_i2c_complete_reqs to do cleanup. We don't have
to

do cleanup "num" times every time. Just do it as needed.

If you do full cleanup here, then you won't required that at the caller site.

quoted

quoted

Moreover, on failures this needs to clean up (free the dmabufs) itself, just
like you did i2c_put_dma_safe_msg_buf() at the end. The caller shouldn't be
required to handle the error cases by freeing up resources.

This function will return the number of requests being successfully prepared
and make sure

resources of the failed request being freed. And virtio_i2c_complete_reqs
will free the

resources of those successful request.

It just looks cleaner to give such responsibility to each and every function,
i.e. if they fail, they should clean stuff up instead of the caller. That's the
normal philosophy you will find across kernel in most of the cases.
  

quoted

quoted

quoted

+		/*
+		 * Condition (req && req == &reqs[i]) should always meet since
+		 * we have total nr requests in the vq.
+		 */
+		if (!failed && (WARN_ON(!(req && req == &reqs[i])) ||
+		    (req->in_hdr.status != VIRTIO_I2C_MSG_OK)))

What about writing this as:

		if (!failed && (WARN_ON(req != &reqs[i]) ||
		    (req->in_hdr.status != VIRTIO_I2C_MSG_OK)))

We don't need to check req here since if req is NULL, we will not do req->in_hdr
at all.

It's right here just because the &reqs[i] will never be NULL in our case.
But if you see

"virtio_i2c_complete_reqs" as an independent function, you need to check the

req. From the perspective of the callee, you can't ask the caller always
give you

the non-NULL parameters.

We need to keep this driver optimized in its current form. If you see your own
argument here, then why don't you test vq or msgs for a valid pointer ? And even
reqs.

If we know for certain that this will never happen, then it should be optimized.
But if you see a case where reqs[i] can be NULL here, then it would be fine.
ot the driver. And we don't need to take care of that.

This is still not enough to convince me.  So I won't change them for now 
until I see it

is the consensus of the majority.

Thank you.


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

On 05-07-21, 14:22, Jie Deng wrote:

On 2021/7/5 12:38, Viresh Kumar wrote:

quoted

On 05-07-21, 11:45, Jie Deng wrote:

quoted

On 2021/7/5 10:40, Viresh Kumar wrote:

quoted

On 02-07-21, 16:46, Jie Deng wrote:
The right way of doing this is is making this function return - Error on failure
and 0 on success. There is no point returning number of successful additions
here.

We need the number for virtio_i2c_complete_reqs to do cleanup. We don't have
to

do cleanup "num" times every time. Just do it as needed.

If you do full cleanup here, then you won't required that at the caller site.

quoted

quoted

Moreover, on failures this needs to clean up (free the dmabufs) itself, just
like you did i2c_put_dma_safe_msg_buf() at the end. The caller shouldn't be
required to handle the error cases by freeing up resources.

This function will return the number of requests being successfully prepared
and make sure

resources of the failed request being freed. And virtio_i2c_complete_reqs
will free the

resources of those successful request.

It just looks cleaner to give such responsibility to each and every function,
i.e. if they fail, they should clean stuff up instead of the caller. That's the
normal philosophy you will find across kernel in most of the cases.

quoted

quoted

quoted

+		/*
+		 * Condition (req && req == &reqs[i]) should always meet since
+		 * we have total nr requests in the vq.
+		 */
+		if (!failed && (WARN_ON(!(req && req == &reqs[i])) ||
+		    (req->in_hdr.status != VIRTIO_I2C_MSG_OK)))

What about writing this as:

		if (!failed && (WARN_ON(req != &reqs[i]) ||
		    (req->in_hdr.status != VIRTIO_I2C_MSG_OK)))

We don't need to check req here since if req is NULL, we will not do req->in_hdr
at all.

It's right here just because the &reqs[i] will never be NULL in our case.
But if you see

"virtio_i2c_complete_reqs" as an independent function, you need to check the

req. From the perspective of the callee, you can't ask the caller always
give you

the non-NULL parameters.

We need to keep this driver optimized in its current form. If you see your own
argument here, then why don't you test vq or msgs for a valid pointer ? And even
reqs.

If we know for certain that this will never happen, then it should be optimized.
But if you see a case where reqs[i] can be NULL here, then it would be fine.
ot the driver. And we don't need to take care of that.

This is still not enough to convince me.  So I won't change them for now
until I see it

is the consensus of the majority.

Do you see reqs[i] to ever be NULL here ? If not, then if (req) is like if
(true).

Why would you want to have something like that ?

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

On 05-07-21, 14:21, Jie Deng wrote:

On 2021/7/5 10:43, Viresh Kumar wrote:

quoted

On 02-07-21, 12:58, Andy Shevchenko wrote:

quoted

On Fri, Jul 02, 2021 at 04:46:47PM +0800, Jie Deng wrote:

quoted

+static int virtio_i2c_complete_reqs(struct virtqueue *vq,
+				    struct virtio_i2c_req *reqs,
+				    struct i2c_msg *msgs, int nr,
+				    bool fail)
+{
+	struct virtio_i2c_req *req;
+	bool failed = fail;

Jie, you can actually get rid of this variable too. Jut rename fail to failed
and everything shall work as you want.

Oh, You are not right. I just found we can't remove this variable. The
"fail" and "failed" have different

meanings for this function. We need fail to return the result.

Ahh, yes. You are right. Maybe rename fail to timedout, it would make it more
readable, else fail and failed look very similar.
 

quoted

quoted

quoted

+	unsigned int len;
+	int i, j = 0;
+
+	for (i = 0; i < nr; i++) {
+		/* Detach the ith request from the vq */
+		req = virtqueue_get_buf(vq, &len);
+
+		/*
+		 * Condition (req && req == &reqs[i]) should always meet since
+		 * we have total nr requests in the vq.
+		 */
+		if (!failed && (WARN_ON(!(req && req == &reqs[i])) ||
+		    (req->in_hdr.status != VIRTIO_I2C_MSG_OK)))
+			failed = true;

...and after failed is true, we are continuing the loop, why?

Actually this function can be called with fail set to true. We proceed as we
need to call i2c_put_dma_safe_msg_buf() for all buffers we allocated earlier.

quoted

quoted

+		i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], !failed);
+		if (!failed)
+			++j;

Besides better to read j++ the j itself can be renamed to something more
verbose.

quoted

+	}
+	return (fail ? -ETIMEDOUT : j);

Redundant parentheses.

quoted

+}

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

On 2021/7/5 14:30, Viresh Kumar wrote:

quoted

This is still not enough to convince me.  So I won't change them for now
until I see it

is the consensus of the majority.

Do you see reqs[i] to ever be NULL here ? If not, then if (req) is like if
(true).

Why would you want to have something like that ?

No. Currently, virtio_i2c_complete_reqs is only called by 
virtio_i2c_xfer, thus we don't

have reqs[i] to be NULL. But I think "virtio_i2c_complete_reqs" as an 
independent function

should consider this from a callee perspective.

Anyway, if you really think it should be changed, it can be fixed 
incrementally as Wolfram said.


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