Linux-i2c Archive on lore.kernel.org
 help / color / Atom feed
* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
  2021-03-23 14:19 [PATCH v10] i2c: virtio: add a virtio i2c frontend driver Jie Deng
@ 2021-03-23  7:27 ` Viresh Kumar
  2021-03-23  8:33   ` Jie Deng
  2021-03-23  9:01 ` Viresh Kumar
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 34+ messages in thread
From: Viresh Kumar @ 2021-03-23  7:27 UTC (permalink / raw)
  To: Jie Deng
  Cc: linux-i2c, virtualization, linux-kernel, mst, wsa, jasowang,
	wsa+renesas, andriy.shevchenko, conghui.chen, arnd, kblaiech,
	jarkko.nikula, Sergey.Semin, rppt, loic.poulain, tali.perry1,
	u.kleine-koenig, bjorn.andersson, yu1.wang, shuo.a.liu, stefanha,
	pbonzini

On 23-03-21, 22:19, Jie Deng wrote:
> +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);
> +}

Sorry for not looking at this earlier, but shouldn't we enclose the above two
within #ifdef CONFIG_PM_SLEEP instead and drop the __maybe_unused ?

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

-- 
viresh

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
  2021-03-23  7:27 ` Viresh Kumar
@ 2021-03-23  8:33   ` Jie Deng
  2021-03-23  8:37     ` Viresh Kumar
  2021-03-23  9:27     ` Arnd Bergmann
  0 siblings, 2 replies; 34+ messages in thread
From: Jie Deng @ 2021-03-23  8:33 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: linux-i2c, virtualization, linux-kernel, mst, wsa, jasowang,
	wsa+renesas, andriy.shevchenko, conghui.chen, arnd, kblaiech,
	jarkko.nikula, Sergey.Semin, rppt, loic.poulain, tali.perry1,
	u.kleine-koenig, bjorn.andersson, yu1.wang, shuo.a.liu, stefanha,
	pbonzini

On 2021/3/23 15:27, Viresh Kumar wrote:

> On 23-03-21, 22:19, Jie Deng wrote:
>> +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);
>> +}
> Sorry for not looking at this earlier, but shouldn't we enclose the above two
> within #ifdef CONFIG_PM_SLEEP instead and drop the __maybe_unused ?


I remembered I was suggested to use "__maybe_unused" instead of "#ifdef".

You may check this https://lore.kernel.org/patchwork/patch/732981/

The reason may be something like that.


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
  2021-03-23  8:33   ` Jie Deng
@ 2021-03-23  8:37     ` Viresh Kumar
  2021-03-23  9:27     ` Arnd Bergmann
  1 sibling, 0 replies; 34+ messages in thread
From: Viresh Kumar @ 2021-03-23  8:37 UTC (permalink / raw)
  To: Jie Deng
  Cc: linux-i2c, virtualization, linux-kernel, mst, wsa, jasowang,
	wsa+renesas, andriy.shevchenko, conghui.chen, arnd, kblaiech,
	jarkko.nikula, Sergey.Semin, rppt, loic.poulain, tali.perry1,
	u.kleine-koenig, bjorn.andersson, yu1.wang, shuo.a.liu, stefanha,
	pbonzini

On 23-03-21, 16:33, Jie Deng wrote:
> On 2021/3/23 15:27, Viresh Kumar wrote:
> 
> > On 23-03-21, 22:19, Jie Deng wrote:
> > > +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);
> > > +}
> > Sorry for not looking at this earlier, but shouldn't we enclose the above two
> > within #ifdef CONFIG_PM_SLEEP instead and drop the __maybe_unused ?
> 
> 
> I remembered I was suggested to use "__maybe_unused" instead of "#ifdef".
> 
> You may check this https://lore.kernel.org/patchwork/patch/732981/
> 
> The reason may be something like that.

Ahh, thanks for the link Jie. Okay you can leave it as is.

-- 
viresh

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
  2021-03-23 14:19 [PATCH v10] i2c: virtio: add a virtio i2c frontend driver Jie Deng
  2021-03-23  7:27 ` Viresh Kumar
@ 2021-03-23  9:01 ` Viresh Kumar
  2021-03-23  9:38   ` Viresh Kumar
  2021-03-24  4:20 ` Viresh Kumar
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 34+ messages in thread
From: Viresh Kumar @ 2021-03-23  9:01 UTC (permalink / raw)
  To: Jie Deng
  Cc: linux-i2c, virtualization, linux-kernel, mst, wsa, jasowang,
	wsa+renesas, andriy.shevchenko, conghui.chen, arnd, kblaiech,
	jarkko.nikula, Sergey.Semin, rppt, loic.poulain, tali.perry1,
	u.kleine-koenig, bjorn.andersson, yu1.wang, shuo.a.liu, stefanha,
	pbonzini

On 23-03-21, 22:19, Jie Deng wrote:
> +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->lock);
> +
> +	ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
> +	if (ret == 0)
> +		goto err_unlock_free;
> +
> +	nr = ret;
> +	reinit_completion(&vi->completion);

I think I may have found a possible bug here. This reinit_completion() must
happen before we call virtio_i2c_send_reqs(). It is certainly possible (surely
in corner cases) that virtio_i2c_msg_done() may get called right after
virtio_i2c_send_reqs() and before we were able to call reinit_completion(). And
in that case we will never see the completion happen at all.

> +	virtqueue_kick(vq);

-- 
viresh

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
  2021-03-23  8:33   ` Jie Deng
  2021-03-23  8:37     ` Viresh Kumar
@ 2021-03-23  9:27     ` Arnd Bergmann
  2021-03-24  1:17       ` Jie Deng
  2021-04-15  6:45       ` Viresh Kumar
  1 sibling, 2 replies; 34+ messages in thread
From: Arnd Bergmann @ 2021-03-23  9:27 UTC (permalink / raw)
  To: Jie Deng
  Cc: Viresh Kumar, Linux I2C, virtualization,
	Linux Kernel Mailing List, Michael S. Tsirkin, Wolfram Sang,
	Jason Wang, Wolfram Sang, Andy Shevchenko, conghui.chen,
	kblaiech, jarkko.nikula, Sergey Semin, Mike Rapoport,
	loic.poulain, Tali Perry, Uwe Kleine-König, Bjorn Andersson,
	yu1.wang, shuo.a.liu, Stefan Hajnoczi, Paolo Bonzini

On Tue, Mar 23, 2021 at 9:33 AM Jie Deng <jie.deng@intel.com> wrote:
>
> On 2021/3/23 15:27, Viresh Kumar wrote:
>
> > On 23-03-21, 22:19, Jie Deng wrote:
> >> +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);
> >> +}
> > Sorry for not looking at this earlier, but shouldn't we enclose the above two
> > within #ifdef CONFIG_PM_SLEEP instead and drop the __maybe_unused ?
>
>
> I remembered I was suggested to use "__maybe_unused" instead of "#ifdef".
>
> You may check this https://lore.kernel.org/patchwork/patch/732981/
>
> The reason may be something like that.

I usually recommend the use of __maybe_unused for the suspend/resume
callbacks for drivers that use SIMPLE_DEV_PM_OPS() or similar helpers
that hide the exact conditions under which the functions get called.

In this driver, there is an explicit #ifdef in the reference to the
functions, so
it would make sense to use the same #ifdef around the definition.

A better question to ask is whether you could use the helpers instead,
and drop the other #ifdef.

       Arnd

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
  2021-03-23  9:01 ` Viresh Kumar
@ 2021-03-23  9:38   ` Viresh Kumar
  2021-03-24  0:53     ` Jie Deng
  0 siblings, 1 reply; 34+ messages in thread
