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

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

[Andy Shevchenko]

On Tue, Sep 22, 2020 at 10:58:43AM +0800, Jie Deng wrote:
> Add an I2C bus driver for virtio para-virtualization.
> 
> The controller can be emulated by the backend driver in
> any device model software by following the virtio protocol.
> 
> This driver communicates with the backend driver through a
> virtio I2C message structure which includes following parts:
> 
> - Header: i2c_msg addr, flags, len.
> - Data buffer: the pointer to the I2C msg data.
> - Status: the processing result from the backend.
> 
> People may implement different backend drivers to emulate
> different controllers according to their needs. A backend
> example can be found in the device model of the open source
> project ACRN. For more information, please refer to
> https://projectacrn.org.
> 
> The virtio device ID 34 is used for this I2C adpter since IDs
> before 34 have been reserved by other virtio devices.
> 
> 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>
> Reviewed-by: Shuo Liu <shuo.a.liu@intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> The device ID request:
>         https://github.com/oasis-tcs/virtio-spec/issues/85
> 
> The specification:
> 	https://lists.oasis-open.org/archives/virtio-comment/202009/msg00021.html
> 
> Changes in v3:
>         - Move the interface into uAPI according to Jason.
>         - Fix issues reported by Dan Carpenter.
> 	- Fix typo reported by Randy.
> 
> Changes in v2:
>         - Addressed comments received from Michael, Andy and Jason.
> 
>  drivers/i2c/busses/Kconfig      |  11 ++
>  drivers/i2c/busses/Makefile     |   3 +
>  drivers/i2c/busses/i2c-virtio.c | 256 ++++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/virtio_i2c.h |  31 +++++
>  include/uapi/linux/virtio_ids.h |   1 +
>  5 files changed, 302 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 293e7a0..f2f6543 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
> +	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 19aff0e..821acfa 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..48fd780
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-virtio.c
> @@ -0,0 +1,256 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Virtio I2C Bus Driver
> + *
> + * Copyright (c) 2020 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_msg - the virtio I2C message structure
> + * @hdr: the virtio I2C message header
> + * @buf: virtio I2C message data buffer
> + * @status: the processing result from the backend
> + */
> +struct virtio_i2c_msg {
> +	struct virtio_i2c_hdr hdr;
> +	u8 *buf;
> +	u8 status;
> +};
> +
> +/**
> + * struct virtio_i2c - virtio I2C data
> + * @vdev: virtio device for this controller
> + * @completion: completion of virtio I2C message
> + * @vmsg: the virtio I2C message for communication
> + * @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 virtio_i2c_msg vmsg;
> +	struct i2c_adapter adap;
> +	struct mutex i2c_lock;
> +	struct virtqueue *vq;
> +};
> +
> +static void virtio_i2c_msg_done(struct virtqueue *vq)
> +{
> +	struct virtio_i2c *vi = vq->vdev->priv;
> +
> +	complete(&vi->completion);
> +}
> +
> +static int virtio_i2c_add_msg(struct virtqueue *vq,
> +			      struct virtio_i2c_msg *vmsg,
> +			      struct i2c_msg *msg)
> +{
> +	struct scatterlist *sgs[3], hdr, bout, bin, status;
> +	int outcnt = 0, incnt = 0;
> +
> +	if (!msg->len)
> +		return -EINVAL;
> +
> +	vmsg->hdr.addr = cpu_to_le16(msg->addr);
> +	vmsg->hdr.flags = cpu_to_le16(msg->flags);
> +	vmsg->hdr.len = cpu_to_le16(msg->len);
> +
> +	vmsg->buf = kzalloc(msg->len, GFP_KERNEL);
> +	if (!vmsg->buf)
> +		return -ENOMEM;
> +
> +	sg_init_one(&hdr, &vmsg->hdr, sizeof(struct virtio_i2c_hdr));
> +	sgs[outcnt++] = &hdr;
> +	if (msg->flags & I2C_M_RD) {
> +		sg_init_one(&bin, vmsg->buf, msg->len);
> +		sgs[outcnt + incnt++] = &bin;
> +	} else {
> +		memcpy(vmsg->buf, msg->buf, msg->len);
> +		sg_init_one(&bout, vmsg->buf, msg->len);
> +		sgs[outcnt++] = &bout;
> +	}
> +	sg_init_one(&status, &vmsg->status, sizeof(vmsg->status));
> +	sgs[outcnt + incnt++] = &status;
> +
> +	return virtqueue_add_sgs(vq, sgs, outcnt, incnt, vmsg, GFP_KERNEL);
> +}
> +
> +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_msg *vmsg;
> +	unsigned long time_left;
> +	int len, i, ret = 0;
> +
> +	mutex_lock(&vi->i2c_lock);
> +	vmsg = &vi->vmsg;
> +	vmsg->buf = NULL;
> +
> +	for (i = 0; i < num; i++) {
> +		ret = virtio_i2c_add_msg(vq, vmsg, &msgs[i]);
> +		if (ret) {
> +			dev_err(&adap->dev, "failed to add msg[%d] to virtqueue.\n", i);
> +			break;
> +		}
> +
> +		virtqueue_kick(vq);
> +
> +		time_left = wait_for_completion_timeout(&vi->completion, adap->timeout);
> +		if (!time_left) {
> +			dev_err(&adap->dev, "msg[%d]: addr=0x%x timeout.\n", i, msgs[i].addr);
> +			break;
> +		}
> +
> +		vmsg = (struct virtio_i2c_msg *)virtqueue_get_buf(vq, &len);
> +		/* vmsg should point to the same address with &vi->vmsg */

> +		if ((!vmsg) || (vmsg != &vi->vmsg)) {

This is a bit difficult to read (esp. taking into account the above comment)
besides the fact of redundant parentheses. Why not simply:

		if (!(vmsg && vmsg == &vi->vmsg)) {

?

> +			dev_err(&adap->dev, "msg[%d]: addr=0x%x virtqueue error.\n",
> +				i, msgs[i].addr);
> +			break;
> +		}
> +
> +		if (vmsg->status != VIRTIO_I2C_MSG_OK) {
> +			dev_err(&adap->dev, "msg[%d]: addr=0x%x error=%d.\n",
> +				i, msgs[i].addr, vmsg->status);
> +			break;
> +		}
> +
> +		if ((msgs[i].flags & I2C_M_RD) && msgs[i].len)
> +			memcpy(msgs[i].buf, vmsg->buf, msgs[i].len);
> +
> +		kfree(vmsg->buf);
> +		vmsg->buf = NULL;
> +
> +		reinit_completion(&vi->completion);
> +	}
> +
> +	mutex_unlock(&vi->i2c_lock);
> +	kfree(vi->vmsg.buf);
> +	vi->vmsg.buf = NULL;
> +	return ((ret < 0) ? ret : i);
> +}
> +
> +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 slave 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..7413e45
> --- /dev/null
> +++ b/include/uapi/linux/virtio_i2c.h
> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later OR BSD-3-Clause */
> +/*
> + * Definitions for virtio I2C Adpter
> + *
> + * Copyright (c) 2020 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_hdr - the virtio I2C message header structure
> + * @addr: i2c_msg addr, the slave address
> + * @flags: i2c_msg flags
> + * @len: i2c_msg len
> + */
> +struct virtio_i2c_hdr {
> +	__le16 addr;
> +	__le16 flags;
> +	__le16 len;
> +};
> +
> +/* 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 b052355..398ef2d 100644
> --- a/include/uapi/linux/virtio_ids.h
> +++ b/include/uapi/linux/virtio_ids.h
> @@ -48,5 +48,6 @@
>  #define VIRTIO_ID_FS           26 /* virtio filesystem */
>  #define VIRTIO_ID_PMEM         27 /* virtio pmem */
>  #define VIRTIO_ID_MAC80211_HWSIM 29 /* virtio mac80211-hwsim */
> +#define VIRTIO_ID_I2C_ADPTER   34 /* virtio i2c adpter */
>  
>  #endif /* _LINUX_VIRTIO_IDS_H */
> -- 
> 2.7.4
> 

