diff mbox series

[v10] i2c: virtio: add a virtio i2c frontend driver

Message ID 226a8d5663b7bb6f5d06ede7701eedb18d1bafa1.1616493817.git.jie.deng@intel.com
State Superseded
Headers show
Series [v10] i2c: virtio: add a virtio i2c frontend driver | expand

Commit Message

Jie Deng March 23, 2021, 2:19 p.m. UTC
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

Comments

Jie Deng March 24, 2021, 1:17 a.m. UTC | #1
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.
Viresh Kumar March 24, 2021, 4 a.m. UTC | #2
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 Kumar March 24, 2021, 4:02 a.m. UTC | #3
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.
Jie Deng March 24, 2021, 6:05 a.m. UTC | #4
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.
Jie Deng March 24, 2021, 6:41 a.m. UTC | #5
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.
Jie Deng April 14, 2021, 2:07 a.m. UTC | #6
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>

>
Viresh Kumar April 14, 2021, 3:52 a.m. UTC | #7
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
Jason Wang April 15, 2021, 3:51 a.m. UTC | #8
在 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 */
Jie Deng April 15, 2021, 6:17 a.m. UTC | #9
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.
Jie Deng April 15, 2021, 6:25 a.m. UTC | #10
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
Viresh Kumar April 15, 2021, 6:45 a.m. UTC | #11
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
Jie Deng April 15, 2021, 6:56 a.m. UTC | #12
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.
Viresh Kumar April 15, 2021, 7:15 a.m. UTC | #13
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
Wolfram Sang April 15, 2021, 7:21 a.m. UTC | #14
> 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.
Viresh Kumar April 15, 2021, 7:24 a.m. UTC | #15
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
Wolfram Sang April 15, 2021, 7:28 a.m. UTC | #16
> 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...
Viresh Kumar April 15, 2021, 7:37 a.m. UTC | #17
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
Jie Deng April 15, 2021, 8:15 a.m. UTC | #18
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.
Wolfram Sang April 15, 2021, 8:18 a.m. UTC | #19
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.
Jie Deng April 15, 2021, 8:20 a.m. UTC | #20
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.
Jie Deng May 12, 2021, 1:37 a.m. UTC | #21
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.
Jie Deng May 27, 2021, 6:49 a.m. UTC | #22
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.



Hi Wolfram,

I didn't receive your feedback yet. Do you have any more comments ?

Thanks.
Wolfram Sang June 28, 2021, 8:39 a.m. UTC | #23
Hi,

sorry for the long delay. I am not familiar with VFIO, so I had to dive
into the topic a little first. I am still not seeing through it
completely, so I have very high-level questions first.

> The device specification can be found on

> https://lists.oasis-open.org/archives/virtio-comment/202101/msg00008.html.


I think we need to start here:

===

If ``length of \field{read_buf}''=0 and ``length of \field{write_buf}''>0,
the request is called write request.

If ``length of \field{read_buf}''>0 and ``length of \field{write_buf}''=0,
the request is called read request.

If ``length of \field{read_buf}''>0 and ``length of \field{write_buf}''>0,
the request is called write-read request. It means an I2C write segment followed
by a read segment. Usually, the write segment provides the number of an I2C
controlled device register to be read.

===

I2C transactions can have an arbitrary number of messages which can
arbitrarily be read or write. As I understand the above, only one read,
write or read-write transaction is supported. If that is the case, it
would be not very much I2C but more SMBus. If my assumptions are true,
we first need to decide if you want to go the I2C way or SMBus subset.

But maybe I need to undestand the following paragraph first:

===

A driver may send one request or multiple requests to the device at a time.
The requests in the virtqueue are both queued and processed in order.

===

What happens if those multiple requests are sent out to the I2C bus
master driver on the host? Is there one transaction with N messages or
are there N transfers with 1 message each. This is a difference in I2C
world (REP_START vs. STOP/START) and some devices really need this
properly handled or they won't work.

===

The case when ``length of \field{write_buf}''=0, and at the same time,
``length of \field{read_buf}''=0 doesn't make any sense.

===

Oh, it does. That's a legal transfer, both in SMBus and I2C. It is used
to e.g. discover devices. I think it should be supported, even though
not all bus master drivers on the host can support it. Is it possible?

Also, as I read it, a whole bus is para-virtualized to the guest, or?
Wouldn't it be better to allow just specific devices on a bus? Again, I
am kinda new to this, so I may have overlooked things.

Thanks and happy hacking,

   Wolfram
Arnd Bergmann June 28, 2021, 9:01 a.m. UTC | #24
On Mon, Jun 28, 2021 at 10:39 AM Wolfram Sang <wsa@kernel.org> wrote:
>

> sorry for the long delay. I am not familiar with VFIO, so I had to dive

> into the topic a little first. I am still not seeing through it

> completely, so I have very high-level questions first.


You probably know this already, but just in case for clarification
these are two different things:

VFIO: kernel feature to make raw (usually PCI) devices available
           to user space drivers and virtual machines from a kernel
           running on bare metal.

virtio: transport protocol for implementing arbitrary paravirtualized
          drivers in (usually) a virtual machine guest without giving the
          guest access to hardware registers.

Both can be used for letting a KVM guest talk to the outside world,
but usually you have one or the other, not both.

> > The device specification can be found on

> > https://lists.oasis-open.org/archives/virtio-comment/202101/msg00008.html.

>

> I think we need to start here:

>

> ===

>

> If ``length of \field{read_buf}''=0 and ``length of \field{write_buf}''>0,

> the request is called write request.

>

> If ``length of \field{read_buf}''>0 and ``length of \field{write_buf}''=0,

> the request is called read request.

>

> If ``length of \field{read_buf}''>0 and ``length of \field{write_buf}''>0,

> the request is called write-read request. It means an I2C write segment followed

> by a read segment. Usually, the write segment provides the number of an I2C

> controlled device register to be read.

>

> ===

>

> I2C transactions can have an arbitrary number of messages which can

> arbitrarily be read or write. As I understand the above, only one read,

> write or read-write transaction is supported. If that is the case, it

> would be not very much I2C but more SMBus. If my assumptions are true,

> we first need to decide if you want to go the I2C way or SMBus subset.


This has come up in previous reviews already. I think it comes down
to the requirement that the virtio i2c protocol should allow passthrough
access to any client devices connected to a physical i2c bus on the host,
and this should ideally be independent of whether the host driver
exposes I2C_RDWR or I2C_SMBUS ioctl interface, or both.

This can be done either by having both interface types in the transport,
or picking one of the two, and translating to the host interface type
in software.

As far as I understand me (please clarify), implementing only the smbus
subset would mean that we cannot communicate with all client devices,
while implementing both would add more complexity than the lower-level
protocol.

