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

Add an I2C bus driver for virtio para-virtualization.

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

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

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

Co-developed-by: Conghui Chen <conghui.chen@intel.com>
Signed-off-by: Conghui Chen <conghui.chen@intel.com>
Signed-off-by: Jie Deng <jie.deng@intel.com>
---
 drivers/i2c/busses/Kconfig      |  11 ++
 drivers/i2c/busses/Makefile     |   3 +
 drivers/i2c/busses/i2c-virtio.c | 289 ++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/virtio_i2c.h |  42 ++++++
 include/uapi/linux/virtio_ids.h |   1 +
 5 files changed, 346 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-virtio.c
 create mode 100644 include/uapi/linux/virtio_i2c.h

Hi Jie,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on wsa/i2c/for-next]
[also build test ERROR on vhost/linux-next linux/master linus/master v5.12-rc1 next-20210303]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Jie-Deng/i2c-virtio-add-a-virtio-i2c-frontend-driver/20210304-100543
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: i386-randconfig-a006-20210304 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/bb645653b6d7750a026229726f8d9d0371457ac6
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jie-Deng/i2c-virtio-add-a-virtio-i2c-frontend-driver/20210304-100543
        git checkout bb645653b6d7750a026229726f8d9d0371457ac6
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from <command-line>:32:
>> ./usr/include/linux/virtio_i2c.h:35:2: error: unknown type name 'u8'
      35 |  u8 status;
         |  ^~

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

On 04-03-21, 09:59, 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>
> ---

Please always supply version history, it makes it difficult to review otherwise.

>  drivers/i2c/busses/Kconfig      |  11 ++
>  drivers/i2c/busses/Makefile     |   3 +
>  drivers/i2c/busses/i2c-virtio.c | 289 ++++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/virtio_i2c.h |  42 ++++++
>  include/uapi/linux/virtio_ids.h |   1 +
>  5 files changed, 346 insertions(+)
>  create mode 100644 drivers/i2c/busses/i2c-virtio.c
>  create mode 100644 include/uapi/linux/virtio_i2c.h
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 05ebf75..0860395 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -21,6 +21,17 @@ config I2C_ALI1535
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called i2c-ali1535.
>  
> +config I2C_VIRTIO
> +	tristate "Virtio I2C Adapter"
> +	depends on VIRTIO

depends on I2C as well ?

> +	help
> +	  If you say yes to this option, support will be included for the virtio
> +	  I2C adapter driver. The hardware can be emulated by any device model
> +	  software according to the virtio protocol.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called i2c-virtio.
> +
>  config I2C_ALI1563
>  	tristate "ALI 1563"
>  	depends on PCI
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 615f35e..b88da08 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -6,6 +6,9 @@
>  # ACPI drivers
>  obj-$(CONFIG_I2C_SCMI)		+= i2c-scmi.o
>  
> +# VIRTIO I2C host controller driver
> +obj-$(CONFIG_I2C_VIRTIO)	+= i2c-virtio.o
> +

Not that it is important but I would have added it towards the end instead of at
the top of the file..

>  # PC SMBus host controller drivers
>  obj-$(CONFIG_I2C_ALI1535)	+= i2c-ali1535.o
>  obj-$(CONFIG_I2C_ALI1563)	+= i2c-ali1563.o
> diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
> new file mode 100644
> index 0000000..98c0fcc
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-virtio.c
> @@ -0,0 +1,289 @@
> +// 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/io.h>
> +#include <linux/jiffies.h>

I don't think above two are required here..

> +#include <linux/kernel.h>
> +#include <linux/module.h>

> +#include <linux/platform_device.h>

same here.

> +#include <linux/wait.h>

same here.

> +

And this blank line as well, since all are standard linux headers.

> +#include <linux/virtio.h>
> +#include <linux/virtio_i2c.h>
> +
> +/**
> + * struct virtio_i2c - virtio I2C data
> + * @vdev: virtio device for this controller
> + * @completion: completion of virtio I2C message
> + * @adap: I2C adapter for this controller
> + * @i2c_lock: lock for virtqueue processing
> + * @vq: the virtio virtqueue for communication
> + */
> +struct virtio_i2c {
> +	struct virtio_device *vdev;
> +	struct completion completion;
> +	struct i2c_adapter adap;
> +	struct mutex i2c_lock;

i2c_ is redundant here. "lock" sounds good enough.

> +	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;
> +	u8 *buf;
> +	struct virtio_i2c_in_hdr in_hdr;
> +};
> +
> +static void virtio_i2c_msg_done(struct virtqueue *vq)
> +{
> +	struct virtio_i2c *vi = vq->vdev->priv;
> +
> +	complete(&vi->completion);
> +}
> +
> +static int virtio_i2c_send_reqs(struct virtqueue *vq,
> +				struct virtio_i2c_req *reqs,
> +				struct i2c_msg *msgs, int nr)
> +{
> +	struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr;
> +	int i, outcnt, incnt, err = 0;
> +	u8 *buf;
> +
> +	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.
> +		 */

Please use proper comment style.

                /*
                 * Only 7-bit mode supported for now, check Virtio I2C
                 * specification for format of "addr" field.
                 */

> +		reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1);
> +
> +		if (i != nr - 1)
> +			reqs[i].out_hdr.flags |= VIRTIO_I2C_FLAGS_FAIL_NEXT;

Since flags field hasn't been touched anywhere, directly assigning it may be
better instead of |=, it makes it more readable.