-- 
With Best Regards,
Andy Shevchenko

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

[Wolfram Sang]

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

Hi,

some super high level questions:

> different controllers according to their needs. A backend
> example can be found in the device model of the open source
> project ACRN. For more information, please refer to
> https://projectacrn.org.

Could you provide a link directly to the backend, please?

> The device ID request:
>         https://github.com/oasis-tcs/virtio-spec/issues/85

Shall we wait for this to be approved? Or will it get only approved once
the driver here is upstream?

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

That means stuff like "limiting which devices on a given bus can be
accessed" will be handled by the backends, or?

What kind of testing has been done with this on which setup?

Thanks and happy hacking,

   Wolfram


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

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

[Jie Deng]

On 2020/10/8 22:01, Wolfram Sang wrote:
> Hi,
>
> some super high level questions:
>
>> different controllers according to their needs. A backend
>> example can be found in the device model of the open source
>> project ACRN. For more information, please refer to
>> https://projectacrn.org.
> Could you provide a link directly to the backend, please?
Sure. Here is the link.
https://raw.githubusercontent.com/projectacrn/acrn-hypervisor/master/devicemodel/hw/pci/virtio/virtio_i2c.c
>> The device ID request:
>>          https://github.com/oasis-tcs/virtio-spec/issues/85
> Shall we wait for this to be approved? Or will it get only approved once
> the driver here is upstream?
That's what I want to know also.
So hi Michael, what's the upstream flow for this patch ?

Thanks.


>> +	  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.
> That means stuff like "limiting which devices on a given bus can be
> accessed" will be handled by the backends, or?
>
> What kind of testing has been done with this on which setup?
>
> Thanks and happy hacking,
>
>     Wolfram
Yes, you can configure what devices can be seen by the guest.
This provides a way to flexibly organize and manage I2C slave devices 
from the guest.

We tested it on Intel APL MRB. There are some docs for you reference.
https://projectacrn.github.io/latest/developer-guides/hld/virtio-i2c.html?highlight=i2c

Regards.

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

[Jason Wang]

On 2020/9/22 上午10:58, 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.
>
> This driver communicates with the backend driver through a
> virtio I2C message structure which includes following parts:
>
> - Header: i2c_msg addr, flags, len.
> - Data buffer: the pointer to the I2C msg data.
> - Status: the processing result from the backend.
>
> People may implement different backend drivers to emulate
> different controllers according to their needs. A backend
> example can be found in the device model of the open source
> project ACRN. For more information, please refer to
> https://projectacrn.org.
>
> The virtio device ID 34 is used for this I2C adpter since IDs
> before 34 have been reserved by other virtio devices.
>
> 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>
> Reviewed-by: Shuo Liu <shuo.a.liu@intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> The device ID request:
>          https://github.com/oasis-tcs/virtio-spec/issues/85
>
> The specification:
> 	https://lists.oasis-open.org/archives/virtio-comment/202009/msg00021.html
>
> Changes in v3:
>          - Move the interface into uAPI according to Jason.
>          - Fix issues reported by Dan Carpenter.
> 	- Fix typo reported by Randy.
>
> Changes in v2:
>          - Addressed comments received from Michael, Andy and Jason.
>
>   drivers/i2c/busses/Kconfig      |  11 ++
>   drivers/i2c/busses/Makefile     |   3 +
>   drivers/i2c/busses/i2c-virtio.c | 256 ++++++++++++++++++++++++++++++++++++++++
>   include/uapi/linux/virtio_i2c.h |  31 +++++
>   include/uapi/linux/virtio_ids.h |   1 +
>   5 files changed, 302 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 293e7a0..f2f6543 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
> +	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 19aff0e..821acfa 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..48fd780
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-virtio.c
> @@ -0,0 +1,256 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Virtio I2C Bus Driver
> + *
> + * Copyright (c) 2020 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_msg - the virtio I2C message structure
> + * @hdr: the virtio I2C message header
> + * @buf: virtio I2C message data buffer
> + * @status: the processing result from the backend
> + */
> +struct virtio_i2c_msg {
> +	struct virtio_i2c_hdr hdr;
> +	u8 *buf;
> +	u8 status;
> +};
> +
> +/**
> + * struct virtio_i2c - virtio I2C data
> + * @vdev: virtio device for this controller
> + * @completion: completion of virtio I2C message
> + * @vmsg: the virtio I2C message for communication
> + * @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 virtio_i2c_msg vmsg;
> +	struct i2c_adapter adap;
> +	struct mutex i2c_lock;
> +	struct virtqueue *vq;
> +};
> +
> +static void virtio_i2c_msg_done(struct virtqueue *vq)
> +{
> +	struct virtio_i2c *vi = vq->vdev->priv;
> +
> +	complete(&vi->completion);
> +}
> +
> +static int virtio_i2c_add_msg(struct virtqueue *vq,
> +			      struct virtio_i2c_msg *vmsg,
> +			      struct i2c_msg *msg)
> +{
> +	struct scatterlist *sgs[3], hdr, bout, bin, status;
> +	int outcnt = 0, incnt = 0;
> +
> +	if (!msg->len)
> +		return -EINVAL;
> +
> +	vmsg->hdr.addr = cpu_to_le16(msg->addr);
> +	vmsg->hdr.flags = cpu_to_le16(msg->flags);
> +	vmsg->hdr.len = cpu_to_le16(msg->len);
> +
> +	vmsg->buf = kzalloc(msg->len, GFP_KERNEL);
> +	if (!vmsg->buf)
> +		return -ENOMEM;
> +
> +	sg_init_one(&hdr, &vmsg->hdr, sizeof(struct virtio_i2c_hdr));
> +	sgs[outcnt++] = &hdr;
> +	if (msg->flags & I2C_M_RD) {
> +		sg_init_one(&bin, vmsg->buf, msg->len);
> +		sgs[outcnt + incnt++] = &bin;
> +	} else {
> +		memcpy(vmsg->buf, msg->buf, msg->len);
> +		sg_init_one(&bout, vmsg->buf, msg->len);
> +		sgs[outcnt++] = &bout;
> +	}
> +	sg_init_one(&status, &vmsg->status, sizeof(vmsg->status));
> +	sgs[outcnt + incnt++] = &status;
> +
> +	return virtqueue_add_sgs(vq, sgs, outcnt, incnt, vmsg, GFP_KERNEL);
> +}
> +
> +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_msg *vmsg;
> +	unsigned long time_left;
> +	int len, i, ret = 0;
> +
> +	mutex_lock(&vi->i2c_lock);
> +	vmsg = &vi->vmsg;
> +	vmsg->buf = NULL;
> +
> +	for (i = 0; i < num; i++) {
> +		ret = virtio_i2c_add_msg(vq, vmsg, &msgs[i]);
> +		if (ret) {
> +			dev_err(&adap->dev, "failed to add msg[%d] to virtqueue.\n", i);
> +			break;
> +		}
> +
> +		virtqueue_kick(vq);
> +
> +		time_left = wait_for_completion_timeout(&vi->completion, adap->timeout);
> +		if (!time_left) {
> +			dev_err(&adap->dev, "msg[%d]: addr=0x%x timeout.\n", i, msgs[i].addr);
> +			break;
> +		}


You don't set error number here. Is this intended?

And using a timeout here is not good, and if the request is finished 
just after the timeout, in the next xfer you may hit the following check.

It's better to use either interrupt here.


> +
> +		vmsg = (struct virtio_i2c_msg *)virtqueue_get_buf(vq, &len);
> +		/* vmsg should point to the same address with &vi->vmsg */
> +		if ((!vmsg) || (vmsg != &vi->vmsg)) {
> +			dev_err(&adap->dev, "msg[%d]: addr=0x%x virtqueue error.\n",
> +				i, msgs[i].addr);
> +			break;
> +		}