> ===

>

> The case when ``length of \field{write_buf}''=0, and at the same time,

> ``length of \field{read_buf}''=0 doesn't make any sense.

>

> ===

>

> Oh, it does. That's a legal transfer, both in SMBus and I2C. It is used

> to e.g. discover devices. I think it should be supported, even though

> not all bus master drivers on the host can support it. Is it possible?

>

> Also, as I read it, a whole bus is para-virtualized to the guest, or?

> Wouldn't it be better to allow just specific devices on a bus? Again, I

> am kinda new to this, so I may have overlooked things.


Do you mean just allowing a single device per bus (as opposed to
having multiple devices as on a real bus), or just allowing
a particular set of client devices that can be identified using
virtio specific configuration (as opposed to relying on device
tree or similar for probing). Both of these are valid questions that
have been discussed before, but that could be revisited.

          Arnd
Wolfram Sang June 28, 2021, 9:25 a.m. UTC | #25
Hi Arnd,

> You probably know this already, but just in case for clarification

> these are two different things:


Ah, yes. I mistyped VFIO. Shows that I am not really fluent with these
terms. But as I spoke of paravirtualized later, I think I meant the
right thing.

> 

> > I2C transactions can have an arbitrary number of messages which can

> > arbitrarily be read or write. As I understand the above, only one read,

> > write or read-write transaction is supported. If that is the case, it

> > would be not very much I2C but more SMBus. If my assumptions are true,

> > we first need to decide if you want to go the I2C way or SMBus subset.

> 

> This has come up in previous reviews already. I think it comes down

> to the requirement that the virtio i2c protocol should allow passthrough

> access to any client devices connected to a physical i2c bus on the host,

> and this should ideally be independent of whether the host driver

> exposes I2C_RDWR or I2C_SMBUS ioctl interface, or both.


If a host driver supports I2C_RDWR (i.e. I2C bus transactions) it will
support I2C_SMBUS (i.e. SMBus transactions), too. It can be emulated and
host drivers just need to set I2C_FUNC_SMBUS_EMUL. With the notable
excaption of zero length transfers. Some cannot do this, so they use
I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK).

The other way around is not possible, SMBus is a subset of I2C.

> This can be done either by having both interface types in the transport,

> or picking one of the two, and translating to the host interface type

> in software.


If you have I2C, you have SMBus as well.

> As far as I understand me (please clarify), implementing only the smbus

> subset would mean that we cannot communicate with all client devices,

> while implementing both would add more complexity than the lower-level

> protocol.


Correct. You need to pick I2C if you want to support all devices. SMBus
will give you only a lot of devices.

> > Also, as I read it, a whole bus is para-virtualized to the guest, or?

> > Wouldn't it be better to allow just specific devices on a bus? Again, I

> > am kinda new to this, so I may have overlooked things.

> 

> Do you mean just allowing a single device per bus (as opposed to

> having multiple devices as on a real bus), or just allowing

> a particular set of client devices that can be identified using

> virtio specific configuration (as opposed to relying on device

> tree or similar for probing). Both of these are valid questions that

> have been discussed before, but that could be revisited.


I am just thinking that a physical bus on the host may have devices that
should be shared with the guest while other devices on the same bus
should not be shared (PMIC!). I'd think this is a minimal requirement
for security. I also wonder if it is possible to have a paravirtualized
bus in the guest which has multiple devices from the host sitting on
different physical busses there. But that may be the cherry on the top.

All the best,

   Wolfram
Arnd Bergmann June 28, 2021, 9:51 a.m. UTC | #26
On Mon, Jun 28, 2021 at 11:25 AM Wolfram Sang <wsa@kernel.org> wrote:
> > As far as I understand me (please clarify), implementing only the smbus

> > subset would mean that we cannot communicate with all client devices,

> > while implementing both would add more complexity than the lower-level

> > protocol.

>

> Correct. You need to pick I2C if you want to support all devices. SMBus

> will give you only a lot of devices.


Ok, that's what I thought. There is one corner case that I've struggled
with though: Suppose the host has an SMBus-only driver, and the
proposed driver exposes this as an I2C device to the guest, which
makes it available to guest user space (or a guest kernel driver)
using the emulated smbus command set. Is it always possible for
the host user space to turn the I2C transaction back into the
expected SMBus transaction on the host?

> > > Also, as I read it, a whole bus is para-virtualized to the guest, or?

> > > Wouldn't it be better to allow just specific devices on a bus? Again, I

> > > am kinda new to this, so I may have overlooked things.

> >

> > Do you mean just allowing a single device per bus (as opposed to

> > having multiple devices as on a real bus), or just allowing

> > a particular set of client devices that can be identified using

> > virtio specific configuration (as opposed to relying on device

> > tree or similar for probing). Both of these are valid questions that

> > have been discussed before, but that could be revisited.

>

> I am just thinking that a physical bus on the host may have devices that

> should be shared with the guest while other devices on the same bus

> should not be shared (PMIC!). I'd think this is a minimal requirement

> for security. I also wonder if it is possible to have a paravirtualized

> bus in the guest which has multiple devices from the host sitting on

> different physical busses there. But that may be the cherry on the top.


This is certainly possible, but is independent of the implementation of
the guest driver. It's up to the host to provision the devices that
are actually passed down to the guest, and this could in theory
be any combination of emulated devices with devices connected to
any of the host's physical buses. The host may also decide to remap
the addresses of the devices during passthrough.

       Arnd
Wolfram Sang June 28, 2021, 10:13 a.m. UTC | #27
> Ok, that's what I thought. There is one corner case that I've struggled

> with though: Suppose the host has an SMBus-only driver, and the

> proposed driver exposes this as an I2C device to the guest, which

> makes it available to guest user space (or a guest kernel driver)

> using the emulated smbus command set. Is it always possible for

> the host user space to turn the I2C transaction back into the

> expected SMBus transaction on the host?


If an SMBus commands gets converted to I2C messages, it can be converted
back to an SMBus command. I don't see anything preventing that right
now. However, the mapping-back code does look a bit clumsy, now that I
envision it. Maybe it is better, after all, to support I2C_SMBUS
directly and pass SMBus transactions as is. It should be a tad more
effiecient, too.

Speaking of it, I recall another gory detail: SMBus has transfers named
"read block data" and "block process call". These also need special
support from I2C host controllers before they can be emulated because
the length of the read needs to be adjusted in flight. These commands
are rare and not hard to implement. However, it makes exposing what is
supported a little more difficult.

> This is certainly possible, but is independent of the implementation of

> the guest driver. It's up to the host to provision the devices that

> are actually passed down to the guest, and this could in theory

> be any combination of emulated devices with devices connected to