> +
> +		outcnt = incnt = 0;
> +		sg_init_one(&out_hdr, &reqs[i].out_hdr, sizeof(reqs[i].out_hdr));
> +		sgs[outcnt++] = &out_hdr;
> +
> +		buf = kzalloc(msgs[i].len, GFP_KERNEL);
> +		if (!buf)
> +			break;
> +
> +		reqs[i].buf = buf;
> +		sg_init_one(&msg_buf, reqs[i].buf, msgs[i].len);
> +
> +		if (msgs[i].flags & I2C_M_RD) {
> +			sgs[outcnt + incnt++] = &msg_buf;
> +		} else {
> +			memcpy(reqs[i].buf, msgs[i].buf, msgs[i].len);
> +			sgs[outcnt++] = &msg_buf;
> +		}
> +
> +		sg_init_one(&in_hdr, &reqs[i].in_hdr, sizeof(reqs[i].in_hdr));
> +		sgs[outcnt + incnt++] = &in_hdr;
> +
> +		err = virtqueue_add_sgs(vq, sgs, outcnt, incnt, &reqs[i], GFP_KERNEL);
> +		if (err < 0) {
> +			pr_err("failed to add msg[%d] to virtqueue.\n", i);
> +			kfree(reqs[i].buf);
> +			reqs[i].buf = NULL;
> +			break;
> +		}
> +	}
> +
> +	return i;
> +}
> +
> +static int virtio_i2c_complete_reqs(struct virtqueue *vq,
> +					struct virtio_i2c_req *reqs,
> +					struct i2c_msg *msgs, int nr)
> +{
> +	struct virtio_i2c_req *req;
> +	unsigned int len;
> +	int i;
> +
> +	for (i = 0; i < nr; i++) {
> +		req = (struct virtio_i2c_req *)virtqueue_get_buf(vq, &len);

No need of cast here since return type is void *.

> +		if (!(req && req == &reqs[i])) {

I find this less readable compared to:
		if (!req || req != &reqs[i]) {

> +			pr_err("msg[%d]: addr=0x%x virtqueue error.\n", i, msgs[i].addr);
> +			break;
> +		}
> +
> +		if (req->in_hdr.status != VIRTIO_I2C_MSG_OK) {
> +			pr_err("msg[%d]: addr=0x%x backend error.\n", i, msgs[i].addr);
> +			break;
> +		}

For all the above errors where you simply break out, you still need to free the
memory for buf, right ?

> +
> +		if (msgs[i].flags & I2C_M_RD)
> +			memcpy(msgs[i].buf, req->buf, msgs[i].len);
> +
> +		kfree(req->buf);
> +		req->buf = NULL;
> +	}
> +
> +	return i;
> +}
> +
> +static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> +{
> +	struct virtio_i2c *vi = i2c_get_adapdata(adap);
> +	struct virtqueue *vq = vi->vq;
> +	struct virtio_i2c_req *reqs;
> +	unsigned long time_left;
> +	int ret, nr;
> +
> +	reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL);
> +	if (!reqs)
> +		return -ENOMEM;
> +
> +	mutex_lock(&vi->i2c_lock);

I have never worked with i2c stuff earlier, but I don't think you need a lock
here. The transactions seem to be serialized by the i2c-core by itself (look at
i2c_transfer() in i2c-core-base.c), though there is another exported version
__i2c_transfer() but the comment over it says the callers must take adapter lock
before calling it.

> +
> +	ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
> +	if (ret == 0)
> +		goto err_unlock_free;
> +
> +	nr = ret;
> +
> +	virtqueue_kick(vq);
> +
> +	time_left = wait_for_completion_timeout(&vi->completion, adap->timeout);
> +	if (!time_left) {
> +		dev_err(&adap->dev, "virtio i2c backend timeout.\n");
> +		ret = -ETIMEDOUT;

You need to free bufs of the requests here as well..

> +		goto err_unlock_free;
> +	}
> +
> +	ret = virtio_i2c_complete_reqs(vq, reqs, msgs, nr);
> +
> +	reinit_completion(&vi->completion);
> +
> +err_unlock_free:
> +	mutex_unlock(&vi->i2c_lock);
> +	kfree(reqs);
> +	return ret;
> +}
> +
> +static void virtio_i2c_del_vqs(struct virtio_device *vdev)
> +{
> +	vdev->config->reset(vdev);
> +	vdev->config->del_vqs(vdev);
> +}
> +
> +static int virtio_i2c_setup_vqs(struct virtio_i2c *vi)
> +{
> +	struct virtio_device *vdev = vi->vdev;
> +
> +	vi->vq = virtio_find_single_vq(vdev, virtio_i2c_msg_done, "msg");
> +	return PTR_ERR_OR_ZERO(vi->vq);
> +}
> +
> +static u32 virtio_i2c_func(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}
> +
> +static struct i2c_algorithm virtio_algorithm = {
> +	.master_xfer = virtio_i2c_xfer,
> +	.functionality = virtio_i2c_func,
> +};
> +
> +static struct i2c_adapter virtio_adapter = {
> +	.owner = THIS_MODULE,
> +	.name = "Virtio I2C Adapter",
> +	.class = I2C_CLASS_DEPRECATED,

Why are we using something that is deprecated here ?

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

better add a blank line here.

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

You can add module-author as well here.

> +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..00f4508
> --- /dev/null
> +++ b/include/uapi/linux/virtio_i2c.h
> @@ -0,0 +1,42 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later WITH Linux-syscall-note */
> +/*
> + * Definitions for virtio I2C Adpter
> + *
> + * Copyright (c) 2021 Intel Corporation. All rights reserved.
> + */
> +
> +#ifndef _UAPI_LINUX_VIRTIO_I2C_H
> +#define _UAPI_LINUX_VIRTIO_I2C_H
> +
> +#include <linux/types.h>
> +#include <linux/virtio_ids.h>
> +#include <linux/virtio_config.h>

Any file (header or c) should only include what it directly needs and nothing
else. And so you should need only types.h here and nothing else.

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

It might be worth setting __packed for the structures here, even when we have
taken care of padding ourselves, for both the structures..

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

I would define this before the above structure.

> +/**
> + * struct virtio_i2c_in_hdr - the virtio I2C message IN header
> + * @status: the processing result from the backend
> + */
> +struct virtio_i2c_in_hdr {
> +	u8 status;
> +};
> +
> +/* The final status written by the device */
> +#define VIRTIO_I2C_MSG_OK	0
> +#define VIRTIO_I2C_MSG_ERR	1
> +
> +#endif /* _UAPI_LINUX_VIRTIO_I2C_H */
> diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
> index bc1c062..6ae32db 100644
> --- a/include/uapi/linux/virtio_ids.h
> +++ b/include/uapi/linux/virtio_ids.h
> @@ -54,5 +54,6 @@
>  #define VIRTIO_ID_FS			26 /* virtio filesystem */
>  #define VIRTIO_ID_PMEM			27 /* virtio pmem */
>  #define VIRTIO_ID_MAC80211_HWSIM	29 /* virtio mac80211-hwsim */
> +#define VIRTIO_ID_I2C_ADPTER		34 /* virtio i2c adpter */
>  
>  #endif /* _LINUX_VIRTIO_IDS_H */

Thanks for yet another version Jie.

On 2021/3/4 9:59 上午, 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>
> ---
>   drivers/i2c/busses/Kconfig      |  11 ++
>   drivers/i2c/busses/Makefile     |   3 +
>   drivers/i2c/busses/i2c-virtio.c | 289 ++++++++++++++++++++++++++++++++++++++++
>   include/uapi/linux/virtio_i2c.h |  42 ++++++
>   include/uapi/linux/virtio_ids.h |   1 +
>   5 files changed, 346 insertions(+)
>   create mode 100644 drivers/i2c/busses/i2c-virtio.c
>   create mode 100644 include/uapi/linux/virtio_i2c.h
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 05ebf75..0860395 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -21,6 +21,17 @@ config I2C_ALI1535
>   	  This driver can also be built as a module.  If so, the module
>   	  will be called i2c-ali1535.
>   
> +config I2C_VIRTIO
> +	tristate "Virtio I2C Adapter"
> +	depends on VIRTIO


Please use select here. There's no prompt for VIRTIO.


> +	help
> +	  If you say yes to this option, support will be included for the virtio
> +	  I2C adapter driver. The hardware can be emulated by any device model
> +	  software according to the virtio protocol.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called i2c-virtio.
> +
>   config I2C_ALI1563
>   	tristate "ALI 1563"
>   	depends on PCI
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 615f35e..b88da08 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -6,6 +6,9 @@
>   # ACPI drivers
>   obj-$(CONFIG_I2C_SCMI)		+= i2c-scmi.o
>   
> +# VIRTIO I2C host controller driver
> +obj-$(CONFIG_I2C_VIRTIO)	+= i2c-virtio.o
> +
>   # PC SMBus host controller drivers
>   obj-$(CONFIG_I2C_ALI1535)	+= i2c-ali1535.o
>   obj-$(CONFIG_I2C_ALI1563)	+= i2c-ali1563.o
> diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
> new file mode 100644
> index 0000000..98c0fcc
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-virtio.c
> @@ -0,0 +1,289 @@
> +// 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/io.h>
> +#include <linux/jiffies.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/wait.h>
> +
> +#include <linux/virtio.h>
> +#include <linux/virtio_i2c.h>
> +
> +/**
> + * struct virtio_i2c - virtio I2C data
> + * @vdev: virtio device for this controller
> + * @completion: completion of virtio I2C message
> + * @adap: I2C adapter for this controller
> + * @i2c_lock: lock for virtqueue processing
> + * @vq: the virtio virtqueue for communication
> + */
> +struct virtio_i2c {
> +	struct virtio_device *vdev;
> +	struct completion completion;
> +	struct i2c_adapter adap;
> +	struct mutex i2c_lock;
> +	struct virtqueue *vq;
> +};
> +
> +/**
> + * 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;
> +	u8 *buf;
> +	struct virtio_i2c_in_hdr in_hdr;
> +};
> +
> +static void virtio_i2c_msg_done(struct virtqueue *vq)
> +{
> +	struct virtio_i2c *vi = vq->vdev->priv;
> +
> +	complete(&vi->completion);
> +}
> +
> +static int virtio_i2c_send_reqs(struct virtqueue *vq,
> +				struct virtio_i2c_req *reqs,
> +				struct i2c_msg *msgs, int nr)
> +{
> +	struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr;
> +	int i, outcnt, incnt, err = 0;
> +	u8 *buf;
> +
> +	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 |= VIRTIO_I2C_FLAGS_FAIL_NEXT;
> +
> +		outcnt = incnt = 0;
> +		sg_init_one(&out_hdr, &reqs[i].out_hdr, sizeof(reqs[i].out_hdr));
> +		sgs[outcnt++] = &out_hdr;
> +
> +		buf = kzalloc(msgs[i].len, GFP_KERNEL);
> +		if (!buf)
> +			break;
> +
> +		reqs[i].buf = buf;
> +		sg_init_one(&msg_buf, reqs[i].buf, msgs[i].len);
> +
> +		if (msgs[i].flags & I2C_M_RD) {
> +			sgs[outcnt + incnt++] = &msg_buf;
> +		} else {
> +			memcpy(reqs[i].buf, msgs[i].buf, msgs[i].len);
> +			sgs[outcnt++] = &msg_buf;
> +		}
> +
> +		sg_init_one(&in_hdr, &reqs[i].in_hdr, sizeof(reqs[i].in_hdr));
> +		sgs[outcnt + incnt++] = &in_hdr;
> +
> +		err = virtqueue_add_sgs(vq, sgs, outcnt, incnt, &reqs[i], GFP_KERNEL);
> +		if (err < 0) {
> +			pr_err("failed to add msg[%d] to virtqueue.\n", i);
> +			kfree(reqs[i].buf);
> +			reqs[i].buf = NULL;
> +			break;
> +		}
> +	}
> +
> +	return i;
> +}
> +
> +static int virtio_i2c_complete_reqs(struct virtqueue *vq,
> +					struct virtio_i2c_req *reqs,
> +					struct i2c_msg *msgs, int nr)
> +{
> +	struct virtio_i2c_req *req;
> +	unsigned int len;
> +	int i;
> +
> +	for (i = 0; i < nr; i++) {
> +		req = (struct virtio_i2c_req *)virtqueue_get_buf(vq, &len);


The case is unnecessary.


> +		if (!(req && req == &reqs[i])) {
> +			pr_err("msg[%d]: addr=0x%x virtqueue error.\n", i, msgs[i].addr);
> +			break;
> +		}


It's better to be more specific here, e.g we can say we saw out of order 
completion here.


> +
> +		if (req->in_hdr.status != VIRTIO_I2C_MSG_OK) {
> +			pr_err("msg[%d]: addr=0x%x backend error.\n", i, msgs[i].addr);
> +			break;


Don't we need to clean used ring by keeping calling virtqueue_get_buf() 
here?


> +		}
> +
> +		if (msgs[i].flags & I2C_M_RD)
> +			memcpy(msgs[i].buf, req->buf, msgs[i].len);


Sorry if I had asked this before but any rason not to use msg[i].buf 
directly?


> +
> +		kfree(req->buf);
> +		req->buf = NULL;
> +	}
> +
> +	return i;
> +}
> +
> +static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> +{
> +	struct virtio_i2c *vi = i2c_get_adapdata(adap);
> +	struct virtqueue *vq = vi->vq;
> +	struct virtio_i2c_req *reqs;
> +	unsigned long time_left;
> +	int ret, nr;
> +
> +	reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL);
> +	if (!reqs)
> +		return -ENOMEM;
> +
> +	mutex_lock(&vi->i2c_lock);
> +
> +	ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
> +	if (ret == 0)
> +		goto err_unlock_free;
> +
> +	nr = ret;
> +
> +	virtqueue_kick(vq);
> +
> +	time_left = wait_for_completion_timeout(&vi->completion, adap->timeout);
> +	if (!time_left) {
> +		dev_err(&adap->dev, "virtio i2c backend timeout.\n");
> +		ret = -ETIMEDOUT;
> +		goto err_unlock_free;


So if the request is finished after the timerout, all the following 
request will fail, is this expected?


> +	}
> +
> +	ret = virtio_i2c_complete_reqs(vq, reqs, msgs, nr);


So consider driver queue N requests, can device raise interrupt if it 
completes the first request? If yes, the code break, if not it need to 
be clarified in the spec.

Acaultly I remember there's no VIRTIO_I2C_FLAGS_FAIL_NEXT in previous 
versions, and after reading the spec I still don't get the motivation 
for that (it may complicates both driver and device actually).


> +
> +	reinit_completion(&vi->completion);


So if a request it timeout but interrupt is raised here, everything is 
broken.

Thanks


> +
> +err_unlock_free:
> +	mutex_unlock(&vi->i2c_lock);
> +	kfree(reqs);
> +	return ret;
> +}
> +
> +static void virtio_i2c_del_vqs(struct virtio_device *vdev)
> +{
> +	vdev->config->reset(vdev);
> +	vdev->config->del_vqs(vdev);
> +}
> +
> +static int virtio_i2c_setup_vqs(struct virtio_i2c *vi)
> +{
> +	struct virtio_device *vdev = vi->vdev;
> +
> +	vi->vq = virtio_find_single_vq(vdev, virtio_i2c_msg_done, "msg");
> +	return PTR_ERR_OR_ZERO(vi->vq);
> +}
> +
> +static u32 virtio_i2c_func(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}
> +
> +static struct i2c_algorithm virtio_algorithm = {
> +	.master_xfer = virtio_i2c_xfer,
> +	.functionality = virtio_i2c_func,
> +};
> +
> +static struct i2c_adapter virtio_adapter = {
> +	.owner = THIS_MODULE,
> +	.name = "Virtio I2C Adapter",
> +	.class = I2C_CLASS_DEPRECATED,
> +	.algo = &virtio_algorithm,
> +};
> +
> +static int virtio_i2c_probe(struct virtio_device *vdev)
> +{
> +	struct device *pdev = vdev->dev.parent;
> +	struct virtio_i2c *vi;
> +	int ret;
> +
> +	vi = devm_kzalloc(&vdev->dev, sizeof(*vi), GFP_KERNEL);
> +	if (!vi)
> +		return -ENOMEM;
> +
> +	vdev->priv = vi;
> +	vi->vdev = vdev;
> +
> +	mutex_init(&vi->i2c_lock);
> +	init_completion(&vi->completion);
> +
> +	ret = virtio_i2c_setup_vqs(vi);
> +	if (ret)
> +		return ret;
> +
> +	vi->adap = virtio_adapter;
> +	i2c_set_adapdata(&vi->adap, vi);
> +	vi->adap.dev.parent = &vdev->dev;
> +	/* Setup ACPI node for controlled devices which will be probed through ACPI */
> +	ACPI_COMPANION_SET(&vi->adap.dev, ACPI_COMPANION(pdev));
> +	vi->adap.timeout = HZ / 10;
> +
> +	ret = i2c_add_adapter(&vi->adap);
> +	if (ret) {
> +		virtio_i2c_del_vqs(vdev);
> +		dev_err(&vdev->dev, "failed to add virtio-i2c adapter.\n");
> +	}
> +
> +	return ret;
> +}
> +
> +static void virtio_i2c_remove(struct virtio_device *vdev)
> +{
> +	struct virtio_i2c *vi = vdev->priv;
> +
> +	i2c_del_adapter(&vi->adap);
> +	virtio_i2c_del_vqs(vdev);
> +}
> +
> +static struct virtio_device_id id_table[] = {
> +	{ VIRTIO_ID_I2C_ADPTER, VIRTIO_DEV_ANY_ID },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(virtio, id_table);
> +
> +static int __maybe_unused virtio_i2c_freeze(struct virtio_device *vdev)
> +{
> +	virtio_i2c_del_vqs(vdev);
> +	return 0;
> +}
> +
> +static int __maybe_unused virtio_i2c_restore(struct virtio_device *vdev)
> +{
> +	return virtio_i2c_setup_vqs(vdev->priv);
> +}
> +
> +static struct virtio_driver virtio_i2c_driver = {
> +	.id_table	= id_table,
> +	.probe		= virtio_i2c_probe,
> +	.remove		= virtio_i2c_remove,
> +	.driver	= {
> +		.name	= "i2c_virtio",
> +	},
> +#ifdef CONFIG_PM_SLEEP
> +	.freeze = virtio_i2c_freeze,
> +	.restore = virtio_i2c_restore,
> +#endif
> +};
> +module_virtio_driver(virtio_i2c_driver);
> +
> +MODULE_DESCRIPTION("Virtio i2c bus driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/uapi/linux/virtio_i2c.h b/include/uapi/linux/virtio_i2c.h
> new file mode 100644
> index 0000000..00f4508
> --- /dev/null
> +++ b/include/uapi/linux/virtio_i2c.h
> @@ -0,0 +1,42 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later WITH Linux-syscall-note */
> +/*
> + * Definitions for virtio I2C Adpter
> + *
> + * Copyright (c) 2021 Intel Corporation. All rights reserved.
> + */
> +
> +#ifndef _UAPI_LINUX_VIRTIO_I2C_H
> +#define _UAPI_LINUX_VIRTIO_I2C_H
> +
> +#include <linux/types.h>
> +#include <linux/virtio_ids.h>
> +#include <linux/virtio_config.h>
> +
> +/**
> + * struct virtio_i2c_out_hdr - the virtio I2C message OUT header
> + * @addr: the controlled device address
> + * @padding: used to pad to full dword
> + * @flags: used for feature extensibility
> + */
> +struct virtio_i2c_out_hdr {
> +	__le16 addr;
> +	__le16 padding;
> +	__le32 flags;
> +};
> +
> +/* The bit 0 of the @virtio_i2c_out_hdr.@flags, used to group the requests */
> +#define VIRTIO_I2C_FLAGS_FAIL_NEXT	0x00000001
> +
> +/**
> + * struct virtio_i2c_in_hdr - the virtio I2C message IN header
> + * @status: the processing result from the backend
> + */
> +struct virtio_i2c_in_hdr {
> +	u8 status;
> +};
> +
> +/* The final status written by the device */
> +#define VIRTIO_I2C_MSG_OK	0
> +#define VIRTIO_I2C_MSG_ERR	1
> +
> +#endif /* _UAPI_LINUX_VIRTIO_I2C_H */
> diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
> index bc1c062..6ae32db 100644
> --- a/include/uapi/linux/virtio_ids.h
> +++ b/include/uapi/linux/virtio_ids.h
> @@ -54,5 +54,6 @@
>   #define VIRTIO_ID_FS			26 /* virtio filesystem */
>   #define VIRTIO_ID_PMEM			27 /* virtio pmem */
>   #define VIRTIO_ID_MAC80211_HWSIM	29 /* virtio mac80211-hwsim */
> +#define VIRTIO_ID_I2C_ADPTER		34 /* virtio i2c adpter */
>   
>   #endif /* _LINUX_VIRTIO_IDS_H */

On 2021/3/4 14:06, Viresh Kumar wrote:
> Please always supply version history, it makes it difficult to review otherwise.

I will add the history.
>>   drivers/i2c/busses/Kconfig      |  11 ++

>>   drivers/i2c/busses/Makefile     |   3 +

>>   drivers/i2c/busses/i2c-virtio.c | 289 ++++++++++++++++++++++++++++++++++++++++

>>   include/uapi/linux/virtio_i2c.h |  42 ++++++

>>   include/uapi/linux/virtio_ids.h |   1 +

>>   5 files changed, 346 insertions(+)

>>   create mode 100644 drivers/i2c/busses/i2c-virtio.c

>>   create mode 100644 include/uapi/linux/virtio_i2c.h

>>

>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig

>> index 05ebf75..0860395 100644

>> --- a/drivers/i2c/busses/Kconfig

>> +++ b/drivers/i2c/busses/Kconfig

>> @@ -21,6 +21,17 @@ config I2C_ALI1535

>>   	  This driver can also be built as a module.  If so, the module

>>   	  will be called i2c-ali1535.

>>   

>> +config I2C_VIRTIO

>> +	tristate "Virtio I2C Adapter"

>> +	depends on VIRTIO

> depends on I2C as well ?

No need that. The dependency of I2C is included in the Kconfig in its 
parent directory.
>

>> +	help

>> +	  If you say yes to this option, support will be included for the virtio

>> +	  I2C adapter driver. The hardware can be emulated by any device model

>> +	  software according to the virtio protocol.

>> +

>> +	  This driver can also be built as a module. If so, the module

>> +	  will be called i2c-virtio.

>> +

>>   config I2C_ALI1563

>>   	tristate "ALI 1563"

>>   	depends on PCI

>> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile

>> index 615f35e..b88da08 100644

>> --- a/drivers/i2c/busses/Makefile

>> +++ b/drivers/i2c/busses/Makefile

>> @@ -6,6 +6,9 @@

>>   # ACPI drivers

>>   obj-$(CONFIG_I2C_SCMI)		+= i2c-scmi.o

>>   

>> +# VIRTIO I2C host controller driver

>> +obj-$(CONFIG_I2C_VIRTIO)	+= i2c-virtio.o

>> +

> Not that it is important but I would have added it towards the end instead of at

> the top of the file..

I'm OK to add it to the end.
>

>>   # PC SMBus host controller drivers

>>   obj-$(CONFIG_I2C_ALI1535)	+= i2c-ali1535.o

>>   obj-$(CONFIG_I2C_ALI1563)	+= i2c-ali1563.o

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

>> new file mode 100644

>> index 0000000..98c0fcc

>> --- /dev/null

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

>> @@ -0,0 +1,289 @@

>> +// 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/io.h>

>> +#include <linux/jiffies.h>

> I don't think above two are required here..

>

>> +#include <linux/kernel.h>

>> +#include <linux/module.h>

>> +#include <linux/platform_device.h>

> same here.

>

>> +#include <linux/wait.h>

> same here.

Will confirm and remove them if they are not required. Thank you.
>> +

> And this blank line as well, since all are standard linux headers.

>> +#include <linux/virtio.h>

>> +#include <linux/virtio_i2c.h>

>> +

>> +/**

>> + * struct virtio_i2c - virtio I2C data

>> + * @vdev: virtio device for this controller

>> + * @completion: completion of virtio I2C message

>> + * @adap: I2C adapter for this controller

>> + * @i2c_lock: lock for virtqueue processing

>> + * @vq: the virtio virtqueue for communication

>> + */

>> +struct virtio_i2c {

>> +	struct virtio_device *vdev;

>> +	struct completion completion;

>> +	struct i2c_adapter adap;

>> +	struct mutex i2c_lock;

> i2c_ is redundant here. "lock" sounds good enough.

OK.
>> +	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;

>> +	u8 *buf;

>> +	struct virtio_i2c_in_hdr in_hdr;

>> +};

>> +

>> +static void virtio_i2c_msg_done(struct virtqueue *vq)

>> +{

>> +	struct virtio_i2c *vi = vq->vdev->priv;

>> +

>> +	complete(&vi->completion);

>> +}

>> +

>> +static int virtio_i2c_send_reqs(struct virtqueue *vq,

>> +				struct virtio_i2c_req *reqs,

>> +				struct i2c_msg *msgs, int nr)

>> +{

>> +	struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr;

>> +	int i, outcnt, incnt, err = 0;

>> +	u8 *buf;

>> +

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

>> +		 */

> Please use proper comment style.

will fix it.
>

>                  /*

>                   * Only 7-bit mode supported for now, check Virtio I2C

>                   * specification for format of "addr" field.

>                   */

>

>> +		reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1);

>> +

>> +		if (i != nr - 1)

>> +			reqs[i].out_hdr.flags |= VIRTIO_I2C_FLAGS_FAIL_NEXT;

> Since flags field hasn't been touched anywhere, directly assigning it may be

> better instead of |=, it makes it more readable.

OK.
>> +

>> +		outcnt = incnt = 0;

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

>> +		sgs[outcnt++] = &out_hdr;

>> +

>> +		buf = kzalloc(msgs[i].len, GFP_KERNEL);

>> +		if (!buf)

>> +			break;

>> +

>> +		reqs[i].buf = buf;

>> +		sg_init_one(&msg_buf, reqs[i].buf, msgs[i].len);

>> +

>> +		if (msgs[i].flags & I2C_M_RD) {

>> +			sgs[outcnt + incnt++] = &msg_buf;

>> +		} else {

>> +			memcpy(reqs[i].buf, msgs[i].buf, msgs[i].len);

>> +			sgs[outcnt++] = &msg_buf;

>> +		}

>> +

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

>> +		sgs[outcnt + incnt++] = &in_hdr;

>> +

>> +		err = virtqueue_add_sgs(vq, sgs, outcnt, incnt, &reqs[i], GFP_KERNEL);

>> +		if (err < 0) {

>> +			pr_err("failed to add msg[%d] to virtqueue.\n", i);

>> +			kfree(reqs[i].buf);

>> +			reqs[i].buf = NULL;

>> +			break;

>> +		}

>> +	}

>> +

>> +	return i;

>> +}