So I think we can remove this check. Consider only one descriptor will 
be used at most, unless there's a bug in the device (and no other driver 
to the similar check), we should not hit this.

Btw, as I replied in the previous version, the device should be cacpable 
of dealing of a batch of requests through the virtqueue, otherwise it's 
meaningless to use a queue here.


> +
> +		if (vmsg->status != VIRTIO_I2C_MSG_OK) {
> +			dev_err(&adap->dev, "msg[%d]: addr=0x%x error=%d.\n",
> +				i, msgs[i].addr, vmsg->status);
> +			break;
> +		}
> +
> +		if ((msgs[i].flags & I2C_M_RD) && msgs[i].len)
> +			memcpy(msgs[i].buf, vmsg->buf, msgs[i].len);
> +
> +		kfree(vmsg->buf);
> +		vmsg->buf = NULL;
> +
> +		reinit_completion(&vi->completion);
> +	}
> +
> +	mutex_unlock(&vi->i2c_lock);
> +	kfree(vi->vmsg.buf);
> +	vi->vmsg.buf = NULL;
> +	return ((ret < 0) ? ret : i);
> +}
> +
> +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 slave 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..7413e45
> --- /dev/null
> +++ b/include/uapi/linux/virtio_i2c.h
> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later OR BSD-3-Clause */
> +/*
> + * Definitions for virtio I2C Adpter
> + *
> + * Copyright (c) 2020 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_hdr - the virtio I2C message header structure
> + * @addr: i2c_msg addr, the slave address
> + * @flags: i2c_msg flags
> + * @len: i2c_msg len
> + */
> +struct virtio_i2c_hdr {
> +	__le16 addr;
> +	__le16 flags;
> +	__le16 len;
> +};


I'm afraid this is not complete. E.g the status is missed.

I suspect what virtio-scsi use is better. Which split the in from the 
out instead of reusing the same buffer. And it can ease the uAPI header 
export.

Thanks


> +
> +/* 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 b052355..398ef2d 100644
> --- a/include/uapi/linux/virtio_ids.h
> +++ b/include/uapi/linux/virtio_ids.h
> @@ -48,5 +48,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 */

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

[Jason Wang]

On 2020/10/12 上午10:45, Jie Deng wrote:
>
>
> On 2020/10/10 11:14, Jason Wang wrote:
>>
>>> +
>>> +        virtqueue_kick(vq);
>>> +
>>> +        time_left = wait_for_completion_timeout(&vi->completion, 
>>> adap->timeout);
>>> +        if (!time_left) {
>>> +            dev_err(&adap->dev, "msg[%d]: addr=0x%x timeout.\n", i, 
>>> msgs[i].addr);
>>> +            break;
>>> +        }
>>
>>
>> You don't set error number here. Is this intended?
>>
>> And using a timeout here is not good, and if the request is finished 
>> just after the timeout, in the next xfer you may hit the following 
>> check.
>>
>> It's better to use either interrupt here.
>>
> Could you check the I2C drivers in the kernel ? The 
> "wait_for_completion_timeout" mechanism
> is commonly used by I2C bus drivers in their i2c_algorithm.master_xfer.


There's a major difference between virtio-i2c and other drivers. In the 
case of virtio, the device could be a software device emulated by a 
remote process. This means the timeout might not be rare.

I don't see how timeout is properly handled in this patch (e.g did you 
notice that you don't set any error when timeout? or is this intended?)


>
>>
>>> +
>>> +        vmsg = (struct virtio_i2c_msg *)virtqueue_get_buf(vq, &len);
>>> +        /* vmsg should point to the same address with &vi->vmsg */
>>> +        if ((!vmsg) || (vmsg != &vi->vmsg)) {
>>> +            dev_err(&adap->dev, "msg[%d]: addr=0x%x virtqueue 
>>> error.\n",
>>> +                i, msgs[i].addr);
>>> +            break;
>>> +        }
>>
>>
>> So I think we can remove this check. Consider only one descriptor 
>> will be used at most, unless there's a bug in the device (and no 
>> other driver to the similar check), we should not hit this.
>>
>> Btw, as I replied in the previous version, the device should be 
>> cacpable of dealing of a batch of requests through the virtqueue, 
>> otherwise it's meaningless to use a queue here.
>>
> We should not assume there is no bug in the device. I don't think we 
> can remove this check if we want our code to be robust.


Can you tell when at which case you may hit !vmsg or vmsg != vi->vmsg?



> As I said, currently, we are using the virtqueue to send the msg one 
> by one to the backend. The mechanism is described in the spec. 


Which part of the spec describes such "one by one" mechanism? If there 
is one, I'd happily give a NACK since it doesn't require a queue to work 
which is conflict with the concept of the virtqueue.