> any of the host's physical buses. The host may also decide to remap

> the addresses of the devices during passthrough.


That sounds good.
Arnd Bergmann June 28, 2021, 11:50 a.m. UTC | #28
On Mon, Jun 28, 2021 at 12:13 PM Wolfram Sang <wsa@kernel.org> wrote:
>

>

> > Ok, that's what I thought. There is one corner case that I've struggled

> > with though: Suppose the host has an SMBus-only driver, and the

> > proposed driver exposes this as an I2C device to the guest, which

> > makes it available to guest user space (or a guest kernel driver)

> > using the emulated smbus command set. Is it always possible for

> > the host user space to turn the I2C transaction back into the

> > expected SMBus transaction on the host?

>

> If an SMBus commands gets converted to I2C messages, it can be converted

> back to an SMBus command. I don't see anything preventing that right

> now. However, the mapping-back code does look a bit clumsy, now that I

> envision it. Maybe it is better, after all, to support I2C_SMBUS

> directly and pass SMBus transactions as is. It should be a tad more

> effiecient, too.


You can fine Viresh's vhost-user implementation at
https://lore.kernel.org/qemu-devel/cover.1617278395.git.viresh.kumar@linaro.org/t/#m3b5044bad9769b170f505e63bd081eb27cef8db2

As you say, it does get a bit clumsy, but I think there is also a good argument
to be made that the clumsiness is based on the host Linux user interface
more than the on the requirements of the physical interface,
and that should not have to be reflected in the virtio specification.

> Speaking of it, I recall another gory detail: SMBus has transfers named

> "read block data" and "block process call". These also need special

> support from I2C host controllers before they can be emulated because

> the length of the read needs to be adjusted in flight. These commands

> are rare and not hard to implement. However, it makes exposing what is

> supported a little more difficult.


Right, this one has come up before as well: the preliminary result
was to assume that this probably won't be needed, but would be easy
enough to add later if necessary.

       Arnd
Wolfram Sang June 28, 2021, 2:58 p.m. UTC | #29
> You can fine Viresh's vhost-user implementation at

> https://lore.kernel.org/qemu-devel/cover.1617278395.git.viresh.kumar@linaro.org/t/#m3b5044bad9769b170f505e63bd081eb27cef8db2


It looks OK so far; yet, it is not complete. But it might be bearable
in the end.

> As you say, it does get a bit clumsy, but I think there is also a good argument

> to be made that the clumsiness is based on the host Linux user interface

> more than the on the requirements of the physical interface,

> and that should not have to be reflected in the virtio specification.


Makes sense to me.

> Right, this one has come up before as well: the preliminary result

> was to assume that this probably won't be needed, but would be easy

> enough to add later if necessary.


If adding support incrementally works for such an interface, this makes
sense as well.

So, where are we? As I understand, this v10 does not support I2C
transactions (or I2C_RDWR as you said). But you want to support all
clients. So, this doesn't match, or?
Jie Deng June 29, 2021, 3:03 a.m. UTC | #30
On 2021/6/28 17:01, Arnd Bergmann wrote:

> On Mon, Jun 28, 2021 at 10:39 AM Wolfram Sang <wsa@kernel.org> wrote:

>> sorry for the long delay. I am not familiar with VFIO, so I had to dive

>> into the topic a little first. I am still not seeing through it

>> completely, so I have very high-level questions first.

> You probably know this already, but just in case for clarification

> these are two different things:

>

> VFIO: kernel feature to make raw (usually PCI) devices available

>             to user space drivers and virtual machines from a kernel

>             running on bare metal.

>

> virtio: transport protocol for implementing arbitrary paravirtualized

>            drivers in (usually) a virtual machine guest without giving the

>            guest access to hardware registers.

>

Thanks Arnd for clarification.

Let me add some more:


The native model is as follows: a specific native I2C driver operates a 
specific hardware.

A specific native I2C driver  <--> A specific hardware


The virtio paravirtualized model is something like:

virtio-i2c <--> virtio I2C interfaces <--> virtio-backend <--> Real hardware

virtio-i2c: is this driver, the frontend driver.

virtio I2C interfaces: which are described in the specification.

https://raw.githubusercontent.com/oasis-tcs/virtio-spec/master/virtio-i2c.tex.

     I had tried to mirror Linux I2C interfaces (like "i2c_msg") into 
virtio I2C interface directly. But

     when I was doing upstream for this specification, I understood the 
virtio TC had the design philosophy

     "VIRTIO devices are not specific to Linux so the specs design 
should avoid the limitations of the

     current Linux driver behavior." So we redefined a minimum virtio 
I2C interfaces to make a working POC.

     and we may extend it in the future according to the need.

virtio-backend: the backend driver communicate with virtio-i2c by 
following virtio I2C interfaces specs.

      The are already two backend drivers developed by Viresh, one in 
QEMU, another in rust-vmm.

      1. vhost-user: 
https://lore.kernel.org/qemu-devel/cover.1617278395.git.viresh.kumar@linaro.org/t/#m3b5044bad9769b170f505e63bd081eb27cef8db2

      2. rust-vmm I2C backend: 
https://github.com/rust-vmm/vhost-device/pull/1


Regards,

Jie
Jie Deng June 29, 2021, 3:04 a.m. UTC | #31
On 2021/6/28 22:58, Wolfram Sang wrote:
> If adding support incrementally works for such an interface, this makes

> sense as well.

>

> So, where are we? As I understand, this v10 does not support I2C

> transactions (or I2C_RDWR as you said). But you want to support all

> clients. So, this doesn't match, or?


I hope we can have a minimum working driver get merged first so that we 
can have a base.

The v10 has three remaining problems:

     1. To remove vi->adap.class = I2C_CLASS_DEPRECATED; (already 
confirmed by Wolfram)

     2. Use #ifdef CONFIG_PM_SLEEP to replace the "__maybe_unused" 
(already confirmed by Arnd)

     3. It seems the I2C core takes care of locking already, so is it 
safy to remove "struct mutex lock in struct virtio_i2c"?

         (Raised by Viresh, still need Wolfram's confirmation)

Regards,

Jie
Viresh Kumar June 29, 2021, 4:10 a.m. UTC | #32
I will be replying here instead of replying to each and every msg :)

On 28-06-21, 16:58, Wolfram Sang wrote:
> 

> > You can fine Viresh's vhost-user implementation at

> > https://lore.kernel.org/qemu-devel/cover.1617278395.git.viresh.kumar@linaro.org/t/#m3b5044bad9769b170f505e63bd081eb27cef8db2

> 

> It looks OK so far; yet, it is not complete. But it might be bearable

> in the end.