From: Viresh Kumar @ 2021-03-23  9:38 UTC (permalink / raw)
  To: Jie Deng
  Cc: linux-i2c, virtualization, linux-kernel, mst, wsa, jasowang,
	wsa+renesas, andriy.shevchenko, conghui.chen, arnd, kblaiech,
	jarkko.nikula, Sergey.Semin, rppt, loic.poulain, tali.perry1,
	u.kleine-koenig, bjorn.andersson, yu1.wang, shuo.a.liu, stefanha,
	pbonzini

On 23-03-21, 14:31, Viresh Kumar wrote:
> On 23-03-21, 22:19, Jie Deng wrote:
> > +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->lock);
> > +
> > +	ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
> > +	if (ret == 0)
> > +		goto err_unlock_free;
> > +
> > +	nr = ret;
> > +	reinit_completion(&vi->completion);
> 
> I think I may have found a possible bug here. This reinit_completion() must
> happen before we call virtio_i2c_send_reqs(). It is certainly possible (surely
> in corner cases) that virtio_i2c_msg_done() may get called right after
> virtio_i2c_send_reqs() and before we were able to call reinit_completion(). And
> in that case we will never see the completion happen at all.
> 
> > +	virtqueue_kick(vq);

I may have misread this. Can the actually start before virtqueue_kick() is
called ? If not, then completion may be fine where it is.

-- 
viresh

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
@ 2021-03-23 14:19 Jie Deng
  2021-03-23  7:27 ` Viresh Kumar
                   ` (4 more replies)
  0 siblings, 5 replies; 34+ messages in thread
From: Jie Deng @ 2021-03-23 14:19 UTC (permalink / raw)
  To: linux-i2c, virtualization, linux-kernel
  Cc: mst, wsa, jasowang, wsa+renesas, andriy.shevchenko, conghui.chen,
	arnd, kblaiech, jarkko.nikula, Sergey.Semin, rppt, jie.deng,
	loic.poulain, tali.perry1, u.kleine-koenig, bjorn.andersson,
	yu1.wang, shuo.a.liu, viresh.kumar, stefanha, pbonzini

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>
---
Changes in v10:
        - Fix some typo errors.
        - Refined the virtio_i2c_complete_reqs to use less code lines.

Changes in v9:
        - Remove the virtio_adapter and update its members in probe.
        - Refined the virtio_i2c_complete_reqs for buf free.

Changes in v8:
        - Make virtio_i2c.adap a pointer.
        - Mark members in virtio_i2c_req with ____cacheline_aligned.

Changes in v7:
        - Remove unused headers.
        - Update Makefile and Kconfig.
        - Add the cleanup after completing reqs.
        - Avoid memcpy for data marked with I2C_M_DMA_SAFE.
        - Fix something reported by kernel test robot.

Changes in v6:
        - Move struct virtio_i2c_req into the driver.
        - Use only one buf in struct virtio_i2c_req.

Changes in v5:
        - The first version based on the acked specification.

 drivers/i2c/busses/Kconfig      |  11 ++
 drivers/i2c/busses/Makefile     |   3 +
 drivers/i2c/busses/i2c-virtio.c | 276 ++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/virtio_i2c.h |  40 ++++++
 include/uapi/linux/virtio_ids.h |   1 +
 5 files changed, 331 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..cb8d0d8 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 615f35e..efdd3f3 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -145,4 +145,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..99a1e30
--- /dev/null
+++ b/drivers/i2c/busses/i2c-virtio.c
@@ -0,0 +1,276 @@
+// 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
+ * @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 lock;
+	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++) {
+		if (!msgs[i].len)
+			break;
+
+		/*
+		 * Only 7-bit mode supported for this moment. For the address format,
+		 * Please check the Virtio I2C Specification.
+		 */
+		reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1);
+
+		if (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;
+
+		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) {
+			pr_err("failed to add msg[%d] to virtqueue.\n", i);
+			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 timeout)
+{
+	struct virtio_i2c_req *req;
+	bool failed = timeout;
+	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 (timeout ? -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, nr;
+
+	reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL);
+	if (!reqs)
+		return -ENOMEM;
+
+	mutex_lock(&vi->lock);
+
+	ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
+	if (ret == 0)
+		goto err_unlock_free;
+
+	nr = ret;
+	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, nr, !time_left);
+
+err_unlock_free:
+	mutex_unlock(&vi->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 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->lock);
+	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), "Virtio I2C Adapter");
+	vi->adap.class = I2C_CLASS_DEPRECATED;
+	vi->adap.algo = &virtio_algorithm;
+	vi->adap.dev.parent = &vdev->dev;
+	vi->adap.timeout = HZ / 10;
+	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);
+		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_ADAPTER, 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_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..bbcfb2c
--- /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	0x00000001
+
+/**
+ * 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 bc1c062..b89391d 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_ADAPTER		34 /* virtio i2c adapter */
 
 #endif /* _LINUX_VIRTIO_IDS_H */
-- 
2.7.4


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
  2021-03-23  9:38   ` Viresh Kumar
@ 2021-03-24  0:53     ` Jie Deng
  2021-03-24  3:52       ` Viresh Kumar
  0 siblings, 1 reply; 34+ messages in thread
From: Jie Deng @ 2021-03-24  0:53 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: linux-i2c, virtualization, linux-kernel, mst, wsa, jasowang,
	wsa+renesas, andriy.shevchenko, conghui.chen, arnd, kblaiech,
	jarkko.nikula, Sergey.Semin, rppt, loic.poulain, tali.perry1,
	u.kleine-koenig, bjorn.andersson, yu1.wang, shuo.a.liu, stefanha,
	pbonzini


On 2021/3/23 17:38, Viresh Kumar wrote:
> On 23-03-21, 14:31, Viresh Kumar wrote:
>> On 23-03-21, 22:19, Jie Deng wrote:
>>> +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->lock);
>>> +
>>> +	ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
>>> +	if (ret == 0)
>>> +		goto err_unlock_free;
>>> +
>>> +	nr = ret;
>>> +	reinit_completion(&vi->completion);
>> I think I may have found a possible bug here. This reinit_completion() must
>> happen before we call virtio_i2c_send_reqs(). It is certainly possible (surely
>> in corner cases) that virtio_i2c_msg_done() may get called right after
>> virtio_i2c_send_reqs() and before we were able to call reinit_completion(). And
>> in that case we will never see the completion happen at all.
>>
>>> +	virtqueue_kick(vq);
> I may have misread this. Can the actually start before virtqueue_kick() is
> called ?


No. It starts when wait_for_completion_timeout is called.

So it should be fine here.


>   If not, then completion may be fine where it is.
>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
  2021-03-23  9:27     ` Arnd Bergmann