> Thanks.
>
>
>>
>>> +
>>>
>>> +
>>> +#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_hdr - the virtio I2C message header structure
>>> + * @addr: i2c_msg addr, the slave address
>>> + * @flags: i2c_msg flags
>>> + * @len: i2c_msg len
>>> + */
>>> +struct virtio_i2c_hdr {
>>> +    __le16 addr;
>>> +    __le16 flags;
>>> +    __le16 len;
>>> +};
>>
>>
>> I'm afraid this is not complete. E.g the status is missed.
>>
>> I suspect what virtio-scsi use is better. Which split the in from the 
>> out instead of reusing the same buffer. And it can ease the uAPI 
>> header export.
>>
>> Thanks
>>
>>
>
> I think following definition in uAPI for the status is enough.
> There is no need to provide a "u8" status in the structure.
>
> /* The final status written by the device */
> #define VIRTIO_I2C_MSG_OK    0
> #define VIRTIO_I2C_MSG_ERR    1
>
> You can see an example in virtio_blk.
>
> In the spec:
>
> struct virtio_blk_req {
> le32 type;
> le32 reserved;
> le64 sector;
> u8 data[];
> u8 status;
> };
>
> In virtio_blk.h, there is only following definitions.
>
> #define VIRTIO_BLK_S_OK        0
> #define VIRTIO_BLK_S_IOERR    1
> #define VIRTIO_BLK_S_UNSUPP    2
>

virtio-blk is a bad example, it's just too late to fix. For any new 
introduced uAPI it should be a complete one.

Thanks


> Thanks.
>
>
>

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

[Jie Deng]

On 2020/10/12 11:43, Jason Wang wrote:
>
> On 2020/10/12 上午10:45, Jie Deng wrote:
>>
>>
>> On 2020/10/10 11:14, Jason Wang wrote:
>>>
>>>> +
>>>> +        virtqueue_kick(vq);
>>>> +
>>>> +        time_left = wait_for_completion_timeout(&vi->completion, 
>>>> adap->timeout);
>>>> +        if (!time_left) {
>>>> +            dev_err(&adap->dev, "msg[%d]: addr=0x%x timeout.\n", 
>>>> i, msgs[i].addr);
>>>> +            break;
>>>> +        }
>>>
>>>
>>> You don't set error number here. Is this intended?
>>>
>>> And using a timeout here is not good, and if the request is finished 
>>> just after the timeout, in the next xfer you may hit the following 
>>> check.
>>>
>>> It's better to use either interrupt here.
>>>
>> Could you check the I2C drivers in the kernel ? The 
>> "wait_for_completion_timeout" mechanism
>> is commonly used by I2C bus drivers in their i2c_algorithm.master_xfer.
>
>
> There's a major difference between virtio-i2c and other drivers. In 
> the case of virtio, the device could be a software device emulated by 
> a remote process. This means the timeout might not be rare.
>
> I don't see how timeout is properly handled in this patch (e.g did you 
> notice that you don't set any error when timeout? or is this intended?)
>
The backend software may operate the physical device. The timeout 
depends on how the backend is designed.
Here if the timeout happens, it will return the actual number of 
messages successfully processed to the I2C core.
Let the I2C core decides how to do next.

Thanks.

>
>>
>>>
>>>> +
>>>> +        vmsg = (struct virtio_i2c_msg *)virtqueue_get_buf(vq, &len);
>>>> +        /* vmsg should point to the same address with &vi->vmsg */
>>>> +        if ((!vmsg) || (vmsg != &vi->vmsg)) {
>>>> +            dev_err(&adap->dev, "msg[%d]: addr=0x%x virtqueue 
>>>> error.\n",
>>>> +                i, msgs[i].addr);
>>>> +            break;
>>>> +        }
>>>
>>>
>>> So I think we can remove this check. Consider only one descriptor 
>>> will be used at most, unless there's a bug in the device (and no 
>>> other driver to the similar check), we should not hit this.
>>>
>>> Btw, as I replied in the previous version, the device should be 
>>> cacpable of dealing of a batch of requests through the virtqueue, 
>>> otherwise it's meaningless to use a queue here.
>>>
>> We should not assume there is no bug in the device. I don't think we 
>> can remove this check if we want our code to be robust.
>
>
> Can you tell when at which case you may hit !vmsg or vmsg != vi->vmsg?
>
Normally, it won't hit here. But the API "virtqueue_get_buf" tells me
"It *may *return NULL or the "data" token handed to virtqueue_add_*()."

 From the perspective of a caller, I just don't care when it happens.
To make the code robust, what I care about is what I should do if this 
is not our case
since the doc says it*may *happen.

If you insist on removing this check, I will remove "vmsg != vi->vmsg" 
and keep the check for !vmsg.
As Dan reported in v2, we should at least check here for NULL.

Thanks.

>
>
>> As I said, currently, we are using the virtqueue to send the msg one 
>> by one to the backend. The mechanism is described in the spec. 
>
>
> Which part of the spec describes such "one by one" mechanism? If there 
> is one, I'd happily give a NACK since it doesn't require a queue to 
> work which is conflict with the concept of the virtqueue.
>
>
What's the concept of the virtqueue ?  Why do you want to restrict how 
users use virtqueue ?

It's like you provide a water glass to user. The user can fill a full 
glass of water and drinks once or
fill half a glass of water and drink twice. It is a user behavior and 
should not be restricted by
the glass provider.

Thanks.


>> Thanks.
>>
>>
>>>
>>>> +
>>>>
>>>> +
>>>> +#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_hdr - the virtio I2C message header structure
>>>> + * @addr: i2c_msg addr, the slave address
>>>> + * @flags: i2c_msg flags
>>>> + * @len: i2c_msg len
>>>> + */
>>>> +struct virtio_i2c_hdr {
>>>> +    __le16 addr;
>>>> +    __le16 flags;
>>>> +    __le16 len;
>>>> +};
>>>
>>>
>>> I'm afraid this is not complete. E.g the status is missed.
>>>
>>> I suspect what virtio-scsi use is better. Which split the in from 
>>> the out instead of reusing the same buffer. And it can ease the uAPI 
>>> header export.
>>>
>>> Thanks
>>>
>>>
>>
>> I think following definition in uAPI for the status is enough.
>> There is no need to provide a "u8" status in the structure.
>>
>> /* The final status written by the device */
>> #define VIRTIO_I2C_MSG_OK    0
>> #define VIRTIO_I2C_MSG_ERR    1
>>
>> You can see an example in virtio_blk.
>>
>> In the spec:
>>
>> struct virtio_blk_req {
>> le32 type;
>> le32 reserved;
>> le64 sector;
>> u8 data[];
>> u8 status;
>> };
>>
>> In virtio_blk.h, there is only following definitions.
>>
>> #define VIRTIO_BLK_S_OK        0
>> #define VIRTIO_BLK_S_IOERR    1
>> #define VIRTIO_BLK_S_UNSUPP    2
>>
>
> virtio-blk is a bad example, it's just too late to fix. For any new 
> introduced uAPI it should be a complete one.
>
> Thanks
>
I checked a relatively new device "virtio_fs".
I found following definition in spec but not in uAPI also.