While we are at it, this has been replaced by a Rust counterpart [1]
(as that makes it hypervisor agnostic, which is the goal of my work
here) and I need someone with I2C knowledge to help review it. It
should be okay even if you don't understand Rust a lot, just review
this file[2] which is where most of i2c specific stuff lies.

> > As you say, it does get a bit clumsy, but I think there is also a good argument

> > to be made that the clumsiness is based on the host Linux user interface

> > more than the on the requirements of the physical interface,

> > and that should not have to be reflected in the virtio specification.

> 

> Makes sense to me.

> 

> > Right, this one has come up before as well: the preliminary result

> > was to assume that this probably won't be needed, but would be easy

> > enough to add later if necessary.

> 

> If adding support incrementally works for such an interface, this makes

> sense as well.


Yes, we don't support few of SMBUS transaction (the block ones) as you
specified.

> So, where are we?


The virtio specification is already merged and here is the latest
version [3].

> As I understand, this v10 does not support I2C transactions (or

> I2C_RDWR as you said).


I am not sure why you say I2C_RDWR isn't supported. The spec and Linux
driver (+ my Rust/qemu backend), they all support I2C_RDWR as well as
SMBUS. To clarify on an earlier point, every virtio transfer may
contain one or more struct i2c_msg instances, all processed together
(as expected).

If you see virtio_i2c_send_reqs() in this patch, you will see that it
converts a stream of i2c_req messages to their virtio counterparts and
send them together, consider it a single transaction.

> But you want to support all clients. So, this doesn't match, or?


-- 
viresh

[1] https://github.com/rust-vmm/vhost-device/pull/1
[2] https://github.com/rust-vmm/vhost-device/blob/5aa22c92faac84ab07b6b15a214513556e8b1d01/src/i2c/src/i2c.rs
[3] https://github.com/oasis-tcs/virtio-spec/blob/master/virtio-i2c.tex
Wolfram Sang June 29, 2021, 8:27 a.m. UTC | #33
Hi Viresh,

> While we are at it, this has been replaced by a Rust counterpart [1]

> (as that makes it hypervisor agnostic, which is the goal of my work

> here) and I need someone with I2C knowledge to help review it. It

> should be okay even if you don't understand Rust a lot, just review

> this file[2] which is where most of i2c specific stuff lies.


Can't promise I can do this before my holidays, but I will try.

> I am not sure why you say I2C_RDWR isn't supported. The spec and Linux


This is how I interpreted Arnd's response. I said mulitple times that I
might be missing something so I double check.

> SMBUS. To clarify on an earlier point, every virtio transfer may

> contain one or more struct i2c_msg instances, all processed together

> (as expected).


That was the information missing for me so far becasue...

> If you see virtio_i2c_send_reqs() in this patch, you will see that it

> converts a stream of i2c_req messages to their virtio counterparts and

> send them together, consider it a single transaction.


... when I checked virtio_i2c_send_reqs(), I also saw
virtqueue_add_sgs() but I had no idea if this will end up as REP_START
on the physical bus or not. But it definately should.

Thanks for the pointers!

   Wolfram
Wolfram Sang June 29, 2021, 8:30 a.m. UTC | #34
>     3. It seems the I2C core takes care of locking already, so is it safy to

> remove "struct mutex lock in struct virtio_i2c"?


Looks to me like the mutex is only to serialize calls to
virtio_i2c_xfer(). Then, it can go. The core does locking. See, we have
i2c_transfer and __i2c_transfer, the unlocked version.
Viresh Kumar June 29, 2021, 8:52 a.m. UTC | #35
On 29-06-21, 10:27, Wolfram Sang wrote:
> > While we are at it, this has been replaced by a Rust counterpart [1]

> > (as that makes it hypervisor agnostic, which is the goal of my work

> > here) and I need someone with I2C knowledge to help review it. It

> > should be okay even if you don't understand Rust a lot, just review

> > this file[2] which is where most of i2c specific stuff lies.

> 

> Can't promise I can do this before my holidays, but I will try.


Thanks.

> > I am not sure why you say I2C_RDWR isn't supported. The spec and Linux

> 

> This is how I interpreted Arnd's response. I said mulitple times that I

> might be missing something so I double check.

> 

> > SMBUS. To clarify on an earlier point, every virtio transfer may

> > contain one or more struct i2c_msg instances, all processed together

> > (as expected).

> 

> That was the information missing for me so far becasue...

> 

> > If you see virtio_i2c_send_reqs() in this patch, you will see that it

> > converts a stream of i2c_req messages to their virtio counterparts and

> > send them together, consider it a single transaction.

> 

> ... when I checked virtio_i2c_send_reqs(), I also saw

> virtqueue_add_sgs() but I had no idea if this will end up as REP_START

> on the physical bus or not. But it definately should.


Just think of virtqueue_add_sgs() as something that setups the
structures for transfer. The actual stuff at the other end (host)
happens only after virtqueue_kick() is called at the guest (this
notifies the host that data is present now), in response the backend
running at host will re-create the struct i2c_msg and issue:

    struct i2c_rdwr_ioctl_data data;
    data.nmsgs = count;
    data.msgs = msgs;

    return ioctl(adapter->fd, I2C_RDWR, &data);

So we will end up recreating the exact situation as when
virtio_i2c_xfer() is called.

-- 
viresh
Wolfram Sang June 29, 2021, 9:07 a.m. UTC | #36
>     struct i2c_rdwr_ioctl_data data;

>     data.nmsgs = count;

>     data.msgs = msgs;

> 

>     return ioctl(adapter->fd, I2C_RDWR, &data);

> 

> So we will end up recreating the exact situation as when

> virtio_i2c_xfer() is called.


Perfect! I was hoping exactly for this. But with my limited experience
of all the blocks involved, I decided to ask for this instead of digging
into various sources. Looks good.
Viresh Kumar June 29, 2021, 9:13 a.m. UTC | #37
On 29-06-21, 10:30, Wolfram Sang wrote:
> 

> >     3. It seems the I2C core takes care of locking already, so is it safy to

> > remove "struct mutex lock in struct virtio_i2c"?

> 

> Looks to me like the mutex is only to serialize calls to

> virtio_i2c_xfer(). Then, it can go. The core does locking. See, we have

> i2c_transfer and __i2c_transfer, the unlocked version.


Right, this is what I have been suggesting as well.

So do you want Jie to send V11 now after fixing these three issues, or
you have more concerns ?

-- 
viresh
Wolfram Sang June 29, 2021, 9:36 a.m. UTC | #38
> So do you want Jie to send V11 now after fixing these three issues, or

> you have more concerns ?


Let me have another more low level look later.
Wolfram Sang June 29, 2021, 10:07 a.m. UTC | #39
Hi,

so some minor comments left:

> +		if (!msgs[i].len)

> +			break;