@ 2021-03-24  1:17       ` Jie Deng
  2021-03-24  4:00         ` Viresh Kumar
  2021-04-15  6:45       ` Viresh Kumar
  1 sibling, 1 reply; 34+ messages in thread
From: Jie Deng @ 2021-03-24  1:17 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Viresh Kumar, Linux I2C, virtualization,
	Linux Kernel Mailing List, Michael S. Tsirkin, Wolfram Sang,
	Jason Wang, Wolfram Sang, Andy Shevchenko, conghui.chen,
	kblaiech, jarkko.nikula, Sergey Semin, Mike Rapoport,
	loic.poulain, Tali Perry, Uwe Kleine-König, Bjorn Andersson,
	yu1.wang, shuo.a.liu, Stefan Hajnoczi, Paolo Bonzini


On 2021/3/23 17:27, Arnd Bergmann wrote:
> On Tue, Mar 23, 2021 at 9:33 AM Jie Deng <jie.deng@intel.com> wrote:
>> On 2021/3/23 15:27, Viresh Kumar wrote:
>>
>>> On 23-03-21, 22:19, Jie Deng wrote:
>>>> +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);
>>>> +}
>>> Sorry for not looking at this earlier, but shouldn't we enclose the above two
>>> within #ifdef CONFIG_PM_SLEEP instead and drop the __maybe_unused ?
>>
>> I remembered I was suggested to use "__maybe_unused" instead of "#ifdef".
>>
>> You may check this https://lore.kernel.org/patchwork/patch/732981/
>>
>> The reason may be something like that.
> I usually recommend the use of __maybe_unused for the suspend/resume
> callbacks for drivers that use SIMPLE_DEV_PM_OPS() or similar helpers
> that hide the exact conditions under which the functions get called.
>
> In this driver, there is an explicit #ifdef in the reference to the
> functions, so
> it would make sense to use the same #ifdef around the definition.
>
> A better question to ask is whether you could use the helpers instead,
> and drop the other #ifdef.
>
>         Arnd


I didn't see the "struct virtio_driver" has a member "struct dev_pm_ops *pm"

It defines its own hooks (freeze and restore) though it includes "struct 
device_driver"

which has a "struct dev_pm_ops *pm".

I just follow other virtio drivers to directly use the hooks defined in 
"struct virtio_driver".

For this driver, Both __maybe_unused and #ifdef are OK to me.



^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
  2021-03-24  0:53     ` Jie Deng
@ 2021-03-24  3:52       ` Viresh Kumar
  2021-03-24  4:00         ` Jie Deng
  0 siblings, 1 reply; 34+ messages in thread
From: Viresh Kumar @ 2021-03-24  3:52 UTC (permalink / raw)
  To: Jie Deng
  Cc: linux-i2c, virtualization, linux-kernel, mst, wsa, jasowang,
	wsa+renesas, andriy.shevchenko, conghui.chen, arnd, kblaiech,
	jarkko.nikula, Sergey.Semin, rppt, loic.poulain, tali.perry1,
	u.kleine-koenig, bjorn.andersson, yu1.wang, shuo.a.liu, stefanha,
	pbonzini

On 24-03-21, 08:53, Jie Deng wrote:
> 
> On 2021/3/23 17:38, Viresh Kumar wrote:
> > On 23-03-21, 14:31, Viresh Kumar wrote:
> > > On 23-03-21, 22:19, Jie Deng wrote:
> > > > +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->lock);
> > > > +
> > > > +	ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
> > > > +	if (ret == 0)
> > > > +		goto err_unlock_free;
> > > > +
> > > > +	nr = ret;
> > > > +	reinit_completion(&vi->completion);
> > > I think I may have found a possible bug here. This reinit_completion() must
> > > happen before we call virtio_i2c_send_reqs(). It is certainly possible (surely
> > > in corner cases) that virtio_i2c_msg_done() may get called right after
> > > virtio_i2c_send_reqs() and before we were able to call reinit_completion(). And
> > > in that case we will never see the completion happen at all.
> > > 
> > > > +	virtqueue_kick(vq);
> > I may have misread this. Can the actually start before virtqueue_kick() is
> > called ?

I didn't write it properly here. I wanted to say,

"Can the _transfer_ actually start before virtqueue_kick() is called ?"
 
> No. It starts when wait_for_completion_timeout is called.

No, the transfer doesn't have anything to do with wait_for_completion_timeout().
And if complete() gets called before wait_for_completion_timeout() is called,
then wait_for_completion_timeout() will simply return back.

> So it should be fine here.
> 
> 
> >   If not, then completion may be fine where it is.
> > 

-- 
viresh

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
  2021-03-24  1:17       ` Jie Deng
@ 2021-03-24  4:00         ` Viresh Kumar
  0 siblings, 0 replies; 34+ messages in thread
From: Viresh Kumar @ 2021-03-24  4:00 UTC (permalink / raw)
  To: Jie Deng
  Cc: Arnd Bergmann, Linux I2C, virtualization,
	Linux Kernel Mailing List, Michael S. Tsirkin, Wolfram Sang,
	Jason Wang, Wolfram Sang, Andy Shevchenko, conghui.chen,
	kblaiech, jarkko.nikula, Sergey Semin, Mike Rapoport,
	loic.poulain, Tali Perry, Uwe Kleine-König, Bjorn Andersson,
	yu1.wang, shuo.a.liu, Stefan Hajnoczi, Paolo Bonzini

On 24-03-21, 09:17, Jie Deng wrote:
> I didn't see the "struct virtio_driver" has a member "struct dev_pm_ops *pm"
> 
> It defines its own hooks (freeze and restore) though it includes "struct
> device_driver"
> 
> which has a "struct dev_pm_ops *pm".
> 
> I just follow other virtio drivers to directly use the hooks defined in
> "struct virtio_driver".

Right, I think we can't use the SIMPLE PM OPS for virtio yet, the core calls
only the freeze and restore callbacks which are part of struct virtio_driver.

> For this driver, Both __maybe_unused and #ifdef are OK to me.

Yes, and so you should use only #ifdef here as Arnd confirmed. You shouldn't be
using __maybe_unused as there is nothing confusing here related to the macros.

-- 
viresh

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
  2021-03-24  3:52       ` Viresh Kumar
@ 2021-03-24  4:00         ` Jie Deng
  2021-03-24  4:02           ` Viresh Kumar
  0 siblings, 1 reply; 34+ messages in thread
From: Jie Deng @ 2021-03-24  4:00 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: linux-i2c, virtualization, linux-kernel, mst, wsa, jasowang,
	wsa+renesas, andriy.shevchenko, conghui.chen, arnd, kblaiech,
	jarkko.nikula, Sergey.Semin, rppt, loic.poulain, tali.perry1,
	u.kleine-koenig, bjorn.andersson, yu1.wang, shuo.a.liu, stefanha,
	pbonzini


On 2021/3/24 11:52, Viresh Kumar wrote:
> On 24-03-21, 08:53, Jie Deng wrote:
>> On 2021/3/23 17:38, Viresh Kumar wrote:
>>> On 23-03-21, 14:31, Viresh Kumar wrote:
>>>> On 23-03-21, 22:19, Jie Deng wrote:
>>>>> +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->lock);
>>>>> +
>>>>> +	ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
>>>>> +	if (ret == 0)
>>>>> +		goto err_unlock_free;
>>>>> +
>>>>> +	nr = ret;
>>>>> +	reinit_completion(&vi->completion);
>>>> I think I may have found a possible bug here. This reinit_completion() must
>>>> happen before we call virtio_i2c_send_reqs(). It is certainly possible (surely
>>>> in corner cases) that virtio_i2c_msg_done() may get called right after
>>>> virtio_i2c_send_reqs() and before we were able to call reinit_completion(). And
>>>> in that case we will never see the completion happen at all.
>>>>
>>>>> +	virtqueue_kick(vq);
>>> I may have misread this. Can the actually start before virtqueue_kick() is
>>> called ?
> I didn't write it properly here. I wanted to say,
>
> "Can the _transfer_ actually start before virtqueue_kick() is called ?"
>   


It can't start until the virtqueue_kick() is called. Call virtqueue_kick 
then wait.



^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
  2021-03-24  4:00         ` Jie Deng