>> +

>> +static int virtio_i2c_complete_reqs(struct virtqueue *vq,

>> +					struct virtio_i2c_req *reqs,

>> +					struct i2c_msg *msgs, int nr)

>> +{

>> +	struct virtio_i2c_req *req;

>> +	unsigned int len;

>> +	int i;

>> +

>> +	for (i = 0; i < nr; i++) {

>> +		req = (struct virtio_i2c_req *)virtqueue_get_buf(vq, &len);

> No need of cast here since return type is void *.

Right. Thank you.
>

>> +		if (!(req && req == &reqs[i])) {

> I find this less readable compared to:

> 		if (!req || req != &reqs[i]) {


Different people may have different tastes.

Please check Andy's comment in this link.

https://lists.linuxfoundation.org/pipermail/virtualization/2020-September/049933.html

>

>> +			pr_err("msg[%d]: addr=0x%x virtqueue error.\n", i, msgs[i].addr);

>> +			break;

>> +		}

>> +

>> +		if (req->in_hdr.status != VIRTIO_I2C_MSG_OK) {

>> +			pr_err("msg[%d]: addr=0x%x backend error.\n", i, msgs[i].addr);

>> +			break;

>> +		}

> For all the above errors where you simply break out, you still need to free the

> memory for buf, right ?

Will try to use reqs[i].buf = msgs[i].buf to avoid allocation.
>> +

>> +		if (msgs[i].flags & I2C_M_RD)

>> +			memcpy(msgs[i].buf, req->buf, msgs[i].len);

>> +

>> +		kfree(req->buf);

>> +		req->buf = NULL;

>> +	}

>> +

>> +	return i;

>> +}

>> +

>> +static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)

>> +{

>> +	struct virtio_i2c *vi = i2c_get_adapdata(adap);

>> +	struct virtqueue *vq = vi->vq;

>> +	struct virtio_i2c_req *reqs;

>> +	unsigned long time_left;

>> +	int ret, nr;

>> +

>> +	reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL);

>> +	if (!reqs)

>> +		return -ENOMEM;

>> +

>> +	mutex_lock(&vi->i2c_lock);

> I have never worked with i2c stuff earlier, but I don't think you need a lock

> here. The transactions seem to be serialized by the i2c-core by itself (look at

> i2c_transfer() in i2c-core-base.c), though there is another exported version

> __i2c_transfer() but the comment over it says the callers must take adapter lock

> before calling it.

Lock is needed since no "lock_ops" is registered in this i2c_adapter.
>

>> +

>> +	ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);

>> +	if (ret == 0)

>> +		goto err_unlock_free;

>> +

>> +	nr = ret;

>> +

>> +	virtqueue_kick(vq);

>> +

>> +	time_left = wait_for_completion_timeout(&vi->completion, adap->timeout);

>> +	if (!time_left) {

>> +		dev_err(&adap->dev, "virtio i2c backend timeout.\n");

>> +		ret = -ETIMEDOUT;

> You need to free bufs of the requests here as well..

>

>> +		goto err_unlock_free;

>> +	}

>> +

>> +	ret = virtio_i2c_complete_reqs(vq, reqs, msgs, nr);

>> +

>> +	reinit_completion(&vi->completion);

>> +

>> +err_unlock_free:

>> +	mutex_unlock(&vi->i2c_lock);

>> +	kfree(reqs);

>> +	return ret;

>> +}

>> +

>> +static void virtio_i2c_del_vqs(struct virtio_device *vdev)

