Re: [PATCH v6] i2c: virtio: add a virtio i2c frontend driver
On 2021/3/10 16:27, Arnd Bergmann wrote: On Wed, Mar 10, 2021 at 4:59 AM Jason Wang wrote: On 2021/3/10 10:22 上午, Jie Deng wrote: On 2021/3/4 17:15, Jason Wang wrote: +} + +if (msgs[i].flags & I2C_M_RD) +memcpy(msgs[i].buf, req->buf, msgs[i].len); Sorry if I had asked this before but any rason not to use msg[i].buf directly? The msg[i].buf is passed by the I2C core. I just noticed that these bufs are not always allocated by kmalloc. They may come from the stack, which may cause the check "sg_init_one -> sg_set_buf -> virt_addr_valid" to fail. Therefore the msg[i].buf is not suitable for direct use here. Right, stack is virtually mapped. Maybe there is (or should be) a way to let the i2c core code handle the bounce buffering in this case. This is surely not a problem that is unique to this driver, and I'm sure it has come up many times in the past. I see that there is a i2c_get_dma_safe_msg_buf() helper for this purpose, but it has to be called by the driver rather than the core, so the driver still needs to keep track of each address when it sends multiple i2c_msg at once, but maybe it can all be done inside the sg_table instead of yet another structure. At least this one avoids copying data that is marked with the I2C_M_DMA_SAFE flag. Arnd Make sense. Thanks Arnd. I will try to use those helper functions. Regards, Jie
Re: [PATCH v6] i2c: virtio: add a virtio i2c frontend driver
On Wed, Mar 10, 2021 at 4:59 AM Jason Wang wrote: > On 2021/3/10 10:22 上午, Jie Deng wrote: > > On 2021/3/4 17:15, Jason Wang wrote: > >> > >> > >>> +} > >>> + > >>> +if (msgs[i].flags & I2C_M_RD) > >>> +memcpy(msgs[i].buf, req->buf, msgs[i].len); > >> > >> > >> Sorry if I had asked this before but any rason not to use msg[i].buf > >> directly? > >> > >> > > The msg[i].buf is passed by the I2C core. I just noticed that these > > bufs are not > > always allocated by kmalloc. They may come from the stack, which may > > cause > > the check "sg_init_one -> sg_set_buf -> virt_addr_valid" to fail. > > Therefore the > > msg[i].buf is not suitable for direct use here. > > Right, stack is virtually mapped. Maybe there is (or should be) a way to let the i2c core code handle the bounce buffering in this case. This is surely not a problem that is unique to this driver, and I'm sure it has come up many times in the past. I see that there is a i2c_get_dma_safe_msg_buf() helper for this purpose, but it has to be called by the driver rather than the core, so the driver still needs to keep track of each address when it sends multiple i2c_msg at once, but maybe it can all be done inside the sg_table instead of yet another structure. At least this one avoids copying data that is marked with the I2C_M_DMA_SAFE flag. Arnd
Re: [PATCH v6] i2c: virtio: add a virtio i2c frontend driver
On 2021/3/10 10:22 上午, Jie Deng wrote: On 2021/3/4 17:15, Jason Wang wrote: + } + + if (msgs[i].flags & I2C_M_RD) + memcpy(msgs[i].buf, req->buf, msgs[i].len); Sorry if I had asked this before but any rason not to use msg[i].buf directly? The msg[i].buf is passed by the I2C core. I just noticed that these bufs are not always allocated by kmalloc. They may come from the stack, which may cause the check "sg_init_one -> sg_set_buf -> virt_addr_valid" to fail. Therefore the msg[i].buf is not suitable for direct use here. Regards. Right, stack is virtually mapped. Thanks
Re: [PATCH v6] i2c: virtio: add a virtio i2c frontend driver
On 2021/3/4 17:15, Jason Wang wrote: + } + + if (msgs[i].flags & I2C_M_RD) + memcpy(msgs[i].buf, req->buf, msgs[i].len); Sorry if I had asked this before but any rason not to use msg[i].buf directly? The msg[i].buf is passed by the I2C core. I just noticed that these bufs are not always allocated by kmalloc. They may come from the stack, which may cause the check "sg_init_one -> sg_set_buf -> virt_addr_valid" to fail. Therefore the msg[i].buf is not suitable for direct use here. Regards.
Re: [PATCH v6] i2c: virtio: add a virtio i2c frontend driver
On 2021/3/5 15:23, Jason Wang wrote:
+ virtqueue_kick(vq);
+
+ time_left = wait_for_completion_timeout(>completion,
adap->timeout);
+ if (!time_left) {
+ dev_err(>dev, "virtio i2c backend timeout.\n");
+ ret = -ETIMEDOUT;
+ goto err_unlock_free;
So if the request is finished after the timerout, all the following
request will fail, is this expected?
This is an expected behavior. If timeout happens, we don't need to
care about the requests whether
really completed by "HW" or not. Just return error and let the i2c
core to decide whether to resend.
So you need at least reinit the completion at least?
Right. Will fix it. Thank you.
+ }
+
+ ret = virtio_i2c_complete_reqs(vq, reqs, msgs, nr);
So consider driver queue N requests, can device raise interrupt if
it completes the first request? If yes, the code break, if not it
need to be clarified in the spec.
The device can raise interrupt when some requests are still not
completed though this is not a good operation.
Then you need forbid this in the spec.
Yeah, but I think we can add some description to explain this clearly
instead of forbid it directly.
In this case, the remaining requests in the vq will be ignored and
the i2c_algorithm. master_xfer will return 1 for
your example. And let the i2c core to decide whether to resend.
Acaultly I remember there's no VIRTIO_I2C_FLAGS_FAIL_NEXT in
previous versions, and after reading the spec I still don't get the
motivation for that (it may complicates both driver and device
actually).
This flag is introduced by Stefan. Please check following link for
the details
https://lists.oasis-open.org/archives/virtio-comment/202012/msg00075.html.
> We just need to make sure that once the driver adds some requests to
the
> virtqueue,
> it should complete them (either success or fail) before adding new
requests.
> I think this
> is a behavior of physical I2C adapter drivers and we can follow.
Is this a driver requirement or device? If it's the driver, the code
have already did something like this. E.g you wait for the completion
of the request and forbid new request via i2c_lock.
Thanks
The driver. VIRTIO_I2C_FLAGS_FAIL_NEXT doesn't help in Linux driver.
But I agree with Stefan that
VIRTIO is not specific to Linux so the specs design should avoid the
limitations of the current
Linux driver behavior.
Re: [PATCH v6] i2c: virtio: add a virtio i2c frontend driver
On 05-03-21, 15:00, Jie Deng wrote:
> On 2021/3/5 11:09, Viresh Kumar wrote:
> > On 05-03-21, 09:46, Jie Deng wrote:
> > > On 2021/3/4 14:06, Viresh Kumar wrote:
> > > > > + mutex_lock(>i2c_lock);
> > > > I have never worked with i2c stuff earlier, but I don't think you need
> > > > a lock
> > > > here. The transactions seem to be serialized by the i2c-core by itself
> > > > (look at
> > > > i2c_transfer() in i2c-core-base.c), though there is another exported
> > > > version
> > > > __i2c_transfer() but the comment over it says the callers must take
> > > > adapter lock
> > > > before calling it.
> > > Lock is needed since no "lock_ops" is registered in this i2c_adapter.
> > drivers/i2c/i2c-core-base.c:
> >
> > static int i2c_register_adapter(struct i2c_adapter *adap)
> > {
> > ...
> >
> > if (!adap->lock_ops)
> > adap->lock_ops = _adapter_lock_ops;
> >
> > ...
> > }
> >
> > This should take care of it ?
>
>
> The problem is that you can't guarantee that adap->algo->master_xfer is only
> called
> from i2c_transfer. Any function who holds the adapter can call
> adap->algo->master_xfer
> directly. So I think it is safer to have a lock in virtio_i2c_xfer.
So I tried to look for such callers in the kernel.
$ git grep -l "\ > > > > +static struct i2c_adapter virtio_adapter = {
> > > > > + .owner = THIS_MODULE,
> > > > > + .name = "Virtio I2C Adapter",
> > > > > + .class = I2C_CLASS_DEPRECATED,
> > > > Why are we using something that is deprecated here ?
> > > Warn users that the adapter doesn't support classes anymore.
> > So this is the right thing to do? Or this is what we expect from new
> > drivers?
> > Sorry, I am just new to this stuff and so...
>
>
> https://patchwork.ozlabs.org/project/linux-i2c/patch/20170729121143.3980-1-...@the-dreams.de/
Frankly this confused me even further :)
The earlier comment in the code said:
"/* Warn users that adapter will stop using classes */"
so this looks more for existing drivers..
Then the commit message says this:
"Hopefully making clear that it is not needed for new drivers."
and comment says:
"/* Warn users that the adapter doesn't support classes anymore */"
Reading this it looks this is only required for existing adapters so they can
warn userspace and shouldn't be required for new drivers.
Am I reading it incorrectly ?
--
viresh
Re: [PATCH v6] i2c: virtio: add a virtio i2c frontend driver
On 2021/3/5 1:47 下午, Jie Deng wrote:
On 2021/3/4 17:15, Jason Wang wrote:
On 2021/3/4 9:59 上午, Jie Deng wrote:
Add an I2C bus driver for virtio para-virtualization.
The controller can be emulated by the backend driver in
any device model software by following the virtio protocol.
The device specification can be found on
https://lists.oasis-open.org/archives/virtio-comment/202101/msg8.html.
By following the specification, people may implement different
backend drivers to emulate different controllers according to
their needs.
Co-developed-by: Conghui Chen
Signed-off-by: Conghui Chen
Signed-off-by: Jie Deng
---
drivers/i2c/busses/Kconfig | 11 ++
drivers/i2c/busses/Makefile | 3 +
drivers/i2c/busses/i2c-virtio.c | 289
include/uapi/linux/virtio_i2c.h | 42 ++
include/uapi/linux/virtio_ids.h | 1 +
5 files changed, 346 insertions(+)
create mode 100644 drivers/i2c/busses/i2c-virtio.c
create mode 100644 include/uapi/linux/virtio_i2c.h
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 05ebf75..0860395 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -21,6 +21,17 @@ config I2C_ALI1535
This driver can also be built as a module. If so, the module
will be called i2c-ali1535.
+config I2C_VIRTIO
+ tristate "Virtio I2C Adapter"
+ depends on VIRTIO
Please use select here. There's no prompt for VIRTIO.
OK.
+ help
+ If you say yes to this option, support will be included for
the virtio
+ I2C adapter driver. The hardware can be emulated by any
device model
+ software according to the virtio protocol.
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-virtio.
+
config I2C_ALI1563
tristate "ALI 1563"
depends on PCI
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 615f35e..b88da08 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -6,6 +6,9 @@
# ACPI drivers
obj-$(CONFIG_I2C_SCMI) += i2c-scmi.o
+# VIRTIO I2C host controller driver
+obj-$(CONFIG_I2C_VIRTIO) += i2c-virtio.o
+
# PC SMBus host controller drivers
obj-$(CONFIG_I2C_ALI1535) += i2c-ali1535.o
obj-$(CONFIG_I2C_ALI1563) += i2c-ali1563.o
diff --git a/drivers/i2c/busses/i2c-virtio.c
b/drivers/i2c/busses/i2c-virtio.c
new file mode 100644
index 000..98c0fcc
--- /dev/null
+++ b/drivers/i2c/busses/i2c-virtio.c
@@ -0,0 +1,289 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Virtio I2C Bus Driver
+ *
+ * The Virtio I2C Specification:
+ *
https://raw.githubusercontent.com/oasis-tcs/virtio-spec/master/virtio-i2c.tex
+ *
+ * Copyright (c) 2021 Intel Corporation. All rights reserved.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+
+/**
+ * struct virtio_i2c - virtio I2C data
+ * @vdev: virtio device for this controller
+ * @completion: completion of virtio I2C message
+ * @adap: I2C adapter for this controller
+ * @i2c_lock: lock for virtqueue processing
+ * @vq: the virtio virtqueue for communication
+ */
+struct virtio_i2c {
+ struct virtio_device *vdev;
+ struct completion completion;
+ struct i2c_adapter adap;
+ struct mutex i2c_lock;
+ struct virtqueue *vq;
+};
+
+/**
+ * struct virtio_i2c_req - the virtio I2C request structure
+ * @out_hdr: the OUT header of the virtio I2C message
+ * @buf: the buffer into which data is read, or from which it's
written
+ * @in_hdr: the IN header of the virtio I2C message
+ */
+struct virtio_i2c_req {
+ struct virtio_i2c_out_hdr out_hdr;
+ u8 *buf;
+ struct virtio_i2c_in_hdr in_hdr;
+};
+
+static void virtio_i2c_msg_done(struct virtqueue *vq)
+{
+ struct virtio_i2c *vi = vq->vdev->priv;
+
+ complete(>completion);
+}
+
+static int virtio_i2c_send_reqs(struct virtqueue *vq,
+ struct virtio_i2c_req *reqs,
+ struct i2c_msg *msgs, int nr)
+{
+ struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr;
+ int i, outcnt, incnt, err = 0;
+ u8 *buf;
+
+ for (i = 0; i < nr; i++) {
+ if (!msgs[i].len)
+ break;
+
+ /* Only 7-bit mode supported for this moment. For the
address format,
+ * Please check the Virtio I2C Specification.
+ */
+ reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1);
+
+ if (i != nr - 1)
+ reqs[i].out_hdr.flags |= VIRTIO_I2C_FLAGS_FAIL_NEXT;
+
+ outcnt = incnt = 0;
+ sg_init_one(_hdr, [i].out_hdr,
sizeof(reqs[i].out_hdr));
+ sgs[outcnt++] = _hdr;
+
+ buf = kzalloc(msgs[i].len, GFP_KERNEL);
+ if (!buf)
+ break;
+
+ reqs[i].buf = buf;
+ sg_init_one(_buf, reqs[i].buf, msgs[i].len);
+
+ if (msgs[i].flags & I2C_M_RD) {
+ sgs[outcnt + incnt++] = _buf;
+ } else {
+
Re: [PATCH v6] i2c: virtio: add a virtio i2c frontend driver
On 2021/3/5 11:09, Viresh Kumar wrote:
On 05-03-21, 09:46, Jie Deng wrote:
On 2021/3/4 14:06, Viresh Kumar wrote:
depends on I2C as well ?
No need that. The dependency of I2C is included in the Kconfig in its parent
directory.
Sorry about that, I must have figured that out myself.
(Though a note on the way we reply to messages, please leave an empty line
before and after your messages, it gets difficult to find the inline replies
otherwise. )
+ if (!(req && req == [i])) {
I find this less readable compared to:
if (!req || req != [i]) {
Different people may have different tastes.
Please check Andy's comment in this link.
https://lists.linuxfoundation.org/pipermail/virtualization/2020-September/049933.html
Heh, everyone wants you to do it differently :)
If we leave compilers optimizations aside (because it will generate the exact
same code for both the cases, I tested it as well to be doubly sure), The
original statement used in this patch has 3 conditional statements in it and the
way I suggested has only two.
Andy, thoughts ?
And anyway, this isn't biggest of my worries, just that I had to notice it
somehow :)
For all the above errors where you simply break out, you still need to free the
memory for buf, right ?
Will try to use reqs[i].buf = msgs[i].buf to avoid allocation.
I think it would be better to have all such deallocations done at a single
place, i.e. after the completion callback is finished.. Trying to solve this
everywhere is going to make this more messy.
I think there is no need to have allocations/deallocations/memcpy for
reqs[i].buf any more
if using msgs[i].buf directly.
+ mutex_lock(>i2c_lock);
I have never worked with i2c stuff earlier, but I don't think you need a lock
here. The transactions seem to be serialized by the i2c-core by itself (look at
i2c_transfer() in i2c-core-base.c), though there is another exported version
__i2c_transfer() but the comment over it says the callers must take adapter lock
before calling it.
Lock is needed since no "lock_ops" is registered in this i2c_adapter.
drivers/i2c/i2c-core-base.c:
static int i2c_register_adapter(struct i2c_adapter *adap)
{
...
if (!adap->lock_ops)
adap->lock_ops = _adapter_lock_ops;
...
}
This should take care of it ?
The problem is that you can't guarantee that adap->algo->master_xfer is
only called
from i2c_transfer. Any function who holds the adapter can call
adap->algo->master_xfer
directly. So I think it is safer to have a lock in virtio_i2c_xfer.
+
+ ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
+ if (ret == 0)
+ goto err_unlock_free;
+
+ nr = ret;
+
+ virtqueue_kick(vq);
+
+ time_left = wait_for_completion_timeout(>completion, adap->timeout);
+ if (!time_left) {
+ dev_err(>dev, "virtio i2c backend timeout.\n");
+ ret = -ETIMEDOUT;
You need to free bufs of the requests here as well..
Just want to make sure you didn't miss this comment.
Will try to use msgs[i].buf directly. So it should be no free bufs any more.
+static struct i2c_adapter virtio_adapter = {
+ .owner = THIS_MODULE,
+ .name = "Virtio I2C Adapter",
+ .class = I2C_CLASS_DEPRECATED,
Why are we using something that is deprecated here ?
Warn users that the adapter doesn't support classes anymore.
So this is the right thing to do? Or this is what we expect from new drivers?
Sorry, I am just new to this stuff and so...
https://patchwork.ozlabs.org/project/linux-i2c/patch/20170729121143.3980-1-...@the-dreams.de/
+struct virtio_i2c_out_hdr {
+ __le16 addr;
+ __le16 padding;
+ __le32 flags;
+};
It might be worth setting __packed for the structures here, even when we have
taken care of padding ourselves, for both the structures..
Please check Michael's comment https://lkml.org/lkml/2020/9/3/339.
I agreed to remove "__packed" .
When Michael commented the structure looked like this:
Actually it can be ignored as the compiler isn't going to add any padding by
itself in this case (since you already took care of it) as the structure will be
aligned to the size of the biggest element here. I generally consider it to be a
good coding-style to make sure we don't add any unwanted stuff in there by
mistake.
Anyway, we can see it in future if this is going to be required or not, if and
when we add new fields here.
Re: [PATCH v6] i2c: virtio: add a virtio i2c frontend driver
On 2021/3/4 17:15, Jason Wang wrote:
On 2021/3/4 9:59 上午, Jie Deng wrote:
Add an I2C bus driver for virtio para-virtualization.
The controller can be emulated by the backend driver in
any device model software by following the virtio protocol.
The device specification can be found on
https://lists.oasis-open.org/archives/virtio-comment/202101/msg8.html.
By following the specification, people may implement different
backend drivers to emulate different controllers according to
their needs.
Co-developed-by: Conghui Chen
Signed-off-by: Conghui Chen
Signed-off-by: Jie Deng
---
drivers/i2c/busses/Kconfig | 11 ++
drivers/i2c/busses/Makefile | 3 +
drivers/i2c/busses/i2c-virtio.c | 289
include/uapi/linux/virtio_i2c.h | 42 ++
include/uapi/linux/virtio_ids.h | 1 +
5 files changed, 346 insertions(+)
create mode 100644 drivers/i2c/busses/i2c-virtio.c
create mode 100644 include/uapi/linux/virtio_i2c.h
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 05ebf75..0860395 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -21,6 +21,17 @@ config I2C_ALI1535
This driver can also be built as a module. If so, the module
will be called i2c-ali1535.
+config I2C_VIRTIO
+ tristate "Virtio I2C Adapter"
+ depends on VIRTIO
Please use select here. There's no prompt for VIRTIO.
OK.
+ help
+ If you say yes to this option, support will be included for
the virtio
+ I2C adapter driver. The hardware can be emulated by any device
model
+ software according to the virtio protocol.
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-virtio.
+
config I2C_ALI1563
tristate "ALI 1563"
depends on PCI
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 615f35e..b88da08 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -6,6 +6,9 @@
# ACPI drivers
obj-$(CONFIG_I2C_SCMI) += i2c-scmi.o
+# VIRTIO I2C host controller driver
+obj-$(CONFIG_I2C_VIRTIO) += i2c-virtio.o
+
# PC SMBus host controller drivers
obj-$(CONFIG_I2C_ALI1535) += i2c-ali1535.o
obj-$(CONFIG_I2C_ALI1563) += i2c-ali1563.o
diff --git a/drivers/i2c/busses/i2c-virtio.c
b/drivers/i2c/busses/i2c-virtio.c
new file mode 100644
index 000..98c0fcc
--- /dev/null
+++ b/drivers/i2c/busses/i2c-virtio.c
@@ -0,0 +1,289 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Virtio I2C Bus Driver
+ *
+ * The Virtio I2C Specification:
+ *
https://raw.githubusercontent.com/oasis-tcs/virtio-spec/master/virtio-i2c.tex
+ *
+ * Copyright (c) 2021 Intel Corporation. All rights reserved.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+
+/**
+ * struct virtio_i2c - virtio I2C data
+ * @vdev: virtio device for this controller
+ * @completion: completion of virtio I2C message
+ * @adap: I2C adapter for this controller
+ * @i2c_lock: lock for virtqueue processing
+ * @vq: the virtio virtqueue for communication
+ */
+struct virtio_i2c {
+ struct virtio_device *vdev;
+ struct completion completion;
+ struct i2c_adapter adap;
+ struct mutex i2c_lock;
+ struct virtqueue *vq;
+};
+
+/**
+ * struct virtio_i2c_req - the virtio I2C request structure
+ * @out_hdr: the OUT header of the virtio I2C message
+ * @buf: the buffer into which data is read, or from which it's written
+ * @in_hdr: the IN header of the virtio I2C message
+ */
+struct virtio_i2c_req {
+ struct virtio_i2c_out_hdr out_hdr;
+ u8 *buf;
+ struct virtio_i2c_in_hdr in_hdr;
+};
+
+static void virtio_i2c_msg_done(struct virtqueue *vq)
+{
+ struct virtio_i2c *vi = vq->vdev->priv;
+
+ complete(>completion);
+}
+
+static int virtio_i2c_send_reqs(struct virtqueue *vq,
+ struct virtio_i2c_req *reqs,
+ struct i2c_msg *msgs, int nr)
+{
+ struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr;
+ int i, outcnt, incnt, err = 0;
+ u8 *buf;
+
+ for (i = 0; i < nr; i++) {
+ if (!msgs[i].len)
+ break;
+
+ /* Only 7-bit mode supported for this moment. For the
address format,
+ * Please check the Virtio I2C Specification.
+ */
+ reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1);
+
+ if (i != nr - 1)
+ reqs[i].out_hdr.flags |= VIRTIO_I2C_FLAGS_FAIL_NEXT;
+
+ outcnt = incnt = 0;
+ sg_init_one(_hdr, [i].out_hdr,
sizeof(reqs[i].out_hdr));
+ sgs[outcnt++] = _hdr;
+
+ buf = kzalloc(msgs[i].len, GFP_KERNEL);
+ if (!buf)
+ break;
+
+ reqs[i].buf = buf;
+ sg_init_one(_buf, reqs[i].buf, msgs[i].len);
+
+ if (msgs[i].flags & I2C_M_RD) {
+ sgs[outcnt + incnt++] = _buf;
+ } else {
+ memcpy(reqs[i].buf, msgs[i].buf,
Re: [PATCH v6] i2c: virtio: add a virtio i2c frontend driver
On 05-03-21, 09:46, Jie Deng wrote:
> On 2021/3/4 14:06, Viresh Kumar wrote:
> > depends on I2C as well ?
> No need that. The dependency of I2C is included in the Kconfig in its parent
> directory.
Sorry about that, I must have figured that out myself.
(Though a note on the way we reply to messages, please leave an empty line
before and after your messages, it gets difficult to find the inline replies
otherwise. )
> > > + if (!(req && req == [i])) {
> > I find this less readable compared to:
> > if (!req || req != [i]) {
>
> Different people may have different tastes.
>
> Please check Andy's comment in this link.
>
> https://lists.linuxfoundation.org/pipermail/virtualization/2020-September/049933.html
Heh, everyone wants you to do it differently :)
If we leave compilers optimizations aside (because it will generate the exact
same code for both the cases, I tested it as well to be doubly sure), The
original statement used in this patch has 3 conditional statements in it and the
way I suggested has only two.
Andy, thoughts ?
And anyway, this isn't biggest of my worries, just that I had to notice it
somehow :)
> > For all the above errors where you simply break out, you still need to free
> > the
> > memory for buf, right ?
> Will try to use reqs[i].buf = msgs[i].buf to avoid allocation.
I think it would be better to have all such deallocations done at a single
place, i.e. after the completion callback is finished.. Trying to solve this
everywhere is going to make this more messy.
> > > + mutex_lock(>i2c_lock);
> > I have never worked with i2c stuff earlier, but I don't think you need a
> > lock
> > here. The transactions seem to be serialized by the i2c-core by itself
> > (look at
> > i2c_transfer() in i2c-core-base.c), though there is another exported version
> > __i2c_transfer() but the comment over it says the callers must take adapter
> > lock
> > before calling it.
> Lock is needed since no "lock_ops" is registered in this i2c_adapter.
drivers/i2c/i2c-core-base.c:
static int i2c_register_adapter(struct i2c_adapter *adap)
{
...
if (!adap->lock_ops)
adap->lock_ops = _adapter_lock_ops;
...
}
This should take care of it ?
> >
> > > +
> > > + ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
> > > + if (ret == 0)
> > > + goto err_unlock_free;
> > > +
> > > + nr = ret;
> > > +
> > > + virtqueue_kick(vq);
> > > +
> > > + time_left = wait_for_completion_timeout(>completion, adap->timeout);
> > > + if (!time_left) {
> > > + dev_err(>dev, "virtio i2c backend timeout.\n");
> > > + ret = -ETIMEDOUT;
> > You need to free bufs of the requests here as well..
Just want to make sure you didn't miss this comment.
> > > +static struct i2c_adapter virtio_adapter = {
> > > + .owner = THIS_MODULE,
> > > + .name = "Virtio I2C Adapter",
> > > + .class = I2C_CLASS_DEPRECATED,
> > Why are we using something that is deprecated here ?
> Warn users that the adapter doesn't support classes anymore.
So this is the right thing to do? Or this is what we expect from new drivers?
Sorry, I am just new to this stuff and so...
> > > +struct virtio_i2c_out_hdr {
> > > + __le16 addr;
> > > + __le16 padding;
> > > + __le32 flags;
> > > +};
> > It might be worth setting __packed for the structures here, even when we
> > have
> > taken care of padding ourselves, for both the structures..
> Please check Michael's comment https://lkml.org/lkml/2020/9/3/339.
> I agreed to remove "__packed" .
When Michael commented the structure looked like this:
Actually it can be ignored as the compiler isn't going to add any padding by
itself in this case (since you already took care of it) as the structure will be
aligned to the size of the biggest element here. I generally consider it to be a
good coding-style to make sure we don't add any unwanted stuff in there by
mistake.
Anyway, we can see it in future if this is going to be required or not, if and
when we add new fields here.
--
viresh
Re: [PATCH v6] i2c: virtio: add a virtio i2c frontend driver
On 2021/3/4 14:06, Viresh Kumar wrote:
Please always supply version history, it makes it difficult to review otherwise.
I will add the history.
drivers/i2c/busses/Kconfig | 11 ++
drivers/i2c/busses/Makefile | 3 +
drivers/i2c/busses/i2c-virtio.c | 289
include/uapi/linux/virtio_i2c.h | 42 ++
include/uapi/linux/virtio_ids.h | 1 +
5 files changed, 346 insertions(+)
create mode 100644 drivers/i2c/busses/i2c-virtio.c
create mode 100644 include/uapi/linux/virtio_i2c.h
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 05ebf75..0860395 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -21,6 +21,17 @@ config I2C_ALI1535
This driver can also be built as a module. If so, the module
will be called i2c-ali1535.
+config I2C_VIRTIO
+ tristate "Virtio I2C Adapter"
+ depends on VIRTIO
depends on I2C as well ?
No need that. The dependency of I2C is included in the Kconfig in its
parent directory.
+ help
+ If you say yes to this option, support will be included for the virtio
+ I2C adapter driver. The hardware can be emulated by any device model
+ software according to the virtio protocol.
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-virtio.
+
config I2C_ALI1563
tristate "ALI 1563"
depends on PCI
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 615f35e..b88da08 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -6,6 +6,9 @@
# ACPI drivers
obj-$(CONFIG_I2C_SCMI)+= i2c-scmi.o
+# VIRTIO I2C host controller driver
+obj-$(CONFIG_I2C_VIRTIO) += i2c-virtio.o
+
Not that it is important but I would have added it towards the end instead of at
the top of the file..
I'm OK to add it to the end.
# PC SMBus host controller drivers
obj-$(CONFIG_I2C_ALI1535) += i2c-ali1535.o
obj-$(CONFIG_I2C_ALI1563) += i2c-ali1563.o
diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
new file mode 100644
index 000..98c0fcc
--- /dev/null
+++ b/drivers/i2c/busses/i2c-virtio.c
@@ -0,0 +1,289 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Virtio I2C Bus Driver
+ *
+ * The Virtio I2C Specification:
+ *
https://raw.githubusercontent.com/oasis-tcs/virtio-spec/master/virtio-i2c.tex
+ *
+ * Copyright (c) 2021 Intel Corporation. All rights reserved.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
I don't think above two are required here..
+#include
+#include
+#include
same here.
+#include
same here.
Will confirm and remove them if they are not required. Thank you.
+
And this blank line as well, since all are standard linux headers.
+#include
+#include
+
+/**
+ * struct virtio_i2c - virtio I2C data
+ * @vdev: virtio device for this controller
+ * @completion: completion of virtio I2C message
+ * @adap: I2C adapter for this controller
+ * @i2c_lock: lock for virtqueue processing
+ * @vq: the virtio virtqueue for communication
+ */
+struct virtio_i2c {
+ struct virtio_device *vdev;
+ struct completion completion;
+ struct i2c_adapter adap;
+ struct mutex i2c_lock;
i2c_ is redundant here. "lock" sounds good enough.
OK.
+ struct virtqueue *vq;
+};
+
+/**
+ * struct virtio_i2c_req - the virtio I2C request structure
+ * @out_hdr: the OUT header of the virtio I2C message
+ * @buf: the buffer into which data is read, or from which it's written
+ * @in_hdr: the IN header of the virtio I2C message
+ */
+struct virtio_i2c_req {
+ struct virtio_i2c_out_hdr out_hdr;
+ u8 *buf;
+ struct virtio_i2c_in_hdr in_hdr;
+};
+
+static void virtio_i2c_msg_done(struct virtqueue *vq)
+{
+ struct virtio_i2c *vi = vq->vdev->priv;
+
+ complete(>completion);
+}
+
+static int virtio_i2c_send_reqs(struct virtqueue *vq,
+ struct virtio_i2c_req *reqs,
+ struct i2c_msg *msgs, int nr)
+{
+ struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr;
+ int i, outcnt, incnt, err = 0;
+ u8 *buf;
+
+ for (i = 0; i < nr; i++) {
+ if (!msgs[i].len)
+ break;
+
+ /* Only 7-bit mode supported for this moment. For the address
format,
+* Please check the Virtio I2C Specification.
+*/
Please use proper comment style.
will fix it.
/*
* Only 7-bit mode supported for now, check Virtio I2C
* specification for format of "addr" field.
*/
+ reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1);
+
+ if (i != nr - 1)
+ reqs[i].out_hdr.flags |= VIRTIO_I2C_FLAGS_FAIL_NEXT;
Since flags field hasn't been touched anywhere,
Re: [PATCH v6] i2c: virtio: add a virtio i2c frontend driver
On 2021/3/4 9:59 上午, Jie Deng wrote:
Add an I2C bus driver for virtio para-virtualization.
The controller can be emulated by the backend driver in
any device model software by following the virtio protocol.
The device specification can be found on
https://lists.oasis-open.org/archives/virtio-comment/202101/msg8.html.
By following the specification, people may implement different
backend drivers to emulate different controllers according to
their needs.
Co-developed-by: Conghui Chen
Signed-off-by: Conghui Chen
Signed-off-by: Jie Deng
---
drivers/i2c/busses/Kconfig | 11 ++
drivers/i2c/busses/Makefile | 3 +
drivers/i2c/busses/i2c-virtio.c | 289
include/uapi/linux/virtio_i2c.h | 42 ++
include/uapi/linux/virtio_ids.h | 1 +
5 files changed, 346 insertions(+)
create mode 100644 drivers/i2c/busses/i2c-virtio.c
create mode 100644 include/uapi/linux/virtio_i2c.h
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 05ebf75..0860395 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -21,6 +21,17 @@ config I2C_ALI1535
This driver can also be built as a module. If so, the module
will be called i2c-ali1535.
+config I2C_VIRTIO
+ tristate "Virtio I2C Adapter"
+ depends on VIRTIO
Please use select here. There's no prompt for VIRTIO.
+ help
+ If you say yes to this option, support will be included for the virtio
+ I2C adapter driver. The hardware can be emulated by any device model
+ software according to the virtio protocol.
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-virtio.
+
config I2C_ALI1563
tristate "ALI 1563"
depends on PCI
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 615f35e..b88da08 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -6,6 +6,9 @@
# ACPI drivers
obj-$(CONFIG_I2C_SCMI)+= i2c-scmi.o
+# VIRTIO I2C host controller driver
+obj-$(CONFIG_I2C_VIRTIO) += i2c-virtio.o
+
# PC SMBus host controller drivers
obj-$(CONFIG_I2C_ALI1535) += i2c-ali1535.o
obj-$(CONFIG_I2C_ALI1563) += i2c-ali1563.o
diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
new file mode 100644
index 000..98c0fcc
--- /dev/null
+++ b/drivers/i2c/busses/i2c-virtio.c
@@ -0,0 +1,289 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Virtio I2C Bus Driver
+ *
+ * The Virtio I2C Specification:
+ *
https://raw.githubusercontent.com/oasis-tcs/virtio-spec/master/virtio-i2c.tex
+ *
+ * Copyright (c) 2021 Intel Corporation. All rights reserved.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+
+/**
+ * struct virtio_i2c - virtio I2C data
+ * @vdev: virtio device for this controller
+ * @completion: completion of virtio I2C message
+ * @adap: I2C adapter for this controller
+ * @i2c_lock: lock for virtqueue processing
+ * @vq: the virtio virtqueue for communication
+ */
+struct virtio_i2c {
+ struct virtio_device *vdev;
+ struct completion completion;
+ struct i2c_adapter adap;
+ struct mutex i2c_lock;
+ struct virtqueue *vq;
+};
+
+/**
+ * struct virtio_i2c_req - the virtio I2C request structure
+ * @out_hdr: the OUT header of the virtio I2C message
+ * @buf: the buffer into which data is read, or from which it's written
+ * @in_hdr: the IN header of the virtio I2C message
+ */
+struct virtio_i2c_req {
+ struct virtio_i2c_out_hdr out_hdr;
+ u8 *buf;
+ struct virtio_i2c_in_hdr in_hdr;
+};
+
+static void virtio_i2c_msg_done(struct virtqueue *vq)
+{
+ struct virtio_i2c *vi = vq->vdev->priv;
+
+ complete(>completion);
+}
+
+static int virtio_i2c_send_reqs(struct virtqueue *vq,
+ struct virtio_i2c_req *reqs,
+ struct i2c_msg *msgs, int nr)
+{
+ struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr;
+ int i, outcnt, incnt, err = 0;
+ u8 *buf;
+
+ for (i = 0; i < nr; i++) {
+ if (!msgs[i].len)
+ break;
+
+ /* Only 7-bit mode supported for this moment. For the address
format,
+* Please check the Virtio I2C Specification.
+*/
+ reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1);
+
+ if (i != nr - 1)
+ reqs[i].out_hdr.flags |= VIRTIO_I2C_FLAGS_FAIL_NEXT;
+
+ outcnt = incnt = 0;
+ sg_init_one(_hdr, [i].out_hdr,
sizeof(reqs[i].out_hdr));
+ sgs[outcnt++] = _hdr;
+
+ buf = kzalloc(msgs[i].len, GFP_KERNEL);
+ if (!buf)
+ break;
+
+ reqs[i].buf = buf;
+
Re: [PATCH v6] i2c: virtio: add a virtio i2c frontend driver
Hi Jie, Thank you for the patch! Yet something to improve: [auto build test ERROR on wsa/i2c/for-next] [also build test ERROR on vhost/linux-next linux/master linus/master v5.12-rc1 next-20210303] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Jie-Deng/i2c-virtio-add-a-virtio-i2c-frontend-driver/20210304-100543 base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next config: x86_64-randconfig-a014-20210304 (attached as .config) compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project eec7f8f7b1226be422a76542cb403d02538f453a) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install x86_64 cross compiling tool for clang build # apt-get install binutils-x86-64-linux-gnu # https://github.com/0day-ci/linux/commit/bb645653b6d7750a026229726f8d9d0371457ac6 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Jie-Deng/i2c-virtio-add-a-virtio-i2c-frontend-driver/20210304-100543 git checkout bb645653b6d7750a026229726f8d9d0371457ac6 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All errors (new ones prefixed by >>): In file included from :1: >> ./usr/include/linux/virtio_i2c.h:35:2: error: unknown type name 'u8' u8 status; ^ 1 error generated. --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org .config.gz Description: application/gzip
Re: [PATCH v6] i2c: virtio: add a virtio i2c frontend driver
On 04-03-21, 09:59, Jie Deng wrote:
> Add an I2C bus driver for virtio para-virtualization.
>
> The controller can be emulated by the backend driver in
> any device model software by following the virtio protocol.
>
> The device specification can be found on
> https://lists.oasis-open.org/archives/virtio-comment/202101/msg8.html.
>
> By following the specification, people may implement different
> backend drivers to emulate different controllers according to
> their needs.
>
> Co-developed-by: Conghui Chen
> Signed-off-by: Conghui Chen
> Signed-off-by: Jie Deng
> ---
Please always supply version history, it makes it difficult to review otherwise.
> drivers/i2c/busses/Kconfig | 11 ++
> drivers/i2c/busses/Makefile | 3 +
> drivers/i2c/busses/i2c-virtio.c | 289
>
> include/uapi/linux/virtio_i2c.h | 42 ++
> include/uapi/linux/virtio_ids.h | 1 +
> 5 files changed, 346 insertions(+)
> create mode 100644 drivers/i2c/busses/i2c-virtio.c
> create mode 100644 include/uapi/linux/virtio_i2c.h
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 05ebf75..0860395 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -21,6 +21,17 @@ config I2C_ALI1535
> This driver can also be built as a module. If so, the module
> will be called i2c-ali1535.
>
> +config I2C_VIRTIO
> + tristate "Virtio I2C Adapter"
> + depends on VIRTIO
depends on I2C as well ?
> + help
> + If you say yes to this option, support will be included for the virtio
> + I2C adapter driver. The hardware can be emulated by any device model
> + software according to the virtio protocol.
> +
> + This driver can also be built as a module. If so, the module
> + will be called i2c-virtio.
> +
> config I2C_ALI1563
> tristate "ALI 1563"
> depends on PCI
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 615f35e..b88da08 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -6,6 +6,9 @@
> # ACPI drivers
> obj-$(CONFIG_I2C_SCMI) += i2c-scmi.o
>
> +# VIRTIO I2C host controller driver
> +obj-$(CONFIG_I2C_VIRTIO) += i2c-virtio.o
> +
Not that it is important but I would have added it towards the end instead of at
the top of the file..
> # PC SMBus host controller drivers
> obj-$(CONFIG_I2C_ALI1535)+= i2c-ali1535.o
> obj-$(CONFIG_I2C_ALI1563)+= i2c-ali1563.o
> diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
> new file mode 100644
> index 000..98c0fcc
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-virtio.c
> @@ -0,0 +1,289 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Virtio I2C Bus Driver
> + *
> + * The Virtio I2C Specification:
> + *
> https://raw.githubusercontent.com/oasis-tcs/virtio-spec/master/virtio-i2c.tex
> + *
> + * Copyright (c) 2021 Intel Corporation. All rights reserved.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
I don't think above two are required here..
> +#include
> +#include
> +#include
same here.
> +#include
same here.
> +
And this blank line as well, since all are standard linux headers.
> +#include
> +#include
> +
> +/**
> + * struct virtio_i2c - virtio I2C data
> + * @vdev: virtio device for this controller
> + * @completion: completion of virtio I2C message
> + * @adap: I2C adapter for this controller
> + * @i2c_lock: lock for virtqueue processing
> + * @vq: the virtio virtqueue for communication
> + */
> +struct virtio_i2c {
> + struct virtio_device *vdev;
> + struct completion completion;
> + struct i2c_adapter adap;
> + struct mutex i2c_lock;
i2c_ is redundant here. "lock" sounds good enough.
> + struct virtqueue *vq;
> +};
> +
> +/**
> + * struct virtio_i2c_req - the virtio I2C request structure
> + * @out_hdr: the OUT header of the virtio I2C message
> + * @buf: the buffer into which data is read, or from which it's written
> + * @in_hdr: the IN header of the virtio I2C message
> + */
> +struct virtio_i2c_req {
> + struct virtio_i2c_out_hdr out_hdr;
> + u8 *buf;
> + struct virtio_i2c_in_hdr in_hdr;
> +};
> +
> +static void virtio_i2c_msg_done(struct virtqueue *vq)
> +{
> + struct virtio_i2c *vi = vq->vdev->priv;
> +
> + complete(>completion);
> +}
> +
> +static int virtio_i2c_send_reqs(struct virtqueue *vq,
> + struct virtio_i2c_req *reqs,
> + struct i2c_msg *msgs, int nr)
> +{
> + struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr;
> + int i, outcnt, incnt, err = 0;
> + u8 *buf;
> +
> + for (i = 0; i < nr; i++) {
> + if (!msgs[i].len)
> + break;
> +
> + /* Only 7-bit mode supported for this moment. For the address
> format,
> + * Please check the Virtio I2C
Re: [PATCH v6] i2c: virtio: add a virtio i2c frontend driver
Hi Jie,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on wsa/i2c/for-next]
[also build test WARNING on vhost/linux-next linux/master linus/master
v5.12-rc1 next-20210303]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Jie-Deng/i2c-virtio-add-a-virtio-i2c-frontend-driver/20210304-100543
base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git
i2c/for-next
config: xtensa-randconfig-s031-20210304 (attached as .config)
compiler: xtensa-linux-gcc (GCC) 9.3.0
reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.3-245-gacc5c298-dirty
#
https://github.com/0day-ci/linux/commit/bb645653b6d7750a026229726f8d9d0371457ac6
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Jie-Deng/i2c-virtio-add-a-virtio-i2c-frontend-driver/20210304-100543
git checkout bb645653b6d7750a026229726f8d9d0371457ac6
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1
CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=xtensa
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
"sparse warnings: (new ones prefixed by >>)"
>> drivers/i2c/busses/i2c-virtio.c:78:47: sparse: sparse: invalid assignment: |=
>> drivers/i2c/busses/i2c-virtio.c:78:47: sparse:left side has type
>> restricted __le32
>> drivers/i2c/busses/i2c-virtio.c:78:47: sparse:right side has type int
vim +78 drivers/i2c/busses/i2c-virtio.c
59
60 static int virtio_i2c_send_reqs(struct virtqueue *vq,
61 struct virtio_i2c_req *reqs,
62 struct i2c_msg *msgs, int nr)
63 {
64 struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr;
65 int i, outcnt, incnt, err = 0;
66 u8 *buf;
67
68 for (i = 0; i < nr; i++) {
69 if (!msgs[i].len)
70 break;
71
72 /* Only 7-bit mode supported for this moment. For the
address format,
73 * Please check the Virtio I2C Specification.
74 */
75 reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1);
76
77 if (i != nr - 1)
> 78 reqs[i].out_hdr.flags |=
VIRTIO_I2C_FLAGS_FAIL_NEXT;
79
80 outcnt = incnt = 0;
81 sg_init_one(_hdr, [i].out_hdr,
sizeof(reqs[i].out_hdr));
82 sgs[outcnt++] = _hdr;
83
84 buf = kzalloc(msgs[i].len, GFP_KERNEL);
85 if (!buf)
86 break;
87
88 reqs[i].buf = buf;
89 sg_init_one(_buf, reqs[i].buf, msgs[i].len);
90
91 if (msgs[i].flags & I2C_M_RD) {
92 sgs[outcnt + incnt++] = _buf;
93 } else {
94 memcpy(reqs[i].buf, msgs[i].buf, msgs[i].len);
95 sgs[outcnt++] = _buf;
96 }
97
98 sg_init_one(_hdr, [i].in_hdr,
sizeof(reqs[i].in_hdr));
99 sgs[outcnt + incnt++] = _hdr;
100
101 err = virtqueue_add_sgs(vq, sgs, outcnt, incnt,
[i], GFP_KERNEL);
102 if (err < 0) {
103 pr_err("failed to add msg[%d] to virtqueue.\n",
i);
104 kfree(reqs[i].buf);
105 reqs[i].buf = NULL;
106 break;
107 }
108 }
109
110 return i;
111 }
112
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
Re: [PATCH v6] i2c: virtio: add a virtio i2c frontend driver
Hi Jie, Thank you for the patch! Yet something to improve: [auto build test ERROR on wsa/i2c/for-next] [also build test ERROR on vhost/linux-next linux/master linus/master v5.12-rc1 next-20210303] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Jie-Deng/i2c-virtio-add-a-virtio-i2c-frontend-driver/20210304-100543 base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next config: i386-randconfig-a006-20210304 (attached as .config) compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 reproduce (this is a W=1 build): # https://github.com/0day-ci/linux/commit/bb645653b6d7750a026229726f8d9d0371457ac6 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Jie-Deng/i2c-virtio-add-a-virtio-i2c-frontend-driver/20210304-100543 git checkout bb645653b6d7750a026229726f8d9d0371457ac6 # save the attached .config to linux build tree make W=1 ARCH=i386 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All errors (new ones prefixed by >>): In file included from :32: >> ./usr/include/linux/virtio_i2c.h:35:2: error: unknown type name 'u8' 35 | u8 status; | ^~ --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org .config.gz Description: application/gzip
[PATCH v6] i2c: virtio: add a virtio i2c frontend driver
Add an I2C bus driver for virtio para-virtualization.
The controller can be emulated by the backend driver in
any device model software by following the virtio protocol.
The device specification can be found on
https://lists.oasis-open.org/archives/virtio-comment/202101/msg8.html.
By following the specification, people may implement different
backend drivers to emulate different controllers according to
their needs.
Co-developed-by: Conghui Chen
Signed-off-by: Conghui Chen
Signed-off-by: Jie Deng
---
drivers/i2c/busses/Kconfig | 11 ++
drivers/i2c/busses/Makefile | 3 +
drivers/i2c/busses/i2c-virtio.c | 289
include/uapi/linux/virtio_i2c.h | 42 ++
include/uapi/linux/virtio_ids.h | 1 +
5 files changed, 346 insertions(+)
create mode 100644 drivers/i2c/busses/i2c-virtio.c
create mode 100644 include/uapi/linux/virtio_i2c.h
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 05ebf75..0860395 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -21,6 +21,17 @@ config I2C_ALI1535
This driver can also be built as a module. If so, the module
will be called i2c-ali1535.
+config I2C_VIRTIO
+ tristate "Virtio I2C Adapter"
+ depends on VIRTIO
+ help
+ If you say yes to this option, support will be included for the virtio
+ I2C adapter driver. The hardware can be emulated by any device model
+ software according to the virtio protocol.
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-virtio.
+
config I2C_ALI1563
tristate "ALI 1563"
depends on PCI
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 615f35e..b88da08 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -6,6 +6,9 @@
# ACPI drivers
obj-$(CONFIG_I2C_SCMI) += i2c-scmi.o
+# VIRTIO I2C host controller driver
+obj-$(CONFIG_I2C_VIRTIO) += i2c-virtio.o
+
# PC SMBus host controller drivers
obj-$(CONFIG_I2C_ALI1535) += i2c-ali1535.o
obj-$(CONFIG_I2C_ALI1563) += i2c-ali1563.o
diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
new file mode 100644
index 000..98c0fcc
--- /dev/null
+++ b/drivers/i2c/busses/i2c-virtio.c
@@ -0,0 +1,289 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Virtio I2C Bus Driver
+ *
+ * The Virtio I2C Specification:
+ *
https://raw.githubusercontent.com/oasis-tcs/virtio-spec/master/virtio-i2c.tex
+ *
+ * Copyright (c) 2021 Intel Corporation. All rights reserved.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+
+/**
+ * struct virtio_i2c - virtio I2C data
+ * @vdev: virtio device for this controller
+ * @completion: completion of virtio I2C message
+ * @adap: I2C adapter for this controller
+ * @i2c_lock: lock for virtqueue processing
+ * @vq: the virtio virtqueue for communication
+ */
+struct virtio_i2c {
+ struct virtio_device *vdev;
+ struct completion completion;
+ struct i2c_adapter adap;
+ struct mutex i2c_lock;
+ struct virtqueue *vq;
+};
+
+/**
+ * struct virtio_i2c_req - the virtio I2C request structure
+ * @out_hdr: the OUT header of the virtio I2C message
+ * @buf: the buffer into which data is read, or from which it's written
+ * @in_hdr: the IN header of the virtio I2C message
+ */
+struct virtio_i2c_req {
+ struct virtio_i2c_out_hdr out_hdr;
+ u8 *buf;
+ struct virtio_i2c_in_hdr in_hdr;
+};
+
+static void virtio_i2c_msg_done(struct virtqueue *vq)
+{
+ struct virtio_i2c *vi = vq->vdev->priv;
+
+ complete(>completion);
+}
+
+static int virtio_i2c_send_reqs(struct virtqueue *vq,
+ struct virtio_i2c_req *reqs,
+ struct i2c_msg *msgs, int nr)
+{
+ struct scatterlist *sgs[3], out_hdr, msg_buf, in_hdr;
+ int i, outcnt, incnt, err = 0;
+ u8 *buf;
+
+ for (i = 0; i < nr; i++) {
+ if (!msgs[i].len)
+ break;
+
+ /* Only 7-bit mode supported for this moment. For the address
format,
+* Please check the Virtio I2C Specification.
+*/
+ reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1);
+
+ if (i != nr - 1)
+ reqs[i].out_hdr.flags |= VIRTIO_I2C_FLAGS_FAIL_NEXT;
+
+ outcnt = incnt = 0;
+ sg_init_one(_hdr, [i].out_hdr,
sizeof(reqs[i].out_hdr));
+ sgs[outcnt++] = _hdr;
+
+ buf = kzalloc(msgs[i].len, GFP_KERNEL);
+ if (!buf)
+ break;
+
+ reqs[i].buf = buf;
+ sg_init_one(_buf, reqs[i].buf, msgs[i].len);
+
+ if (msgs[i].flags & I2C_M_RD) {
+ sgs[outcnt +