@ 2021-03-24  4:02           ` Viresh Kumar
  0 siblings, 0 replies; 34+ messages in thread
From: Viresh Kumar @ 2021-03-24  4:02 UTC (permalink / raw)
  To: Jie Deng
  Cc: linux-i2c, virtualization, linux-kernel, mst, wsa, jasowang,
	wsa+renesas, andriy.shevchenko, conghui.chen, arnd, kblaiech,
	jarkko.nikula, Sergey.Semin, rppt, loic.poulain, tali.perry1,
	u.kleine-koenig, bjorn.andersson, yu1.wang, shuo.a.liu, stefanha,
	pbonzini

On 24-03-21, 12:00, Jie Deng wrote:
> 
> On 2021/3/24 11:52, Viresh Kumar wrote:
> > On 24-03-21, 08:53, Jie Deng wrote:
> > > On 2021/3/23 17:38, Viresh Kumar wrote:
> > > > On 23-03-21, 14:31, Viresh Kumar wrote:
> > > > > On 23-03-21, 22:19, Jie Deng wrote:
> > > > > > +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->lock);
> > > > > > +
> > > > > > +	ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
> > > > > > +	if (ret == 0)
> > > > > > +		goto err_unlock_free;
> > > > > > +
> > > > > > +	nr = ret;
> > > > > > +	reinit_completion(&vi->completion);
> > > > > I think I may have found a possible bug here. This reinit_completion() must
> > > > > happen before we call virtio_i2c_send_reqs(). It is certainly possible (surely
> > > > > in corner cases) that virtio_i2c_msg_done() may get called right after
> > > > > virtio_i2c_send_reqs() and before we were able to call reinit_completion(). And
> > > > > in that case we will never see the completion happen at all.
> > > > > 
> > > > > > +	virtqueue_kick(vq);
> > > > I may have misread this. Can the actually start before virtqueue_kick() is
> > > > called ?
> > I didn't write it properly here. I wanted to say,
> > 
> > "Can the _transfer_ actually start before virtqueue_kick() is called ?"
> 
> 
> It can't start until the virtqueue_kick() is called. Call virtqueue_kick
> then wait.

Great, thanks for the confirmation. The code is fine then.

-- 
viresh

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
  2021-03-23 14:19 [PATCH v10] i2c: virtio: add a virtio i2c frontend driver Jie Deng
  2021-03-23  7:27 ` Viresh Kumar
  2021-03-23  9:01 ` Viresh Kumar
@ 2021-03-24  4:20 ` Viresh Kumar
  2021-03-24  6:05   ` Jie Deng
  2021-04-14  2:07 ` Jie Deng
  2021-04-15  3:51 ` Jason Wang
  4 siblings, 1 reply; 34+ messages in thread
From: Viresh Kumar @ 2021-03-24  4:20 UTC (permalink / raw)
  To: Jie Deng
  Cc: linux-i2c, virtualization, linux-kernel, mst, wsa, jasowang,
	wsa+renesas, andriy.shevchenko, conghui.chen, arnd, kblaiech,
	jarkko.nikula, Sergey.Semin, rppt, loic.poulain, tali.perry1,
	u.kleine-koenig, bjorn.andersson, yu1.wang, shuo.a.liu, stefanha,
	pbonzini

On 23-03-21, 22:19, Jie Deng wrote:
> +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->lock);
> +
> +	ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
> +	if (ret == 0)
> +		goto err_unlock_free;
> +
> +	nr = ret;
> +	reinit_completion(&vi->completion);
> +	virtqueue_kick(vq);

Coming back to this again, what is the expectation from the other side for this
? I mean there is no obvious relation between the *msgs* which we are going to
transfer (from the other side's or host's point of view). When should the host
OS call its virtqueue_kick() counterpart ?

Lemme give an example for this. Lets say that we need to transfer 3 messages
here in this routine. What we did was we prepared virtqueue for all 3 messages
together and then called virtqueue_kick().

Now if the other side (host) processes the first message and sends its reply
(with virtqueue_kick() counterpart) before processing the other two messages,
then it will end up calling virtio_i2c_msg_done() here. That will make us call
virtio_i2c_complete_reqs(), while only the first messages is processed until
now and so we will fail for the other two messages straight away.

Should we send only 1 message from i2c-virtio linux driver and then wait for
virtio_i2c_msg_done() to be called, before sending the next message to make sure
it doesn't break ?

-- 
viresh

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
  2021-03-24  4:20 ` Viresh Kumar
@ 2021-03-24  6:05   ` Jie Deng
  2021-03-24  6:09     ` Viresh Kumar
  0 siblings, 1 reply; 34+ messages in thread
From: Jie Deng @ 2021-03-24  6:05 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: linux-i2c, virtualization, linux-kernel, mst, wsa, jasowang,
	wsa+renesas, andriy.shevchenko, conghui.chen, arnd, kblaiech,
	jarkko.nikula, Sergey.Semin, rppt, loic.poulain, tali.perry1,
	u.kleine-koenig, bjorn.andersson, yu1.wang, shuo.a.liu, stefanha,
	pbonzini


On 2021/3/24 12:20, Viresh Kumar wrote:
> On 23-03-21, 22:19, Jie Deng wrote:
>> +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->lock);
>> +
>> +	ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
>> +	if (ret == 0)
>> +		goto err_unlock_free;
>> +
>> +	nr = ret;
>> +	reinit_completion(&vi->completion);
>> +	virtqueue_kick(vq);
> Coming back to this again, what is the expectation from the other side for this
> ? I mean there is no obvious relation between the *msgs* which we are going to
> transfer (from the other side's or host's point of view). When should the host
> OS call its virtqueue_kick() counterpart ?
>
> Lemme give an example for this. Lets say that we need to transfer 3 messages
> here in this routine. What we did was we prepared virtqueue for all 3 messages
> together and then called virtqueue_kick().
>
> Now if the other side (host) processes the first message and sends its reply
> (with virtqueue_kick() counterpart) before processing the other two messages,
> then it will end up calling virtio_i2c_msg_done() here. That will make us call
> virtio_i2c_complete_reqs(), while only the first messages is processed until
> now and so we will fail for the other two messages straight away.
>
> Should we send only 1 message from i2c-virtio linux driver and then wait for
> virtio_i2c_msg_done() to be called, before sending the next message to make sure
> it doesn't break ?


For simplicity, the original patch sent only 1 message to vq each time . 
I changed the way to send

a batch of requests in one time in order to improve efficiency according 
to Jason' suggestion.

As we discussed in the previous emails, the device can raise interrupt 
when some requests are still not completed

though this is not a good operation.  In this case, the remaining 
requests in the vq will be ignored and

the i2c_algorithm. master_xfer will return 1 for your example. I will 
clarify this in the specs.




^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
  2021-03-24  6:05   ` Jie Deng
@ 2021-03-24  6:09     ` Viresh Kumar
  2021-03-24  6:41       ` Jie Deng
  0 siblings, 1 reply; 34+ messages in thread
From: Viresh Kumar @ 2021-03-24  6:09 UTC (permalink / raw)
  To: Jie Deng
  Cc: linux-i2c, virtualization, linux-kernel, mst, wsa, jasowang,
	wsa+renesas, andriy.shevchenko, conghui.chen, arnd, kblaiech,
	jarkko.nikula, Sergey.Semin, rppt, loic.poulain, tali.perry1,
	u.kleine-koenig, bjorn.andersson, yu1.wang, shuo.a.liu, stefanha,
	pbonzini

On 24-03-21, 14:05, Jie Deng wrote:
> For simplicity, the original patch sent only 1 message to vq each time . I
> changed the way to send

I missed those earlier discussions :)

> a batch of requests in one time in order to improve efficiency according to
> Jason' suggestion.

I agree.

> As we discussed in the previous emails, the device can raise interrupt when
> some requests are still not completed
> 
> though this is not a good operation.  In this case, the remaining requests
> in the vq will be ignored and
> 
> the i2c_algorithm. master_xfer will return 1 for your example. I will
> clarify this in the specs.

Right, this needs to be clarified that the receiver shall generate the interrupt
only once the virtqueue is empty, not in the middle of it.

Or, now that I think about it a bit more, another thing we can do here is see if
virtqueue_get_buf() returns NULL, if it does then we should keep expecting more
messages as it may be early interrupt. What do you say ?


-- 
viresh

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
  2021-03-24  6:09     ` Viresh Kumar