>> +{

>> +	vdev->config->reset(vdev);

>> +	vdev->config->del_vqs(vdev);

>> +}

>> +

>> +static int virtio_i2c_setup_vqs(struct virtio_i2c *vi)

>> +{

>> +	struct virtio_device *vdev = vi->vdev;

>> +

>> +	vi->vq = virtio_find_single_vq(vdev, virtio_i2c_msg_done, "msg");

>> +	return PTR_ERR_OR_ZERO(vi->vq);

>> +}

>> +

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

>> +{

>> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;

>> +}

>> +

>> +static struct i2c_algorithm virtio_algorithm = {

>> +	.master_xfer = virtio_i2c_xfer,

>> +	.functionality = virtio_i2c_func,

>> +};

>> +

>> +static struct i2c_adapter virtio_adapter = {

>> +	.owner = THIS_MODULE,

>> +	.name = "Virtio I2C Adapter",

>> +	.class = I2C_CLASS_DEPRECATED,

> Why are we using something that is deprecated here ?

Warn users that the adapter doesn't support classes anymore.
>

>> +	.algo = &virtio_algorithm,

>> +};

>> +

>> +static int virtio_i2c_probe(struct virtio_device *vdev)

>> +{

>> +	struct device *pdev = vdev->dev.parent;

>> +	struct virtio_i2c *vi;

>> +	int ret;

>> +

>> +	vi = devm_kzalloc(&vdev->dev, sizeof(*vi), GFP_KERNEL);

>> +	if (!vi)

>> +		return -ENOMEM;

>> +

>> +	vdev->priv = vi;

>> +	vi->vdev = vdev;

>> +

>> +	mutex_init(&vi->i2c_lock);

>> +	init_completion(&vi->completion);

>> +

>> +	ret = virtio_i2c_setup_vqs(vi);

>> +	if (ret)

>> +		return ret;

>> +

>> +	vi->adap = virtio_adapter;

>> +	i2c_set_adapdata(&vi->adap, vi);

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

> better add a blank line here.

OK.
>> +	/* Setup ACPI node for controlled devices which will be probed through ACPI */

>> +	ACPI_COMPANION_SET(&vi->adap.dev, ACPI_COMPANION(pdev));

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

>> +

>> +	ret = i2c_add_adapter(&vi->adap);

>> +	if (ret) {

>> +		virtio_i2c_del_vqs(vdev);

>> +		dev_err(&vdev->dev, "failed to add virtio-i2c adapter.\n");

>> +	}

>> +

>> +	return ret;

>> +}

>> +

>> +static void virtio_i2c_remove(struct virtio_device *vdev)

>> +{

>> +	struct virtio_i2c *vi = vdev->priv;

>> +

>> +	i2c_del_adapter(&vi->adap);

>> +	virtio_i2c_del_vqs(vdev);

>> +}

>> +

>> +static struct virtio_device_id id_table[] = {

>> +	{ VIRTIO_ID_I2C_ADPTER, VIRTIO_DEV_ANY_ID },

>> +	{}

>> +};

>> +MODULE_DEVICE_TABLE(virtio, id_table);

>> +

>> +static int __maybe_unused virtio_i2c_freeze(struct virtio_device *vdev)

>> +{

>> +	virtio_i2c_del_vqs(vdev);

>> +	return 0;

>> +}

>> +

>> +static int __maybe_unused virtio_i2c_restore(struct virtio_device *vdev)

>> +{

>> +	return virtio_i2c_setup_vqs(vdev->priv);

>> +}

>> +

>> +static struct virtio_driver virtio_i2c_driver = {

>> +	.id_table	= id_table,

>> +	.probe		= virtio_i2c_probe,

>> +	.remove		= virtio_i2c_remove,

>> +	.driver	= {

>> +		.name	= "i2c_virtio",

>> +	},

>> +#ifdef CONFIG_PM_SLEEP

>> +	.freeze = virtio_i2c_freeze,

>> +	.restore = virtio_i2c_restore,

>> +#endif

>> +};

>> +module_virtio_driver(virtio_i2c_driver);

>> +

>> +MODULE_DESCRIPTION("Virtio i2c bus driver");

> You can add module-author as well here.

Sure. I'm glad to do that.
>> +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..00f4508

>> --- /dev/null

>> +++ b/include/uapi/linux/virtio_i2c.h

>> @@ -0,0 +1,42 @@

>> +/* SPDX-License-Identifier: GPL-2.0-or-later WITH Linux-syscall-note */

>> +/*

>> + * Definitions for virtio I2C Adpter

>> + *

>> + * Copyright (c) 2021 Intel Corporation. All rights reserved.

>> + */

>> +

>> +#ifndef _UAPI_LINUX_VIRTIO_I2C_H

>> +#define _UAPI_LINUX_VIRTIO_I2C_H

>> +

>> +#include <linux/types.h>

>> +#include <linux/virtio_ids.h>

>> +#include <linux/virtio_config.h>

> Any file (header or c) should only include what it directly needs and nothing

> else. And so you should need only types.h here and nothing else.

OK.
>> +

>> +/**

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

>> +};

> It might be worth setting __packed for the structures here, even when we have

> taken care of padding ourselves, for both the structures..

Please check Michael's comment https://lkml.org/lkml/2020/9/3/339.
I agreed to remove "__packed" .
>> +

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

>> +#define VIRTIO_I2C_FLAGS_FAIL_NEXT	0x00000001

>> +

> I would define this before the above structure.

OK.
>> +/**

>> + * struct virtio_i2c_in_hdr - the virtio I2C message IN header

>> + * @status: the processing result from the backend

>> + */

>> +struct virtio_i2c_in_hdr {

>> +	u8 status;

>> +};

>> +

>> +/* The final status written by the device */

>> +#define VIRTIO_I2C_MSG_OK	0

>> +#define VIRTIO_I2C_MSG_ERR	1

>> +

>> +#endif /* _UAPI_LINUX_VIRTIO_I2C_H */

>> diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h

>> index bc1c062..6ae32db 100644

>> --- a/include/uapi/linux/virtio_ids.h

>> +++ b/include/uapi/linux/virtio_ids.h

>> @@ -54,5 +54,6 @@

>>   #define VIRTIO_ID_FS			26 /* virtio filesystem */

>>   #define VIRTIO_ID_PMEM			27 /* virtio pmem */

>>   #define VIRTIO_ID_MAC80211_HWSIM	29 /* virtio mac80211-hwsim */

>> +#define VIRTIO_ID_I2C_ADPTER		34 /* virtio i2c adpter */

>>   

>>   #endif /* _LINUX_VIRTIO_IDS_H */

> Thanks for yet another version Jie.

>

On 05-03-21, 09:46, Jie Deng wrote:
> On 2021/3/4 14:06, Viresh Kumar wrote:

> > depends on I2C as well ?

> No need that. The dependency of I2C is included in the Kconfig in its parent

> directory.


Sorry about that, I must have figured that out myself.

(Though a note on the way we reply to messages, please leave an empty line
before and after your messages, it gets difficult to find the inline replies
otherwise. )

> > > +		if (!(req && req == &reqs[i])) {

> > I find this less readable compared to:

> > 		if (!req || req != &reqs[i]) {

> 

> Different people may have different tastes.

> 

> Please check Andy's comment in this link.

> 

> https://lists.linuxfoundation.org/pipermail/virtualization/2020-September/049933.html


Heh, everyone wants you to do it differently :)

If we leave compilers optimizations aside (because it will generate the exact
same code for both the cases, I tested it as well to be doubly sure), The
original statement used in this patch has 3 conditional statements in it and the
way I suggested has only two.

Andy, thoughts ?

And anyway, this isn't biggest of my worries, just that I had to notice it
somehow :)

> > For all the above errors where you simply break out, you still need to free the

> > memory for buf, right ?

> Will try to use reqs[i].buf = msgs[i].buf to avoid allocation.


I think it would be better to have all such deallocations done at a single
place, i.e. after the completion callback is finished.. Trying to solve this
everywhere is going to make this more messy.

> > > +	mutex_lock(&vi->i2c_lock);

> > I have never worked with i2c stuff earlier, but I don't think you need a lock

> > here. The transactions seem to be serialized by the i2c-core by itself (look at

> > i2c_transfer() in i2c-core-base.c), though there is another exported version

> > __i2c_transfer() but the comment over it says the callers must take adapter lock

> > before calling it.

> Lock is needed since no "lock_ops" is registered in this i2c_adapter.


drivers/i2c/i2c-core-base.c:

static int i2c_register_adapter(struct i2c_adapter *adap)
{
        ...

        if (!adap->lock_ops)
                adap->lock_ops = &i2c_adapter_lock_ops;

        ...
}

This should take care of it ?

> > 

> > > +

> > > +	ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);

> > > +	if (ret == 0)

> > > +		goto err_unlock_free;

> > > +

> > > +	nr = ret;

> > > +

> > > +	virtqueue_kick(vq);

> > > +

> > > +	time_left = wait_for_completion_timeout(&vi->completion, adap->timeout);