I hope this can extended in the future to allow zero-length messages. If
this is impossible we need to set an adapter quirk instead.

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


Is it really helpful for the user to know that msg5 failed? We don't
even say which transfer.

> +static u32 virtio_i2c_func(struct i2c_adapter *adap)

> +{

> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;


You are not emulating I2C_FUNC_SMBUS_QUICK, so you need to mask it out.

> +	snprintf(vi->adap.name, sizeof(vi->adap.name), "Virtio I2C Adapter");


Is there something to add so you can distinguish multiple instances?
Most people want that.

> +	vi->adap.class = I2C_CLASS_DEPRECATED;

> +	vi->adap.algo = &virtio_algorithm;

> +	vi->adap.dev.parent = &vdev->dev;

> +	vi->adap.timeout = HZ / 10;


Why so short? HZ is the kinda default value.

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


Won't the driver core print that for us?

> +	}

> +

> +	return ret;

> +}

> +


> +/* The bit 0 of the @virtio_i2c_out_hdr.@flags, used to group the requests */

> +#define VIRTIO_I2C_FLAGS_FAIL_NEXT	0x00000001


BIT(0)?

Happy hacking,

   Wolfram
Viresh Kumar June 29, 2021, 10:16 a.m. UTC | #40
On 29-06-21, 12:07, Wolfram Sang wrote:
> > +static u32 virtio_i2c_func(struct i2c_adapter *adap)

> > +{

> > +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;

> 

> You are not emulating I2C_FUNC_SMBUS_QUICK, so you need to mask it out.


What is it that we need to have to emulate it ? I did use it in my
qemu and rust backends, not sure if this was ever sent by device I
used for testing SMBUS though.

-- 
viresh
Wolfram Sang June 29, 2021, 10:23 a.m. UTC | #41
> > You are not emulating I2C_FUNC_SMBUS_QUICK, so you need to mask it out.

> 

> What is it that we need to have to emulate it ? I did use it in my

> qemu and rust backends, not sure if this was ever sent by device I

> used for testing SMBUS though.


The biggest use is to scan busses for devices, i.e. use 'i2cdetect'
without the -r parameter.
Viresh Kumar June 29, 2021, 10:30 a.m. UTC | #42
On 29-06-21, 12:23, Wolfram Sang wrote:
> 

> > > You are not emulating I2C_FUNC_SMBUS_QUICK, so you need to mask it out.

> > 

> > What is it that we need to have to emulate it ? I did use it in my

> > qemu and rust backends, not sure if this was ever sent by device I

> > used for testing SMBUS though.

> 

> The biggest use is to scan busses for devices, i.e. use 'i2cdetect'

> without the -r parameter.


Okay. But what is missing in the driver because of which it should
mask out I2C_FUNC_SMBUS_QUICK.

-- 
viresh
Wolfram Sang June 29, 2021, 10:42 a.m. UTC | #43
> Okay. But what is missing in the driver because of which it should

> mask out I2C_FUNC_SMBUS_QUICK.


From the spec:

The case when ``length of \field{write_buf}''=0, and at the same time,
``length of \field{read_buf}''=0 doesn't make any sense.

I mentioned this in my first reply and to my understanding I did not get
a reply that this has changed meanwhile.
Wolfram Sang June 29, 2021, 10:43 a.m. UTC | #44
> From the spec:

> 

> The case when ``length of \field{write_buf}''=0, and at the same time,

> ``length of \field{read_buf}''=0 doesn't make any sense.

> 

> I mentioned this in my first reply and to my understanding I did not get

> a reply that this has changed meanwhile.

> 


Also, this code as mentioned before:

> +             if (!msgs[i].len)

> +                     break;


I hope this can extended in the future to allow zero-length messages. If
this is impossible we need to set an adapter quirk instead.
Viresh Kumar June 29, 2021, 10:56 a.m. UTC | #45
On 29-06-21, 12:43, Wolfram Sang wrote:
> 

> > From the spec:

> > 

> > The case when ``length of \field{write_buf}''=0, and at the same time,

> > ``length of \field{read_buf}''=0 doesn't make any sense.

> > 

> > I mentioned this in my first reply and to my understanding I did not get

> > a reply that this has changed meanwhile.

> > 

> 

> Also, this code as mentioned before:

> 

> > +             if (!msgs[i].len)

> > +                     break;

> 

> I hope this can extended in the future to allow zero-length messages. If

> this is impossible we need to set an adapter quirk instead.


Ahh, yeah I saw these messages but I wasn't able to relate them to the
I2C_FUNC_SMBUS_QUICK thing. My bad.

Looked at Spec, Linux driver and my backends, I don't there is
anything that breaks if we allow this. So the best thing (looking
ahead) is if Jie sends a patch for spec to be modified like this.

The case when ``length of \field{write_buf}''=0, and at the same time,
``length of \field{read_buf}''=0 is called not-a-read-write request
and result for such a request is I2C device specific.

-- 
viresh
Wolfram Sang June 29, 2021, 11:11 a.m. UTC | #46
> The case when ``length of \field{write_buf}''=0, and at the same time,

> ``length of \field{read_buf}''=0 is called not-a-read-write request

> and result for such a request is I2C device specific.


Obviously, I don't know much about the specs and their wording. Still I
wonder if we can't call it a zero length transfer? This is allowed by
the I2C standard and SMBus has even a proper name for it (SMBUS_QUICK).
From my point of view, I would not say it is device specific because
devices are expected to ACK such a message.
Viresh Kumar June 29, 2021, 11:16 a.m. UTC | #47
On 29-06-21, 13:11, Wolfram Sang wrote:
> 

> > The case when ``length of \field{write_buf}''=0, and at the same time,

> > ``length of \field{read_buf}''=0 is called not-a-read-write request

> > and result for such a request is I2C device specific.

> 

> Obviously, I don't know much about the specs and their wording. Still I

> wonder if we can't call it a zero length transfer?


Maybe that.

> This is allowed by

> the I2C standard and SMBus has even a proper name for it (SMBUS_QUICK).

> From my point of view, I would not say it is device specific because

> devices are expected to ACK such a message.


Actually we should skip the last line from my diff, i.e. completely
drop "and result for such a request is I2C device specific".

The device (host in virtio spec terminology) still needs to return
success/failure as it does for other requests. Nothing special here.

-- 
viresh
Wolfram Sang June 29, 2021, 11:48 a.m. UTC | #48
> > Obviously, I don't know much about the specs and their wording. Still I

> > wonder if we can't call it a zero length transfer?

> 

> Maybe that.


I'd prefer it.

> > This is allowed by

> > the I2C standard and SMBus has even a proper name for it (SMBUS_QUICK).

> > From my point of view, I would not say it is device specific because

> > devices are expected to ACK such a message.

> 

> Actually we should skip the last line from my diff, i.e. completely

> drop "and result for such a request is I2C device specific".


Sounds good.

> The device (host in virtio spec terminology) still needs to return

> success/failure as it does for other requests. Nothing special here.


Ack.
Jie Deng June 30, 2021, 6:45 a.m. UTC | #49
On 2021/6/29 18:07, Wolfram Sang wrote:

> Hi,

>

> so some minor comments left:

>

>> +		if (!msgs[i].len)

>> +			break;

> I hope this can extended in the future to allow zero-length messages. If

> this is impossible we need to set an adapter quirk instead.



Yes, we can support it by removing this check and call it zero-length 
request.
It don't think it will break anything.


>

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

> Is it really helpful for the user to know that msg5 failed? We don't

> even say which transfer.



OK. I will remove this print.


>> +static u32 virtio_i2c_func(struct i2c_adapter *adap)

>> +{

>> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;

> You are not emulating I2C_FUNC_SMBUS_QUICK, so you need to mask it out.



I will remove the check of  zero-length message.


>

>> +	snprintf(vi->adap.name, sizeof(vi->adap.name), "Virtio I2C Adapter");

> Is there something to add so you can distinguish multiple instances?

> Most people want that.



I find the I2C core will set a device name "i2c-%d" for this purpose, right?

I think this name can be used to distinguish the adapter types while 
"i2c-%d" can be used to

distinguish instances. Does it make sense ?


>> +	vi->adap.class = I2C_CLASS_DEPRECATED;

>> +	vi->adap.algo = &virtio_algorithm;

>> +	vi->adap.dev.parent = &vdev->dev;

>> +	vi->adap.timeout = HZ / 10;

> Why so short? HZ is the kinda default value.



Ah... I didn't know the I2C core had already set a default value.
I will remove this line to use the default one.


>

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

> Won't the driver core print that for us?



Yes. It seems unnecessary. Will remove it.


>

>> +	}

>> +

>> +	return ret;

>> +}