@ 2021-03-24  6:41       ` Jie Deng
  2021-03-24  6:44         ` Viresh Kumar
  0 siblings, 1 reply; 34+ messages in thread
From: Jie Deng @ 2021-03-24  6:41 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: linux-i2c, virtualization, linux-kernel, mst, wsa, jasowang,
	wsa+renesas, andriy.shevchenko, conghui.chen, arnd, kblaiech,
	jarkko.nikula, Sergey.Semin, rppt, loic.poulain, tali.perry1,
	u.kleine-koenig, bjorn.andersson, yu1.wang, shuo.a.liu, stefanha,
	pbonzini


On 2021/3/24 14:09, Viresh Kumar wrote:
> On 24-03-21, 14:05, Jie Deng wrote:
> Or, now that I think about it a bit more, another thing we can do here is see if
> virtqueue_get_buf() returns NULL, if it does then we should keep expecting more
> messages as it may be early interrupt. What do you say ?

I don't think we really need this because for this device, early 
interrupt is a bad operation
which should be avoided. I can't think of why this device need to send 
early interrupt, what
we can do is to clarify that this means loss of the remaining requests. 
A device should never
do this, if it does then loss is the expected result.



^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
  2021-03-24  6:41       ` Jie Deng
@ 2021-03-24  6:44         ` Viresh Kumar
  0 siblings, 0 replies; 34+ messages in thread
From: Viresh Kumar @ 2021-03-24  6:44 UTC (permalink / raw)
  To: Jie Deng
  Cc: linux-i2c, virtualization, linux-kernel, mst, wsa, jasowang,
	wsa+renesas, andriy.shevchenko, conghui.chen, arnd, kblaiech,
	jarkko.nikula, Sergey.Semin, rppt, loic.poulain, tali.perry1,
	u.kleine-koenig, bjorn.andersson, yu1.wang, shuo.a.liu, stefanha,
	pbonzini

On 24-03-21, 14:41, Jie Deng wrote:
> 
> On 2021/3/24 14:09, Viresh Kumar wrote:
> > On 24-03-21, 14:05, Jie Deng wrote:
> > Or, now that I think about it a bit more, another thing we can do here is see if
> > virtqueue_get_buf() returns NULL, if it does then we should keep expecting more
> > messages as it may be early interrupt. What do you say ?
> 
> I don't think we really need this because for this device, early interrupt
> is a bad operation
> which should be avoided. I can't think of why this device need to send early
> interrupt, what
> we can do is to clarify that this means loss of the remaining requests. A
> device should never
> do this, if it does then loss is the expected result.

Fair enough.

-- 
viresh

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
  2021-03-23 14:19 [PATCH v10] i2c: virtio: add a virtio i2c frontend driver Jie Deng
                   ` (2 preceding siblings ...)
  2021-03-24  4:20 ` Viresh Kumar
@ 2021-04-14  2:07 ` Jie Deng
  2021-04-14  3:52   ` Viresh Kumar
  2021-04-15  3:51 ` Jason Wang
  4 siblings, 1 reply; 34+ messages in thread
From: Jie Deng @ 2021-04-14  2:07 UTC (permalink / raw)
  To: wsa, wsa, mst, linux-i2c, virtualization, linux-kernel
  Cc: wsa, jasowang, wsa+renesas, andriy.shevchenko, conghui.chen,
	arnd, kblaiech, jarkko.nikula, Sergey.Semin, rppt, loic.poulain,
	tali.perry1, u.kleine-koenig, bjorn.andersson, yu1.wang,
	shuo.a.liu, viresh.kumar, stefanha, mst

Hi maintainers,

What's the status of this patch ? Is i2c/for-next the right tree to 
merge it ?

Thanks,

Jie


On 2021/3/23 22:19, 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.
>
> 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>
>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
  2021-04-14  2:07 ` Jie Deng
@ 2021-04-14  3:52   ` Viresh Kumar
  2021-04-15  6:25     ` Jie Deng
  0 siblings, 1 reply; 34+ messages in thread
From: Viresh Kumar @ 2021-04-14  3:52 UTC (permalink / raw)
  To: Jie Deng
  Cc: wsa, wsa, mst, linux-i2c, virtualization, linux-kernel, jasowang,
	wsa+renesas, andriy.shevchenko, conghui.chen, arnd, kblaiech,
	jarkko.nikula, Sergey.Semin, rppt, loic.poulain, tali.perry1,
	u.kleine-koenig, bjorn.andersson, yu1.wang, shuo.a.liu, stefanha

On 14-04-21, 10:07, Jie Deng wrote:
> Hi maintainers,
> 
> What's the status of this patch based on the review comments you got ?

I was expecting a new version to be honest..

> Is i2c/for-next the right tree to merge it
> ?

It should be.

-- 
viresh

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
  2021-03-23 14:19 [PATCH v10] i2c: virtio: add a virtio i2c frontend driver Jie Deng
                   ` (3 preceding siblings ...)
  2021-04-14  2:07 ` Jie Deng
@ 2021-04-15  3:51 ` Jason Wang
  2021-04-15  6:17   ` Jie Deng
  4 siblings, 1 reply; 34+ messages in thread
From: Jason Wang @ 2021-04-15  3:51 UTC (permalink / raw)
  To: Jie Deng, linux-i2c, virtualization, linux-kernel
  Cc: mst, wsa, wsa+renesas, andriy.shevchenko, conghui.chen, arnd,
	kblaiech, jarkko.nikula, Sergey.Semin, rppt, loic.poulain,
	tali.perry1, u.kleine-koenig, bjorn.andersson, yu1.wang,
	shuo.a.liu, viresh.kumar, stefanha, pbonzini