> > > +	if (!time_left) {

> > > +		dev_err(&adap->dev, "virtio i2c backend timeout.\n");

> > > +		ret = -ETIMEDOUT;

> > You need to free bufs of the requests here as well..


Just want to make sure you didn't miss this comment.

> > > +static struct i2c_adapter virtio_adapter = {

> > > +	.owner = THIS_MODULE,

> > > +	.name = "Virtio I2C Adapter",

> > > +	.class = I2C_CLASS_DEPRECATED,

> > Why are we using something that is deprecated here ?

> Warn users that the adapter doesn't support classes anymore.


So this is the right thing to do? Or this is what we expect from new drivers?
Sorry, I am just new to this stuff and so...

> > > +struct virtio_i2c_out_hdr {

> > > +	__le16 addr;

> > > +	__le16 padding;

> > > +	__le32 flags;

> > > +};

> > It might be worth setting __packed for the structures here, even when we have

> > taken care of padding ourselves, for both the structures..

> Please check Michael's comment https://lkml.org/lkml/2020/9/3/339.

> I agreed to remove "__packed" .


When Michael commented the structure looked like this:

Actually it can be ignored as the compiler isn't going to add any padding by
itself in this case (since you already took care of it) as the structure will be
aligned to the size of the biggest element here. I generally consider it to be a
good coding-style to make sure we don't add any unwanted stuff in there by
mistake.

Anyway, we can see it in future if this is going to be required or not, if and
when we add new fields here.

-- 
viresh

On 2021/3/4 17:15, Jason Wang wrote:
>

> On 2021/3/4 9:59 上午, 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>

>> ---

>>   drivers/i2c/busses/Kconfig      |  11 ++

>>   drivers/i2c/busses/Makefile     |   3 +

>>   drivers/i2c/busses/i2c-virtio.c | 289 

>> ++++++++++++++++++++++++++++++++++++++++

>>   include/uapi/linux/virtio_i2c.h |  42 ++++++

>>   include/uapi/linux/virtio_ids.h |   1 +

>>   5 files changed, 346 insertions(+)

>>   create mode 100644 drivers/i2c/busses/i2c-virtio.c

>>   create mode 100644 include/uapi/linux/virtio_i2c.h

>>

>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig

>> index 05ebf75..0860395 100644

>> --- a/drivers/i2c/busses/Kconfig

>> +++ b/drivers/i2c/busses/Kconfig

>> @@ -21,6 +21,17 @@ config I2C_ALI1535

>>         This driver can also be built as a module.  If so, the module

>>         will be called i2c-ali1535.

>>   +config I2C_VIRTIO

>> +    tristate "Virtio I2C Adapter"

>> +    depends on VIRTIO

>

>

> Please use select here. There's no prompt for VIRTIO.

>

OK.
>

>> +    help

>> +      If you say yes to this option, support will be included for 

>> the virtio

>> +      I2C adapter driver. The hardware can be emulated by any device 

>> model

>> +      software according to the virtio protocol.

>> +

>> +      This driver can also be built as a module. If so, the module

>> +      will be called i2c-virtio.

>> +

>>   config I2C_ALI1563

>>       tristate "ALI 1563"

>>       depends on PCI

>> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile

>> index 615f35e..b88da08 100644

>> --- a/drivers/i2c/busses/Makefile

>> +++ b/drivers/i2c/busses/Makefile

>> @@ -6,6 +6,9 @@

>>   # ACPI drivers

>>   obj-$(CONFIG_I2C_SCMI)        += i2c-scmi.o

>>   +# VIRTIO I2C host controller driver

>> +obj-$(CONFIG_I2C_VIRTIO)    += i2c-virtio.o

>> +

>>   # PC SMBus host controller drivers

>>   obj-$(CONFIG_I2C_ALI1535)    += i2c-ali1535.o

>>   obj-$(CONFIG_I2C_ALI1563)    += i2c-ali1563.o

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

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

>> new file mode 100644

>> index 0000000..98c0fcc

>> --- /dev/null

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

>> @@ -0,0 +1,289 @@

>> +// 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/io.h>

>> +#include <linux/jiffies.h>

>> +#include <linux/kernel.h>

>> +#include <linux/module.h>

>> +#include <linux/platform_device.h>

>> +#include <linux/wait.h>

>> +

>> +#include <linux/virtio.h>

>> +#include <linux/virtio_i2c.h>

>> +

>> +/**

>> + * struct virtio_i2c - virtio I2C data

>> + * @vdev: virtio device for this controller

>> + * @completion: completion of virtio I2C message

>> + * @adap: I2C adapter for this controller

>> + * @i2c_lock: lock for virtqueue processing

>> + * @vq: the virtio virtqueue for communication

>> + */

>> +struct virtio_i2c {

>> +    struct virtio_device *vdev;

>> +    struct completion completion;

>> +    struct i2c_adapter adap;

>> +    struct mutex i2c_lock;

>> +    struct virtqueue *vq;

>> +};

>> +

>> +/**

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

>> +    u8 *buf;

>> +    struct virtio_i2c_in_hdr in_hdr;

>> +};

>> +

>> +static void virtio_i2c_msg_done(struct virtqueue *vq)

>> +{

>> +    struct virtio_i2c *vi = vq->vdev->priv;

>> +

>> +    complete(&vi->completion);

>> +}

>> +

>> +static int virtio_i2c_send_reqs(struct virtqueue *vq,

>> +                struct virtio_i2c_req *reqs,

>> +                struct i2c_msg *msgs, int nr)

>> +{

>> +    struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr;

>> +    int i, outcnt, incnt, err = 0;

>> +    u8 *buf;

>> +

>> +    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 |= VIRTIO_I2C_FLAGS_FAIL_NEXT;

>> +

>> +        outcnt = incnt = 0;

>> +        sg_init_one(&out_hdr, &reqs[i].out_hdr, 

>> sizeof(reqs[i].out_hdr));

>> +        sgs[outcnt++] = &out_hdr;

>> +

>> +        buf = kzalloc(msgs[i].len, GFP_KERNEL);

>> +        if (!buf)

>> +            break;

>> +

>> +        reqs[i].buf = buf;

>> +        sg_init_one(&msg_buf, reqs[i].buf, msgs[i].len);

>> +

>> +        if (msgs[i].flags & I2C_M_RD) {

>> +            sgs[outcnt + incnt++] = &msg_buf;

>> +        } else {

>> +            memcpy(reqs[i].buf, msgs[i].buf, msgs[i].len);

>> +            sgs[outcnt++] = &msg_buf;

>> +        }

>> +

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

>> +        sgs[outcnt + incnt++] = &in_hdr;

>> +

>> +        err = virtqueue_add_sgs(vq, sgs, outcnt, incnt, &reqs[i], 

>> GFP_KERNEL);

>> +        if (err < 0) {

>> +            pr_err("failed to add msg[%d] to virtqueue.\n", i);

>> +            kfree(reqs[i].buf);

>> +            reqs[i].buf = NULL;

>> +            break;

>> +        }

>> +    }

>> +

>> +    return i;

>> +}

>> +

>> +static int virtio_i2c_complete_reqs(struct virtqueue *vq,

>> +                    struct virtio_i2c_req *reqs,

>> +                    struct i2c_msg *msgs, int nr)

>> +{

>> +    struct virtio_i2c_req *req;

>> +    unsigned int len;

>> +    int i;

>> +

>> +    for (i = 0; i < nr; i++) {

>> +        req = (struct virtio_i2c_req *)virtqueue_get_buf(vq, &len);

>

>

> The case is unnecessary.

>

Right. Will remove the conversion.
>

>> +        if (!(req && req == &reqs[i])) {

>> +            pr_err("msg[%d]: addr=0x%x virtqueue error.\n", i, 

>> msgs[i].addr);

>> +            break;

>> +        }

>

>

> It's better to be more specific here, e.g we can say we saw out of 

> order completion here.

>

OK.
>

>> +

>> +        if (req->in_hdr.status != VIRTIO_I2C_MSG_OK) {

>> +            pr_err("msg[%d]: addr=0x%x backend error.\n", i, 

>> msgs[i].addr);

>> +            break;

>

>

> Don't we need to clean used ring by keeping calling 

> virtqueue_get_buf() here?

>

You are right. Need to detach all the used buffers from the vq before 
queuing new buffers.

>> +        }

>> +

>> +        if (msgs[i].flags & I2C_M_RD)

>> +            memcpy(msgs[i].buf, req->buf, msgs[i].len);

>

>

> Sorry if I had asked this before but any rason not to use msg[i].buf 

> directly?

>

Will try to use msg[i].buf.
>

>> +

>> +        kfree(req->buf);

>> +        req->buf = NULL;

>> +    }

>> +

>> +    return i;

>> +}

>> +

>> +static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg 

>> *msgs, int num)

>> +{

>> +    struct virtio_i2c *vi = i2c_get_adapdata(adap);

>> +    struct virtqueue *vq = vi->vq;

>> +    struct virtio_i2c_req *reqs;

>> +    unsigned long time_left;

>> +    int ret, nr;

>> +

>> +    reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL);

>> +    if (!reqs)

>> +        return -ENOMEM;

>> +

>> +    mutex_lock(&vi->i2c_lock);

>> +

>> +    ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);

>> +    if (ret == 0)

>> +        goto err_unlock_free;

>> +

>> +    nr = ret;

>> +

>> +    virtqueue_kick(vq);

>> +

>> +    time_left = wait_for_completion_timeout(&vi->completion, 

>> adap->timeout);

>> +    if (!time_left) {

>> +        dev_err(&adap->dev, "virtio i2c backend timeout.\n");

>> +        ret = -ETIMEDOUT;

>> +        goto err_unlock_free;

>

>

> So if the request is finished after the timerout, all the following 

> request will fail, is this expected?

>

>

This is an expected behavior. 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.
>> +    }

>> +

>> +    ret = virtio_i2c_complete_reqs(vq, reqs, msgs, nr);

>

>

> So consider driver queue N requests, can device raise interrupt if it 

> completes the first request? If yes, the code break, if not it need to 

> be clarified in the spec.

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. And let the i2c core to decide whether to resend.
>

> Acaultly I remember there's no VIRTIO_I2C_FLAGS_FAIL_NEXT in previous 

> versions, and after reading the spec I still don't get the motivation 

> for that (it may complicates both driver and device actually).

>

This flag is introduced by Stefan. Please check following link for the 
details
https://lists.oasis-open.org/archives/virtio-comment/202012/msg00075.html.
>

>> +

>> +    reinit_completion(&vi->completion);

>

>

> So if a request it timeout but interrupt is raised here, everything is 

> broken.

>

> Thanks

>

If interrupt is raised after timeout, those requests in the vq will be 
ignored.
No need to care about them being handled or not. Just let the i2c core to
decide whether need to resend or not.
>

>> +

>> +err_unlock_free:

>> +    mutex_unlock(&vi->i2c_lock);

>> +    kfree(reqs);

>> +    return ret;

>> +}

>> +

>> +static void virtio_i2c_del_vqs(struct virtio_device *vdev)

>> +{

>> +    vdev->config->reset(vdev);

>> +    vdev->config->del_vqs(vdev);

>> +}

>> +

>> +static int virtio_i2c_setup_vqs(struct virtio_i2c *vi)

>> +{

>> +    struct virtio_device *vdev = vi->vdev;

>> +

>> +    vi->vq = virtio_find_single_vq(vdev, virtio_i2c_msg_done, "msg");

>> +    return PTR_ERR_OR_ZERO(vi->vq);

>> +}

>> +

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

>> +{

>> +    return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;

>> +}

>> +

>> +static struct i2c_algorithm virtio_algorithm = {

>> +    .master_xfer = virtio_i2c_xfer,

>> +    .functionality = virtio_i2c_func,

>> +};

>> +

>> +static struct i2c_adapter virtio_adapter = {

>> +    .owner = THIS_MODULE,

>> +    .name = "Virtio I2C Adapter",

>> +    .class = I2C_CLASS_DEPRECATED,

>> +    .algo = &virtio_algorithm,

>> +};

>> +

>> +static int virtio_i2c_probe(struct virtio_device *vdev)

>> +{

>> +    struct device *pdev = vdev->dev.parent;

>> +    struct virtio_i2c *vi;

>> +    int ret;

>> +

>> +    vi = devm_kzalloc(&vdev->dev, sizeof(*vi), GFP_KERNEL);

>> +    if (!vi)

>> +        return -ENOMEM;

>> +

>> +    vdev->priv = vi;

>> +    vi->vdev = vdev;

>> +

>> +    mutex_init(&vi->i2c_lock);

>> +    init_completion(&vi->completion);

>> +

>> +    ret = virtio_i2c_setup_vqs(vi);

>> +    if (ret)

>> +        return ret;

>> +

>> +    vi->adap = virtio_adapter;

>> +    i2c_set_adapdata(&vi->adap, vi);

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

>> +    /* Setup ACPI node for controlled devices which will be probed 

>> through ACPI */

>> +    ACPI_COMPANION_SET(&vi->adap.dev, ACPI_COMPANION(pdev));

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

>> +

>> +    ret = i2c_add_adapter(&vi->adap);

>> +    if (ret) {

>> +        virtio_i2c_del_vqs(vdev);

>> +        dev_err(&vdev->dev, "failed to add virtio-i2c adapter.\n");

>> +    }

>> +

>> +    return ret;

>> +}

>> +

>> +static void virtio_i2c_remove(struct virtio_device *vdev)

>> +{

>> +    struct virtio_i2c *vi = vdev->priv;

>> +

>> +    i2c_del_adapter(&vi->adap);

>> +    virtio_i2c_del_vqs(vdev);

>> +}

>> +

>> +static struct virtio_device_id id_table[] = {

>> +    { VIRTIO_ID_I2C_ADPTER, VIRTIO_DEV_ANY_ID },

>> +    {}

>> +};

>> +MODULE_DEVICE_TABLE(virtio, id_table);

>> +

>> +static int __maybe_unused virtio_i2c_freeze(struct virtio_device *vdev)

>> +{

>> +    virtio_i2c_del_vqs(vdev);

>> +    return 0;

>> +}

>> +

>> +static int __maybe_unused virtio_i2c_restore(struct virtio_device 

>> *vdev)

>> +{

>> +    return virtio_i2c_setup_vqs(vdev->priv);

>> +}

>> +

>> +static struct virtio_driver virtio_i2c_driver = {

>> +    .id_table    = id_table,

>> +    .probe        = virtio_i2c_probe,

>> +    .remove        = virtio_i2c_remove,

>> +    .driver    = {

>> +        .name    = "i2c_virtio",

>> +    },

>> +#ifdef CONFIG_PM_SLEEP

>> +    .freeze = virtio_i2c_freeze,

>> +    .restore = virtio_i2c_restore,

>> +#endif

>> +};

>> +module_virtio_driver(virtio_i2c_driver);

>> +

>> +MODULE_DESCRIPTION("Virtio i2c bus driver");

>> +MODULE_LICENSE("GPL");

>> diff --git a/include/uapi/linux/virtio_i2c.h 

>> b/include/uapi/linux/virtio_i2c.h

>> new file mode 100644

>> index 0000000..00f4508

>> --- /dev/null

>> +++ b/include/uapi/linux/virtio_i2c.h

>> @@ -0,0 +1,42 @@

>> +/* SPDX-License-Identifier: GPL-2.0-or-later WITH Linux-syscall-note */

>> +/*

>> + * Definitions for virtio I2C Adpter

>> + *

>> + * Copyright (c) 2021 Intel Corporation. All rights reserved.

>> + */

>> +

>> +#ifndef _UAPI_LINUX_VIRTIO_I2C_H

>> +#define _UAPI_LINUX_VIRTIO_I2C_H

>> +

>> +#include <linux/types.h>

>> +#include <linux/virtio_ids.h>

>> +#include <linux/virtio_config.h>

>> +

>> +/**

>> + * struct virtio_i2c_out_hdr - the virtio I2C message OUT header

>> + * @addr: the controlled device address

>> + * @padding: used to pad to full dword

>> + * @flags: used for feature extensibility

>> + */

>> +struct virtio_i2c_out_hdr {

>> +    __le16 addr;

>> +    __le16 padding;

>> +    __le32 flags;

>> +};

>> +

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

>> requests */

>> +#define VIRTIO_I2C_FLAGS_FAIL_NEXT    0x00000001

>> +

>> +/**

>> + * struct virtio_i2c_in_hdr - the virtio I2C message IN header

>> + * @status: the processing result from the backend

>> + */

>> +struct virtio_i2c_in_hdr {

>> +    u8 status;

>> +};

>> +

>> +/* The final status written by the device */

>> +#define VIRTIO_I2C_MSG_OK    0

>> +#define VIRTIO_I2C_MSG_ERR    1

>> +

>> +#endif /* _UAPI_LINUX_VIRTIO_I2C_H */

>> diff --git a/include/uapi/linux/virtio_ids.h 

>> b/include/uapi/linux/virtio_ids.h

>> index bc1c062..6ae32db 100644

>> --- a/include/uapi/linux/virtio_ids.h

>> +++ b/include/uapi/linux/virtio_ids.h

>> @@ -54,5 +54,6 @@

>>   #define VIRTIO_ID_FS            26 /* virtio filesystem */

>>   #define VIRTIO_ID_PMEM            27 /* virtio pmem */

>>   #define VIRTIO_ID_MAC80211_HWSIM    29 /* virtio mac80211-hwsim */

>> +#define VIRTIO_ID_I2C_ADPTER        34 /* virtio i2c adpter */

>>     #endif /* _LINUX_VIRTIO_IDS_H */

>

On 2021/3/5 11:09, Viresh Kumar wrote:
> On 05-03-21, 09:46, Jie Deng wrote:

>> On 2021/3/4 14:06, Viresh Kumar wrote:

>>> depends on I2C as well ?

>> No need that. The dependency of I2C is included in the Kconfig in its parent

>> directory.

> Sorry about that, I must have figured that out myself.

>

> (Though a note on the way we reply to messages, please leave an empty line

> before and after your messages, it gets difficult to find the inline replies

> otherwise. )

>

>>>> +		if (!(req && req == &reqs[i])) {

>>> I find this less readable compared to:

>>> 		if (!req || req != &reqs[i]) {

>> Different people may have different tastes.

>>

>> Please check Andy's comment in this link.

>>

>> https://lists.linuxfoundation.org/pipermail/virtualization/2020-September/049933.html

> Heh, everyone wants you to do it differently :)

>

> If we leave compilers optimizations aside (because it will generate the exact

> same code for both the cases, I tested it as well to be doubly sure), The

> original statement used in this patch has 3 conditional statements in it and the

> way I suggested has only two.

>

> Andy, thoughts ?

>

> And anyway, this isn't biggest of my worries, just that I had to notice it

> somehow :)

>

>>> For all the above errors where you simply break out, you still need to free the

>>> memory for buf, right ?

>> Will try to use reqs[i].buf = msgs[i].buf to avoid allocation.

> I think it would be better to have all such deallocations done at a single

> place, i.e. after the completion callback is finished.. Trying to solve this

> everywhere is going to make this more messy.



I think there is no need to have allocations/deallocations/memcpy for 
reqs[i].buf any more
if using msgs[i].buf directly.


>>>> +	mutex_lock(&vi->i2c_lock);

>>> I have never worked with i2c stuff earlier, but I don't think you need a lock

>>> here. The transactions seem to be serialized by the i2c-core by itself (look at

>>> i2c_transfer() in i2c-core-base.c), though there is another exported version

>>> __i2c_transfer() but the comment over it says the callers must take adapter lock

>>> before calling it.

>> Lock is needed since no "lock_ops" is registered in this i2c_adapter.

> drivers/i2c/i2c-core-base.c:

>

> static int i2c_register_adapter(struct i2c_adapter *adap)

> {

>          ...

>

>          if (!adap->lock_ops)

>                  adap->lock_ops = &i2c_adapter_lock_ops;

>

>          ...

> }

>

> This should take care of it ?



The problem is that you can't guarantee that adap->algo->master_xfer is 
only called
from i2c_transfer. Any function who holds the adapter can call 
adap->algo->master_xfer
directly. So I think it is safer to have a lock in virtio_i2c_xfer.


>>>> +

>>>> +	ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);