>> +

>> +/* The bit 0 of the @virtio_i2c_out_hdr.@flags, used to group the requests */

>> +#define VIRTIO_I2C_FLAGS_FAIL_NEXT	0x00000001

> BIT(0)?



That's better. Thank you.


>

> Happy hacking,

>

>     Wolfram

>
Wolfram Sang June 30, 2021, 7:32 a.m. UTC | #50
> > > +	snprintf(vi->adap.name, sizeof(vi->adap.name), "Virtio I2C Adapter");

> > Is there something to add so you can distinguish multiple instances?

> > Most people want that.

> 

> 

> I find the I2C core will set a device name "i2c-%d" for this purpose, right?

> 

> I think this name can be used to distinguish the adapter types while

> "i2c-%d" can be used to

> 

> distinguish instances. Does it make sense ?


That alone does not help. See the 'i2cdetect -l' output of my Renesas
board here:

i2c-4	i2c       	e66d8000.i2c                    	I2C adapter
i2c-2	i2c       	e6510000.i2c                    	I2C adapter
i2c-7	i2c       	e60b0000.i2c                    	I2C adapter

Notice that the third column carries the base address, so you know which
i2c-%d is which physical bus. I don't know if it makes sense in your
"virtual" case, but so far it would always print "Virtio I2C Adapter".
Maybe it makes sense to add some parent device name, too?

And if this is not reasonable, just skip it. As I said, it can be
helpful at times, but it is definately not a show stopper.

> > > +	vi->adap.timeout = HZ / 10;

> > Why so short? HZ is the kinda default value.

> 

> 

> Ah... I didn't know the I2C core had already set a default value.

> I will remove this line to use the default one.


Sounds good.

Looking forward to the next version!

Happy hacking,

   Wolfram
Jie Deng June 30, 2021, 7:51 a.m. UTC | #51
On 2021/6/30 15:32, Wolfram Sang wrote:
>>>> +	snprintf(vi->adap.name, sizeof(vi->adap.name), "Virtio I2C Adapter");

>>> Is there something to add so you can distinguish multiple instances?

>>> Most people want that.

>>

>> I find the I2C core will set a device name "i2c-%d" for this purpose, right?

>>

>> I think this name can be used to distinguish the adapter types while

>> "i2c-%d" can be used to

>>

>> distinguish instances. Does it make sense ?

> That alone does not help. See the 'i2cdetect -l' output of my Renesas

> board here:

>

> i2c-4	i2c       	e66d8000.i2c                    	I2C adapter

> i2c-2	i2c       	e6510000.i2c                    	I2C adapter

> i2c-7	i2c       	e60b0000.i2c                    	I2C adapter

>

> Notice that the third column carries the base address, so you know which

> i2c-%d is which physical bus. I don't know if it makes sense in your

> "virtual" case, but so far it would always print "Virtio I2C Adapter".

> Maybe it makes sense to add some parent device name, too?

>

> And if this is not reasonable, just skip it. As I said, it can be

> helpful at times, but it is definately not a show stopper.



OK. I will add the virtio_device index for this purpose.
which indicates the unique position on the virtio bus.

Thanks Wolfram, I will fix it and send the v11.
Arnd Bergmann June 30, 2021, 7:55 a.m. UTC | #52
On Wed, Jun 30, 2021 at 9:51 AM Jie Deng <jie.deng@intel.com> wrote:
> On 2021/6/30 15:32, Wolfram Sang wrote:

> >>>> +  snprintf(vi->adap.name, sizeof(vi->adap.name), "Virtio I2C Adapter");

> >>> Is there something to add so you can distinguish multiple instances?

> >>> Most people want that.

> >>

> >> I find the I2C core will set a device name "i2c-%d" for this purpose, right?

> >>

> >> I think this name can be used to distinguish the adapter types while

> >> "i2c-%d" can be used to

> >>

> >> distinguish instances. Does it make sense ?

> > That alone does not help. See the 'i2cdetect -l' output of my Renesas

> > board here:

> >

> > i2c-4 i2c             e66d8000.i2c                            I2C adapter

> > i2c-2 i2c             e6510000.i2c                            I2C adapter

> > i2c-7 i2c             e60b0000.i2c                            I2C adapter

> >

> > Notice that the third column carries the base address, so you know which

> > i2c-%d is which physical bus. I don't know if it makes sense in your

> > "virtual" case, but so far it would always print "Virtio I2C Adapter".

> > Maybe it makes sense to add some parent device name, too?

> >

> > And if this is not reasonable, just skip it. As I said, it can be

> > helpful at times, but it is definately not a show stopper.

>

>

> OK. I will add the virtio_device index for this purpose.

> which indicates the unique position on the virtio bus.