struct virtio_fs_req {
// Device -readable part
struct fuse_in_header in;
u8 datain[];
// Device -writable part
struct fuse_out_header out;
u8 dataout[];
};

So is this also a bad example which has not been fixed yet.
Or what's your mean about "complete" here ? Is there any definition 
about "complete uAPI" ?

Thanks.


>
>> Thanks.
>>
>>
>>
>

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

[Jason Wang]

On 2020/10/13 下午3:16, Jie Deng wrote:
>
> On 2020/10/12 11:43, Jason Wang wrote:
>>
>> On 2020/10/12 上午10:45, Jie Deng wrote:
>>>
>>>
>>> On 2020/10/10 11:14, Jason Wang wrote:
>>>>
>>>>> +
>>>>> +        virtqueue_kick(vq);
>>>>> +
>>>>> +        time_left = wait_for_completion_timeout(&vi->completion, 
>>>>> adap->timeout);
>>>>> +        if (!time_left) {
>>>>> +            dev_err(&adap->dev, "msg[%d]: addr=0x%x timeout.\n", 
>>>>> i, msgs[i].addr);
>>>>> +            break;
>>>>> +        }
>>>>
>>>>
>>>> You don't set error number here. Is this intended?
>>>>
>>>> And using a timeout here is not good, and if the request is 
>>>> finished just after the timeout, in the next xfer you may hit the 
>>>> following check.
>>>>
>>>> It's better to use either interrupt here.
>>>>
>>> Could you check the I2C drivers in the kernel ? The 
>>> "wait_for_completion_timeout" mechanism
>>> is commonly used by I2C bus drivers in their i2c_algorithm.master_xfer.
>>
>>
>> There's a major difference between virtio-i2c and other drivers. In 
>> the case of virtio, the device could be a software device emulated by 
>> a remote process. This means the timeout might not be rare.
>>
>> I don't see how timeout is properly handled in this patch (e.g did 
>> you notice that you don't set any error when timeout? or is this 
>> intended?)
>>
> The backend software may operate the physical device. The timeout 
> depends on how the backend is designed.
> Here if the timeout happens, it will return the actual number of 
> messages successfully processed to the I2C core.
> Let the I2C core decides how to do next.


So let's consider the following case:

1) driver:virtio_i2c_add_msg(msgA)
2) driver:timeout, and return to I2C core
3) driver:virtio_i2c_add_msg(msgB)
4) device: complete msgA
5) driver: virtqueue_get_buf() returns msgA, since the token is always 
vi->vmsg, the driver may think msgB has been completed.


>
> Thanks.
>
>>
>>>
>>>>
>>>>> +
>>>>> +        vmsg = (struct virtio_i2c_msg *)virtqueue_get_buf(vq, &len);
>>>>> +        /* vmsg should point to the same address with &vi->vmsg */
>>>>> +        if ((!vmsg) || (vmsg != &vi->vmsg)) {
>>>>> +            dev_err(&adap->dev, "msg[%d]: addr=0x%x virtqueue 
>>>>> error.\n",
>>>>> +                i, msgs[i].addr);
>>>>> +            break;
>>>>> +        }
>>>>
>>>>
>>>> So I think we can remove this check. Consider only one descriptor 
>>>> will be used at most, unless there's a bug in the device (and no 
>>>> other driver to the similar check), we should not hit this.
>>>>
>>>> Btw, as I replied in the previous version, the device should be 
>>>> cacpable of dealing of a batch of requests through the virtqueue, 
>>>> otherwise it's meaningless to use a queue here.
>>>>
>>> We should not assume there is no bug in the device. I don't think we 
>>> can remove this check if we want our code to be robust.
>>
>>
>> Can you tell when at which case you may hit !vmsg or vmsg != vi->vmsg?
>>
> Normally, it won't hit here. But the API "virtqueue_get_buf" tells me
> "It *may *return NULL or the "data" token handed to virtqueue_add_*()."


Note that we had the following check already in virtqueue_get_buf_ctx(), 
so the the virtio core had already have the ability to figure out the 
wrong head.

     if (unlikely(id >= vq->packed.vring.num)) {
         BAD_RING(vq, "id %u out of range\n", id);
         return NULL;
     }
     if (unlikely(!vq->packed.desc_state[id].data)) {
         BAD_RING(vq, "id %u is not a head!\n", id);
         return NULL;
     }

And when it returns a NULL, it's not necessarily an error of the device, 
it might just require more time to finish the processing.


>
> From the perspective of a caller, I just don't care when it happens.
> To make the code robust, what I care about is what I should do if this 
> is not our case
> since the doc says it*may *happen.
>
> If you insist on removing this check, I will remove "vmsg != vi->vmsg" 
> and keep the check for !vmsg.
> As Dan reported in v2, we should at least check here for NULL.
>
> Thanks.
>
>>
>>
>>> As I said, currently, we are using the virtqueue to send the msg one 
>>> by one to the backend. The mechanism is described in the spec. 
>>
>>
>> Which part of the spec describes such "one by one" mechanism? If 
>> there is one, I'd happily give a NACK since it doesn't require a 
>> queue to work which is conflict with the concept of the virtqueue.
>>
>>
> What's the concept of the virtqueue ?  Why do you want to restrict how 
> users use virtqueue ?


So I think there's some misunderstanding here. The point is not to 
restrict how to use virtqueue.

What I meant is:

- we should not invent a device with a virtqueue that can only accept 
one buffer at a time
- I don't see any mechanism like "one by one" described in the spec, so 
it's ok but if it'd happen to have, I will NACK


>
> It's like you provide a water glass to user. The user can fill a full 
> glass of water and drinks once or
> fill half a glass of water and drink twice. It is a user behavior and 
> should not be restricted by
> the glass provider.


That's my point as well, we should not describe the "once" behavior in 
the spec.


>
> Thanks.
>
>
>>> Thanks.
>>>
>>>
>>>>
>>>>> +
>>>>>
>>>>> +
>>>>> +#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_hdr - the virtio I2C message header structure
>>>>> + * @addr: i2c_msg addr, the slave address
>>>>> + * @flags: i2c_msg flags
>>>>> + * @len: i2c_msg len
>>>>> + */
>>>>> +struct virtio_i2c_hdr {
>>>>> +    __le16 addr;
>>>>> +    __le16 flags;
>>>>> +    __le16 len;
>>>>> +};
>>>>
>>>>
>>>> I'm afraid this is not complete. E.g the status is missed.
>>>>
>>>> I suspect what virtio-scsi use is better. Which split the in from 
>>>> the out instead of reusing the same buffer. And it can ease the 
>>>> uAPI header export.
>>>>
>>>> Thanks
>>>>
>>>>
>>>
>>> I think following definition in uAPI for the status is enough.
>>> There is no need to provide a "u8" status in the structure.
>>>
>>> /* The final status written by the device */
>>> #define VIRTIO_I2C_MSG_OK    0
>>> #define VIRTIO_I2C_MSG_ERR    1
>>>
>>> You can see an example in virtio_blk.
>>>
>>> In the spec:
>>>
>>> struct virtio_blk_req {
>>> le32 type;
>>> le32 reserved;
>>> le64 sector;
>>> u8 data[];
>>> u8 status;
>>> };
>>>
>>> In virtio_blk.h, there is only following definitions.
>>>
>>> #define VIRTIO_BLK_S_OK        0
>>> #define VIRTIO_BLK_S_IOERR    1
>>> #define VIRTIO_BLK_S_UNSUPP    2
>>>
>>
>> virtio-blk is a bad example, it's just too late to fix. For any new 
>> introduced uAPI it should be a complete one.
>>
>> Thanks
>>
> I checked a relatively new device "virtio_fs".
> I found following definition in spec but not in uAPI also.
>
> struct virtio_fs_req {
> // Device -readable part
> struct fuse_in_header in;
> u8 datain[];
> // Device -writable part
> struct fuse_out_header out;
> u8 dataout[];
> };
>
> So is this also a bad example which has not been fixed yet.