在 2021/3/23 下午10:19, 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>
> ---
> Changes in v10:
>          - Fix some typo errors.
>          - Refined the virtio_i2c_complete_reqs to use less code lines.
>
> Changes in v9:
>          - Remove the virtio_adapter and update its members in probe.
>          - Refined the virtio_i2c_complete_reqs for buf free.
>
> Changes in v8:
>          - Make virtio_i2c.adap a pointer.
>          - Mark members in virtio_i2c_req with ____cacheline_aligned.
>
> Changes in v7:
>          - Remove unused headers.
>          - Update Makefile and Kconfig.
>          - Add the cleanup after completing reqs.
>          - Avoid memcpy for data marked with I2C_M_DMA_SAFE.
>          - Fix something reported by kernel test robot.
>
> Changes in v6:
>          - Move struct virtio_i2c_req into the driver.
>          - Use only one buf in struct virtio_i2c_req.
>
> Changes in v5:
>          - The first version based on the acked specification.
>
>   drivers/i2c/busses/Kconfig      |  11 ++
>   drivers/i2c/busses/Makefile     |   3 +
>   drivers/i2c/busses/i2c-virtio.c | 276 ++++++++++++++++++++++++++++++++++++++++
>   include/uapi/linux/virtio_i2c.h |  40 ++++++
>   include/uapi/linux/virtio_ids.h |   1 +
>   5 files changed, 331 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..cb8d0d8 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 615f35e..efdd3f3 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -145,4 +145,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..99a1e30
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-virtio.c
> @@ -0,0 +1,276 @@
> +// 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
> + * @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 lock;
> +	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++) {
> +		if (!msgs[i].len)
> +			break;
> +
> +		/*
> +		 * Only 7-bit mode supported for this moment. For the address format,
> +		 * Please check the Virtio I2C Specification.
> +		 */
> +		reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1);
> +
> +		if (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;
> +
> +		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) {
> +			pr_err("failed to add msg[%d] to virtqueue.\n", i);
> +			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 timeout)
> +{
> +	struct virtio_i2c_req *req;
> +	bool failed = timeout;
> +	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.


So this assumes the requests are completed in order. Is this mandated in 
the spec?


> +		 */
> +		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 (timeout ? -ETIMEDOUT : j);


Checking timeout is fragile, what happens if the request are completed 
after wait_for_completion() but before virtio_i2c_complete_reqs()?


> +}
> +
> +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->lock);
> +
> +	ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
> +	if (ret == 0)
> +		goto err_unlock_free;
> +
> +	nr = ret;
> +	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, nr, !time_left);
> +
> +err_unlock_free:
> +	mutex_unlock(&vi->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 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->lock);
> +	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), "Virtio I2C Adapter");
> +	vi->adap.class = I2C_CLASS_DEPRECATED;
> +	vi->adap.algo = &virtio_algorithm;
> +	vi->adap.dev.parent = &vdev->dev;
> +	vi->adap.timeout = HZ / 10;
> +	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);
> +		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_ADAPTER, 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_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..bbcfb2c
> --- /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	0x00000001
> +
> +/**
> + * 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 bc1c062..b89391d 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_ADAPTER		34 /* virtio i2c adapter */
>   
>   #endif /* _LINUX_VIRTIO_IDS_H */


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
  2021-04-15  3:51 ` Jason Wang
@ 2021-04-15  6:17   ` Jie Deng
  0 siblings, 0 replies; 34+ messages in thread
From: Jie Deng @ 2021-04-15  6:17 UTC (permalink / raw)
  To: Jason Wang, linux-i2c, virtualization, linux-kernel
  Cc: mst, wsa, wsa+renesas, andriy.shevchenko, conghui.chen, arnd,
	kblaiech, jarkko.nikula, Sergey.Semin, rppt, loic.poulain,
	tali.perry1, u.kleine-koenig, bjorn.andersson, yu1.wang,
	shuo.a.liu, viresh.kumar, stefanha, pbonzini


On 2021/4/15 11:51, Jason Wang wrote:
>
>> +    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.
>
>
> So this assumes the requests are completed in order. Is this mandated 
> in the spec?
>
>

This is a mandatory device requirements in spec.


>> +         */
>> +        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 (timeout ? -ETIMEDOUT : j);
>
>
> Checking timeout is fragile, what happens if the request are completed 
> after wait_for_completion() but before virtio_i2c_complete_reqs()?
>
We have discussed this before in v6. 
https://lists.linuxfoundation.org/pipermail/virtualization/2021-March/053093.html.

If timeout happens, we don't need to care about the requests whether 
really completed by "HW" or not.

Just return error and let the i2c core to decide whether to resend. And 
currently, the timeout is statically configured in driver.

We may extend a device timeout value configuration in spec for driver to 
use if there is actual need in the future.



^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
  2021-04-14  3:52   ` Viresh Kumar
@ 2021-04-15  6:25     ` Jie Deng
  0 siblings, 0 replies; 34+ messages in thread
From: Jie Deng @ 2021-04-15  6:25 UTC (permalink / raw)
  To: Viresh Kumar, Wolfram Sang, wsa
  Cc: wsa, wsa, mst, linux-i2c, virtualization, linux-kernel, jasowang,
	wsa+renesas, andriy.shevchenko, conghui.chen, arnd, kblaiech,
	jarkko.nikula, Sergey.Semin, rppt, loic.poulain, tali.perry1,
	u.kleine-koenig, bjorn.andersson, yu1.wang, shuo.a.liu, stefanha


On 2021/4/14 11:52, Viresh Kumar wrote:
>
>> Is i2c/for-next the right tree to merge it
>> ?
> It should be.

Thanks Viresh.


Hi Wolfram,

Do you have any comments for this patch ? Your opinion will be important 
to improve this patch

since you are the maintainer of I2C.

Thanks,

Jie


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
  2021-03-23  9:27     ` Arnd Bergmann
  2021-03-24  1:17       ` Jie Deng
@ 2021-04-15  6:45       ` Viresh Kumar
  2021-04-15  6:56         ` Jie Deng
  1 sibling, 1 reply; 34+ messages in thread
From: Viresh Kumar @ 2021-04-15  6:45 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Jie Deng, Linux I2C, virtualization, Linux Kernel Mailing List,
	Michael S. Tsirkin, Wolfram Sang, Jason Wang, Wolfram Sang,
	Andy Shevchenko, conghui.chen, kblaiech, jarkko.nikula,
	Sergey Semin, Mike Rapoport, loic.poulain, Tali Perry,
	Uwe Kleine-König, Bjorn Andersson, yu1.wang, shuo.a.liu,
	Stefan Hajnoczi, Paolo Bonzini

On 23-03-21, 10:27, Arnd Bergmann wrote:
> I usually recommend the use of __maybe_unused for the suspend/resume
> callbacks for drivers that use SIMPLE_DEV_PM_OPS() or similar helpers
> that hide the exact conditions under which the functions get called.
> 
> In this driver, there is an explicit #ifdef in the reference to the
> functions, so
> it would make sense to use the same #ifdef around the definition.

Jie,

I was talking about this comment when I said I was expecting a new
version. I think you still need to make this change.

-- 
viresh

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
  2021-04-15  6:45       ` Viresh Kumar
@ 2021-04-15  6:56         ` Jie Deng
  2021-04-15  7:15           ` Viresh Kumar
  2021-04-15  7:21           ` Wolfram Sang
  0 siblings, 2 replies; 34+ messages in thread
From: Jie Deng @ 2021-04-15  6:56 UTC (permalink / raw)
  To: Viresh Kumar, Arnd Bergmann
  Cc: Linux I2C, virtualization, Linux Kernel Mailing List,
	Michael S. Tsirkin, Wolfram Sang, Jason Wang, Wolfram Sang,
	Andy Shevchenko, conghui.chen, kblaiech, jarkko.nikula,
	Sergey Semin, Mike Rapoport, loic.poulain, Tali Perry,
	Uwe Kleine-König, Bjorn Andersson, yu1.wang, shuo.a.liu,
	Stefan Hajnoczi, Paolo Bonzini


On 2021/4/15 14:45, Viresh Kumar wrote:
> On 23-03-21, 10:27, Arnd Bergmann wrote:
>> I usually recommend the use of __maybe_unused for the suspend/resume
>> callbacks for drivers that use SIMPLE_DEV_PM_OPS() or similar helpers
>> that hide the exact conditions under which the functions get called.
>>
>> In this driver, there is an explicit #ifdef in the reference to the
>> functions, so
>> it would make sense to use the same #ifdef around the definition.
> Jie,
>
> I was talking about this comment when I said I was expecting a new
> version. I think you still need to make this change.


I didn't forget this. It is a very small change. I'm not sure if the 
maintainer Wolfram

has any comments so that I can address them together in one version.

Thanks.