>>>> +	if (ret == 0)

>>>> +		goto err_unlock_free;

>>>> +

>>>> +	nr = ret;

>>>> +

>>>> +	virtqueue_kick(vq);

>>>> +

>>>> +	time_left = wait_for_completion_timeout(&vi->completion, adap->timeout);

>>>> +	if (!time_left) {

>>>> +		dev_err(&adap->dev, "virtio i2c backend timeout.\n");

>>>> +		ret = -ETIMEDOUT;

>>> You need to free bufs of the requests here as well..

> Just want to make sure you didn't miss this comment.



Will try to use msgs[i].buf directly. So it should be no free bufs any more.


>>>> +static struct i2c_adapter virtio_adapter = {

>>>> +	.owner = THIS_MODULE,

>>>> +	.name = "Virtio I2C Adapter",

>>>> +	.class = I2C_CLASS_DEPRECATED,

>>> Why are we using something that is deprecated here ?

>> Warn users that the adapter doesn't support classes anymore.

> So this is the right thing to do? Or this is what we expect from new drivers?

> Sorry, I am just new to this stuff and so...



https://patchwork.ozlabs.org/project/linux-i2c/patch/20170729121143.3980-1-wsa@the-dreams.de/


>>>> +struct virtio_i2c_out_hdr {

>>>> +	__le16 addr;

>>>> +	__le16 padding;

>>>> +	__le32 flags;

>>>> +};

>>> It might be worth setting __packed for the structures here, even when we have

>>> taken care of padding ourselves, for both the structures..

>> Please check Michael's comment https://lkml.org/lkml/2020/9/3/339.

>> I agreed to remove "__packed" .

> When Michael commented the structure looked like this:

>

> Actually it can be ignored as the compiler isn't going to add any padding by

> itself in this case (since you already took care of it) as the structure will be

> aligned to the size of the biggest element here. I generally consider it to be a

> good coding-style to make sure we don't add any unwanted stuff in there by

> mistake.

>

> Anyway, we can see it in future if this is going to be required or not, if and

> when we add new fields here.

>

On 2021/3/5 1:47 下午, Jie Deng wrote:
>

> On 2021/3/4 17:15, Jason Wang wrote:

>>

>> On 2021/3/4 9:59 上午, 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>

>>> ---

>>>   drivers/i2c/busses/Kconfig      |  11 ++

>>>   drivers/i2c/busses/Makefile     |   3 +

>>>   drivers/i2c/busses/i2c-virtio.c | 289 

>>> ++++++++++++++++++++++++++++++++++++++++

>>>   include/uapi/linux/virtio_i2c.h |  42 ++++++

>>>   include/uapi/linux/virtio_ids.h |   1 +

>>>   5 files changed, 346 insertions(+)

>>>   create mode 100644 drivers/i2c/busses/i2c-virtio.c

>>>   create mode 100644 include/uapi/linux/virtio_i2c.h

>>>

>>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig

>>> index 05ebf75..0860395 100644

>>> --- a/drivers/i2c/busses/Kconfig

>>> +++ b/drivers/i2c/busses/Kconfig

>>> @@ -21,6 +21,17 @@ config I2C_ALI1535

>>>         This driver can also be built as a module.  If so, the module

>>>         will be called i2c-ali1535.

>>>   +config I2C_VIRTIO

>>> +    tristate "Virtio I2C Adapter"

>>> +    depends on VIRTIO

>>

>>

>> Please use select here. There's no prompt for VIRTIO.

>>

> OK.

>>

>>> +    help

>>> +      If you say yes to this option, support will be included for 

>>> the virtio

>>> +      I2C adapter driver. The hardware can be emulated by any 

>>> device model

>>> +      software according to the virtio protocol.

>>> +

>>> +      This driver can also be built as a module. If so, the module

>>> +      will be called i2c-virtio.

>>> +

>>>   config I2C_ALI1563

>>>       tristate "ALI 1563"

>>>       depends on PCI

>>> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile

>>> index 615f35e..b88da08 100644

>>> --- a/drivers/i2c/busses/Makefile

>>> +++ b/drivers/i2c/busses/Makefile

>>> @@ -6,6 +6,9 @@

>>>   # ACPI drivers

>>>   obj-$(CONFIG_I2C_SCMI)        += i2c-scmi.o

>>>   +# VIRTIO I2C host controller driver

>>> +obj-$(CONFIG_I2C_VIRTIO)    += i2c-virtio.o

>>> +

>>>   # PC SMBus host controller drivers

>>>   obj-$(CONFIG_I2C_ALI1535)    += i2c-ali1535.o

>>>   obj-$(CONFIG_I2C_ALI1563)    += i2c-ali1563.o

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

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

>>> new file mode 100644

>>> index 0000000..98c0fcc

>>> --- /dev/null

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

>>> @@ -0,0 +1,289 @@

>>> +// 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/io.h>

>>> +#include <linux/jiffies.h>

>>> +#include <linux/kernel.h>

>>> +#include <linux/module.h>

>>> +#include <linux/platform_device.h>

>>> +#include <linux/wait.h>

>>> +

>>> +#include <linux/virtio.h>

>>> +#include <linux/virtio_i2c.h>

>>> +

>>> +/**

>>> + * struct virtio_i2c - virtio I2C data

>>> + * @vdev: virtio device for this controller

>>> + * @completion: completion of virtio I2C message

>>> + * @adap: I2C adapter for this controller

>>> + * @i2c_lock: lock for virtqueue processing

>>> + * @vq: the virtio virtqueue for communication

>>> + */

>>> +struct virtio_i2c {

>>> +    struct virtio_device *vdev;

>>> +    struct completion completion;

>>> +    struct i2c_adapter adap;

>>> +    struct mutex i2c_lock;

>>> +    struct virtqueue *vq;

>>> +};

>>> +

>>> +/**

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

>>> +    u8 *buf;

>>> +    struct virtio_i2c_in_hdr in_hdr;

>>> +};

>>> +

>>> +static void virtio_i2c_msg_done(struct virtqueue *vq)

>>> +{

>>> +    struct virtio_i2c *vi = vq->vdev->priv;

>>> +

>>> +    complete(&vi->completion);

>>> +}

>>> +

>>> +static int virtio_i2c_send_reqs(struct virtqueue *vq,

>>> +                struct virtio_i2c_req *reqs,

>>> +                struct i2c_msg *msgs, int nr)

>>> +{

>>> +    struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr;

>>> +    int i, outcnt, incnt, err = 0;

>>> +    u8 *buf;

>>> +

>>> +    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 |= VIRTIO_I2C_FLAGS_FAIL_NEXT;

>>> +

>>> +        outcnt = incnt = 0;

>>> +        sg_init_one(&out_hdr, &reqs[i].out_hdr, 

>>> sizeof(reqs[i].out_hdr));

>>> +        sgs[outcnt++] = &out_hdr;

>>> +

>>> +        buf = kzalloc(msgs[i].len, GFP_KERNEL);

>>> +        if (!buf)

>>> +            break;

>>> +

>>> +        reqs[i].buf = buf;

>>> +        sg_init_one(&msg_buf, reqs[i].buf, msgs[i].len);

>>> +

>>> +        if (msgs[i].flags & I2C_M_RD) {

>>> +            sgs[outcnt + incnt++] = &msg_buf;

>>> +        } else {

>>> +            memcpy(reqs[i].buf, msgs[i].buf, msgs[i].len);

>>> +            sgs[outcnt++] = &msg_buf;

>>> +        }

>>> +

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

>>> +        sgs[outcnt + incnt++] = &in_hdr;

>>> +

>>> +        err = virtqueue_add_sgs(vq, sgs, outcnt, incnt, &reqs[i], 

>>> GFP_KERNEL);

>>> +        if (err < 0) {

>>> +            pr_err("failed to add msg[%d] to virtqueue.\n", i);

>>> +            kfree(reqs[i].buf);

>>> +            reqs[i].buf = NULL;

>>> +            break;

>>> +        }

>>> +    }

>>> +

>>> +    return i;

>>> +}

>>> +

>>> +static int virtio_i2c_complete_reqs(struct virtqueue *vq,

>>> +                    struct virtio_i2c_req *reqs,

>>> +                    struct i2c_msg *msgs, int nr)

>>> +{

>>> +    struct virtio_i2c_req *req;

>>> +    unsigned int len;

>>> +    int i;

>>> +

>>> +    for (i = 0; i < nr; i++) {

>>> +        req = (struct virtio_i2c_req *)virtqueue_get_buf(vq, &len);

>>

>>

>> The case is unnecessary.

>>

> Right. Will remove the conversion.

>>

>>> +        if (!(req && req == &reqs[i])) {

>>> +            pr_err("msg[%d]: addr=0x%x virtqueue error.\n", i, 

>>> msgs[i].addr);

>>> +            break;

>>> +        }

>>

>>

>> It's better to be more specific here, e.g we can say we saw out of 

>> order completion here.

>>

> OK.

>>

>>> +

>>> +        if (req->in_hdr.status != VIRTIO_I2C_MSG_OK) {

>>> +            pr_err("msg[%d]: addr=0x%x backend error.\n", i, 

>>> msgs[i].addr);

>>> +            break;

>>

>>

>> Don't we need to clean used ring by keeping calling 

>> virtqueue_get_buf() here?

>>

> You are right. Need to detach all the used buffers from the vq before 

> queuing new buffers.

>

>>> +        }

>>> +

>>> +        if (msgs[i].flags & I2C_M_RD)

>>> +            memcpy(msgs[i].buf, req->buf, msgs[i].len);

>>

>>

>> Sorry if I had asked this before but any rason not to use msg[i].buf 

>> directly?

>>

> Will try to use msg[i].buf.

>>

>>> +

>>> +        kfree(req->buf);

>>> +        req->buf = NULL;

>>> +    }

>>> +

>>> +    return i;

>>> +}

>>> +

>>> +static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg 

>>> *msgs, int num)

>>> +{

>>> +    struct virtio_i2c *vi = i2c_get_adapdata(adap);

>>> +    struct virtqueue *vq = vi->vq;

>>> +    struct virtio_i2c_req *reqs;

>>> +    unsigned long time_left;

>>> +    int ret, nr;

>>> +

>>> +    reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL);

>>> +    if (!reqs)

>>> +        return -ENOMEM;

>>> +

>>> +    mutex_lock(&vi->i2c_lock);

>>> +

>>> +    ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);

>>> +    if (ret == 0)

>>> +        goto err_unlock_free;

>>> +

>>> +    nr = ret;

>>> +

>>> +    virtqueue_kick(vq);

>>> +

>>> +    time_left = wait_for_completion_timeout(&vi->completion, 

>>> adap->timeout);

>>> +    if (!time_left) {

>>> +        dev_err(&adap->dev, "virtio i2c backend timeout.\n");

>>> +        ret = -ETIMEDOUT;

>>> +        goto err_unlock_free;

>>

>>

>> So if the request is finished after the timerout, all the following 

>> request will fail, is this expected?

>>

>>

> This is an expected behavior. 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.



So you need at least reinit the completion at least?


>>> +    }

>>> +

>>> +    ret = virtio_i2c_complete_reqs(vq, reqs, msgs, nr);

>>

>>

>> So consider driver queue N requests, can device raise interrupt if it 

>> completes the first request? If yes, the code break, if not it need 

>> to be clarified in the spec.

> The device can raise interrupt when some requests are still not 

> completed though this is not a good operation.



Then you need forbid this in the spec.


> In this case, the remaining requests in the vq will be ignored and the 

> i2c_algorithm. master_xfer will return 1 for

> your example. And let the i2c core to decide whether to resend.

>>

>> Acaultly I remember there's no VIRTIO_I2C_FLAGS_FAIL_NEXT in previous 

>> versions, and after reading the spec I still don't get the motivation 

>> for that (it may complicates both driver and device actually).

>>

> This flag is introduced by Stefan. Please check following link for the 

> details

> https://lists.oasis-open.org/archives/virtio-comment/202012/msg00075.html. 

>



 > We just need to make sure that once the driver adds some requests to the

 > virtqueue,

 > it should complete them (either success or fail) before adding new 

requests.
 > I think this

 > is a behavior of physical I2C adapter drivers and we can follow.



Is this a driver requirement or device? If it's the driver, the code 
have already did something like this. E.g you wait for the completion of 
the request and forbid new request via i2c_lock.

Thanks


>>

>>> +

>>> +    reinit_completion(&vi->completion);

>>

>>

>> So if a request it timeout but interrupt is raised here, everything 

>> is broken.

>>

>> Thanks

>>

> If interrupt is raised after timeout, those requests in the vq will be 

> ignored.

> No need to care about them being handled or not. Just let the i2c core to

> decide whether need to resend or not.

>>

>>> +

>>> +err_unlock_free:

>>> +    mutex_unlock(&vi->i2c_lock);

>>> +    kfree(reqs);

>>> +    return ret;

>>> +}

>>> +

>>> +static void virtio_i2c_del_vqs(struct virtio_device *vdev)

>>> +{

>>> +    vdev->config->reset(vdev);

>>> +    vdev->config->del_vqs(vdev);

>>> +}

>>> +

>>> +static int virtio_i2c_setup_vqs(struct virtio_i2c *vi)

>>> +{

>>> +    struct virtio_device *vdev = vi->vdev;

>>> +

>>> +    vi->vq = virtio_find_single_vq(vdev, virtio_i2c_msg_done, "msg");

>>> +    return PTR_ERR_OR_ZERO(vi->vq);

>>> +}

>>> +

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

>>> +{

>>> +    return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;

>>> +}

>>> +

>>> +static struct i2c_algorithm virtio_algorithm = {

>>> +    .master_xfer = virtio_i2c_xfer,

>>> +    .functionality = virtio_i2c_func,

>>> +};

>>> +

>>> +static struct i2c_adapter virtio_adapter = {

>>> +    .owner = THIS_MODULE,

>>> +    .name = "Virtio I2C Adapter",

>>> +    .class = I2C_CLASS_DEPRECATED,

>>> +    .algo = &virtio_algorithm,

>>> +};

>>> +

>>> +static int virtio_i2c_probe(struct virtio_device *vdev)

>>> +{

>>> +    struct device *pdev = vdev->dev.parent;

>>> +    struct virtio_i2c *vi;

>>> +    int ret;

>>> +

>>> +    vi = devm_kzalloc(&vdev->dev, sizeof(*vi), GFP_KERNEL);

>>> +    if (!vi)

>>> +        return -ENOMEM;

>>> +

>>> +    vdev->priv = vi;

>>> +    vi->vdev = vdev;

>>> +

>>> +    mutex_init(&vi->i2c_lock);

>>> +    init_completion(&vi->completion);

>>> +

>>> +    ret = virtio_i2c_setup_vqs(vi);

>>> +    if (ret)

>>> +        return ret;

>>> +

>>> +    vi->adap = virtio_adapter;

>>> +    i2c_set_adapdata(&vi->adap, vi);

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

>>> +    /* Setup ACPI node for controlled devices which will be probed 

>>> through ACPI */

>>> +    ACPI_COMPANION_SET(&vi->adap.dev, ACPI_COMPANION(pdev));

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

>>> +

>>> +    ret = i2c_add_adapter(&vi->adap);

>>> +    if (ret) {

>>> +        virtio_i2c_del_vqs(vdev);

>>> +        dev_err(&vdev->dev, "failed to add virtio-i2c adapter.\n");

>>> +    }

>>> +

>>> +    return ret;

>>> +}

>>> +

>>> +static void virtio_i2c_remove(struct virtio_device *vdev)

>>> +{

>>> +    struct virtio_i2c *vi = vdev->priv;

>>> +

>>> +    i2c_del_adapter(&vi->adap);

>>> +    virtio_i2c_del_vqs(vdev);

>>> +}

>>> +

>>> +static struct virtio_device_id id_table[] = {

>>> +    { VIRTIO_ID_I2C_ADPTER, VIRTIO_DEV_ANY_ID },

>>> +    {}

>>> +};

>>> +MODULE_DEVICE_TABLE(virtio, id_table);

>>> +

>>> +static int __maybe_unused virtio_i2c_freeze(struct virtio_device 

>>> *vdev)

>>> +{

>>> +    virtio_i2c_del_vqs(vdev);

>>> +    return 0;

>>> +}

>>> +

>>> +static int __maybe_unused virtio_i2c_restore(struct virtio_device 

>>> *vdev)

>>> +{

>>> +    return virtio_i2c_setup_vqs(vdev->priv);

>>> +}

>>> +

>>> +static struct virtio_driver virtio_i2c_driver = {

>>> +    .id_table    = id_table,

>>> +    .probe        = virtio_i2c_probe,

>>> +    .remove        = virtio_i2c_remove,

>>> +    .driver    = {

>>> +        .name    = "i2c_virtio",

>>> +    },

>>> +#ifdef CONFIG_PM_SLEEP

>>> +    .freeze = virtio_i2c_freeze,

>>> +    .restore = virtio_i2c_restore,

>>> +#endif

>>> +};

>>> +module_virtio_driver(virtio_i2c_driver);

>>> +

>>> +MODULE_DESCRIPTION("Virtio i2c bus driver");

>>> +MODULE_LICENSE("GPL");

>>> diff --git a/include/uapi/linux/virtio_i2c.h 

>>> b/include/uapi/linux/virtio_i2c.h

>>> new file mode 100644

>>> index 0000000..00f4508

>>> --- /dev/null

>>> +++ b/include/uapi/linux/virtio_i2c.h

>>> @@ -0,0 +1,42 @@

>>> +/* SPDX-License-Identifier: GPL-2.0-or-later WITH 

>>> Linux-syscall-note */

>>> +/*

>>> + * Definitions for virtio I2C Adpter

>>> + *

>>> + * Copyright (c) 2021 Intel Corporation. All rights reserved.

>>> + */

>>> +

>>> +#ifndef _UAPI_LINUX_VIRTIO_I2C_H

>>> +#define _UAPI_LINUX_VIRTIO_I2C_H

>>> +

>>> +#include <linux/types.h>

>>> +#include <linux/virtio_ids.h>

>>> +#include <linux/virtio_config.h>

>>> +

>>> +/**

>>> + * struct virtio_i2c_out_hdr - the virtio I2C message OUT header

>>> + * @addr: the controlled device address

>>> + * @padding: used to pad to full dword

>>> + * @flags: used for feature extensibility

>>> + */

>>> +struct virtio_i2c_out_hdr {

>>> +    __le16 addr;

>>> +    __le16 padding;

>>> +    __le32 flags;

>>> +};

>>> +

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

>>> requests */

>>> +#define VIRTIO_I2C_FLAGS_FAIL_NEXT    0x00000001

>>> +

>>> +/**

>>> + * struct virtio_i2c_in_hdr - the virtio I2C message IN header

>>> + * @status: the processing result from the backend

>>> + */

>>> +struct virtio_i2c_in_hdr {

>>> +    u8 status;

>>> +};

>>> +

>>> +/* The final status written by the device */

>>> +#define VIRTIO_I2C_MSG_OK    0

>>> +#define VIRTIO_I2C_MSG_ERR    1

>>> +

>>> +#endif /* _UAPI_LINUX_VIRTIO_I2C_H */

>>> diff --git a/include/uapi/linux/virtio_ids.h 

>>> b/include/uapi/linux/virtio_ids.h

>>> index bc1c062..6ae32db 100644

>>> --- a/include/uapi/linux/virtio_ids.h

>>> +++ b/include/uapi/linux/virtio_ids.h

>>> @@ -54,5 +54,6 @@

>>>   #define VIRTIO_ID_FS            26 /* virtio filesystem */

>>>   #define VIRTIO_ID_PMEM            27 /* virtio pmem */

>>>   #define VIRTIO_ID_MAC80211_HWSIM    29 /* virtio mac80211-hwsim */

>>> +#define VIRTIO_ID_I2C_ADPTER        34 /* virtio i2c adpter */

>>>     #endif /* _LINUX_VIRTIO_IDS_H */

>>

>

On 05-03-21, 15:00, Jie Deng wrote:
> On 2021/3/5 11:09, Viresh Kumar wrote:

> > On 05-03-21, 09:46, Jie Deng wrote:

> > > On 2021/3/4 14:06, Viresh Kumar wrote:

> > > > > +	mutex_lock(&vi->i2c_lock);

> > > > I have never worked with i2c stuff earlier, but I don't think you need a lock

> > > > here. The transactions seem to be serialized by the i2c-core by itself (look at

> > > > i2c_transfer() in i2c-core-base.c), though there is another exported version

> > > > __i2c_transfer() but the comment over it says the callers must take adapter lock

> > > > before calling it.

> > > Lock is needed since no "lock_ops" is registered in this i2c_adapter.

> > drivers/i2c/i2c-core-base.c:

> > 

> > static int i2c_register_adapter(struct i2c_adapter *adap)

> > {

> >          ...

> > 

> >          if (!adap->lock_ops)

> >                  adap->lock_ops = &i2c_adapter_lock_ops;

> > 

> >          ...

> > }

> > 

> > This should take care of it ?

> 

> 

> The problem is that you can't guarantee that adap->algo->master_xfer is only

> called

> from i2c_transfer. Any function who holds the adapter can call

> adap->algo->master_xfer

> directly. So I think it is safer to have a lock in virtio_i2c_xfer.


So I tried to look for such callers in the kernel.

$ git grep -l "\<master_xfer(" 
Documentation/i2c/dev-interface.rst
drivers/gpu/drm/gma500/intel_gmbus.c
drivers/gpu/drm/gma500/psb_intel_sdvo.c
drivers/gpu/drm/i915/display/intel_gmbus.c
drivers/gpu/drm/i915/display/intel_sdvo.c
drivers/i2c/busses/i2c-iop3xx.c
drivers/i2c/i2c-core-base.c
drivers/media/usb/dvb-usb/dw2102.c
drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c
drivers/mfd/88pm860x-i2c.c
include/uapi/linux/i2c.h

Out of these only one caller is not registering the adapter itself.

drivers/mfd/88pm860x-i2c.c

I was expecting everyone to call the generic functions provided by the i2c core,
not sure why this ended up calling the master_xfer stuff directly.

So this should be general practice to go via i2c core I believe, unless I am
missing something here.

Wolfram, can you please clarify if locking is required here or not ?

> > > > > +static struct i2c_adapter virtio_adapter = {

> > > > > +	.owner = THIS_MODULE,

> > > > > +	.name = "Virtio I2C Adapter",

> > > > > +	.class = I2C_CLASS_DEPRECATED,

> > > > Why are we using something that is deprecated here ?

> > > Warn users that the adapter doesn't support classes anymore.

> > So this is the right thing to do? Or this is what we expect from new drivers?

> > Sorry, I am just new to this stuff and so...

> 

> 

> https://patchwork.ozlabs.org/project/linux-i2c/patch/20170729121143.3980-1-wsa@the-dreams.de/


Frankly this confused me even further :)

The earlier comment in the code said:
        "/* Warn users that adapter will stop using classes */"

so this looks more for existing drivers..

Then the commit message says this:

        "Hopefully making clear that it is not needed for new drivers."

and comment says:

        "/* Warn users that the adapter doesn't support classes anymore */"

Reading this it looks this is only required for existing adapters so they can
warn userspace and shouldn't be required for new drivers.

Am I reading it incorrectly ?

-- 
viresh

On 2021/3/5 15:23, Jason Wang wrote:

>

>>>> +    virtqueue_kick(vq);

>>>> +

>>>> +    time_left = wait_for_completion_timeout(&vi->completion, 

>>>> adap->timeout);

>>>> +    if (!time_left) {

>>>> +        dev_err(&adap->dev, "virtio i2c backend timeout.\n");

>>>> +        ret = -ETIMEDOUT;

>>>> +        goto err_unlock_free;

>>>

>>>

>>> So if the request is finished after the timerout, all the following 

>>> request will fail, is this expected?

>>>

>>>

>> This is an expected behavior. 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.

>

>

> So you need at least reinit the completion at least?

>


Right. Will fix it. Thank you.


>

>>>> +    }

>>>> +

>>>> +    ret = virtio_i2c_complete_reqs(vq, reqs, msgs, nr);

>>>

>>>

>>> So consider driver queue N requests, can device raise interrupt if 

>>> it completes the first request? If yes, the code break, if not it 

>>> need to be clarified in the spec.

>> The device can raise interrupt when some requests are still not 

>> completed though this is not a good operation.

>

>

> Then you need forbid this in the spec.

>


Yeah, but I think we can add some description to explain this clearly 
instead of forbid it directly.


>

>> In this case, the remaining requests in the vq will be ignored and 

>> the i2c_algorithm. master_xfer will return 1 for

>> your example. And let the i2c core to decide whether to resend.

>>>

>>> Acaultly I remember there's no VIRTIO_I2C_FLAGS_FAIL_NEXT in 

>>> previous versions, and after reading the spec I still don't get the 

>>> motivation for that (it may complicates both driver and device 

>>> actually).

>>>

>> This flag is introduced by Stefan. Please check following link for 

>> the details

>> https://lists.oasis-open.org/archives/virtio-comment/202012/msg00075.html. 

>>

>

>

> > We just need to make sure that once the driver adds some requests to 

> the

> > virtqueue,

> > it should complete them (either success or fail) before adding new 

> requests.

> > I think this

> > is a behavior of physical I2C adapter drivers and we can follow.

>

>

> Is this a driver requirement or device? If it's the driver, the code 

> have already did something like this. E.g you wait for the completion 

> of the request and forbid new request via i2c_lock.

>

> Thanks

>


The driver.  VIRTIO_I2C_FLAGS_FAIL_NEXT doesn't help in Linux driver. 
But I agree with Stefan that
VIRTIO is not specific to Linux so the specs design should avoid the 
limitations of the current
Linux driver behavior.


>

>>

>

On 2021/3/4 17:15, Jason Wang wrote:
>

>

>> +        }