Is that position stable across kernel versions? We do have stable naming
for PCI devices and for platform devices that are the parent of a virtio
device, but I would expect the virtio device to be numbered in probe
order instead.

On a related note, we are apparently still missing the bit in the virtio bus
layer that fills in the dev->of_node pointer of the virtio device. Without
this, it is not actually possible to automatically probe i2c devices connected
to a virtio-i2c bus. The same problem came up again with the virtio-gpio
driver that suffers from the same issue.

       Arnd
Wolfram Sang June 30, 2021, 8:04 a.m. UTC | #53
> Is that position stable across kernel versions? We do have stable naming

> for PCI devices and for platform devices that are the parent of a virtio

> device, but I would expect the virtio device to be numbered in probe

> order instead.


For me, it would be good enough to know who (= which device) created
this adapter when I look at the name at runtime. I wouldn't require this
to be stable across kernel versions. In general, this is just an info
string with no guarantees. But maybe you have reasons to insist on it
being stable nonetheless.
Andy Shevchenko June 30, 2021, 8:07 a.m. UTC | #54
On Wed, Jun 30, 2021 at 09:55:49AM +0200, Arnd Bergmann wrote:
> On Wed, Jun 30, 2021 at 9:51 AM Jie Deng <jie.deng@intel.com> wrote:


...

> On a related note, we are apparently still missing the bit in the virtio bus

> layer that fills in the dev->of_node pointer of the virtio device. Without

> this, it is not actually possible to automatically probe i2c devices connected

> to a virtio-i2c bus. The same problem came up again with the virtio-gpio

> driver that suffers from the same issue.


Don't we need to take care about fwnode handle as well?

-- 
With Best Regards,
Andy Shevchenko
Arnd Bergmann June 30, 2021, 8:29 a.m. UTC | #55
On Wed, Jun 30, 2021 at 10:09 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>

> On Wed, Jun 30, 2021 at 09:55:49AM +0200, Arnd Bergmann wrote:

> > On Wed, Jun 30, 2021 at 9:51 AM Jie Deng <jie.deng@intel.com> wrote:

>

> ...

>

> > On a related note, we are apparently still missing the bit in the virtio bus

> > layer that fills in the dev->of_node pointer of the virtio device. Without

> > this, it is not actually possible to automatically probe i2c devices connected

> > to a virtio-i2c bus. The same problem came up again with the virtio-gpio

> > driver that suffers from the same issue.

>

> Don't we need to take care about fwnode handle as well?


I'm fairly sure this gets set up automatically on DT based systems, based
on the dev->of_node of the virtio device, with no changes to the i2c
core core.

If you want to automatically probe i2c devices on a virtio-i2c controller
with ACPI, I have no idea if that would require changes to both
i2c-core-acpi.c as well as the virtio core, or just one of them.
So far, my assumption was that this would not be needed with ACPI.

        Arnd
Wolfram Sang June 30, 2021, 2:38 p.m. UTC | #56
Hi Viresh,

> While we are at it, this has been replaced by a Rust counterpart [1]

> (as that makes it hypervisor agnostic, which is the goal of my work

> here) and I need someone with I2C knowledge to help review it. It

> should be okay even if you don't understand Rust a lot, just review

> this file[2] which is where most of i2c specific stuff lies.


From the high level review I can provide, it looks good to me. Block
transfers are missing, but I think you said that already. Mising Rust
experience, I might miss details, of course. But the general approach
seems fine to me. smbus_prepare() will get a bit more messy when you add
block transfers, but it still looks bearable, I think.

Happy hacking!

   Wolfram
Viresh Kumar June 30, 2021, 3:09 p.m. UTC | #57
On 30-06-21, 16:38, Wolfram Sang wrote:
> > While we are at it, this has been replaced by a Rust counterpart [1]

> > (as that makes it hypervisor agnostic, which is the goal of my work

> > here) and I need someone with I2C knowledge to help review it. It

> > should be okay even if you don't understand Rust a lot, just review

> > this file[2] which is where most of i2c specific stuff lies.

> 

> From the high level review I can provide, it looks good to me. Block

> transfers are missing, but I think you said that already. Mising Rust

> experience, I might miss details, of course. But the general approach

> seems fine to me. smbus_prepare() will get a bit more messy when you add

> block transfers, but it still looks bearable, I think.


Thanks for having a look.

-- 
viresh
Viresh Kumar July 5, 2021, 12:18 p.m. UTC | #58
On 29-06-21, 12:43, Wolfram Sang wrote:
> 

> > From the spec:

> > 

> > The case when ``length of \field{write_buf}''=0, and at the same time,

> > ``length of \field{read_buf}''=0 doesn't make any sense.

> > 

> > I mentioned this in my first reply and to my understanding I did not get

> > a reply that this has changed meanwhile.

> > 

> 

> Also, this code as mentioned before:

> 

> > +             if (!msgs[i].len)

> > +                     break;

> 

> I hope this can extended in the future to allow zero-length messages. If

> this is impossible we need to set an adapter quirk instead.


Wolfram,

I stumbled again upon this while working at the backend implementation.