^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
  2021-04-15  6:56         ` Jie Deng
@ 2021-04-15  7:15           ` Viresh Kumar
  2021-04-15  7:21           ` Wolfram Sang
  1 sibling, 0 replies; 34+ messages in thread
From: Viresh Kumar @ 2021-04-15  7:15 UTC (permalink / raw)
  To: Jie Deng
  Cc: Arnd Bergmann, Linux I2C, virtualization,
	Linux Kernel Mailing List, Michael S. Tsirkin, Wolfram Sang,
	Jason Wang, Wolfram Sang, Andy Shevchenko, conghui.chen,
	kblaiech, jarkko.nikula, Sergey Semin, Mike Rapoport,
	loic.poulain, Tali Perry, Uwe Kleine-König, Bjorn Andersson,
	yu1.wang, shuo.a.liu, Stefan Hajnoczi, Paolo Bonzini

On 15-04-21, 14:56, Jie Deng wrote:
> 
> On 2021/4/15 14:45, Viresh Kumar wrote:
> > On 23-03-21, 10:27, Arnd Bergmann wrote:
> > > I usually recommend the use of __maybe_unused for the suspend/resume
> > > callbacks for drivers that use SIMPLE_DEV_PM_OPS() or similar helpers
> > > that hide the exact conditions under which the functions get called.
> > > 
> > > In this driver, there is an explicit #ifdef in the reference to the
> > > functions, so
> > > it would make sense to use the same #ifdef around the definition.
> > Jie,
> > 
> > I was talking about this comment when I said I was expecting a new
> > version. I think you still need to make this change.
> 
> 
> I didn't forget this. It is a very small change. I'm not sure if the
> maintainer Wolfram
> 
> has any comments so that I can address them together in one version.

Ahh, okay then. That's fine. I have been waiting for the final version
to give my Tested/reviewed by :)

-- 
viresh

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
  2021-04-15  6:56         ` Jie Deng
  2021-04-15  7:15           ` Viresh Kumar
@ 2021-04-15  7:21           ` Wolfram Sang
  2021-04-15  7:24             ` Viresh Kumar
  2021-05-12  1:37             ` Jie Deng
  1 sibling, 2 replies; 34+ messages in thread
From: Wolfram Sang @ 2021-04-15  7:21 UTC (permalink / raw)
  To: Jie Deng
  Cc: Viresh Kumar, Arnd Bergmann, Linux I2C, virtualization,
	Linux Kernel Mailing List, Michael S. Tsirkin, Jason Wang,
	Andy Shevchenko, conghui.chen, kblaiech, jarkko.nikula,
	Sergey Semin, Mike Rapoport, loic.poulain, Tali Perry,
	Uwe Kleine-König, Bjorn Andersson, yu1.wang, shuo.a.liu,
	Stefan Hajnoczi, Paolo Bonzini


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


> I didn't forget this. It is a very small change. I'm not sure if the
> maintainer Wolfram
> 
> has any comments so that I can address them together in one version.

Noted. I'll have a look in the next days.


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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
  2021-04-15  7:21           ` Wolfram Sang
@ 2021-04-15  7:24             ` Viresh Kumar
  2021-04-15  7:28               ` Wolfram Sang
  2021-05-12  1:37             ` Jie Deng
  1 sibling, 1 reply; 34+ messages in thread
From: Viresh Kumar @ 2021-04-15  7:24 UTC (permalink / raw)
  To: Wolfram Sang, Jie Deng, Arnd Bergmann, Linux I2C, virtualization,
	Linux Kernel Mailing List, Michael S. Tsirkin, Jason Wang,
	Andy Shevchenko, conghui.chen, kblaiech, jarkko.nikula,
	Sergey Semin, Mike Rapoport, loic.poulain, Tali Perry,
	Uwe Kleine-König, Bjorn Andersson, yu1.wang, shuo.a.liu,
	Stefan Hajnoczi, Paolo Bonzini

On 15-04-21, 09:21, Wolfram Sang wrote:
> 
> > I didn't forget this. It is a very small change. I'm not sure if the
> > maintainer Wolfram
> > 
> > has any comments so that I can address them together in one version.
> 
> Noted. I'll have a look in the next days.

Now that we were able to catch you, I will use the opportunity to
clarify the doubts I had.

- struct mutex lock in struct virtio_i2c, I don't think this is
  required since the core takes care of locking in absence of this.

- Use of I2C_CLASS_DEPRECATED flag, I don't think it is required for
  new drivers.

:)

-- 
viresh

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
  2021-04-15  7:24             ` Viresh Kumar
@ 2021-04-15  7:28               ` Wolfram Sang
  2021-04-15  7:37                 ` Viresh Kumar
  2021-04-15  8:15                 ` Jie Deng
  0 siblings, 2 replies; 34+ messages in thread
From: Wolfram Sang @ 2021-04-15  7:28 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Jie Deng, Arnd Bergmann, Linux I2C, virtualization,
	Linux Kernel Mailing List, Michael S. Tsirkin, Jason Wang,
	Andy Shevchenko, conghui.chen, kblaiech, jarkko.nikula,
	Sergey Semin, Mike Rapoport, loic.poulain, Tali Perry,
	Uwe Kleine-König, Bjorn Andersson, yu1.wang, shuo.a.liu,
	Stefan Hajnoczi, Paolo Bonzini


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


> Now that we were able to catch you, I will use the opportunity to
> clarify the doubts I had.
> 
> - struct mutex lock in struct virtio_i2c, I don't think this is
>   required since the core takes care of locking in absence of this.

This is likely correct.

> - Use of I2C_CLASS_DEPRECATED flag, I don't think it is required for
>   new drivers.

This is definately correct :)

Let's see if I will have more questions...


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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
  2021-04-15  7:28               ` Wolfram Sang
@ 2021-04-15  7:37                 ` Viresh Kumar
  2021-04-15  8:15                 ` Jie Deng
  1 sibling, 0 replies; 34+ messages in thread
From: Viresh Kumar @ 2021-04-15  7:37 UTC (permalink / raw)
  To: Wolfram Sang, Jie Deng, Arnd Bergmann, Linux I2C, virtualization,
	Linux Kernel Mailing List, Michael S. Tsirkin, Jason Wang,
	Andy Shevchenko, conghui.chen, kblaiech, jarkko.nikula,
	Sergey Semin, Mike Rapoport, loic.poulain, Tali Perry,
	Uwe Kleine-König, Bjorn Andersson, yu1.wang, shuo.a.liu,
	Stefan Hajnoczi, Paolo Bonzini

On 15-04-21, 09:28, Wolfram Sang wrote:
> 
> > Now that we were able to catch you, I will use the opportunity to
> > clarify the doubts I had.
> > 
> > - struct mutex lock in struct virtio_i2c, I don't think this is
> >   required since the core takes care of locking in absence of this.
> 
> This is likely correct.
> 
> > - Use of I2C_CLASS_DEPRECATED flag, I don't think it is required for
> >   new drivers.
> 
> This is definately correct :)

Glad to hear that. Thanks.

-- 
viresh

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
  2021-04-15  7:28               ` Wolfram Sang
  2021-04-15  7:37                 ` Viresh Kumar
@ 2021-04-15  8:15                 ` Jie Deng
  2021-04-15  8:18                   ` Wolfram Sang
  1 sibling, 1 reply; 34+ messages in thread
From: Jie Deng @ 2021-04-15  8:15 UTC (permalink / raw)
  To: Wolfram Sang, Viresh Kumar, Arnd Bergmann, Linux I2C,
	virtualization, Linux Kernel Mailing List, Michael S. Tsirkin,
	Jason Wang, Andy Shevchenko, conghui.chen, kblaiech,
	jarkko.nikula, Sergey Semin, Mike Rapoport, loic.poulain,
	Tali Perry, Uwe Kleine-König, Bjorn Andersson, yu1.wang,
	shuo.a.liu, Stefan Hajnoczi, Paolo Bonzini

On 2021/4/15 15:28, Wolfram Sang wrote:

>> Now that we were able to catch you, I will use the opportunity to
>> clarify the doubts I had.
>>
>> - struct mutex lock in struct virtio_i2c, I don't think this is
>>    required since the core takes care of locking in absence of this.
> This is likely correct.


OK. Then I will remove the lock.


>> - Use of I2C_CLASS_DEPRECATED flag, I don't think it is required for
>>    new drivers.
> This is definately correct :)


Do you mean a new driver doesn't need to set the following ?

vi->adap.class = I2C_CLASS_DEPRECATED;

Just leave the class to be 0 ?


>
> Let's see if I will have more questions...


OK. Thank you.


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
  2021-04-15  8:15                 ` Jie Deng