Cc Stefan for the answer.


> Or what's your mean about "complete" here ? Is there any definition 
> about "complete uAPI" ?


My understanding it should contain all the fields defined in the virtio 
spec.

Thanks


>
> Thanks.
>
>
>>
>>> Thanks.
>>>
>>>
>>>
>>
>

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

[Jie Deng]

On 2020/10/13 16:00, Jason Wang wrote:
>
>>>>>> +
>>>>>> +        virtqueue_kick(vq);
>>>>>> +
>>>>>> +        time_left = wait_for_completion_timeout(&vi->completion, 
>>>>>> adap->timeout);
>>>>>> +        if (!time_left) {
>>>>>> +            dev_err(&adap->dev, "msg[%d]: addr=0x%x timeout.\n", 
>>>>>> i, msgs[i].addr);
>>>>>> +            break;
>>>>>> +        }
>>>>>
>>>>>
>>>>> You don't set error number here. Is this intended?
>>>>>
>>>>> And using a timeout here is not good, and if the request is 
>>>>> finished just after the timeout, in the next xfer you may hit the 
>>>>> following check.
>>>>>
>>>>> It's better to use either interrupt here.
>>>>>
>>>> Could you check the I2C drivers in the kernel ? The 
>>>> "wait_for_completion_timeout" mechanism
>>>> is commonly used by I2C bus drivers in their 
>>>> i2c_algorithm.master_xfer.
>>>
>>>
>>> There's a major difference between virtio-i2c and other drivers. In 
>>> the case of virtio, the device could be a software device emulated 
>>> by a remote process. This means the timeout might not be rare.
>>>
>>> I don't see how timeout is properly handled in this patch (e.g did 
>>> you notice that you don't set any error when timeout? or is this 
>>> intended?)
>>>
>> The backend software may operate the physical device. The timeout 
>> depends on how the backend is designed.
>> Here if the timeout happens, it will return the actual number of 
>> messages successfully processed to the I2C core.
>> Let the I2C core decides how to do next.
>
>
> So let's consider the following case:
>
> 1) driver:virtio_i2c_add_msg(msgA)
> 2) driver:timeout, and return to I2C core
> 3) driver:virtio_i2c_add_msg(msgB)
> 4) device: complete msgA
> 5) driver: virtqueue_get_buf() returns msgA, since the token is always 
> vi->vmsg, the driver may think msgB has been completed.
>
>
If this case does happen, it is exactly a case that the condition 
"((!vmsg) || (vmsg != &vi->vmsg))" are met.
Currently, the timeout value is hard-coded in the driver. Generally 
speaking, timeout rarely happens.
It can also be designed as a device configuration if needed.

Thanks.

>>
>> Thanks.
>>
>>>
>>>>
>>>>>
>>>>>> +
>>>>>> +        vmsg = (struct virtio_i2c_msg *)virtqueue_get_buf(vq, 
>>>>>> &len);
>>>>>> +        /* vmsg should point to the same address with &vi->vmsg */
>>>>>> +        if ((!vmsg) || (vmsg != &vi->vmsg)) {
>>>>>> +            dev_err(&adap->dev, "msg[%d]: addr=0x%x virtqueue 
>>>>>> error.\n",
>>>>>> +                i, msgs[i].addr);
>>>>>> +            break;
>>>>>> +        }
>>>>>
>>>>>
>>>>> So I think we can remove this check. Consider only one descriptor 
>>>>> will be used at most, unless there's a bug in the device (and no 
>>>>> other driver to the similar check), we should not hit this.
>>>>>
>>>>> Btw, as I replied in the previous version, the device should be 
>>>>> cacpable of dealing of a batch of requests through the virtqueue, 
>>>>> otherwise it's meaningless to use a queue here.
>>>>>
>>>> We should not assume there is no bug in the device. I don't think 
>>>> we can remove this check if we want our code to be robust.
>>>
>>>
>>> Can you tell when at which case you may hit !vmsg or vmsg != vi->vmsg?
>>>
>> Normally, it won't hit here. But the API "virtqueue_get_buf" tells me
>> "It *may *return NULL or the "data" token handed to virtqueue_add_*()."
>
>
> Note that we had the following check already in 
> virtqueue_get_buf_ctx(), so the the virtio core had already have the 
> ability to figure out the wrong head.
>
>     if (unlikely(id >= vq->packed.vring.num)) {
>         BAD_RING(vq, "id %u out of range\n", id);
>         return NULL;
>     }
>     if (unlikely(!vq->packed.desc_state[id].data)) {
>         BAD_RING(vq, "id %u is not a head!\n", id);
>         return NULL;
>     }
>
> And when it returns a NULL, it's not necessarily an error of the 
> device, it might just require more time to finish the processing.
>

That's why we just returned the actual number of messages successfully 
processed in this case,
and let the I2C core to try one more time.

Actually we have no idea if this is a device error or not. Try one more 
time can also fail if it is a backend error.
Of course, there is another option. We can return error for timeout, no 
matter what reason.

Thanks.


>
>>
>> From the perspective of a caller, I just don't care when it happens.
>> To make the code robust, what I care about is what I should do if 
>> this is not our case
>> since the doc says it*may *happen.
>>
>> If you insist on removing this check, I will remove "vmsg != 
>> vi->vmsg" and keep the check for !vmsg.
>> As Dan reported in v2, we should at least check here for NULL.
>>
>> Thanks.
>>
>>>
>>>
>>>> As I said, currently, we are using the virtqueue to send the msg 
>>>> one by one to the backend. The mechanism is described in the spec. 
>>>
>>>
>>> Which part of the spec describes such "one by one" mechanism? If 
>>> there is one, I'd happily give a NACK since it doesn't require a 
>>> queue to work which is conflict with the concept of the virtqueue.
>>>
>>>
>> What's the concept of the virtqueue ?  Why do you want to restrict 
>> how users use virtqueue ?
>
>
> So I think there's some misunderstanding here. The point is not to 
> restrict how to use virtqueue.
>
> What I meant is:
>
> - we should not invent a device with a virtqueue that can only accept 
> one buffer at a time
> - I don't see any mechanism like "one by one" described in the spec, 
> so it's ok but if it'd happen to have, I will NACK
>
>
Thanks for your clarification. I didn't restrict how to use the 
virtqueue in the spec.
The code is just one implementation. I'd like to have this simple driver 
been merged first.
It may have optimization in the future according to the needs.