>> +

>> +        if (msgs[i].flags & I2C_M_RD)

>> +            memcpy(msgs[i].buf, req->buf, msgs[i].len);

>

>

> Sorry if I had asked this before but any rason not to use msg[i].buf 

> directly?

>

>

The msg[i].buf is passed by the I2C core. I just noticed that these bufs 
are not
always allocated by kmalloc. They may come from the stack, which may cause
the check "sg_init_one -> sg_set_buf -> virt_addr_valid"  to fail. 
Therefore the
msg[i].buf is not suitable for direct use here.

Regards.

On 2021/3/10 10:22 上午, Jie Deng wrote:
>

> On 2021/3/4 17:15, Jason Wang wrote:

>>

>>

>>> +        }

>>> +

>>> +        if (msgs[i].flags & I2C_M_RD)

>>> +            memcpy(msgs[i].buf, req->buf, msgs[i].len);

>>

>>

>> Sorry if I had asked this before but any rason not to use msg[i].buf 

>> directly?

>>

>>

> The msg[i].buf is passed by the I2C core. I just noticed that these 

> bufs are not

> always allocated by kmalloc. They may come from the stack, which may 

> cause

> the check "sg_init_one -> sg_set_buf -> virt_addr_valid"  to fail. 

> Therefore the

> msg[i].buf is not suitable for direct use here.