@ 2021-04-15  8:18                   ` Wolfram Sang
  2021-04-15  8:20                     ` Jie Deng
  0 siblings, 1 reply; 34+ messages in thread
From: Wolfram Sang @ 2021-04-15  8:18 UTC (permalink / raw)
  To: Jie Deng
  Cc: Viresh Kumar, Arnd Bergmann, Linux I2C, virtualization,
	Linux Kernel Mailing List, Michael S. Tsirkin, Jason Wang,
	Andy Shevchenko, conghui.chen, kblaiech, jarkko.nikula,
	Sergey Semin, Mike Rapoport, loic.poulain, Tali Perry,
	Uwe Kleine-König, Bjorn Andersson, yu1.wang, shuo.a.liu,
	Stefan Hajnoczi, Paolo Bonzini


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

On Thu, Apr 15, 2021 at 04:15:07PM +0800, Jie Deng wrote:
> On 2021/4/15 15:28, Wolfram Sang wrote:
> 
> > > Now that we were able to catch you, I will use the opportunity to
> > > clarify the doubts I had.
> > > 
> > > - struct mutex lock in struct virtio_i2c, I don't think this is
> > >    required since the core takes care of locking in absence of this.
> > This is likely correct.
> 
> OK. Then I will remove the lock.

Let me have a look first, please.

> > > - Use of I2C_CLASS_DEPRECATED flag, I don't think it is required for
> > >    new drivers.
> > This is definately correct :)
> 
> Do you mean a new driver doesn't need to set the following ?
> 
> vi->adap.class = I2C_CLASS_DEPRECATED;
> 
> Just leave the class to be 0 ?

Yes. DEPRECATED is only for drivers which used to have a class and then
chose to remove it.


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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
  2021-04-15  8:18                   ` Wolfram Sang
@ 2021-04-15  8:20                     ` Jie Deng
  0 siblings, 0 replies; 34+ messages in thread
From: Jie Deng @ 2021-04-15  8:20 UTC (permalink / raw)
  To: Wolfram Sang, Viresh Kumar, Arnd Bergmann, Linux I2C,
	virtualization, Linux Kernel Mailing List, Michael S. Tsirkin,
	Jason Wang, Andy Shevchenko, conghui.chen, kblaiech,
	jarkko.nikula, Sergey Semin, Mike Rapoport, loic.poulain,
	Tali Perry, Uwe Kleine-König, Bjorn Andersson, yu1.wang,
	shuo.a.liu, Stefan Hajnoczi, Paolo Bonzini


On 2021/4/15 16:18, Wolfram Sang wrote:
> On Thu, Apr 15, 2021 at 04:15:07PM +0800, Jie Deng wrote:
>> On 2021/4/15 15:28, Wolfram Sang wrote:
>>
>>>> Now that we were able to catch you, I will use the opportunity to
>>>> clarify the doubts I had.
>>>>
>>>> - struct mutex lock in struct virtio_i2c, I don't think this is
>>>>     required since the core takes care of locking in absence of this.
>>> This is likely correct.
>> OK. Then I will remove the lock.
> Let me have a look first, please.


Sure. Thank you.


>>>> - Use of I2C_CLASS_DEPRECATED flag, I don't think it is required for
>>>>     new drivers.
>>> This is definately correct :)
>> Do you mean a new driver doesn't need to set the following ?
>>
>> vi->adap.class = I2C_CLASS_DEPRECATED;
>>
>> Just leave the class to be 0 ?
> Yes. DEPRECATED is only for drivers which used to have a class and then
> chose to remove it.


Got it. Thanks for your clarification.



^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
  2021-04-15  7:21           ` Wolfram Sang
  2021-04-15  7:24             ` Viresh Kumar
@ 2021-05-12  1:37             ` Jie Deng
  1 sibling, 0 replies; 34+ messages in thread
From: Jie Deng @ 2021-05-12  1:37 UTC (permalink / raw)
  To: Wolfram Sang, Viresh Kumar, Arnd Bergmann, Linux I2C,
	virtualization, Linux Kernel Mailing List, Michael S. Tsirkin,
	Jason Wang, Andy Shevchenko, conghui.chen, kblaiech,
	jarkko.nikula, Sergey Semin, Mike Rapoport, loic.poulain,
	Tali Perry, Uwe Kleine-König, Bjorn Andersson, yu1.wang,
	shuo.a.liu, Stefan Hajnoczi

On 2021/4/15 15:21, Wolfram Sang wrote:

>> I didn't forget this. It is a very small change. I'm not sure if the
>> maintainer Wolfram
>>
>> has any comments so that I can address them together in one version.
> Noted. I'll have a look in the next days.

Hi Wolfram,

Kindly reminder. Hope this patch hasn't been forgotten. :)

Thanks.


^ permalink raw reply	[flat|nested] 34+ messages in thread

end of thread, back to index

Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-23 14:19 [PATCH v10] i2c: virtio: add a virtio i2c frontend driver Jie Deng
2021-03-23  7:27 ` Viresh Kumar
2021-03-23  8:33   ` Jie Deng
2021-03-23  8:37     ` Viresh Kumar
2021-03-23  9:27     ` Arnd Bergmann
2021-03-24  1:17       ` Jie Deng
2021-03-24  4:00         ` Viresh Kumar
2021-04-15  6:45       ` Viresh Kumar
2021-04-15  6:56         ` Jie Deng
2021-04-15  7:15           ` Viresh Kumar
2021-04-15  7:21           ` Wolfram Sang
2021-04-15  7:24             ` Viresh Kumar
2021-04-15  7:28               ` Wolfram Sang
2021-04-15  7:37                 ` Viresh Kumar
2021-04-15  8:15                 ` Jie Deng
2021-04-15  8:18                   ` Wolfram Sang
2021-04-15  8:20                     ` Jie Deng
2021-05-12  1:37             ` Jie Deng
2021-03-23  9:01 ` Viresh Kumar
2021-03-23  9:38   ` Viresh Kumar
2021-03-24  0:53     ` Jie Deng
2021-03-24  3:52       ` Viresh Kumar
2021-03-24  4:00         ` Jie Deng
2021-03-24  4:02           ` Viresh Kumar
2021-03-24  4:20 ` Viresh Kumar
2021-03-24  6:05   ` Jie Deng
2021-03-24  6:09     ` Viresh Kumar
2021-03-24  6:41       ` Jie Deng
2021-03-24  6:44         ` Viresh Kumar
2021-04-14  2:07 ` Jie Deng
2021-04-14  3:52   ` Viresh Kumar
2021-04-15  6:25     ` Jie Deng
2021-04-15  3:51 ` Jason Wang
2021-04-15  6:17   ` Jie Deng

Linux-i2c Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-i2c/0 linux-i2c/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-i2c linux-i2c/ https://lore.kernel.org/linux-i2c \
		linux-i2c@vger.kernel.org
	public-inbox-index linux-i2c

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-i2c


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git