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

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

[Jie Deng]

Sorry , sent the wrong version. Please ignore this. I will resend.

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

[Viresh Kumar]

I saw your email about wrong version being sent, I already wrote some
reviews. Sending them anyway for FWIW :)

On 12-03-21, 21:33, Jie Deng wrote:
> diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
> new file mode 100644
> index 0000000..bbde8de
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-virtio.c
> @@ -0,0 +1,287 @@
> +// 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
> + * @i2c_lock: lock for virtqueue processing
> + * @vq: the virtio virtqueue for communication
> + */
> +struct virtio_i2c {
> +	struct virtio_device *vdev;
> +	struct completion completion;
> +	struct i2c_adapter adap;
> +	struct mutex lock;

As I said in the previous version (Yes, we were both waiting for
Wolfram to answer that), this lock shouldn't be required at all.

And since none of us have a use-case at hand where we will have a
problem without this lock, we should really skip it. We can always
come back and add it if we find an issue somewhere. Until then, better
to keep it simple.

> +	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;
> +	uint8_t *buf;
> +	struct virtio_i2c_in_hdr in_hdr;
> +};
> +
> +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)
> +{
> +	struct virtio_i2c_req *req;
> +	unsigned int len, unused;
> +	int i, j;
> +
> +	for (i = 0; i < nr; i++) {
> +		req = virtqueue_get_buf(vq, &len);
> +		if (!(req && req == &reqs[i])) {
> +			pr_err("msg[%d]: addr=0x%x is out of order.\n", i, msgs[i].addr);
> +			break;
> +		}
> +
> +		if (req->in_hdr.status != VIRTIO_I2C_MSG_OK) {
> +			pr_err("msg[%d]: addr=0x%x backend error.\n", i, msgs[i].addr);
> +			break;
> +		}
> +
> +		i2c_put_dma_safe_msg_buf(req->buf, &msgs[i], true);
> +	}
> +
> +	/* Release unused DMA safe buffer if any */
> +	for (j = i; j < nr; j++)
> +		i2c_put_dma_safe_msg_buf(req->buf, &msgs[j], false);
> +
> +	/* Detach all the used buffers from the vq */
> +	while (virtqueue_get_buf(vq, &unused))
> +		;
> +
> +	return i;
> +}
> +
> +static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> +{
> +	struct virtio_i2c *vi = i2c_get_adapdata(adap);
> +	struct virtqueue *vq = vi->vq;
> +	struct virtio_i2c_req *reqs;
> +	unsigned long time_left;
> +	int ret, nr;
> +
> +	reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL);
> +	if (!reqs)
> +		return -ENOMEM;
> +
> +	mutex_lock(&vi->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 = -ETIMEDOUT;
> +		goto err_unlock_free;
> +	}
> +
> +	ret = virtio_i2c_complete_reqs(vq, reqs, msgs, nr);
> +
> +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 struct i2c_adapter virtio_adapter = {
> +	.owner = THIS_MODULE,
> +	.name = "Virtio I2C Adapter",
> +	.class = I2C_CLASS_DEPRECATED,

What happened to this discussion ?

https://lore.kernel.org/lkml/20210305072903.wtw645rukmqr5hx5@vireshk-i7/

> +	.algo = &virtio_algorithm,
> +};
> +
> +static int virtio_i2c_probe(struct virtio_device *vdev)
> +{
> +	struct device *pdev = vdev->dev.parent;
> +	struct virtio_i2c *vi;
> +	int ret;
> +
> +	vi = devm_kzalloc(&vdev->dev, sizeof(*vi), GFP_KERNEL);
> +	if (!vi)
> +		return -ENOMEM;
> +
> +	vdev->priv = vi;
> +	vi->vdev = vdev;
> +
> +	mutex_init(&vi->lock);
> +	init_completion(&vi->completion);
> +
> +	ret = virtio_i2c_setup_vqs(vi);
> +	if (ret)
> +		return ret;
> +
> +	vi->adap = virtio_adapter;

This is strange, why are you allocating memory for adapter twice ?
Once for virtio_adapter and once for vi->adap ? Either fill the fields
directly for v->adap here and remove virtio_adapter or make vi->adap a
pointer.

> +	i2c_set_adapdata(&vi->adap, vi);
> +	vi->adap.dev.parent = &vdev->dev;
> +
> +	/* Setup ACPI node for controlled devices which will be probed through ACPI */
> +	ACPI_COMPANION_SET(&vi->adap.dev, ACPI_COMPANION(pdev));
> +	vi->adap.timeout = HZ / 10;
> +
> +	ret = i2c_add_adapter(&vi->adap);
> +	if (ret) {
> +		virtio_i2c_del_vqs(vdev);
> +		dev_err(&vdev->dev, "failed to add virtio-i2c adapter.\n");
> +	}
> +
> +	return ret;
> +}
> +
> +static void virtio_i2c_remove(struct virtio_device *vdev)
> +{
> +	struct virtio_i2c *vi = vdev->priv;
> +
> +	i2c_del_adapter(&vi->adap);
> +	virtio_i2c_del_vqs(vdev);
> +}
> +
> +static struct virtio_device_id id_table[] = {
> +	{ VIRTIO_ID_I2C_ADPTER, VIRTIO_DEV_ANY_ID },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(virtio, id_table);
> +
> +static int __maybe_unused virtio_i2c_freeze(struct virtio_device *vdev)
> +{
> +	virtio_i2c_del_vqs(vdev);
> +	return 0;
> +}
> +
> +static int __maybe_unused virtio_i2c_restore(struct virtio_device *vdev)
> +{
> +	return virtio_i2c_setup_vqs(vdev->priv);
> +}
> +
> +static struct virtio_driver virtio_i2c_driver = {
> +	.id_table	= id_table,
> +	.probe		= virtio_i2c_probe,
> +	.remove		= virtio_i2c_remove,
> +	.driver	= {
> +		.name	= "i2c_virtio",
> +	},
> +#ifdef CONFIG_PM_SLEEP
> +	.freeze = virtio_i2c_freeze,
> +	.restore = virtio_i2c_restore,
> +#endif
> +};
> +module_virtio_driver(virtio_i2c_driver);
> +
> +MODULE_AUTHOR("Jie Deng <jie.deng@intel.com>");
> +MODULE_AUTHOR("Conghui Chen <conghui.chen@intel.com>");
> +MODULE_DESCRIPTION("Virtio i2c bus driver");
> +MODULE_LICENSE("GPL");
-- 
viresh

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

[Jie Deng]

On 2021/3/12 14:10, Viresh Kumar wrote:
> I saw your email about wrong version being sent, I already wrote some
> reviews. Sending them anyway for FWIW :)
>
> On 12-03-21, 21:33, Jie Deng wrote:
>> +struct virtio_i2c {
>> +	struct virtio_device *vdev;
>> +	struct completion completion;
>> +	struct i2c_adapter adap;
>> +	struct mutex lock;
> As I said in the previous version (Yes, we were both waiting for
> Wolfram to answer that), this lock shouldn't be required at all.
>
> And since none of us have a use-case at hand where we will have a
> problem without this lock, we should really skip it. We can always
> come back and add it if we find an issue somewhere. Until then, better
> to keep it simple.
The problem is you can't guarantee that adap->algo->master_xfer
is only called from i2c_transfer. Any function who holds the adapter can 
call
adap->algo->master_xfer directly. I prefer to avoid potential issues 
rather than
find a issue then fix.
>
>> +
>> +static struct i2c_adapter virtio_adapter = {
>> +	.owner = THIS_MODULE,
>> +	.name = "Virtio I2C Adapter",
>> +	.class = I2C_CLASS_DEPRECATED,
> What happened to this discussion ?
>
> https://lore.kernel.org/lkml/20210305072903.wtw645rukmqr5hx5@vireshk-i7/

My understanding is that new driver sets this to warn users that the 
adapter doesn't support classes anymore.

I'm not sure if Wolfram can explain it more clear for you.

>
>> +	.algo = &virtio_algorithm,
>> +
>> +		return ret;
>> +
>> +	vi->adap = virtio_adapter;
> This is strange, why are you allocating memory for adapter twice ?
> Once for virtio_adapter and once for vi->adap ? Either fill the fields
> directly for v->adap here and remove virtio_adapter or make vi->adap a
> pointer.

Will make vi->adap a pointer. Thanks.

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

[Viresh Kumar]

On 12-03-21, 15:51, Jie Deng wrote:
> 
> On 2021/3/12 14:10, Viresh Kumar wrote:
> > I saw your email about wrong version being sent, I already wrote some
> > reviews. Sending them anyway for FWIW :)
> > 
> > On 12-03-21, 21:33, Jie Deng wrote:
> > > +struct virtio_i2c {
> > > +	struct virtio_device *vdev;
> > > +	struct completion completion;
> > > +	struct i2c_adapter adap;
> > > +	struct mutex lock;
> > As I said in the previous version (Yes, we were both waiting for
> > Wolfram to answer that), this lock shouldn't be required at all.
> > 
> > And since none of us have a use-case at hand where we will have a
> > problem without this lock, we should really skip it. We can always
> > come back and add it if we find an issue somewhere. Until then, better
> > to keep it simple.

> The problem is you can't guarantee that adap->algo->master_xfer
> is only called from i2c_transfer. Any function who holds the adapter can
> call
> adap->algo->master_xfer directly.

See my last reply here, (almost) no one in the mainline kernel call it
directly. And perhaps you can consider the caller broken in that case
and so there is no need of an extra lock, unless you have a case that
is broken.

https://lore.kernel.org/lkml/20210305072903.wtw645rukmqr5hx5@vireshk-i7/

> I prefer to avoid potential issues rather
> than
> find a issue then fix.

This is a very hypothetical issue IMHO as the kernel code doesn't have
such a user. There is no need of locks here, else the i2c core won't
have handled it by itself.

> > 
> > > +
> > > +static struct i2c_adapter virtio_adapter = {
> > > +	.owner = THIS_MODULE,
> > > +	.name = "Virtio I2C Adapter",
> > > +	.class = I2C_CLASS_DEPRECATED,
> > What happened to this discussion ?
> > 
> > https://lore.kernel.org/lkml/20210305072903.wtw645rukmqr5hx5@vireshk-i7/
> 
> My understanding is that new driver sets this to warn users that the adapter
> doesn't support classes anymore.

I think the warning is relevant for old drivers who used to support
classes and not for new ones.

> I'm not sure if Wolfram can explain it more clear for you.

Okay, lemme dig in a bit then.

$ git grep -l i2c_add_adapter drivers/i2c/busses/ | wc -l
77

$ git grep -l I2C_CLASS_DEPRECATED drivers/i2c/busses/
drivers/i2c/busses/i2c-at91-core.c
drivers/i2c/busses/i2c-bcm2835.c
drivers/i2c/busses/i2c-davinci.c
drivers/i2c/busses/i2c-designware-platdrv.c
drivers/i2c/busses/i2c-mv64xxx.c
drivers/i2c/busses/i2c-nomadik.c
drivers/i2c/busses/i2c-ocores.c
drivers/i2c/busses/i2c-omap.c
drivers/i2c/busses/i2c-rcar.c
drivers/i2c/busses/i2c-s3c2410.c
drivers/i2c/busses/i2c-tegra.c
drivers/i2c/busses/i2c-virtio.c
drivers/i2c/busses/i2c-xiic.c

i.e. only 13 of 77 drivers are using this flag.

The latest addition among these drivers is i2c-bcm2835.c and it was
added back in 2013 and the flag was added to it in 2014:

commit 37888f71e2c9 ("i2c: i2c-bcm2835: deprecate class based instantiation")

FWIW, I also checked all the new drivers added since kernel release
v4.0 and none of them set this flag.

-- 
viresh

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

[Jie Deng]

On 2021/3/12 16:11, Viresh Kumar wrote:
> On 12-03-21, 15:51, Jie Deng wrote:
>> On 2021/3/12 14:10, Viresh Kumar wrote:
>>> I saw your email about wrong version being sent, I already wrote some
>>> reviews. Sending them anyway for FWIW :)
>>>
>>> On 12-03-21, 21:33, Jie Deng wrote:
>>>> +struct virtio_i2c {
>>>> +	struct virtio_device *vdev;
>>>> +	struct completion completion;
>>>> +	struct i2c_adapter adap;
>>>> +	struct mutex lock;
>>> As I said in the previous version (Yes, we were both waiting for
>>> Wolfram to answer that), this lock shouldn't be required at all.
>>>
>>> And since none of us have a use-case at hand where we will have a
>>> problem without this lock, we should really skip it. We can always
>>> come back and add it if we find an issue somewhere. Until then, better
>>> to keep it simple.
>> The problem is you can't guarantee that adap->algo->master_xfer
>> is only called from i2c_transfer. Any function who holds the adapter can
>> call
>> adap->algo->master_xfer directly.
> See my last reply here, (almost) no one in the mainline kernel call it
> directly. And perhaps you can consider the caller broken in that case
> and so there is no need of an extra lock, unless you have a case that
> is broken.
>
> https://lore.kernel.org/lkml/20210305072903.wtw645rukmqr5hx5@vireshk-i7/
>
>> I prefer to avoid potential issues rather
>> than
>> find a issue then fix.
> This is a very hypothetical issue IMHO as the kernel code doesn't have
> such a user. There is no need of locks here, else the i2c core won't
> have handled it by itself.

I'd like to see Wolfram's opinion.
Is it safe to remove lock in adap->algo->master_xfer ?

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

[Arnd Bergmann]

On Fri, Mar 12, 2021 at 2:33 PM Jie Deng <jie.deng@intel.com> wrote:

> +
> +/**
> + * 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;
> +       uint8_t *buf;
> +       struct virtio_i2c_in_hdr in_hdr;
> +};

The simpler request structure clearly looks better than the previous version,
but I think I found another problem here, at least a theoretical one:

When you map the headers into the DMA address space, they should
be in separate cache lines, to allow the DMA mapping interfaces to
perform cache management on each one without accidentally clobbering
another member.

So far I think there is an assumption that virtio buffers are always
on cache-coherent devices, but if you ever have a virtio-i2c device
backend on a physical interconnect that is not cache coherent (e.g. a
microcontroller that shares the memory bus), this breaks down.

You could avoid this by either allocating arrays of each type separately,
or by marking each member that you pass to the device as
____cacheline_aligned.

      Arnd

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

[Jie Deng]

Add an I2C bus driver for virtio para-virtualization.

The controller can be emulated by the backend driver in
any device model software by following the virtio protocol.

The device specification can be found on
https://lists.oasis-open.org/archives/virtio-comment/202101/msg00008.html.

By following the specification, people may implement different
backend drivers to emulate different controllers according to
their needs.

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

 drivers/i2c/busses/Kconfig      |  11 ++
 drivers/i2c/busses/Makefile     |   3 +
 drivers/i2c/busses/i2c-virtio.c | 287 ++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/virtio_i2c.h |  40 ++++++
 include/uapi/linux/virtio_ids.h |   1 +
 5 files changed, 342 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..bbde8de
--- /dev/null
+++ b/drivers/i2c/busses/i2c-virtio.c
@@ -0,0 +1,287 @@
+// 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
+ * @i2c_lock: lock for virtqueue processing
+ * @vq: the virtio virtqueue for communication
+ */
+struct virtio_i2c {
+	struct virtio_device *vdev;
+	struct completion completion;
+	struct i2c_adapter adap;
+	struct mutex 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;
+	uint8_t *buf;
+	struct virtio_i2c_in_hdr in_hdr;
+};
+
+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)
+{
+	struct virtio_i2c_req *req;
+	unsigned int len, unused;
+	int i, j;
+
+	for (i = 0; i < nr; i++) {
+		req = virtqueue_get_buf(vq, &len);
+		if (!(req && req == &reqs[i])) {
+			pr_err("msg[%d]: addr=0x%x is out of order.\n", i, msgs[i].addr);
+			break;
+		}
+
+		if (req->in_hdr.status != VIRTIO_I2C_MSG_OK) {
+			pr_err("msg[%d]: addr=0x%x backend error.\n", i, msgs[i].addr);
+			break;
+		}
+
+		i2c_put_dma_safe_msg_buf(req->buf, &msgs[i], true);
+	}
+
+	/* Release unused DMA safe buffer if any */
+	for (j = i; j < nr; j++)
+		i2c_put_dma_safe_msg_buf(req->buf, &msgs[j], false);
+
+	/* Detach all the used buffers from the vq */
+	while (virtqueue_get_buf(vq, &unused))
+		;
+
+	return i;
+}
+
+static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
+{
+	struct virtio_i2c *vi = i2c_get_adapdata(adap);
+	struct virtqueue *vq = vi->vq;
+	struct virtio_i2c_req *reqs;
+	unsigned long time_left;
+	int ret, nr;
+
+	reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL);
+	if (!reqs)
+		return -ENOMEM;
+
+	mutex_lock(&vi->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 = -ETIMEDOUT;
+		goto err_unlock_free;
+	}
+
+	ret = virtio_i2c_complete_reqs(vq, reqs, msgs, nr);
+
+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 struct i2c_adapter virtio_adapter = {
+	.owner = THIS_MODULE,
+	.name = "Virtio I2C Adapter",
+	.class = I2C_CLASS_DEPRECATED,
+	.algo = &virtio_algorithm,
+};
+
+static int virtio_i2c_probe(struct virtio_device *vdev)
+{
+	struct device *pdev = vdev->dev.parent;
+	struct virtio_i2c *vi;
+	int ret;
+
+	vi = devm_kzalloc(&vdev->dev, sizeof(*vi), GFP_KERNEL);
+	if (!vi)
+		return -ENOMEM;
+
+	vdev->priv = vi;
+	vi->vdev = vdev;
+
+	mutex_init(&vi->lock);
+	init_completion(&vi->completion);
+
+	ret = virtio_i2c_setup_vqs(vi);
+	if (ret)
+		return ret;
+
+	vi->adap = virtio_adapter;
+	i2c_set_adapdata(&vi->adap, vi);
+	vi->adap.dev.parent = &vdev->dev;
+
+	/* Setup ACPI node for controlled devices which will be probed through ACPI */
+	ACPI_COMPANION_SET(&vi->adap.dev, ACPI_COMPANION(pdev));
+	vi->adap.timeout = HZ / 10;
+
+	ret = i2c_add_adapter(&vi->adap);
+	if (ret) {
+		virtio_i2c_del_vqs(vdev);
+		dev_err(&vdev->dev, "failed to add virtio-i2c adapter.\n");
+	}
+
+	return ret;
+}
+
+static void virtio_i2c_remove(struct virtio_device *vdev)
+{
+	struct virtio_i2c *vi = vdev->priv;
+
+	i2c_del_adapter(&vi->adap);
+	virtio_i2c_del_vqs(vdev);
+}
+
+static struct virtio_device_id id_table[] = {
+	{ VIRTIO_ID_I2C_ADPTER, VIRTIO_DEV_ANY_ID },
+	{}
+};
+MODULE_DEVICE_TABLE(virtio, id_table);
+
+static int __maybe_unused virtio_i2c_freeze(struct virtio_device *vdev)
+{
+	virtio_i2c_del_vqs(vdev);
+	return 0;
+}
+
+static int __maybe_unused virtio_i2c_restore(struct virtio_device *vdev)
+{
+	return virtio_i2c_setup_vqs(vdev->priv);
+}
+
+static struct virtio_driver virtio_i2c_driver = {
+	.id_table	= id_table,
+	.probe		= virtio_i2c_probe,
+	.remove		= virtio_i2c_remove,
+	.driver	= {
+		.name	= "i2c_virtio",
+	},
+#ifdef CONFIG_PM_SLEEP
+	.freeze = virtio_i2c_freeze,
+	.restore = virtio_i2c_restore,
+#endif
+};
+module_virtio_driver(virtio_i2c_driver);
+
+MODULE_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..6ae32db 100644
--- a/include/uapi/linux/virtio_ids.h
+++ b/include/uapi/linux/virtio_ids.h
@@ -54,5 +54,6 @@
 #define VIRTIO_ID_FS			26 /* virtio filesystem */
 #define VIRTIO_ID_PMEM			27 /* virtio pmem */
 #define VIRTIO_ID_MAC80211_HWSIM	29 /* virtio mac80211-hwsim */
+#define VIRTIO_ID_I2C_ADPTER		34 /* virtio i2c adpter */
 
 #endif /* _LINUX_VIRTIO_IDS_H */
-- 
2.7.4

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

[Jie Deng]

On 2021/3/12 16:58, Arnd Bergmann wrote:
> On Fri, Mar 12, 2021 at 2:33 PM Jie Deng <jie.deng@intel.com> wrote:
>
>> +
>> +/**
>> + * 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;
>> +       uint8_t *buf;
>> +       struct virtio_i2c_in_hdr in_hdr;
>> +};
> The simpler request structure clearly looks better than the previous version,
> but I think I found another problem here, at least a theoretical one:
>
> When you map the headers into the DMA address space, they should
> be in separate cache lines, to allow the DMA mapping interfaces to
> perform cache management on each one without accidentally clobbering
> another member.
>
> So far I think there is an assumption that virtio buffers are always
> on cache-coherent devices, but if you ever have a virtio-i2c device
> backend on a physical interconnect that is not cache coherent (e.g. a
> microcontroller that shares the memory bus), this breaks down.
>
> You could avoid this by either allocating arrays of each type separately,
> or by marking each member that you pass to the device as
> ____cacheline_aligned.
>
>        Arnd
The virtio devices are software emulated. The backend software may need to
consider this since it may exchange data with physical devices. But I 
don't think
we need it for this interface, because no DMA operation is involved 
between the
frontend driver and backend driver.

Regards,

Jie

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

[Jason Wang]

On 2021/3/15 9:14 上午, Jie Deng wrote:
>
> On 2021/3/12 16:58, Arnd Bergmann wrote:
>> On Fri, Mar 12, 2021 at 2:33 PM Jie Deng <jie.deng@intel.com> wrote:
>>
>>> +
>>> +/**
>>> + * 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;
>>> +       uint8_t *buf;
>>> +       struct virtio_i2c_in_hdr in_hdr;
>>> +};
>> The simpler request structure clearly looks better than the previous 
>> version,
>> but I think I found another problem here, at least a theoretical one:
>>
>> When you map the headers into the DMA address space, they should
>> be in separate cache lines, to allow the DMA mapping interfaces to
>> perform cache management on each one without accidentally clobbering
>> another member.
>>
>> So far I think there is an assumption that virtio buffers are always
>> on cache-coherent devices, but if you ever have a virtio-i2c device
>> backend on a physical interconnect that is not cache coherent (e.g. a
>> microcontroller that shares the memory bus), this breaks down.
>>
>> You could avoid this by either allocating arrays of each type 
>> separately,
>> or by marking each member that you pass to the device as
>> ____cacheline_aligned.
>>
>>        Arnd
> The virtio devices are software emulated.


This is not correct. There're already a brunch hardware virtio devices.

Thanks


> The backend software may need to
> consider this since it may exchange data with physical devices. But I 
> don't think
> we need it for this interface, because no DMA operation is involved 
> between the
> frontend driver and backend driver.
>
> Regards,
>
> Jie
>
>

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

[Jie Deng]

On 2021/3/15 11:13, Jason Wang wrote:
>
> On 2021/3/15 9:14 上午, Jie Deng wrote:
>>
>> On 2021/3/12 16:58, Arnd Bergmann wrote:
>>> On Fri, Mar 12, 2021 at 2:33 PM Jie Deng <jie.deng@intel.com> wrote:
>>>
>>>> +
>>>> +/**
>>>> + * 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;
>>>> +       uint8_t *buf;
>>>> +       struct virtio_i2c_in_hdr in_hdr;
>>>> +};
>>> The simpler request structure clearly looks better than the previous 
>>> version,
>>> but I think I found another problem here, at least a theoretical one:
>>>
>>> When you map the headers into the DMA address space, they should
>>> be in separate cache lines, to allow the DMA mapping interfaces to
>>> perform cache management on each one without accidentally clobbering
>>> another member.
>>>
>>> So far I think there is an assumption that virtio buffers are always
>>> on cache-coherent devices, but if you ever have a virtio-i2c device
>>> backend on a physical interconnect that is not cache coherent (e.g. a
>>> microcontroller that shares the memory bus), this breaks down.
>>>
>>> You could avoid this by either allocating arrays of each type 
>>> separately,
>>> or by marking each member that you pass to the device as
>>> ____cacheline_aligned.
>>>
>>>        Arnd
>> The virtio devices are software emulated.
>
>
> This is not correct. There're already a brunch hardware virtio devices.
>
> Thanks
>
Then do you think it is necessary to mark the virtio bufs with 
____cacheline_aligned ?

I haven't seen any virtio interface being marked yet. If this is a 
problem, I believe it

should  be common for all virtio devices, right ?

Thanks.


>
>> The backend software may need to
>> consider this since it may exchange data with physical devices. But I 
>> don't think
>> we need it for this interface, because no DMA operation is involved 
>> between the
>> frontend driver and backend driver.
>>
>> Regards,
>>
>> Jie
>>
>>
>

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

[Arnd Bergmann]

On Mon, Mar 15, 2021 at 6:54 AM Jie Deng <jie.deng@intel.com> wrote:
> On 2021/3/15 11:13, Jason Wang wrote:
> > On 2021/3/15 9:14 上午, Jie Deng wrote:
> >> On 2021/3/12 16:58, Arnd Bergmann wrote:
> >
> Then do you think it is necessary to mark the virtio bufs with
> ____cacheline_aligned ?

I think so, yes.

> I haven't seen any virtio interface being marked yet. If this is a
> problem, I believe it should  be common for all virtio devices, right ?

Yes, but it's not a problem if the buffers are allocated separately
because kmalloc provinces a cachelinen aligned buffer on architectures
that need it.

It's only a problem here because there is a single allocation for three
objects that have different ownership states during the DMA (device
owned to-device, cpu-owned, device owned to-cpu).

       Arnd

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

[Jie Deng]

On 2021/3/15 15:52, Arnd Bergmann wrote:
> On Mon, Mar 15, 2021 at 6:54 AM Jie Deng <jie.deng@intel.com> wrote:
>> On 2021/3/15 11:13, Jason Wang wrote:
>>> On 2021/3/15 9:14 上午, Jie Deng wrote:
>>>> On 2021/3/12 16:58, Arnd Bergmann wrote:
>> Then do you think it is necessary to mark the virtio bufs with
>> ____cacheline_aligned ?
> I think so, yes.
>
>> I haven't seen any virtio interface being marked yet. If this is a
>> problem, I believe it should  be common for all virtio devices, right ?
> Yes, but it's not a problem if the buffers are allocated separately
> because kmalloc provinces a cachelinen aligned buffer on architectures
> that need it.
>
> It's only a problem here because there is a single allocation for three
> objects that have different ownership states during the DMA (device
> owned to-device, cpu-owned, device owned to-cpu).
>
>         Arnd
I'm not sure if this will actually cause a problem. But I'm OK to mark 
the items
in struct virtio_i2c_req with  ____cacheline_aligned to avoid potential 
problem
as you said.

Thank you.

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

[Viresh Kumar]

On 12-03-21, 13:41, Viresh Kumar wrote:
> > > > +static struct i2c_adapter virtio_adapter = {
> > > > +	.owner = THIS_MODULE,
> > > > +	.name = "Virtio I2C Adapter",
> > > > +	.class = I2C_CLASS_DEPRECATED,
> > > What happened to this discussion ?
> > > 
> > > https://lore.kernel.org/lkml/20210305072903.wtw645rukmqr5hx5@vireshk-i7/
> > 
> > My understanding is that new driver sets this to warn users that the adapter
> > doesn't support classes anymore.
> 
> I think the warning is relevant for old drivers who used to support
> classes and not for new ones.
> 
> > I'm not sure if Wolfram can explain it more clear for you.
> 
> Okay, lemme dig in a bit then.
> 
> $ git grep -l i2c_add_adapter drivers/i2c/busses/ | wc -l
> 77
> 
> $ git grep -l I2C_CLASS_DEPRECATED drivers/i2c/busses/
> drivers/i2c/busses/i2c-at91-core.c
> drivers/i2c/busses/i2c-bcm2835.c
> drivers/i2c/busses/i2c-davinci.c
> drivers/i2c/busses/i2c-designware-platdrv.c
> drivers/i2c/busses/i2c-mv64xxx.c
> drivers/i2c/busses/i2c-nomadik.c
> drivers/i2c/busses/i2c-ocores.c
> drivers/i2c/busses/i2c-omap.c
> drivers/i2c/busses/i2c-rcar.c
> drivers/i2c/busses/i2c-s3c2410.c
> drivers/i2c/busses/i2c-tegra.c
> drivers/i2c/busses/i2c-virtio.c
> drivers/i2c/busses/i2c-xiic.c
> 
> i.e. only 13 of 77 drivers are using this flag.
> 
> The latest addition among these drivers is i2c-bcm2835.c and it was
> added back in 2013 and the flag was added to it in 2014:
> 
> commit 37888f71e2c9 ("i2c: i2c-bcm2835: deprecate class based instantiation")
> 
> FWIW, I also checked all the new drivers added since kernel release
> v4.0 and none of them set this flag.

What happened with this ? I didn't get a reply to it and the new
version still has it.

-- 
viresh