If you look at i2c_smbus_xfer_emulated(), the command is always sent via
msgbuf0[0]. Even in the case of I2C_SMBUS_QUICK, where we set msg[0].len = 0, we
still send the buf. This is really confusing :(

Do I understand correctly that we always need to send msg[0].buf even when
msg[0].len is 0 ?

If so, it would be difficult to implement this with the current i2c virtio
specification, as the msg.len isn't really passed from guest to host, rather it
is inferred using the length of the buffer itself. And so we can't really pass a
buffer if length is 0.

Moreover, the driver uses i2c_get_dma_safe_msg_buf(), which also depends on the
length parameter here to allocate the buffer and copy data to it.

All in all, the latest version of the driver doesn't work with "i2cdetect -q <bus>".

To make it work, I had to add this:

diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
index 731267d42292..5b8bd98ae38e 100644
--- a/drivers/i2c/busses/i2c-virtio.c
+++ b/drivers/i2c/busses/i2c-virtio.c
@@ -73,6 +73,9 @@ static int virtio_i2c_prepare_reqs(struct virtqueue *vq,
                sg_init_one(&out_hdr, &reqs[i].out_hdr, sizeof(reqs[i].out_hdr));
                sgs[outcnt++] = &out_hdr;

+               if (!msgs[i].len)
+                       msgs[i].len = 1;
+
                if (msgs[i].len) {
                        reqs[i].buf = i2c_get_dma_safe_msg_buf(&msgs[i], 1);
                        if (!reqs[i].buf)

which made it I2C_SMBUS_BYTE instead of I2C_SMBUS_QUICK.

What should we do here Wolfram?


Jie, while wolfram comes back and replies to this, I think you need to switch
back to NOT supporting zero length transfer and set update virtio_i2c_func() to
return:

        I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);

Support for zero-length transfers and I2C_FUNC_SMBUS_QUICK can be added
separately.

Thanks.

-- 
viresh
Jie Deng July 6, 2021, 1:50 a.m. UTC | #59
On 2021/7/5 20:18, Viresh Kumar wrote:
> On 29-06-21, 12:43, Wolfram Sang wrote:

>>>  From the spec:

>>>

>>> The case when ``length of \field{write_buf}''=0, and at the same time,

>>> ``length of \field{read_buf}''=0 doesn't make any sense.

>>>

>>> I mentioned this in my first reply and to my understanding I did not get

>>> a reply that this has changed meanwhile.

>>>

>> Also, this code as mentioned before:

>>

>>> +             if (!msgs[i].len)

>>> +                     break;

>> I hope this can extended in the future to allow zero-length messages. If

>> this is impossible we need to set an adapter quirk instead.

> Wolfram,

>

> I stumbled again upon this while working at the backend implementation.

>

> If you look at i2c_smbus_xfer_emulated(), the command is always sent via

> msgbuf0[0]. Even in the case of I2C_SMBUS_QUICK, where we set msg[0].len = 0, we

> still send the buf. This is really confusing :(

>

> Do I understand correctly that we always need to send msg[0].buf even when

> msg[0].len is 0 ?

>

> If so, it would be difficult to implement this with the current i2c virtio

> specification, as the msg.len isn't really passed from guest to host, rather it

> is inferred using the length of the buffer itself. And so we can't really pass a

> buffer if length is 0.

>

> Moreover, the driver uses i2c_get_dma_safe_msg_buf(), which also depends on the

> length parameter here to allocate the buffer and copy data to it.

>

> All in all, the latest version of the driver doesn't work with "i2cdetect -q <bus>".

>

> To make it work, I had to add this:

>

> diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c

> index 731267d42292..5b8bd98ae38e 100644

> --- a/drivers/i2c/busses/i2c-virtio.c

> +++ b/drivers/i2c/busses/i2c-virtio.c

> @@ -73,6 +73,9 @@ static int virtio_i2c_prepare_reqs(struct virtqueue *vq,

>                  sg_init_one(&out_hdr, &reqs[i].out_hdr, sizeof(reqs[i].out_hdr));

>                  sgs[outcnt++] = &out_hdr;

>

> +               if (!msgs[i].len)

> +                       msgs[i].len = 1;

> +

>                  if (msgs[i].len) {

>                          reqs[i].buf = i2c_get_dma_safe_msg_buf(&msgs[i], 1);

>                          if (!reqs[i].buf)

>

> which made it I2C_SMBUS_BYTE instead of I2C_SMBUS_QUICK.

>

> What should we do here Wolfram?

>

>

> Jie, while wolfram comes back and replies to this, I think you need to switch

> back to NOT supporting zero length transfer and set update virtio_i2c_func() to

> return:

>

>          I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);

>

> Support for zero-length transfers and I2C_FUNC_SMBUS_QUICK can be added

> separately.

>

> Thanks.



It's OK to me. Let's see what Wolfram says when he comes back.

I will send the updated version then.

Thanks.
Wolfram Sang July 22, 2021, 3:15 p.m. UTC | #60
Hi Viresh,

> If you look at i2c_smbus_xfer_emulated(), the command is always sent via

> msgbuf0[0]. Even in the case of I2C_SMBUS_QUICK, where we set msg[0].len = 0, we

> still send the buf. This is really confusing :(

> 

> Do I understand correctly that we always need to send msg[0].buf even when

> msg[0].len is 0 ?


Nope, I think you misinterpreted that. SMBUS_QUICK will not send any
byte. After the address phase (with the RW bit as data), a STOP will
immediately follow. len = 0 will ensure that.

msgbuf0[0] is set to 'command' because every mode except SMBUS_QUICK
will need that. So, it is convenient to always do it. For SMBUS_QUICK
it is superfluous but does not hurt.

> If so, it would be difficult to implement this with the current i2c virtio

> specification, as the msg.len isn't really passed from guest to host, rather it

> is inferred using the length of the buffer itself. And so we can't really pass a

> buffer if length is 0.


And you can't leave out the buffer and assume len = 0 then? Otherwise,
you can't do SMBUS_QUICK and we need to set a struct i2c_adapter_quirks
with I2C_AQ_NO_ZERO_LEN.

Speaking of adapter quirks, currently they are not exported to
userspace. So, you can't inherit any quirks from the bus driver of the
host. It is not too bad, I think. It would mean that the host driver
will return -EOPNOTSUPP for transfers it cannot handle. Otherwise, if
quirks were inherited, we could bail out sooner at guest level, but
well...

Does this help?

Happy hacking,

   Wolfram
Viresh Kumar July 23, 2021, 2:28 a.m. UTC | #61
Hi Wolfram,

On 22-07-21, 17:15, Wolfram Sang wrote:
> Nope, I think you misinterpreted that. SMBUS_QUICK will not send any

> byte. After the address phase (with the RW bit as data), a STOP will

> immediately follow. len = 0 will ensure that.

> 

> msgbuf0[0] is set to 'command' because every mode except SMBUS_QUICK

> will need that. So, it is convenient to always do it. For SMBUS_QUICK

> it is superfluous but does not hurt.


Yeah, I think I was confused by this stuff.

> > If so, it would be difficult to implement this with the current i2c virtio

> > specification, as the msg.len isn't really passed from guest to host, rather it

> > is inferred using the length of the buffer itself. And so we can't really pass a

> > buffer if length is 0.

> 

> And you can't leave out the buffer and assume len = 0 then?


Would need a spec update, which I am going to send.

We would also need another update to spec to make the Quick thing
working. Lemme do it separately and we merge the latest version of the
driver for linux-next until then.

I checked the code with i2cdetect -q and it worked fine, I was
required to do some changes to the backend (and spec) to make it work.
I will propose the changes to the spec first for the same.

-- 
viresh
Viresh Kumar July 23, 2021, 5:28 a.m. UTC | #62
On 23-07-21, 07:58, Viresh Kumar wrote:
> Would need a spec update, which I am going to send.

> 

> We would also need another update to spec to make the Quick thing

> working. Lemme do it separately and we merge the latest version of the

> driver for linux-next until then.

> 

> I checked the code with i2cdetect -q and it worked fine, I was

> required to do some changes to the backend (and spec) to make it work.

> I will propose the changes to the spec first for the same.


I have sent all the necessary changes for the spec here:

https://lists.oasis-open.org/archives/virtio-dev/202107/msg00167.html

-- 
viresh
diff mbox series

Patch

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