>

> Regards.



Right, stack is virtually mapped.

Thanks

On Wed, Mar 10, 2021 at 4:59 AM Jason Wang <jasowang@redhat.com> wrote:
> On 2021/3/10 10:22 上午, Jie Deng wrote:

> > On 2021/3/4 17:15, Jason Wang wrote:

> >>

> >>

> >>> +        }

> >>> +

> >>> +        if (msgs[i].flags & I2C_M_RD)

> >>> +            memcpy(msgs[i].buf, req->buf, msgs[i].len);

> >>

> >>

> >> Sorry if I had asked this before but any rason not to use msg[i].buf

> >> directly?

> >>

> >>

> > The msg[i].buf is passed by the I2C core. I just noticed that these

> > bufs are not

> > always allocated by kmalloc. They may come from the stack, which may

> > cause

> > the check "sg_init_one -> sg_set_buf -> virt_addr_valid"  to fail.

> > Therefore the

> > msg[i].buf is not suitable for direct use here.

>

> Right, stack is virtually mapped.


Maybe there is (or should be) a way to let the i2c core code handle
the bounce buffering in this case. This is surely not a problem that
is unique to this driver, and I'm sure it has come up many times in
the past.

I see that there is a i2c_get_dma_safe_msg_buf() helper for this
purpose, but it has to be called by the driver rather than the core,
so the driver still needs to keep track of each address when it
sends multiple i2c_msg at once, but maybe it can all be done
inside the sg_table instead of yet another structure.

At least this one avoids copying data that is marked with the
I2C_M_DMA_SAFE flag.

       Arnd

On 2021/3/10 16:27, Arnd Bergmann wrote:
> On Wed, Mar 10, 2021 at 4:59 AM Jason Wang <jasowang@redhat.com> wrote:

>> On 2021/3/10 10:22 上午, Jie Deng wrote:

>>> On 2021/3/4 17:15, Jason Wang wrote:

>>>>

>>>>> +        }

>>>>> +

>>>>> +        if (msgs[i].flags & I2C_M_RD)

>>>>> +            memcpy(msgs[i].buf, req->buf, msgs[i].len);

>>>>

>>>> Sorry if I had asked this before but any rason not to use msg[i].buf

>>>> directly?

>>>>

>>>>

>>> The msg[i].buf is passed by the I2C core. I just noticed that these

>>> bufs are not

>>> always allocated by kmalloc. They may come from the stack, which may

>>> cause

>>> the check "sg_init_one -> sg_set_buf -> virt_addr_valid"  to fail.

>>> Therefore the

>>> msg[i].buf is not suitable for direct use here.

>> Right, stack is virtually mapped.

> Maybe there is (or should be) a way to let the i2c core code handle

> the bounce buffering in this case. This is surely not a problem that

> is unique to this driver, and I'm sure it has come up many times in

> the past.

>

> I see that there is a i2c_get_dma_safe_msg_buf() helper for this

> purpose, but it has to be called by the driver rather than the core,

> so the driver still needs to keep track of each address when it

> sends multiple i2c_msg at once, but maybe it can all be done

> inside the sg_table instead of yet another structure.

>

> At least this one avoids copying data that is marked with the

> I2C_M_DMA_SAFE flag.

>

>         Arnd


Make sense. Thanks Arnd. I will try to use those helper functions.

Regards,
Jie