* [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
@ 2021-03-23 14:19 Jie Deng
[not found] ` <20210323072704.rgoelmq62fl2wjjf@vireshk-i7>
` (6 more replies)
0 siblings, 7 replies; 39+ messages in thread
From: Jie Deng @ 2021-03-23 14:19 UTC (permalink / raw)
To: linux-i2c, virtualization, linux-kernel
Cc: Sergey.Semin, bjorn.andersson, loic.poulain, arnd, mst,
viresh.kumar, shuo.a.liu, tali.perry1, wsa, wsa+renesas,
pbonzini, jarkko.nikula, stefanha, u.kleine-koenig, kblaiech,
andriy.shevchenko, conghui.chen, rppt, yu1.wang
Add an I2C bus driver for virtio para-virtualization.
The controller can be emulated by the backend driver in
any device model software by following the virtio protocol.
The device specification can be found on
https://lists.oasis-open.org/archives/virtio-comment/202101/msg00008.html.
By following the specification, people may implement different
backend drivers to emulate different controllers according to
their needs.
Co-developed-by: Conghui Chen <conghui.chen@intel.com>
Signed-off-by: Conghui Chen <conghui.chen@intel.com>
Signed-off-by: Jie Deng <jie.deng@intel.com>
---
Changes in v10:
- Fix some typo errors.
- Refined the virtio_i2c_complete_reqs to use less code lines.
Changes in v9:
- Remove the virtio_adapter and update its members in probe.
- Refined the virtio_i2c_complete_reqs for buf free.
Changes in v8:
- Make virtio_i2c.adap a pointer.
- Mark members in virtio_i2c_req with ____cacheline_aligned.
Changes in v7:
- Remove unused headers.
- Update Makefile and Kconfig.
- Add the cleanup after completing reqs.
- Avoid memcpy for data marked with I2C_M_DMA_SAFE.
- Fix something reported by kernel test robot.
Changes in v6:
- Move struct virtio_i2c_req into the driver.
- Use only one buf in struct virtio_i2c_req.
Changes in v5:
- The first version based on the acked specification.
drivers/i2c/busses/Kconfig | 11 ++
drivers/i2c/busses/Makefile | 3 +
drivers/i2c/busses/i2c-virtio.c | 276 ++++++++++++++++++++++++++++++++++++++++
include/uapi/linux/virtio_i2c.h | 40 ++++++
include/uapi/linux/virtio_ids.h | 1 +
5 files changed, 331 insertions(+)
create mode 100644 drivers/i2c/busses/i2c-virtio.c
create mode 100644 include/uapi/linux/virtio_i2c.h
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 05ebf75..cb8d0d8 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -21,6 +21,17 @@ config I2C_ALI1535
This driver can also be built as a module. If so, the module
will be called i2c-ali1535.
+config I2C_VIRTIO
+ tristate "Virtio I2C Adapter"
+ select VIRTIO
+ help
+ If you say yes to this option, support will be included for the virtio
+ I2C adapter driver. The hardware can be emulated by any device model
+ software according to the virtio protocol.
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-virtio.
+
config I2C_ALI1563
tristate "ALI 1563"
depends on PCI
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 615f35e..efdd3f3 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -145,4 +145,7 @@ obj-$(CONFIG_I2C_XGENE_SLIMPRO) += i2c-xgene-slimpro.o
obj-$(CONFIG_SCx200_ACB) += scx200_acb.o
obj-$(CONFIG_I2C_FSI) += i2c-fsi.o
+# VIRTIO I2C host controller driver
+obj-$(CONFIG_I2C_VIRTIO) += i2c-virtio.o
+
ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
new file mode 100644
index 0000000..99a1e30
--- /dev/null
+++ b/drivers/i2c/busses/i2c-virtio.c
@@ -0,0 +1,276 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Virtio I2C Bus Driver
+ *
+ * The Virtio I2C Specification:
+ * https://raw.githubusercontent.com/oasis-tcs/virtio-spec/master/virtio-i2c.tex
+ *
+ * Copyright (c) 2021 Intel Corporation. All rights reserved.
+ */
+
+#include <linux/acpi.h>
+#include <linux/completion.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/virtio.h>
+#include <linux/virtio_ids.h>
+#include <linux/virtio_config.h>
+#include <linux/virtio_i2c.h>
+
+/**
+ * struct virtio_i2c - virtio I2C data
+ * @vdev: virtio device for this controller
+ * @completion: completion of virtio I2C message
+ * @adap: I2C adapter for this controller
+ * @lock: lock for virtqueue processing
+ * @vq: the virtio virtqueue for communication
+ */
+struct virtio_i2c {
+ struct virtio_device *vdev;
+ struct completion completion;
+ struct i2c_adapter adap;
+ struct mutex lock;
+ struct virtqueue *vq;
+};
+
+/**
+ * struct virtio_i2c_req - the virtio I2C request structure
+ * @out_hdr: the OUT header of the virtio I2C message
+ * @buf: the buffer into which data is read, or from which it's written
+ * @in_hdr: the IN header of the virtio I2C message
+ */
+struct virtio_i2c_req {
+ struct virtio_i2c_out_hdr out_hdr ____cacheline_aligned;
+ uint8_t *buf ____cacheline_aligned;
+ struct virtio_i2c_in_hdr in_hdr ____cacheline_aligned;
+};
+
+static void virtio_i2c_msg_done(struct virtqueue *vq)
+{
+ struct virtio_i2c *vi = vq->vdev->priv;
+
+ complete(&vi->completion);
+}
+
+static int virtio_i2c_send_reqs(struct virtqueue *vq,
+ struct virtio_i2c_req *reqs,
+ struct i2c_msg *msgs, int nr)
+{
+ struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr;
+ int i, outcnt, incnt, err = 0;
+
+ for (i = 0; i < nr; i++) {
+ if (!msgs[i].len)
+ break;
+
+ /*
+ * Only 7-bit mode supported for this moment. For the address format,
+ * Please check the Virtio I2C Specification.
+ */
+ reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1);
+
+ if (i != nr - 1)
+ reqs[i].out_hdr.flags = cpu_to_le32(VIRTIO_I2C_FLAGS_FAIL_NEXT);
+
+ outcnt = incnt = 0;
+ sg_init_one(&out_hdr, &reqs[i].out_hdr, sizeof(reqs[i].out_hdr));
+ sgs[outcnt++] = &out_hdr;
+
+ reqs[i].buf = i2c_get_dma_safe_msg_buf(&msgs[i], 1);
+ if (!reqs[i].buf)
+ break;
+
+ sg_init_one(&msg_buf, reqs[i].buf, msgs[i].len);
+
+ if (msgs[i].flags & I2C_M_RD)
+ sgs[outcnt + incnt++] = &msg_buf;
+ else
+ sgs[outcnt++] = &msg_buf;
+
+ sg_init_one(&in_hdr, &reqs[i].in_hdr, sizeof(reqs[i].in_hdr));
+ sgs[outcnt + incnt++] = &in_hdr;
+
+ err = virtqueue_add_sgs(vq, sgs, outcnt, incnt, &reqs[i], GFP_KERNEL);
+ if (err < 0) {
+ pr_err("failed to add msg[%d] to virtqueue.\n", i);
+ i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], false);
+ break;
+ }
+ }
+
+ return i;
+}
+
+static int virtio_i2c_complete_reqs(struct virtqueue *vq,
+ struct virtio_i2c_req *reqs,
+ struct i2c_msg *msgs, int nr,
+ bool timeout)
+{
+ struct virtio_i2c_req *req;
+ bool failed = timeout;
+ unsigned int len;
+ int i, j = 0;
+
+ for (i = 0; i < nr; i++) {
+ /* Detach the ith request from the vq */
+ req = virtqueue_get_buf(vq, &len);
+
+ /*
+ * Condition (req && req == &reqs[i]) should always meet since
+ * we have total nr requests in the vq.
+ */
+ if (!failed && (WARN_ON(!(req && req == &reqs[i])) ||
+ (req->in_hdr.status != VIRTIO_I2C_MSG_OK)))
+ failed = true;
+
+ i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], !failed);
+ if (!failed)
+ ++j;
+ }
+
+ return (timeout ? -ETIMEDOUT : j);
+}
+
+static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
+{
+ struct virtio_i2c *vi = i2c_get_adapdata(adap);
+ struct virtqueue *vq = vi->vq;
+ struct virtio_i2c_req *reqs;
+ unsigned long time_left;
+ int ret, nr;
+
+ reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL);
+ if (!reqs)
+ return -ENOMEM;
+
+ mutex_lock(&vi->lock);
+
+ ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
+ if (ret == 0)
+ goto err_unlock_free;
+
+ nr = ret;
+ reinit_completion(&vi->completion);
+ virtqueue_kick(vq);
+
+ time_left = wait_for_completion_timeout(&vi->completion, adap->timeout);
+ if (!time_left)
+ dev_err(&adap->dev, "virtio i2c backend timeout.\n");
+
+ ret = virtio_i2c_complete_reqs(vq, reqs, msgs, nr, !time_left);
+
+err_unlock_free:
+ mutex_unlock(&vi->lock);
+ kfree(reqs);
+ return ret;
+}
+
+static void virtio_i2c_del_vqs(struct virtio_device *vdev)
+{
+ vdev->config->reset(vdev);
+ vdev->config->del_vqs(vdev);
+}
+
+static int virtio_i2c_setup_vqs(struct virtio_i2c *vi)
+{
+ struct virtio_device *vdev = vi->vdev;
+
+ vi->vq = virtio_find_single_vq(vdev, virtio_i2c_msg_done, "msg");
+ return PTR_ERR_OR_ZERO(vi->vq);
+}
+
+static u32 virtio_i2c_func(struct i2c_adapter *adap)
+{
+ return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static struct i2c_algorithm virtio_algorithm = {
+ .master_xfer = virtio_i2c_xfer,
+ .functionality = virtio_i2c_func,
+};
+
+static int virtio_i2c_probe(struct virtio_device *vdev)
+{
+ struct device *pdev = vdev->dev.parent;
+ struct virtio_i2c *vi;
+ int ret;
+
+ vi = devm_kzalloc(&vdev->dev, sizeof(*vi), GFP_KERNEL);
+ if (!vi)
+ return -ENOMEM;
+
+ vdev->priv = vi;
+ vi->vdev = vdev;
+
+ mutex_init(&vi->lock);
+ init_completion(&vi->completion);
+
+ ret = virtio_i2c_setup_vqs(vi);
+ if (ret)
+ return ret;
+
+ vi->adap.owner = THIS_MODULE;
+ snprintf(vi->adap.name, sizeof(vi->adap.name), "Virtio I2C Adapter");
+ vi->adap.class = I2C_CLASS_DEPRECATED;
+ vi->adap.algo = &virtio_algorithm;
+ vi->adap.dev.parent = &vdev->dev;
+ vi->adap.timeout = HZ / 10;
+ i2c_set_adapdata(&vi->adap, vi);
+
+ /* Setup ACPI node for controlled devices which will be probed through ACPI */
+ ACPI_COMPANION_SET(&vi->adap.dev, ACPI_COMPANION(pdev));
+
+ ret = i2c_add_adapter(&vi->adap);
+ if (ret) {
+ virtio_i2c_del_vqs(vdev);
+ dev_err(&vdev->dev, "failed to add virtio-i2c adapter.\n");
+ }
+
+ return ret;
+}
+
+static void virtio_i2c_remove(struct virtio_device *vdev)
+{
+ struct virtio_i2c *vi = vdev->priv;
+
+ i2c_del_adapter(&vi->adap);
+ virtio_i2c_del_vqs(vdev);
+}
+
+static struct virtio_device_id id_table[] = {
+ { VIRTIO_ID_I2C_ADAPTER, VIRTIO_DEV_ANY_ID },
+ {}
+};
+MODULE_DEVICE_TABLE(virtio, id_table);
+
+static int __maybe_unused virtio_i2c_freeze(struct virtio_device *vdev)
+{
+ virtio_i2c_del_vqs(vdev);
+ return 0;
+}
+
+static int __maybe_unused virtio_i2c_restore(struct virtio_device *vdev)
+{
+ return virtio_i2c_setup_vqs(vdev->priv);
+}
+
+static struct virtio_driver virtio_i2c_driver = {
+ .id_table = id_table,
+ .probe = virtio_i2c_probe,
+ .remove = virtio_i2c_remove,
+ .driver = {
+ .name = "i2c_virtio",
+ },
+#ifdef CONFIG_PM_SLEEP
+ .freeze = virtio_i2c_freeze,
+ .restore = virtio_i2c_restore,
+#endif
+};
+module_virtio_driver(virtio_i2c_driver);
+
+MODULE_AUTHOR("Jie Deng <jie.deng@intel.com>");
+MODULE_AUTHOR("Conghui Chen <conghui.chen@intel.com>");
+MODULE_DESCRIPTION("Virtio i2c bus driver");
+MODULE_LICENSE("GPL");
diff --git a/include/uapi/linux/virtio_i2c.h b/include/uapi/linux/virtio_i2c.h
new file mode 100644
index 0000000..bbcfb2c
--- /dev/null
+++ b/include/uapi/linux/virtio_i2c.h
@@ -0,0 +1,40 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later WITH Linux-syscall-note */
+/*
+ * Definitions for virtio I2C Adpter
+ *
+ * Copyright (c) 2021 Intel Corporation. All rights reserved.
+ */
+
+#ifndef _UAPI_LINUX_VIRTIO_I2C_H
+#define _UAPI_LINUX_VIRTIO_I2C_H
+
+#include <linux/types.h>
+
+/* The bit 0 of the @virtio_i2c_out_hdr.@flags, used to group the requests */
+#define VIRTIO_I2C_FLAGS_FAIL_NEXT 0x00000001
+
+/**
+ * struct virtio_i2c_out_hdr - the virtio I2C message OUT header
+ * @addr: the controlled device address
+ * @padding: used to pad to full dword
+ * @flags: used for feature extensibility
+ */
+struct virtio_i2c_out_hdr {
+ __le16 addr;
+ __le16 padding;
+ __le32 flags;
+};
+
+/**
+ * struct virtio_i2c_in_hdr - the virtio I2C message IN header
+ * @status: the processing result from the backend
+ */
+struct virtio_i2c_in_hdr {
+ __u8 status;
+};
+
+/* The final status written by the device */
+#define VIRTIO_I2C_MSG_OK 0
+#define VIRTIO_I2C_MSG_ERR 1
+
+#endif /* _UAPI_LINUX_VIRTIO_I2C_H */
diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
index bc1c062..b89391d 100644
--- a/include/uapi/linux/virtio_ids.h
+++ b/include/uapi/linux/virtio_ids.h
@@ -54,5 +54,6 @@
#define VIRTIO_ID_FS 26 /* virtio filesystem */
#define VIRTIO_ID_PMEM 27 /* virtio pmem */
#define VIRTIO_ID_MAC80211_HWSIM 29 /* virtio mac80211-hwsim */
+#define VIRTIO_ID_I2C_ADAPTER 34 /* virtio i2c adapter */
#endif /* _LINUX_VIRTIO_IDS_H */
--
2.7.4
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 39+ messages in thread
[parent not found: <20210323072704.rgoelmq62fl2wjjf@vireshk-i7>]
* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
[not found] ` <20210323072704.rgoelmq62fl2wjjf@vireshk-i7>
@ 2021-03-23 8:33 ` Jie Deng
2021-03-23 9:27 ` Arnd Bergmann
0 siblings, 1 reply; 39+ messages in thread
From: Jie Deng @ 2021-03-23 8:33 UTC (permalink / raw)
To: Viresh Kumar
Cc: mst, bjorn.andersson, wsa+renesas, linux-i2c, wsa,
andriy.shevchenko, yu1.wang, u.kleine-koenig, kblaiech,
virtualization, arnd, stefanha, tali.perry1, conghui.chen,
loic.poulain, linux-kernel, Sergey.Semin, jarkko.nikula,
shuo.a.liu, pbonzini, rppt
On 2021/3/23 15:27, Viresh Kumar wrote:
> On 23-03-21, 22:19, Jie Deng wrote:
>> +static int __maybe_unused virtio_i2c_freeze(struct virtio_device *vdev)
>> +{
>> + virtio_i2c_del_vqs(vdev);
>> + return 0;
>> +}
>> +
>> +static int __maybe_unused virtio_i2c_restore(struct virtio_device *vdev)
>> +{
>> + return virtio_i2c_setup_vqs(vdev->priv);
>> +}
> Sorry for not looking at this earlier, but shouldn't we enclose the above two
> within #ifdef CONFIG_PM_SLEEP instead and drop the __maybe_unused ?
I remembered I was suggested to use "__maybe_unused" instead of "#ifdef".
You may check this https://lore.kernel.org/patchwork/patch/732981/
The reason may be something like that.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
2021-03-23 8:33 ` Jie Deng
@ 2021-03-23 9:27 ` Arnd Bergmann
2021-03-24 1:17 ` Jie Deng
[not found] ` <20210415064538.a4vf7egk6l3u6zfz@vireshk-i7>
0 siblings, 2 replies; 39+ messages in thread
From: Arnd Bergmann @ 2021-03-23 9:27 UTC (permalink / raw)
To: Jie Deng
Cc: Michael S. Tsirkin, Viresh Kumar, Bjorn Andersson, Wolfram Sang,
Linux I2C, Wolfram Sang, Andy Shevchenko, yu1.wang,
Uwe Kleine-König, kblaiech, virtualization, Stefan Hajnoczi,
Tali Perry, conghui.chen, loic.poulain,
Linux Kernel Mailing List, Sergey Semin, jarkko.nikula,
shuo.a.liu, Paolo Bonzini, Mike Rapoport
On Tue, Mar 23, 2021 at 9:33 AM Jie Deng <jie.deng@intel.com> wrote:
>
> On 2021/3/23 15:27, Viresh Kumar wrote:
>
> > On 23-03-21, 22:19, Jie Deng wrote:
> >> +static int __maybe_unused virtio_i2c_freeze(struct virtio_device *vdev)
> >> +{
> >> + virtio_i2c_del_vqs(vdev);
> >> + return 0;
> >> +}
> >> +
> >> +static int __maybe_unused virtio_i2c_restore(struct virtio_device *vdev)
> >> +{
> >> + return virtio_i2c_setup_vqs(vdev->priv);
> >> +}
> > Sorry for not looking at this earlier, but shouldn't we enclose the above two
> > within #ifdef CONFIG_PM_SLEEP instead and drop the __maybe_unused ?
>
>
> I remembered I was suggested to use "__maybe_unused" instead of "#ifdef".
>
> You may check this https://lore.kernel.org/patchwork/patch/732981/
>
> The reason may be something like that.
I usually recommend the use of __maybe_unused for the suspend/resume
callbacks for drivers that use SIMPLE_DEV_PM_OPS() or similar helpers
that hide the exact conditions under which the functions get called.
In this driver, there is an explicit #ifdef in the reference to the
functions, so
it would make sense to use the same #ifdef around the definition.
A better question to ask is whether you could use the helpers instead,
and drop the other #ifdef.
Arnd
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
2021-03-23 9:27 ` Arnd Bergmann
@ 2021-03-24 1:17 ` Jie Deng
[not found] ` <20210415064538.a4vf7egk6l3u6zfz@vireshk-i7>
1 sibling, 0 replies; 39+ messages in thread
From: Jie Deng @ 2021-03-24 1:17 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Michael S. Tsirkin, Viresh Kumar, Bjorn Andersson, Wolfram Sang,
Linux I2C, Wolfram Sang, Andy Shevchenko, yu1.wang,
Uwe Kleine-König, kblaiech, virtualization, Stefan Hajnoczi,
Tali Perry, conghui.chen, loic.poulain,
Linux Kernel Mailing List, Sergey Semin, jarkko.nikula,
shuo.a.liu, Paolo Bonzini, Mike Rapoport
On 2021/3/23 17:27, Arnd Bergmann wrote:
> On Tue, Mar 23, 2021 at 9:33 AM Jie Deng <jie.deng@intel.com> wrote:
>> On 2021/3/23 15:27, Viresh Kumar wrote:
>>
>>> On 23-03-21, 22:19, Jie Deng wrote:
>>>> +static int __maybe_unused virtio_i2c_freeze(struct virtio_device *vdev)
>>>> +{
>>>> + virtio_i2c_del_vqs(vdev);
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int __maybe_unused virtio_i2c_restore(struct virtio_device *vdev)
>>>> +{
>>>> + return virtio_i2c_setup_vqs(vdev->priv);
>>>> +}
>>> Sorry for not looking at this earlier, but shouldn't we enclose the above two
>>> within #ifdef CONFIG_PM_SLEEP instead and drop the __maybe_unused ?
>>
>> I remembered I was suggested to use "__maybe_unused" instead of "#ifdef".
>>
>> You may check this https://lore.kernel.org/patchwork/patch/732981/
>>
>> The reason may be something like that.
> I usually recommend the use of __maybe_unused for the suspend/resume
> callbacks for drivers that use SIMPLE_DEV_PM_OPS() or similar helpers
> that hide the exact conditions under which the functions get called.
>
> In this driver, there is an explicit #ifdef in the reference to the
> functions, so
> it would make sense to use the same #ifdef around the definition.
>
> A better question to ask is whether you could use the helpers instead,
> and drop the other #ifdef.
>
> Arnd
I didn't see the "struct virtio_driver" has a member "struct dev_pm_ops *pm"
It defines its own hooks (freeze and restore) though it includes "struct
device_driver"
which has a "struct dev_pm_ops *pm".
I just follow other virtio drivers to directly use the hooks defined in
"struct virtio_driver".
For this driver, Both __maybe_unused and #ifdef are OK to me.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 39+ messages in thread
[parent not found: <20210415064538.a4vf7egk6l3u6zfz@vireshk-i7>]
* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
[not found] ` <20210415064538.a4vf7egk6l3u6zfz@vireshk-i7>
@ 2021-04-15 6:56 ` Jie Deng
[not found] ` <20210415072131.GA1006@kunai>
0 siblings, 1 reply; 39+ messages in thread
From: Jie Deng @ 2021-04-15 6:56 UTC (permalink / raw)
To: Viresh Kumar, Arnd Bergmann
Cc: Sergey Semin, Bjorn Andersson, loic.poulain, Tali Perry,
yu1.wang, Michael S. Tsirkin, shuo.a.liu,
Linux Kernel Mailing List, virtualization, Wolfram Sang,
Wolfram Sang, Paolo Bonzini, jarkko.nikula, Linux I2C,
Stefan Hajnoczi, Uwe Kleine-König, kblaiech,
Andy Shevchenko, conghui.chen, Mike Rapoport
On 2021/4/15 14:45, Viresh Kumar wrote:
> On 23-03-21, 10:27, Arnd Bergmann wrote:
>> I usually recommend the use of __maybe_unused for the suspend/resume
>> callbacks for drivers that use SIMPLE_DEV_PM_OPS() or similar helpers
>> that hide the exact conditions under which the functions get called.
>>
>> In this driver, there is an explicit #ifdef in the reference to the
>> functions, so
>> it would make sense to use the same #ifdef around the definition.
> Jie,
>
> I was talking about this comment when I said I was expecting a new
> version. I think you still need to make this change.
I didn't forget this. It is a very small change. I'm not sure if the
maintainer Wolfram
has any comments so that I can address them together in one version.
Thanks.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 39+ messages in thread
[parent not found: <20210323090108.ygx76exdgzudeeqi@vireshk-i7>]
[parent not found: <20210324042046.idkctj2t7cxi53jf@vireshk-i7>]
* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
[not found] ` <20210324042046.idkctj2t7cxi53jf@vireshk-i7>
@ 2021-03-24 6:05 ` Jie Deng
[not found] ` <20210324060907.nwilmghg2xcdz7nv@vireshk-i7>
0 siblings, 1 reply; 39+ messages in thread
From: Jie Deng @ 2021-03-24 6:05 UTC (permalink / raw)
To: Viresh Kumar
Cc: mst, bjorn.andersson, wsa+renesas, linux-i2c, wsa,
andriy.shevchenko, yu1.wang, u.kleine-koenig, kblaiech,
virtualization, arnd, stefanha, tali.perry1, conghui.chen,
loic.poulain, linux-kernel, Sergey.Semin, jarkko.nikula,
shuo.a.liu, pbonzini, rppt
On 2021/3/24 12:20, Viresh Kumar wrote:
> On 23-03-21, 22:19, Jie Deng wrote:
>> +static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>> +{
>> + struct virtio_i2c *vi = i2c_get_adapdata(adap);
>> + struct virtqueue *vq = vi->vq;
>> + struct virtio_i2c_req *reqs;
>> + unsigned long time_left;
>> + int ret, nr;
>> +
>> + reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL);
>> + if (!reqs)
>> + return -ENOMEM;
>> +
>> + mutex_lock(&vi->lock);
>> +
>> + ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
>> + if (ret == 0)
>> + goto err_unlock_free;
>> +
>> + nr = ret;
>> + reinit_completion(&vi->completion);
>> + virtqueue_kick(vq);
> Coming back to this again, what is the expectation from the other side for this
> ? I mean there is no obvious relation between the *msgs* which we are going to
> transfer (from the other side's or host's point of view). When should the host
> OS call its virtqueue_kick() counterpart ?
>
> Lemme give an example for this. Lets say that we need to transfer 3 messages
> here in this routine. What we did was we prepared virtqueue for all 3 messages
> together and then called virtqueue_kick().
>
> Now if the other side (host) processes the first message and sends its reply
> (with virtqueue_kick() counterpart) before processing the other two messages,
> then it will end up calling virtio_i2c_msg_done() here. That will make us call
> virtio_i2c_complete_reqs(), while only the first messages is processed until
> now and so we will fail for the other two messages straight away.
>
> Should we send only 1 message from i2c-virtio linux driver and then wait for
> virtio_i2c_msg_done() to be called, before sending the next message to make sure
> it doesn't break ?
For simplicity, the original patch sent only 1 message to vq each time .
I changed the way to send
a batch of requests in one time in order to improve efficiency according
to Jason' suggestion.
As we discussed in the previous emails, the device can raise interrupt
when some requests are still not completed
though this is not a good operation. In this case, the remaining
requests in the vq will be ignored and
the i2c_algorithm. master_xfer will return 1 for your example. I will
clarify this in the specs.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
2021-03-23 14:19 [PATCH v10] i2c: virtio: add a virtio i2c frontend driver Jie Deng
` (2 preceding siblings ...)
[not found] ` <20210324042046.idkctj2t7cxi53jf@vireshk-i7>
@ 2021-04-14 2:07 ` Jie Deng
[not found] ` <20210414035229.7uqfdcd6dy2ryg3s@vireshk-i7>
2021-04-15 3:51 ` Jason Wang
` (2 subsequent siblings)
6 siblings, 1 reply; 39+ messages in thread
From: Jie Deng @ 2021-04-14 2:07 UTC (permalink / raw)
To: wsa, wsa, mst, linux-i2c, virtualization, linux-kernel
Cc: Sergey.Semin, bjorn.andersson, loic.poulain, yu1.wang, arnd, mst,
viresh.kumar, shuo.a.liu, tali.perry1, wsa, wsa+renesas,
jarkko.nikula, stefanha, u.kleine-koenig, kblaiech,
andriy.shevchenko, conghui.chen, rppt
Hi maintainers,
What's the status of this patch ? Is i2c/for-next the right tree to
merge it ?
Thanks,
Jie
On 2021/3/23 22:19, Jie Deng wrote:
> Add an I2C bus driver for virtio para-virtualization.
>
> The controller can be emulated by the backend driver in
> any device model software by following the virtio protocol.
>
> The device specification can be found on
> https://lists.oasis-open.org/archives/virtio-comment/202101/msg00008.html.
>
> By following the specification, people may implement different
> backend drivers to emulate different controllers according to
> their needs.
>
> Co-developed-by: Conghui Chen <conghui.chen@intel.com>
> Signed-off-by: Conghui Chen <conghui.chen@intel.com>
> Signed-off-by: Jie Deng <jie.deng@intel.com>
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
2021-03-23 14:19 [PATCH v10] i2c: virtio: add a virtio i2c frontend driver Jie Deng
` (3 preceding siblings ...)
2021-04-14 2:07 ` Jie Deng
@ 2021-04-15 3:51 ` Jason Wang
2021-04-15 6:17 ` Jie Deng
[not found] ` <YNmK0MP5ffQpiipt@ninjato>
[not found] ` <YNrw4rxihFLuqLtY@ninjato>
6 siblings, 1 reply; 39+ messages in thread
From: Jason Wang @ 2021-04-15 3:51 UTC (permalink / raw)
To: Jie Deng, linux-i2c, virtualization, linux-kernel
Cc: Sergey.Semin, bjorn.andersson, loic.poulain, yu1.wang, arnd, mst,
viresh.kumar, shuo.a.liu, tali.perry1, wsa, wsa+renesas,
pbonzini, jarkko.nikula, stefanha, u.kleine-koenig, kblaiech,
andriy.shevchenko, conghui.chen, rppt
在 2021/3/23 下午10:19, Jie Deng 写道:
> Add an I2C bus driver for virtio para-virtualization.
>
> The controller can be emulated by the backend driver in
> any device model software by following the virtio protocol.
>
> The device specification can be found on
> https://lists.oasis-open.org/archives/virtio-comment/202101/msg00008.html.
>
> By following the specification, people may implement different
> backend drivers to emulate different controllers according to
> their needs.
>
> Co-developed-by: Conghui Chen <conghui.chen@intel.com>
> Signed-off-by: Conghui Chen <conghui.chen@intel.com>
> Signed-off-by: Jie Deng <jie.deng@intel.com>
> ---
> Changes in v10:
> - Fix some typo errors.
> - Refined the virtio_i2c_complete_reqs to use less code lines.
>
> Changes in v9:
> - Remove the virtio_adapter and update its members in probe.
> - Refined the virtio_i2c_complete_reqs for buf free.
>
> Changes in v8:
> - Make virtio_i2c.adap a pointer.
> - Mark members in virtio_i2c_req with ____cacheline_aligned.
>
> Changes in v7:
> - Remove unused headers.
> - Update Makefile and Kconfig.
> - Add the cleanup after completing reqs.
> - Avoid memcpy for data marked with I2C_M_DMA_SAFE.
> - Fix something reported by kernel test robot.
>
> Changes in v6:
> - Move struct virtio_i2c_req into the driver.
> - Use only one buf in struct virtio_i2c_req.
>
> Changes in v5:
> - The first version based on the acked specification.
>
> drivers/i2c/busses/Kconfig | 11 ++
> drivers/i2c/busses/Makefile | 3 +
> drivers/i2c/busses/i2c-virtio.c | 276 ++++++++++++++++++++++++++++++++++++++++
> include/uapi/linux/virtio_i2c.h | 40 ++++++
> include/uapi/linux/virtio_ids.h | 1 +
> 5 files changed, 331 insertions(+)
> create mode 100644 drivers/i2c/busses/i2c-virtio.c
> create mode 100644 include/uapi/linux/virtio_i2c.h
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 05ebf75..cb8d0d8 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -21,6 +21,17 @@ config I2C_ALI1535
> This driver can also be built as a module. If so, the module
> will be called i2c-ali1535.
>
> +config I2C_VIRTIO
> + tristate "Virtio I2C Adapter"
> + select VIRTIO
> + help
> + If you say yes to this option, support will be included for the virtio
> + I2C adapter driver. The hardware can be emulated by any device model
> + software according to the virtio protocol.
> +
> + This driver can also be built as a module. If so, the module
> + will be called i2c-virtio.
> +
> config I2C_ALI1563
> tristate "ALI 1563"
> depends on PCI
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 615f35e..efdd3f3 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -145,4 +145,7 @@ obj-$(CONFIG_I2C_XGENE_SLIMPRO) += i2c-xgene-slimpro.o
> obj-$(CONFIG_SCx200_ACB) += scx200_acb.o
> obj-$(CONFIG_I2C_FSI) += i2c-fsi.o
>
> +# VIRTIO I2C host controller driver
> +obj-$(CONFIG_I2C_VIRTIO) += i2c-virtio.o
> +
> ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
> diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
> new file mode 100644
> index 0000000..99a1e30
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-virtio.c
> @@ -0,0 +1,276 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Virtio I2C Bus Driver
> + *
> + * The Virtio I2C Specification:
> + * https://raw.githubusercontent.com/oasis-tcs/virtio-spec/master/virtio-i2c.tex
> + *
> + * Copyright (c) 2021 Intel Corporation. All rights reserved.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/completion.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/virtio.h>
> +#include <linux/virtio_ids.h>
> +#include <linux/virtio_config.h>
> +#include <linux/virtio_i2c.h>
> +
> +/**
> + * struct virtio_i2c - virtio I2C data
> + * @vdev: virtio device for this controller
> + * @completion: completion of virtio I2C message
> + * @adap: I2C adapter for this controller
> + * @lock: lock for virtqueue processing
> + * @vq: the virtio virtqueue for communication
> + */
> +struct virtio_i2c {
> + struct virtio_device *vdev;
> + struct completion completion;
> + struct i2c_adapter adap;
> + struct mutex lock;
> + struct virtqueue *vq;
> +};
> +
> +/**
> + * struct virtio_i2c_req - the virtio I2C request structure
> + * @out_hdr: the OUT header of the virtio I2C message
> + * @buf: the buffer into which data is read, or from which it's written
> + * @in_hdr: the IN header of the virtio I2C message
> + */
> +struct virtio_i2c_req {
> + struct virtio_i2c_out_hdr out_hdr ____cacheline_aligned;
> + uint8_t *buf ____cacheline_aligned;
> + struct virtio_i2c_in_hdr in_hdr ____cacheline_aligned;
> +};
> +
> +static void virtio_i2c_msg_done(struct virtqueue *vq)
> +{
> + struct virtio_i2c *vi = vq->vdev->priv;
> +
> + complete(&vi->completion);
> +}
> +
> +static int virtio_i2c_send_reqs(struct virtqueue *vq,
> + struct virtio_i2c_req *reqs,
> + struct i2c_msg *msgs, int nr)
> +{
> + struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr;
> + int i, outcnt, incnt, err = 0;
> +
> + for (i = 0; i < nr; i++) {
> + if (!msgs[i].len)
> + break;
> +
> + /*
> + * Only 7-bit mode supported for this moment. For the address format,
> + * Please check the Virtio I2C Specification.
> + */
> + reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1);
> +
> + if (i != nr - 1)
> + reqs[i].out_hdr.flags = cpu_to_le32(VIRTIO_I2C_FLAGS_FAIL_NEXT);
> +
> + outcnt = incnt = 0;
> + sg_init_one(&out_hdr, &reqs[i].out_hdr, sizeof(reqs[i].out_hdr));
> + sgs[outcnt++] = &out_hdr;
> +
> + reqs[i].buf = i2c_get_dma_safe_msg_buf(&msgs[i], 1);
> + if (!reqs[i].buf)
> + break;
> +
> + sg_init_one(&msg_buf, reqs[i].buf, msgs[i].len);
> +
> + if (msgs[i].flags & I2C_M_RD)
> + sgs[outcnt + incnt++] = &msg_buf;
> + else
> + sgs[outcnt++] = &msg_buf;
> +
> + sg_init_one(&in_hdr, &reqs[i].in_hdr, sizeof(reqs[i].in_hdr));
> + sgs[outcnt + incnt++] = &in_hdr;
> +
> + err = virtqueue_add_sgs(vq, sgs, outcnt, incnt, &reqs[i], GFP_KERNEL);
> + if (err < 0) {
> + pr_err("failed to add msg[%d] to virtqueue.\n", i);
> + i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], false);
> + break;
> + }
> + }
> +
> + return i;
> +}
> +
> +static int virtio_i2c_complete_reqs(struct virtqueue *vq,
> + struct virtio_i2c_req *reqs,
> + struct i2c_msg *msgs, int nr,
> + bool timeout)
> +{
> + struct virtio_i2c_req *req;
> + bool failed = timeout;
> + unsigned int len;
> + int i, j = 0;
> +
> + for (i = 0; i < nr; i++) {
> + /* Detach the ith request from the vq */
> + req = virtqueue_get_buf(vq, &len);
> +
> + /*
> + * Condition (req && req == &reqs[i]) should always meet since
> + * we have total nr requests in the vq.
So this assumes the requests are completed in order. Is this mandated in
the spec?
> + */
> + if (!failed && (WARN_ON(!(req && req == &reqs[i])) ||
> + (req->in_hdr.status != VIRTIO_I2C_MSG_OK)))
> + failed = true;
> +
> + i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], !failed);
> + if (!failed)
> + ++j;
> + }
> +
> + return (timeout ? -ETIMEDOUT : j);
Checking timeout is fragile, what happens if the request are completed
after wait_for_completion() but before virtio_i2c_complete_reqs()?
> +}
> +
> +static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> +{
> + struct virtio_i2c *vi = i2c_get_adapdata(adap);
> + struct virtqueue *vq = vi->vq;
> + struct virtio_i2c_req *reqs;
> + unsigned long time_left;
> + int ret, nr;
> +
> + reqs = kcalloc(num, sizeof(*reqs), GFP_KERNEL);
> + if (!reqs)
> + return -ENOMEM;
> +
> + mutex_lock(&vi->lock);
> +
> + ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
> + if (ret == 0)
> + goto err_unlock_free;
> +
> + nr = ret;
> + reinit_completion(&vi->completion);
> + virtqueue_kick(vq);
> +
> + time_left = wait_for_completion_timeout(&vi->completion, adap->timeout);
> + if (!time_left)
> + dev_err(&adap->dev, "virtio i2c backend timeout.\n");
> +
> + ret = virtio_i2c_complete_reqs(vq, reqs, msgs, nr, !time_left);
> +
> +err_unlock_free:
> + mutex_unlock(&vi->lock);
> + kfree(reqs);
> + return ret;
> +}
> +
> +static void virtio_i2c_del_vqs(struct virtio_device *vdev)
> +{
> + vdev->config->reset(vdev);
> + vdev->config->del_vqs(vdev);
> +}
> +
> +static int virtio_i2c_setup_vqs(struct virtio_i2c *vi)
> +{
> + struct virtio_device *vdev = vi->vdev;
> +
> + vi->vq = virtio_find_single_vq(vdev, virtio_i2c_msg_done, "msg");
> + return PTR_ERR_OR_ZERO(vi->vq);
> +}
> +
> +static u32 virtio_i2c_func(struct i2c_adapter *adap)
> +{
> + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}
> +
> +static struct i2c_algorithm virtio_algorithm = {
> + .master_xfer = virtio_i2c_xfer,
> + .functionality = virtio_i2c_func,
> +};
> +
> +static int virtio_i2c_probe(struct virtio_device *vdev)
> +{
> + struct device *pdev = vdev->dev.parent;
> + struct virtio_i2c *vi;
> + int ret;
> +
> + vi = devm_kzalloc(&vdev->dev, sizeof(*vi), GFP_KERNEL);
> + if (!vi)
> + return -ENOMEM;
> +
> + vdev->priv = vi;
> + vi->vdev = vdev;
> +
> + mutex_init(&vi->lock);
> + init_completion(&vi->completion);
> +
> + ret = virtio_i2c_setup_vqs(vi);
> + if (ret)
> + return ret;
> +
> + vi->adap.owner = THIS_MODULE;
> + snprintf(vi->adap.name, sizeof(vi->adap.name), "Virtio I2C Adapter");
> + vi->adap.class = I2C_CLASS_DEPRECATED;
> + vi->adap.algo = &virtio_algorithm;
> + vi->adap.dev.parent = &vdev->dev;
> + vi->adap.timeout = HZ / 10;
> + i2c_set_adapdata(&vi->adap, vi);
> +
> + /* Setup ACPI node for controlled devices which will be probed through ACPI */
> + ACPI_COMPANION_SET(&vi->adap.dev, ACPI_COMPANION(pdev));
> +
> + ret = i2c_add_adapter(&vi->adap);
> + if (ret) {
> + virtio_i2c_del_vqs(vdev);
> + dev_err(&vdev->dev, "failed to add virtio-i2c adapter.\n");
> + }
> +
> + return ret;
> +}
> +
> +static void virtio_i2c_remove(struct virtio_device *vdev)
> +{
> + struct virtio_i2c *vi = vdev->priv;
> +
> + i2c_del_adapter(&vi->adap);
> + virtio_i2c_del_vqs(vdev);
> +}
> +
> +static struct virtio_device_id id_table[] = {
> + { VIRTIO_ID_I2C_ADAPTER, VIRTIO_DEV_ANY_ID },
> + {}
> +};
> +MODULE_DEVICE_TABLE(virtio, id_table);
> +
> +static int __maybe_unused virtio_i2c_freeze(struct virtio_device *vdev)
> +{
> + virtio_i2c_del_vqs(vdev);
> + return 0;
> +}
> +
> +static int __maybe_unused virtio_i2c_restore(struct virtio_device *vdev)
> +{
> + return virtio_i2c_setup_vqs(vdev->priv);
> +}
> +
> +static struct virtio_driver virtio_i2c_driver = {
> + .id_table = id_table,
> + .probe = virtio_i2c_probe,
> + .remove = virtio_i2c_remove,
> + .driver = {
> + .name = "i2c_virtio",
> + },
> +#ifdef CONFIG_PM_SLEEP
> + .freeze = virtio_i2c_freeze,
> + .restore = virtio_i2c_restore,
> +#endif
> +};
> +module_virtio_driver(virtio_i2c_driver);
> +
> +MODULE_AUTHOR("Jie Deng <jie.deng@intel.com>");
> +MODULE_AUTHOR("Conghui Chen <conghui.chen@intel.com>");
> +MODULE_DESCRIPTION("Virtio i2c bus driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/uapi/linux/virtio_i2c.h b/include/uapi/linux/virtio_i2c.h
> new file mode 100644
> index 0000000..bbcfb2c
> --- /dev/null
> +++ b/include/uapi/linux/virtio_i2c.h
> @@ -0,0 +1,40 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later WITH Linux-syscall-note */
> +/*
> + * Definitions for virtio I2C Adpter
> + *
> + * Copyright (c) 2021 Intel Corporation. All rights reserved.
> + */
> +
> +#ifndef _UAPI_LINUX_VIRTIO_I2C_H
> +#define _UAPI_LINUX_VIRTIO_I2C_H
> +
> +#include <linux/types.h>
> +
> +/* The bit 0 of the @virtio_i2c_out_hdr.@flags, used to group the requests */
> +#define VIRTIO_I2C_FLAGS_FAIL_NEXT 0x00000001
> +
> +/**
> + * struct virtio_i2c_out_hdr - the virtio I2C message OUT header
> + * @addr: the controlled device address
> + * @padding: used to pad to full dword
> + * @flags: used for feature extensibility
> + */
> +struct virtio_i2c_out_hdr {
> + __le16 addr;
> + __le16 padding;
> + __le32 flags;
> +};
> +
> +/**
> + * struct virtio_i2c_in_hdr - the virtio I2C message IN header
> + * @status: the processing result from the backend
> + */
> +struct virtio_i2c_in_hdr {
> + __u8 status;
> +};
> +
> +/* The final status written by the device */
> +#define VIRTIO_I2C_MSG_OK 0
> +#define VIRTIO_I2C_MSG_ERR 1
> +
> +#endif /* _UAPI_LINUX_VIRTIO_I2C_H */
> diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
> index bc1c062..b89391d 100644
> --- a/include/uapi/linux/virtio_ids.h
> +++ b/include/uapi/linux/virtio_ids.h
> @@ -54,5 +54,6 @@
> #define VIRTIO_ID_FS 26 /* virtio filesystem */
> #define VIRTIO_ID_PMEM 27 /* virtio pmem */
> #define VIRTIO_ID_MAC80211_HWSIM 29 /* virtio mac80211-hwsim */
> +#define VIRTIO_ID_I2C_ADAPTER 34 /* virtio i2c adapter */
>
> #endif /* _LINUX_VIRTIO_IDS_H */
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
2021-04-15 3:51 ` Jason Wang
@ 2021-04-15 6:17 ` Jie Deng
0 siblings, 0 replies; 39+ messages in thread
From: Jie Deng @ 2021-04-15 6:17 UTC (permalink / raw)
To: Jason Wang, linux-i2c, virtualization, linux-kernel
Cc: Sergey.Semin, bjorn.andersson, loic.poulain, yu1.wang, arnd, mst,
viresh.kumar, shuo.a.liu, tali.perry1, wsa, wsa+renesas,
pbonzini, jarkko.nikula, stefanha, u.kleine-koenig, kblaiech,
andriy.shevchenko, conghui.chen, rppt
On 2021/4/15 11:51, Jason Wang wrote:
>
>> + for (i = 0; i < nr; i++) {
>> + /* Detach the ith request from the vq */
>> + req = virtqueue_get_buf(vq, &len);
>> +
>> + /*
>> + * Condition (req && req == &reqs[i]) should always meet since
>> + * we have total nr requests in the vq.
>
>
> So this assumes the requests are completed in order. Is this mandated
> in the spec?
>
>
This is a mandatory device requirements in spec.
>> + */
>> + if (!failed && (WARN_ON(!(req && req == &reqs[i])) ||
>> + (req->in_hdr.status != VIRTIO_I2C_MSG_OK)))
>> + failed = true;
>> +
>> + i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], !failed);
>> + if (!failed)
>> + ++j;
>> + }
>> +
>> + return (timeout ? -ETIMEDOUT : j);
>
>
> Checking timeout is fragile, what happens if the request are completed
> after wait_for_completion() but before virtio_i2c_complete_reqs()?
>
We have discussed this before in v6.
https://lists.linuxfoundation.org/pipermail/virtualization/2021-March/053093.html.
If timeout happens, we don't need to care about the requests whether
really completed by "HW" or not.
Just return error and let the i2c core to decide whether to resend. And
currently, the timeout is statically configured in driver.
We may extend a device timeout value configuration in spec for driver to
use if there is actual need in the future.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 39+ messages in thread
[parent not found: <YNmK0MP5ffQpiipt@ninjato>]
* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
[not found] ` <YNmK0MP5ffQpiipt@ninjato>
@ 2021-06-28 9:01 ` Arnd Bergmann
[not found] ` <YNmVg3ZhshshlbSx@ninjato>
2021-06-29 3:03 ` Jie Deng
0 siblings, 2 replies; 39+ messages in thread
From: Arnd Bergmann @ 2021-06-28 9:01 UTC (permalink / raw)
To: Wolfram Sang, Jie Deng, Linux I2C, virtualization,
Linux Kernel Mailing List, Michael S. Tsirkin, Jason Wang,
Andy Shevchenko, conghui.chen, Arnd Bergmann, kblaiech,
jarkko.nikula, Sergey Semin, Mike Rapoport, loic.poulain,
Tali Perry, Uwe Kleine-König, Bjorn Andersson, yu1.wang,
shuo.a.liu, Viresh Kumar, Stefan Hajnoczi, Paolo Bonzini
On Mon, Jun 28, 2021 at 10:39 AM Wolfram Sang <wsa@kernel.org> wrote:
>
> sorry for the long delay. I am not familiar with VFIO, so I had to dive
> into the topic a little first. I am still not seeing through it
> completely, so I have very high-level questions first.
You probably know this already, but just in case for clarification
these are two different things:
VFIO: kernel feature to make raw (usually PCI) devices available
to user space drivers and virtual machines from a kernel
running on bare metal.
virtio: transport protocol for implementing arbitrary paravirtualized
drivers in (usually) a virtual machine guest without giving the
guest access to hardware registers.
Both can be used for letting a KVM guest talk to the outside world,
but usually you have one or the other, not both.
> > The device specification can be found on
> > https://lists.oasis-open.org/archives/virtio-comment/202101/msg00008.html.
>
> I think we need to start here:
>
> ===
>
> If ``length of \field{read_buf}''=0 and ``length of \field{write_buf}''>0,
> the request is called write request.
>
> If ``length of \field{read_buf}''>0 and ``length of \field{write_buf}''=0,
> the request is called read request.
>
> If ``length of \field{read_buf}''>0 and ``length of \field{write_buf}''>0,
> the request is called write-read request. It means an I2C write segment followed
> by a read segment. Usually, the write segment provides the number of an I2C
> controlled device register to be read.
>
> ===
>
> I2C transactions can have an arbitrary number of messages which can
> arbitrarily be read or write. As I understand the above, only one read,
> write or read-write transaction is supported. If that is the case, it
> would be not very much I2C but more SMBus. If my assumptions are true,
> we first need to decide if you want to go the I2C way or SMBus subset.
This has come up in previous reviews already. I think it comes down
to the requirement that the virtio i2c protocol should allow passthrough
access to any client devices connected to a physical i2c bus on the host,
and this should ideally be independent of whether the host driver
exposes I2C_RDWR or I2C_SMBUS ioctl interface, or both.
This can be done either by having both interface types in the transport,
or picking one of the two, and translating to the host interface type
in software.
As far as I understand me (please clarify), implementing only the smbus
subset would mean that we cannot communicate with all client devices,
while implementing both would add more complexity than the lower-level
protocol.
> ===
>
> The case when ``length of \field{write_buf}''=0, and at the same time,
> ``length of \field{read_buf}''=0 doesn't make any sense.
>
> ===
>
> Oh, it does. That's a legal transfer, both in SMBus and I2C. It is used
> to e.g. discover devices. I think it should be supported, even though
> not all bus master drivers on the host can support it. Is it possible?
>
> Also, as I read it, a whole bus is para-virtualized to the guest, or?
> Wouldn't it be better to allow just specific devices on a bus? Again, I
> am kinda new to this, so I may have overlooked things.
Do you mean just allowing a single device per bus (as opposed to
having multiple devices as on a real bus), or just allowing
a particular set of client devices that can be identified using
virtio specific configuration (as opposed to relying on device
tree or similar for probing). Both of these are valid questions that
have been discussed before, but that could be revisited.
Arnd
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 39+ messages in thread
[parent not found: <YNmVg3ZhshshlbSx@ninjato>]
* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
[not found] ` <YNmVg3ZhshshlbSx@ninjato>
@ 2021-06-28 9:51 ` Arnd Bergmann
[not found] ` <YNmg2IEpUlArZXPK@ninjato>
0 siblings, 1 reply; 39+ messages in thread
From: Arnd Bergmann @ 2021-06-28 9:51 UTC (permalink / raw)
To: Wolfram Sang, Arnd Bergmann, Jie Deng, Linux I2C, virtualization,
Linux Kernel Mailing List, Michael S. Tsirkin, Jason Wang,
Andy Shevchenko, conghui.chen, kblaiech, jarkko.nikula,
Sergey Semin, Mike Rapoport, loic.poulain, Tali Perry,
Uwe Kleine-König, Bjorn Andersson, yu1.wang, shuo.a.liu,
Viresh Kumar, Stefan Hajnoczi, Paolo Bonzini
On Mon, Jun 28, 2021 at 11:25 AM Wolfram Sang <wsa@kernel.org> wrote:
> > As far as I understand me (please clarify), implementing only the smbus
> > subset would mean that we cannot communicate with all client devices,
> > while implementing both would add more complexity than the lower-level
> > protocol.
>
> Correct. You need to pick I2C if you want to support all devices. SMBus
> will give you only a lot of devices.
Ok, that's what I thought. There is one corner case that I've struggled
with though: Suppose the host has an SMBus-only driver, and the
proposed driver exposes this as an I2C device to the guest, which
makes it available to guest user space (or a guest kernel driver)
using the emulated smbus command set. Is it always possible for
the host user space to turn the I2C transaction back into the
expected SMBus transaction on the host?
> > > Also, as I read it, a whole bus is para-virtualized to the guest, or?
> > > Wouldn't it be better to allow just specific devices on a bus? Again, I
> > > am kinda new to this, so I may have overlooked things.
> >
> > Do you mean just allowing a single device per bus (as opposed to
> > having multiple devices as on a real bus), or just allowing
> > a particular set of client devices that can be identified using
> > virtio specific configuration (as opposed to relying on device
> > tree or similar for probing). Both of these are valid questions that
> > have been discussed before, but that could be revisited.
>
> I am just thinking that a physical bus on the host may have devices that
> should be shared with the guest while other devices on the same bus
> should not be shared (PMIC!). I'd think this is a minimal requirement
> for security. I also wonder if it is possible to have a paravirtualized
> bus in the guest which has multiple devices from the host sitting on
> different physical busses there. But that may be the cherry on the top.
This is certainly possible, but is independent of the implementation of
the guest driver. It's up to the host to provision the devices that
are actually passed down to the guest, and this could in theory
be any combination of emulated devices with devices connected to
any of the host's physical buses. The host may also decide to remap
the addresses of the devices during passthrough.
Arnd
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
2021-06-28 9:01 ` Arnd Bergmann
[not found] ` <YNmVg3ZhshshlbSx@ninjato>
@ 2021-06-29 3:03 ` Jie Deng
1 sibling, 0 replies; 39+ messages in thread
From: Jie Deng @ 2021-06-29 3:03 UTC (permalink / raw)
To: Arnd Bergmann, Wolfram Sang, Linux I2C, virtualization,
Linux Kernel Mailing List, Michael S. Tsirkin, Jason Wang,
Andy Shevchenko, conghui.chen, kblaiech, jarkko.nikula,
Sergey Semin, Mike Rapoport, loic.poulain, Tali Perry,
Uwe Kleine-König, Bjorn Andersson, yu1.wang, shuo.a.liu,
Viresh Kumar, Stefan Hajnoczi, Paolo Bonzini
On 2021/6/28 17:01, Arnd Bergmann wrote:
> On Mon, Jun 28, 2021 at 10:39 AM Wolfram Sang <wsa@kernel.org> wrote:
>> sorry for the long delay. I am not familiar with VFIO, so I had to dive
>> into the topic a little first. I am still not seeing through it
>> completely, so I have very high-level questions first.
> You probably know this already, but just in case for clarification
> these are two different things:
>
> VFIO: kernel feature to make raw (usually PCI) devices available
> to user space drivers and virtual machines from a kernel
> running on bare metal.
>
> virtio: transport protocol for implementing arbitrary paravirtualized
> drivers in (usually) a virtual machine guest without giving the
> guest access to hardware registers.
>
Thanks Arnd for clarification.
Let me add some more:
The native model is as follows: a specific native I2C driver operates a
specific hardware.
A specific native I2C driver <--> A specific hardware
The virtio paravirtualized model is something like:
virtio-i2c <--> virtio I2C interfaces <--> virtio-backend <--> Real hardware
virtio-i2c: is this driver, the frontend driver.
virtio I2C interfaces: which are described in the specification.
https://raw.githubusercontent.com/oasis-tcs/virtio-spec/master/virtio-i2c.tex.
I had tried to mirror Linux I2C interfaces (like "i2c_msg") into
virtio I2C interface directly. But
when I was doing upstream for this specification, I understood the
virtio TC had the design philosophy
"VIRTIO devices are not specific to Linux so the specs design
should avoid the limitations of the
current Linux driver behavior." So we redefined a minimum virtio
I2C interfaces to make a working POC.
and we may extend it in the future according to the need.
virtio-backend: the backend driver communicate with virtio-i2c by
following virtio I2C interfaces specs.
The are already two backend drivers developed by Viresh, one in
QEMU, another in rust-vmm.
1. vhost-user:
https://lore.kernel.org/qemu-devel/cover.1617278395.git.viresh.kumar@linaro.org/t/#m3b5044bad9769b170f505e63bd081eb27cef8db2
2. rust-vmm I2C backend:
https://github.com/rust-vmm/vhost-device/pull/1
Regards,
Jie
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 39+ messages in thread
[parent not found: <YNrw4rxihFLuqLtY@ninjato>]
* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
[not found] ` <YNrw4rxihFLuqLtY@ninjato>
@ 2021-06-29 10:16 ` Viresh Kumar
[not found] ` <YNr0uDx1fv+Gjd7m@ninjato>
2021-06-30 6:45 ` Jie Deng
1 sibling, 1 reply; 39+ messages in thread
From: Viresh Kumar @ 2021-06-29 10:16 UTC (permalink / raw)
To: Wolfram Sang, Jie Deng, linux-i2c, virtualization, linux-kernel,
mst, jasowang, andriy.shevchenko, conghui.chen, arnd, kblaiech,
jarkko.nikula, Sergey.Semin, rppt, loic.poulain, tali.perry1,
u.kleine-koenig, bjorn.andersson, yu1.wang, shuo.a.liu, stefanha,
pbonzini
On 29-06-21, 12:07, Wolfram Sang wrote:
> > +static u32 virtio_i2c_func(struct i2c_adapter *adap)
> > +{
> > + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
>
> You are not emulating I2C_FUNC_SMBUS_QUICK, so you need to mask it out.
What is it that we need to have to emulate it ? I did use it in my
qemu and rust backends, not sure if this was ever sent by device I
used for testing SMBUS though.
--
viresh
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
[not found] ` <YNrw4rxihFLuqLtY@ninjato>
2021-06-29 10:16 ` Viresh Kumar
@ 2021-06-30 6:45 ` Jie Deng
[not found] ` <YNwd/t3DMKSOrTAT@ninjato>
1 sibling, 1 reply; 39+ messages in thread
From: Jie Deng @ 2021-06-30 6:45 UTC (permalink / raw)
To: Wolfram Sang, linux-i2c, virtualization, linux-kernel, mst,
jasowang, andriy.shevchenko, conghui.chen, arnd, kblaiech,
jarkko.nikula, Sergey.Semin, rppt, loic.poulain, tali.perry1,
u.kleine-koenig, bjorn.andersson, yu1.wang, shuo.a.liu,
viresh.kumar, stefanha, pbonzini
On 2021/6/29 18:07, Wolfram Sang wrote:
> Hi,
>
> so some minor comments left:
>
>> + if (!msgs[i].len)
>> + break;
> I hope this can extended in the future to allow zero-length messages. If
> this is impossible we need to set an adapter quirk instead.
Yes, we can support it by removing this check and call it zero-length
request.
It don't think it will break anything.
>
>> + err = virtqueue_add_sgs(vq, sgs, outcnt, incnt, &reqs[i], GFP_KERNEL);
>> + if (err < 0) {
>> + pr_err("failed to add msg[%d] to virtqueue.\n", i);
> Is it really helpful for the user to know that msg5 failed? We don't
> even say which transfer.
OK. I will remove this print.
>> +static u32 virtio_i2c_func(struct i2c_adapter *adap)
>> +{
>> + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> You are not emulating I2C_FUNC_SMBUS_QUICK, so you need to mask it out.
I will remove the check of zero-length message.
>
>> + snprintf(vi->adap.name, sizeof(vi->adap.name), "Virtio I2C Adapter");
> Is there something to add so you can distinguish multiple instances?
> Most people want that.
I find the I2C core will set a device name "i2c-%d" for this purpose, right?
I think this name can be used to distinguish the adapter types while
"i2c-%d" can be used to
distinguish instances. Does it make sense ?
>> + vi->adap.class = I2C_CLASS_DEPRECATED;
>> + vi->adap.algo = &virtio_algorithm;
>> + vi->adap.dev.parent = &vdev->dev;
>> + vi->adap.timeout = HZ / 10;
> Why so short? HZ is the kinda default value.
Ah... I didn't know the I2C core had already set a default value.
I will remove this line to use the default one.
>
>> + i2c_set_adapdata(&vi->adap, vi);
>> +
>> + /* Setup ACPI node for controlled devices which will be probed through ACPI */
>> + ACPI_COMPANION_SET(&vi->adap.dev, ACPI_COMPANION(pdev));
>> +
>> + ret = i2c_add_adapter(&vi->adap);
>> + if (ret) {
>> + virtio_i2c_del_vqs(vdev);
>> + dev_err(&vdev->dev, "failed to add virtio-i2c adapter.\n");
> Won't the driver core print that for us?
Yes. It seems unnecessary. Will remove it.
>
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +/* The bit 0 of the @virtio_i2c_out_hdr.@flags, used to group the requests */
>> +#define VIRTIO_I2C_FLAGS_FAIL_NEXT 0x00000001
> BIT(0)?
That's better. Thank you.
>
> Happy hacking,
>
> Wolfram
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 39+ messages in thread
end of thread, other threads:[~2021-07-23 5:28 UTC | newest]
Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-23 14:19 [PATCH v10] i2c: virtio: add a virtio i2c frontend driver Jie Deng
[not found] ` <20210323072704.rgoelmq62fl2wjjf@vireshk-i7>
2021-03-23 8:33 ` Jie Deng
2021-03-23 9:27 ` Arnd Bergmann
2021-03-24 1:17 ` Jie Deng
[not found] ` <20210415064538.a4vf7egk6l3u6zfz@vireshk-i7>
2021-04-15 6:56 ` Jie Deng
[not found] ` <20210415072131.GA1006@kunai>
[not found] ` <20210415072431.apntpcwrk5hp6zg4@vireshk-i7>
[not found] ` <20210415072823.GB1006@kunai>
2021-04-15 8:15 ` Jie Deng
[not found] ` <20210415081828.GD1006@kunai>
2021-04-15 8:20 ` Jie Deng
2021-05-27 6:49 ` Jie Deng
2021-05-12 1:37 ` Jie Deng
[not found] ` <20210323090108.ygx76exdgzudeeqi@vireshk-i7>
[not found] ` <20210323093839.n7cq7f5poebqdwit@vireshk-i7>
2021-03-24 0:53 ` Jie Deng
[not found] ` <20210324035225.skkllxexjl65gs6x@vireshk-i7>
2021-03-24 4:00 ` Jie Deng
[not found] ` <20210324042046.idkctj2t7cxi53jf@vireshk-i7>
2021-03-24 6:05 ` Jie Deng
[not found] ` <20210324060907.nwilmghg2xcdz7nv@vireshk-i7>
2021-03-24 6:41 ` Jie Deng
2021-04-14 2:07 ` Jie Deng
[not found] ` <20210414035229.7uqfdcd6dy2ryg3s@vireshk-i7>
2021-04-15 6:25 ` Jie Deng
2021-04-15 3:51 ` Jason Wang
2021-04-15 6:17 ` Jie Deng
[not found] ` <YNmK0MP5ffQpiipt@ninjato>
2021-06-28 9:01 ` Arnd Bergmann
[not found] ` <YNmVg3ZhshshlbSx@ninjato>
2021-06-28 9:51 ` Arnd Bergmann
[not found] ` <YNmg2IEpUlArZXPK@ninjato>
2021-06-28 11:50 ` Arnd Bergmann
[not found] ` <YNnjh3xxyaZZSo9N@ninjato>
2021-06-29 3:04 ` Jie Deng
[not found] ` <YNraQMl3yJyZ6d5+@kunai>
2021-06-29 9:13 ` Viresh Kumar
2021-06-29 4:10 ` Viresh Kumar
[not found] ` <YNrZVho/98qgJS9N@kunai>
2021-06-29 8:52 ` Viresh Kumar
[not found] ` <YNyB/+fNK0u2bI6j@kunai>
2021-06-30 15:09 ` Viresh Kumar
2021-06-29 3:03 ` Jie Deng
[not found] ` <YNrw4rxihFLuqLtY@ninjato>
2021-06-29 10:16 ` Viresh Kumar
[not found] ` <YNr0uDx1fv+Gjd7m@ninjato>
2021-06-29 10:30 ` Viresh Kumar
[not found] ` <YNr5Jf3WDTH7U5b7@ninjato>
[not found] ` <YNr5ZRhT3qn+e9/m@ninjato>
2021-06-29 10:56 ` Viresh Kumar
[not found] ` <YNr/2E/T4FRjLOgy@ninjato>
2021-06-29 11:16 ` Viresh Kumar
2021-07-05 12:18 ` Viresh Kumar
2021-07-06 1:50 ` Jie Deng
[not found] ` <YPmLoeLSPS1tfYUK@ninjato>
2021-07-23 2:28 ` Viresh Kumar
2021-07-23 5:28 ` Viresh Kumar
2021-06-30 6:45 ` Jie Deng
[not found] ` <YNwd/t3DMKSOrTAT@ninjato>
2021-06-30 7:51 ` Jie Deng
2021-06-30 7:55 ` Arnd Bergmann
2021-06-30 8:07 ` Andy Shevchenko
2021-06-30 8:29 ` Arnd Bergmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).