Thanks.


>>
>> It's like you provide a water glass to user. The user can fill a full 
>> glass of water and drinks once or
>> fill half a glass of water and drink twice. It is a user behavior and 
>> should not be restricted by
>> the glass provider.
>
>
> That's my point as well, we should not describe the "once" behavior in 
> the spec.
>
>
>>
>> Thanks.
>>
>>
>>>> Thanks.
>>>>
>>>>
>>>>>
>>>>>> +
>>>>>>
>>>>>> +
>>>>>> +#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_hdr - the virtio I2C message header structure
>>>>>> + * @addr: i2c_msg addr, the slave address
>>>>>> + * @flags: i2c_msg flags
>>>>>> + * @len: i2c_msg len
>>>>>> + */
>>>>>> +struct virtio_i2c_hdr {
>>>>>> +    __le16 addr;
>>>>>> +    __le16 flags;
>>>>>> +    __le16 len;
>>>>>> +};
>>>>>
>>>>>
>>>>> I'm afraid this is not complete. E.g the status is missed.
>>>>>
>>>>> I suspect what virtio-scsi use is better. Which split the in from 
>>>>> the out instead of reusing the same buffer. And it can ease the 
>>>>> uAPI header export.
>>>>>
>>>>> Thanks
>>>>>
>>>>>
>>>>
>>>> I think following definition in uAPI for the status is enough.
>>>> There is no need to provide a "u8" status in the structure.
>>>>
>>>> /* The final status written by the device */
>>>> #define VIRTIO_I2C_MSG_OK    0
>>>> #define VIRTIO_I2C_MSG_ERR    1
>>>>
>>>> You can see an example in virtio_blk.
>>>>
>>>> In the spec:
>>>>
>>>> struct virtio_blk_req {
>>>> le32 type;
>>>> le32 reserved;
>>>> le64 sector;
>>>> u8 data[];
>>>> u8 status;
>>>> };
>>>>
>>>> In virtio_blk.h, there is only following definitions.
>>>>
>>>> #define VIRTIO_BLK_S_OK        0
>>>> #define VIRTIO_BLK_S_IOERR    1
>>>> #define VIRTIO_BLK_S_UNSUPP    2
>>>>
>>>
>>> virtio-blk is a bad example, it's just too late to fix. For any new 
>>> introduced uAPI it should be a complete one.
>>>
>>> Thanks
>>>
>> I checked a relatively new device "virtio_fs".
>> I found following definition in spec but not in uAPI also.
>>
>> struct virtio_fs_req {
>> // Device -readable part
>> struct fuse_in_header in;
>> u8 datain[];
>> // Device -writable part
>> struct fuse_out_header out;
>> u8 dataout[];
>> };
>>
>> So is this also a bad example which has not been fixed yet.
>
>
> Cc Stefan for the answer.
>
>
>> Or what's your mean about "complete" here ? Is there any definition 
>> about "complete uAPI" ?
>
>
> My understanding it should contain all the fields defined in the 
> virtio spec.
>
> Thanks
>
OK. I noticed this isn't strictly implemented in the current virtio codes.
I'm not sure if this is already a consensus. I will follow it if this is 
the opinion of the majority.

Thanks.

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

[Jason Wang]

On 2020/10/14 下午4:37, Jie Deng wrote:
>
> On 2020/10/13 16:00, Jason Wang wrote:
>>
>>>>>>> +
>>>>>>> +        virtqueue_kick(vq);
>>>>>>> +
>>>>>>> +        time_left = 
>>>>>>> wait_for_completion_timeout(&vi->completion, adap->timeout);
>>>>>>> +        if (!time_left) {
>>>>>>> +            dev_err(&adap->dev, "msg[%d]: addr=0x%x 
>>>>>>> timeout.\n", i, msgs[i].addr);
>>>>>>> +            break;
>>>>>>> +        }
>>>>>>
>>>>>>
>>>>>> You don't set error number here. Is this intended?
>>>>>>
>>>>>> And using a timeout here is not good, and if the request is 
>>>>>> finished just after the timeout, in the next xfer you may hit the 
>>>>>> following check.
>>>>>>
>>>>>> It's better to use either interrupt here.
>>>>>>
>>>>> Could you check the I2C drivers in the kernel ? The 
>>>>> "wait_for_completion_timeout" mechanism
>>>>> is commonly used by I2C bus drivers in their 
>>>>> i2c_algorithm.master_xfer.
>>>>
>>>>
>>>> There's a major difference between virtio-i2c and other drivers. In 
>>>> the case of virtio, the device could be a software device emulated 
>>>> by a remote process. This means the timeout might not be rare.
>>>>
>>>> I don't see how timeout is properly handled in this patch (e.g did 
>>>> you notice that you don't set any error when timeout? or is this 
>>>> intended?)
>>>>
>>> The backend software may operate the physical device. The timeout 
>>> depends on how the backend is designed.
>>> Here if the timeout happens, it will return the actual number of 
>>> messages successfully processed to the I2C core.
>>> Let the I2C core decides how to do next.
>>
>>
>> So let's consider the following case:
>>
>> 1) driver:virtio_i2c_add_msg(msgA)
>> 2) driver:timeout, and return to I2C core
>> 3) driver:virtio_i2c_add_msg(msgB)
>> 4) device: complete msgA
>> 5) driver: virtqueue_get_buf() returns msgA, since the token is 
>> always vi->vmsg, the driver may think msgB has been completed.
>>
>>
> If this case does happen, it is exactly a case that the condition 
> "((!vmsg) || (vmsg != &vi->vmsg))" are met.


I may miss something, but you always use vi->vmsg as token so vmsg is 
equal to &vi->vmsg here


> Currently, the timeout value is hard-coded in the driver. Generally 
> speaking, timeout rarely happens.


Well, it's better to no have such assumption consider the device could 
be a emulated one.


> It can also be designed as a device configuration if needed.


In any case, the timeout should be handled correctly regardless of its 
frequency.


>
> Thanks.
>
>>>
>>> Thanks.
>>>
>>>>
>>>>>
>>>>>>
>>>>>>> +
>>>>>>> +        vmsg = (struct virtio_i2c_msg *)virtqueue_get_buf(vq, 
>>>>>>> &len);
>>>>>>> +        /* vmsg should point to the same address with &vi->vmsg */
>>>>>>> +        if ((!vmsg) || (vmsg != &vi->vmsg)) {
>>>>>>> +            dev_err(&adap->dev, "msg[%d]: addr=0x%x virtqueue 
>>>>>>> error.\n",
>>>>>>> +                i, msgs[i].addr);
>>>>>>> +            break;
>>>>>>> +        }
>>>>>>
>>>>>>
>>>>>> So I think we can remove this check. Consider only one descriptor 
>>>>>> will be used at most, unless there's a bug in the device (and no 
>>>>>> other driver to the similar check), we should not hit this.
>>>>>>
>>>>>> Btw, as I replied in the previous version, the device should be 
>>>>>> cacpable of dealing of a batch of requests through the virtqueue, 
>>>>>> otherwise it's meaningless to use a queue here.
>>>>>>
>>>>> We should not assume there is no bug in the device. I don't think 
>>>>> we can remove this check if we want our code to be robust.
>>>>
>>>>
>>>> Can you tell when at which case you may hit !vmsg or vmsg != vi->vmsg?
>>>>
>>> Normally, it won't hit here. But the API "virtqueue_get_buf" tells me
>>> "It *may *return NULL or the "data" token handed to virtqueue_add_*()."
>>
>>
>> Note that we had the following check already in 
>> virtqueue_get_buf_ctx(), so the the virtio core had already have the 
>> ability to figure out the wrong head.
>>
>>     if (unlikely(id >= vq->packed.vring.num)) {
>>         BAD_RING(vq, "id %u out of range\n", id);
>>         return NULL;
>>     }
>>     if (unlikely(!vq->packed.desc_state[id].data)) {
>>         BAD_RING(vq, "id %u is not a head!\n", id);
>>         return NULL;
>>     }
>>
>> And when it returns a NULL, it's not necessarily an error of the 
>> device, it might just require more time to finish the processing.
>>
>
> That's why we just returned the actual number of messages successfully 
> processed in this case,
> and let the I2C core to try one more time.
>
> Actually we have no idea if this is a device error or not. Try one 
> more time can also fail if it is a backend error.
> Of course, there is another option. We can return error for timeout, 
> no matter what reason.
>
> Thanks.
>
>
>>
>>>
>>> From the perspective of a caller, I just don't care when it happens.
>>> To make the code robust, what I care about is what I should do if 
>>> this is not our case
>>> since the doc says it*may *happen.
>>>
>>> If you insist on removing this check, I will remove "vmsg != 
>>> vi->vmsg" and keep the check for !vmsg.
>>> As Dan reported in v2, we should at least check here for NULL.
>>>
>>> Thanks.
>>>
>>>>
>>>>
>>>>> As I said, currently, we are using the virtqueue to send the msg 
>>>>> one by one to the backend. The mechanism is described in the spec. 
>>>>
>>>>
>>>> Which part of the spec describes such "one by one" mechanism? If 
>>>> there is one, I'd happily give a NACK since it doesn't require a 
>>>> queue to work which is conflict with the concept of the virtqueue.
>>>>
>>>>
>>> What's the concept of the virtqueue ?  Why do you want to restrict 
>>> how users use virtqueue ?
>>
>>
>> So I think there's some misunderstanding here. The point is not to 
>> restrict how to use virtqueue.
>>
>> What I meant is:
>>
>> - we should not invent a device with a virtqueue that can only accept 
>> one buffer at a time
>> - I don't see any mechanism like "one by one" described in the spec, 
>> so it's ok but if it'd happen to have, I will NACK
>>
>>
> Thanks for your clarification. I didn't restrict how to use the 
> virtqueue in the spec.
> The code is just one implementation. I'd like to have this simple 
> driver been merged first.
> It may have optimization in the future according to the needs.
>
> Thanks.
>
>
>>>
>>> It's like you provide a water glass to user. The user can fill a 
>>> full glass of water and drinks once or
>>> fill half a glass of water and drink twice. It is a user behavior 
>>> and should not be restricted by
>>> the glass provider.
>>
>>
>> That's my point as well, we should not describe the "once" behavior 
>> in the spec.
>>
>>
>>>
>>> Thanks.
>>>
>>>
>>>>> Thanks.
>>>>>
>>>>>
>>>>>>
>>>>>>> +
>>>>>>>
>>>>>>> +
>>>>>>> +#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_hdr - the virtio I2C message header structure
>>>>>>> + * @addr: i2c_msg addr, the slave address
>>>>>>> + * @flags: i2c_msg flags
>>>>>>> + * @len: i2c_msg len
>>>>>>> + */
>>>>>>> +struct virtio_i2c_hdr {
>>>>>>> +    __le16 addr;
>>>>>>> +    __le16 flags;
>>>>>>> +    __le16 len;
>>>>>>> +};
>>>>>>
>>>>>>
>>>>>> I'm afraid this is not complete. E.g the status is missed.
>>>>>>
>>>>>> I suspect what virtio-scsi use is better. Which split the in from 
>>>>>> the out instead of reusing the same buffer. And it can ease the 
>>>>>> uAPI header export.
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>>
>>>>>
>>>>> I think following definition in uAPI for the status is enough.
>>>>> There is no need to provide a "u8" status in the structure.
>>>>>
>>>>> /* The final status written by the device */
>>>>> #define VIRTIO_I2C_MSG_OK    0
>>>>> #define VIRTIO_I2C_MSG_ERR    1
>>>>>
>>>>> You can see an example in virtio_blk.
>>>>>
>>>>> In the spec:
>>>>>
>>>>> struct virtio_blk_req {
>>>>> le32 type;
>>>>> le32 reserved;
>>>>> le64 sector;
>>>>> u8 data[];
>>>>> u8 status;
>>>>> };
>>>>>
>>>>> In virtio_blk.h, there is only following definitions.
>>>>>
>>>>> #define VIRTIO_BLK_S_OK        0
>>>>> #define VIRTIO_BLK_S_IOERR    1
>>>>> #define VIRTIO_BLK_S_UNSUPP    2
>>>>>
>>>>
>>>> virtio-blk is a bad example, it's just too late to fix. For any new 
>>>> introduced uAPI it should be a complete one.
>>>>
>>>> Thanks
>>>>
>>> I checked a relatively new device "virtio_fs".
>>> I found following definition in spec but not in uAPI also.
>>>
>>> struct virtio_fs_req {
>>> // Device -readable part
>>> struct fuse_in_header in;
>>> u8 datain[];
>>> // Device -writable part
>>> struct fuse_out_header out;
>>> u8 dataout[];
>>> };
>>>
>>> So is this also a bad example which has not been fixed yet.
>>
>>
>> Cc Stefan for the answer.
>>
>>
>>> Or what's your mean about "complete" here ? Is there any definition 
>>> about "complete uAPI" ?
>>
>>
>> My understanding it should contain all the fields defined in the 
>> virtio spec.
>>
>> Thanks
>>
> OK. I noticed this isn't strictly implemented in the current virtio 
> codes.
> I'm not sure if this is already a consensus. I will follow it if this 
> is the opinion of the majority.


Please do that, this forces us to maintain uABI compatibility which is 
what Linux try to maintain for ever.

Thanks


>
> Thanks.
>
>