Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On 2021/3/3 17:38, Viresh Kumar wrote: On 03-03-21, 16:46, Jie Deng wrote: This is not a problem. My original proposal was to mirror the struct i2c_msg. The code you looked at was based on that. However, the virtio TC prefer not to mirror it. They have some concerns. For example, there is a bit I2C_M_RD in i2c_msg.flag which has the same meaning with the R/W in virtio descriptor. This is a repetition which may cause problems. So the virtio_i2c_out_hdr.flags is used to instead of i2c_msg.flags for extension. So by default we don't support any of the existing flags except I2C_M_RD? Yes. That's the current status. #define I2C_M_TEN 0x0010 /* this is a ten bit chip address */ #define I2C_M_RD0x0001 /* read data, from slave to master */ #define I2C_M_STOP 0x8000 /* if I2C_FUNC_PROTOCOL_MANGLING */ #define I2C_M_NOSTART 0x4000 /* if I2C_FUNC_NOSTART */ #define I2C_M_REV_DIR_ADDR 0x2000 /* if I2C_FUNC_PROTOCOL_MANGLING */ #define I2C_M_IGNORE_NAK0x1000 /* if I2C_FUNC_PROTOCOL_MANGLING */ #define I2C_M_NO_RD_ACK 0x0800 /* if I2C_FUNC_PROTOCOL_MANGLING */ #define I2C_M_RECV_LEN 0x0400 /* length will be first received byte */ How do we work with clients who want to use such flags now ? My plan is to have a minimum driver get merged. Then we have a base and we can update virtio_i2c_out_hdr.flags for the feature extensibility. Then, If you want to help to develop this stuff, you can just follow the same flow. First, you can update the Spec by sending comments to virtio-comm...@lists.oasis-open.org. Once your Spec patch is acked by the virtio TC, you can then send patches to update the corresponding drivers. Thanks. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On Tue, Mar 02, 2021 at 11:54:02AM +0100, Arnd Bergmann wrote: > On Tue, Mar 2, 2021 at 10:51 AM Stefan Hajnoczi wrote: > > On Tue, Mar 02, 2021 at 10:42:06AM +0800, Jie Deng wrote: > > > > > +/* > > > > > + * 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 > > > > Why is this a uapi header? Can't this all be moved into the driver > > > > itself? > > > > Linux VIRTIO drivers provide a uapi header with structs and constants > > that describe the device interface. This allows other software like > > QEMU, other operating systems, etc to reuse these headers instead of > > redefining everything. > > > > These files should contain: > > 1. Device-specific feature bits (VIRTIO__F_) > > 2. VIRTIO Configuration Space layout (struct virtio__config) > > 3. Virtqueue request layout (struct virtio__) > > > > For examples, see and . > > Ok, makes sense. So it's not strictly uapi but just helpful for > virtio applications to reference these. I suppose it is slightly harder > when building qemu on other operating systems though, how does > it get these headers on BSD or Windows? qemu.git imports Linux headers from time to time and can use them instead of system headers: https://gitlab.com/qemu-project/qemu/-/blob/master/scripts/update-linux-headers.sh Stefan signature.asc Description: PGP signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On 2021/3/3 15:54, Viresh Kumar wrote:
On 01-03-21, 14:41, Jie Deng wrote:
diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
+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;
+
+ 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;
+
+ if (msgs[i].flags & I2C_M_RD) {
+ reqs[i].read_buf = buf;
+ sg_init_one(_buf, reqs[i].read_buf, msgs[i].len);
+ sgs[outcnt + incnt++] = _buf;
+ } else {
+ reqs[i].write_buf = buf;
+ memcpy(reqs[i].write_buf, msgs[i].buf, msgs[i].len);
+ sg_init_one(_buf, reqs[i].write_buf, msgs[i].len);
+ sgs[outcnt++] = _buf;
+ }
+
+ sg_init_one(_hdr, [i].in_hdr, sizeof(reqs[i].in_hdr));
+ sgs[outcnt + incnt++] = _hdr;
+
+ err = virtqueue_add_sgs(vq, sgs, outcnt, incnt, [i],
GFP_KERNEL);
+ if (err < 0) {
+ pr_err("failed to add msg[%d] to virtqueue.\n", i);
+ if (msgs[i].flags & I2C_M_RD) {
+ kfree(reqs[i].read_buf);
+ reqs[i].read_buf = NULL;
+ } else {
+ kfree(reqs[i].write_buf);
+ reqs[i].write_buf = NULL;
+ }
+ break;
+ }
+ }
+
+ return i;
+}
diff --git a/include/uapi/linux/virtio_i2c.h b/include/uapi/linux/virtio_i2c.h
+/**
+ * struct virtio_i2c_out_hdr - the virtio I2C message OUT header
+ * @addr: the controlled device address
+ * @padding: used to pad to full dword
+ * @flags: used for feature extensibility
+ */
+struct virtio_i2c_out_hdr {
+ __le16 addr;
+ __le16 padding;
+ __le32 flags;
+};
Both this code and the virtio spec (which is already merged) are
missing msgs[i].flags and they are never sent to backend. The only
flags available here are the ones defined by virtio spec and these are
not i2c flags.
I also looked at your i2c backend for acrn and it mistakenly copies
the hdr.flag, which is the virtio flag and not i2c flag.
https://github.com/projectacrn/acrn-hypervisor/blob/master/devicemodel/hw/pci/virtio/virtio_i2c.c#L539
I will send a fix for the specs if you agree that there is a problem
here.
what am I missing here ? This should have been caught in your testing
and so I feel I must be missing something.
This is not a problem. My original proposal was to mirror the struct
i2c_msg.
The code you looked at was based on that.
However, the virtio TC prefer not to mirror it. They have some concerns.
For example, there is a bit I2C_M_RD in i2c_msg.flag which has the same
meaning with
the R/W in virtio descriptor. This is a repetition which may cause problems.
So the virtio_i2c_out_hdr.flags is used to instead of i2c_msg.flags for
extension.
You can check this link
https://github.com/oasis-tcs/virtio-spec/issues/88 to learn the whole story.
There are discussions about the spec (v1 ~ v7).
I'm updating these drivers step by step to make sure they finally follow
the spec.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On Tue, Mar 2, 2021 at 10:51 AM Stefan Hajnoczi wrote: > On Tue, Mar 02, 2021 at 10:42:06AM +0800, Jie Deng wrote: > > > > +/* > > > > + * 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 > > > Why is this a uapi header? Can't this all be moved into the driver > > > itself? > > Linux VIRTIO drivers provide a uapi header with structs and constants > that describe the device interface. This allows other software like > QEMU, other operating systems, etc to reuse these headers instead of > redefining everything. > > These files should contain: > 1. Device-specific feature bits (VIRTIO__F_) > 2. VIRTIO Configuration Space layout (struct virtio__config) > 3. Virtqueue request layout (struct virtio__) > > For examples, see and . Ok, makes sense. So it's not strictly uapi but just helpful for virtio applications to reference these. I suppose it is slightly harder when building qemu on other operating systems though, how does it get these headers on BSD or Windows? Arnd ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On Tue, Mar 02, 2021 at 10:42:06AM +0800, Jie Deng wrote:
>
> On 2021/3/1 23:19, Arnd Bergmann wrote:
> > On Mon, Mar 1, 2021 at 7:41 AM Jie Deng wrote:
> >
> > > --- /dev/null
> > > +++ b/include/uapi/linux/virtio_i2c.h
> > > @@ -0,0 +1,56 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-or-later WITH Linux-syscall-note */
The uapi VIRTIO header files are BSD licensed so they can be easily used
by other projects (including other operating systems and hypervisors
that don't use Linux). Please see the license headers in
or .
> > > +/*
> > > + * 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
> > Why is this a uapi header? Can't this all be moved into the driver
> > itself?
Linux VIRTIO drivers provide a uapi header with structs and constants
that describe the device interface. This allows other software like
QEMU, other operating systems, etc to reuse these headers instead of
redefining everything.
These files should contain:
1. Device-specific feature bits (VIRTIO__F_)
2. VIRTIO Configuration Space layout (struct virtio__config)
3. Virtqueue request layout (struct virtio__)
For examples, see and .
> > > +/**
> > > + * struct virtio_i2c_req - the virtio I2C request structure
> > > + * @out_hdr: the OUT header of the virtio I2C message
> > > + * @write_buf: contains one I2C segment being written to the device
> > > + * @read_buf: contains one I2C segment being read from the device
> > > + * @in_hdr: the IN header of the virtio I2C message
> > > + */
> > > +struct virtio_i2c_req {
> > > + struct virtio_i2c_out_hdr out_hdr;
> > > + u8 *write_buf;
> > > + u8 *read_buf;
> > > + struct virtio_i2c_in_hdr in_hdr;
> > > +};
> > In particular, this structure looks like it is only ever usable between
> > the transfer functions in the driver itself, it is shared with neither
> > user space nor the virtio host side.
I agree. This struct is not part of the device interface. It's part of
the Linux driver implementation. This belongs inside the driver code and
not in include/uapi/ where public headers are located.
Stefan
signature.asc
Description: PGP signature
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On 2021/3/2 15:24, Viresh Kumar wrote: On 02-03-21, 14:24, Jie Deng wrote: Not for the full duplex. As Paolo explained in those links. We defined a combined request called "write-read request" " This is when a write is followed by a read: the master starts off the transmission with a write, then sends a second START, then continues with a read from the same address. I think above talks about the real I2C protocol at bus level ? In theory there's no difference between one multi-segment transaction and many single-segment transactions _in a single-master scenario_. However, it is a plausible configuration to have multiple guests sharing an I2C host device as if they were multiple master. So the spec should provide a way at least to support for transactions with 1 write and 1 read segment in one request to the same address. " From the perspective of specification design, it hopes to provide more choices while from the perspective of specific implementation, we can choose what we need as long as it does not violate the specification. That is fine, but what I was not able to understand was how do we get to know if we should expect both write and read bufs after the out header or only one of them ? I think I have understood that part now, we need to look at incnt and outcnt and make out what kind of transfer we need to do. - If outcnt == 1 and incnt == 2, then it is read operation. - If outcnt == 2 and incnt == 1, then it is write operation. - If outcnt == 2 and incnt == 2, then it is read-write operation. Anything else is invalid. Is my understanding correct here ? Correct. By checking the sequences of descriptor's R/W in the virtqueue, You can know the type of request. A simple state machine can be used to classify in this case. O I I : read request. O O I : write request. O O I I : read-write request. But if only using the first two types like in this driver, the backend will be much easier to implement since the request is fixed to 3 descriptors and we only need to check the second descriptor to know the type. Since the current Linux driver doesn't use this mechanism. I'm considering to move the "struct virtio_i2c_req" into the driver and use one "buf" instead. Linux can very much have its own definition of the structure and so I am not in favor of any such structure in the spec as well, it confuses people (like me) :). ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On 2021/3/1 20:07, Andy Shevchenko wrote:
On Mon, Mar 01, 2021 at 02:41:35PM +0800, Jie Deng wrote:
Add an I2C bus driver for virtio para-virtualization.
The controller can be emulated by the backend driver in
any device model software by following the virtio protocol.
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.
...
+ buf = kzalloc(msgs[i].len, GFP_KERNEL);
+ if (!buf)
+ break;
+
+ if (msgs[i].flags & I2C_M_RD) {
kzalloc()
+ reqs[i].read_buf = buf;
+ sg_init_one(_buf, reqs[i].read_buf, msgs[i].len);
+ sgs[outcnt + incnt++] = _buf;
+ } else {
+ reqs[i].write_buf = buf;
+ memcpy(reqs[i].write_buf, msgs[i].buf, msgs[i].len);
kmemdup() ?
Do you mean using "kzalloc" in the if condition and "kmemdup" in the
else condition ?
Then we have to check the NULL twice which is also not good.
+ sg_init_one(_buf, reqs[i].write_buf, msgs[i].len);
+ sgs[outcnt++] = _buf;
+ }
...
+
+
One blank line is enough.
Will fix it. Thank you.
...
+ ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
+ if (ret == 0)
+ goto err_unlock_free;
+ else
Redundant.
Good catch !
+ nr = ret;
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On 2021/3/2 11:43, Viresh Kumar wrote: On 02-03-21, 10:21, Jie Deng wrote: On 2021/3/1 19:54, Viresh Kumar wrote: That's my original proposal. I used to mirror this interface with "struct i2c_msg". But the design philosophy of virtio TC is that VIRTIO devices are not specific to Linux so the specs design should avoid the limitations of the current Linux driver behavior. Right, I understand that. We had some discussion about this. You may check these links to learn the story. https://lists.oasis-open.org/archives/virtio-comment/202010/msg00016.html https://lists.oasis-open.org/archives/virtio-comment/202010/msg00033.html https://lists.oasis-open.org/archives/virtio-comment/202011/msg00025.html So the thing is that we want to support full duplex mode, right ? How will that work protocol wise ? I mean how would we know if we are expecting both tx and rx buffers in a transfer ? Not for the full duplex. As Paolo explained in those links. We defined a combined request called "write-read request" " This is when a write is followed by a read: the master starts off the transmission with a write, then sends a second START, then continues with a read from the same address. In theory there's no difference between one multi-segment transaction and many single-segment transactions _in a single-master scenario_. However, it is a plausible configuration to have multiple guests sharing an I2C host device as if they were multiple master. So the spec should provide a way at least to support for transactions with 1 write and 1 read segment in one request to the same address. " From the perspective of specification design, it hopes to provide more choices while from the perspective of specific implementation, we can choose what we need as long as it does not violate the specification. Since the current Linux driver doesn't use this mechanism. I'm considering to move the "struct virtio_i2c_req" into the driver and use one "buf" instead. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On 2021/3/2 11:43, Viresh Kumar wrote: On 02-03-21, 10:21, Jie Deng wrote: On 2021/3/1 19:54, Viresh Kumar wrote: That's my original proposal. I used to mirror this interface with "struct i2c_msg". But the design philosophy of virtio TC is that VIRTIO devices are not specific to Linux so the specs design should avoid the limitations of the current Linux driver behavior. Right, I understand that. We had some discussion about this. You may check these links to learn the story. https://lists.oasis-open.org/archives/virtio-comment/202010/msg00016.html https://lists.oasis-open.org/archives/virtio-comment/202010/msg00033.html https://lists.oasis-open.org/archives/virtio-comment/202011/msg00025.html So the thing is that we want to support full duplex mode, right ? How will that work protocol wise ? I mean how would we know if we are expecting both tx and rx buffers in a transfer ? Not for the full duplex. As Paolo explained in those links. We defined a combined request called "write-read request" " This is when a write is followed by a read: the master starts off the transmission with a write, then sends a second START, then continues with a read from the same address. In theory there's no difference between one multi-segment transaction and many single-segment transactions _in a single-master scenario_. However, it is a plausible configuration to have multiple guests sharing an I2C host device as if they were multiple master. So the spec should provide a way at least to support for transactions with 1 write and 1 read segment in one request to the same address. " From the perspective of specification design, it hopes to provide more choices while from the perspective of specific implementation, we can choose what we need as long as it does not violate the specification. Since the current Linux driver doesn't use this mechanism. I'm considering to move the "struct virtio_i2c_req" into the driver and use one "buf" instead. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On 2021/3/2 1:16 下午, Viresh Kumar wrote: On 02-03-21, 13:06, Jie Deng wrote: Yeah. Actually, the backend only needs "struct virtio_i2c_out_hdr out_hdr" and "struct virtio_i2c_in_hdr in_hdr" for communication. So we only need to keep the first two in uapi and move "struct virtio_i2c_req" into the driver. But Jason wanted to include "struct virtio_i2c_req" in uapi. He explained in this link https://lists.linuxfoundation.org/pipermail/virtualization/2020-October/050222.html. Do you agree with that explanation ? I am not sure I understood his reasoning well, but it doesn't make any sense to keep this in uapi header if this is never going to get transferred over the wire. I think I was wrong. It should be sufficient have in_hdr and out_hdr in uAPI. Thanks Moreover, the struct virtio_i2c_req in spec is misleading to me and rather creates unnecessary confusion. There is no structure like this which ever get passed here, but rather there are multiple vq transactions which take place, one with just the out header, then one with buffer and finally one with in header. I am not sure what's the right way of documenting it or if this is a standard virtio world follows. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On 2021/3/2 12:42, Viresh Kumar wrote:
On 01-03-21, 14:41, Jie Deng wrote:
+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;
+
+ reqs[i].out_hdr.addr = cpu_to_le16(msgs[i].addr << 1);
And this won't work for 10 bit addressing right? Don't we support that
in kernel ?
From Spec:
\begin{tabular}{ |l||l|l|l|l|l|l|l|l|l|l|l|l|l|l|l|l| }
\hline
Bits & 15 & 14 & 13 & 12 & 11 & 10 & 9 & 8 & 7 & 6 & 5 & 4 & 3 & 2
& 1 & 0 \\
\hline
7-bit address & 0 & 0 & 0 & 0 & 0 & 0 & 0 & 0 & A6 & A5 & A4 & A3 & A2 & A1
& A0 & 0 \\
\hline
10-bit address & A7 & A6 & A5 & A4 & A3 & A2 & A1 & A0 & 1 & 1 & 1 & 1 & 0 & A9
& A8 & 0 \\
\hline
\end{tabular}
Currently, to make things simple, this driver only supports 7 bit mode.
It doesn't declare "I2C_FUNC_10BIT_ADDR" in the functionality.
We may add in the future according to the need.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On 2021/3/2 12:22, Viresh Kumar wrote:
On 02-03-21, 09:31, Viresh Kumar wrote:
On 01-03-21, 16:19, Arnd Bergmann wrote:
On Mon, Mar 1, 2021 at 7:41 AM Jie Deng wrote:
--- /dev/null
+++ b/include/uapi/linux/virtio_i2c.h
@@ -0,0 +1,56 @@
+/* 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
Why is this a uapi header? Can't this all be moved into the driver
itself?
+/**
+ * struct virtio_i2c_req - the virtio I2C request structure
+ * @out_hdr: the OUT header of the virtio I2C message
+ * @write_buf: contains one I2C segment being written to the device
+ * @read_buf: contains one I2C segment being read from the device
+ * @in_hdr: the IN header of the virtio I2C message
+ */
+struct virtio_i2c_req {
+ struct virtio_i2c_out_hdr out_hdr;
+ u8 *write_buf;
+ u8 *read_buf;
+ struct virtio_i2c_in_hdr in_hdr;
+};
In particular, this structure looks like it is only ever usable between
the transfer functions in the driver itself, it is shared with neither
user space nor the virtio host side.
Why is it so ? Won't you expect hypervisors or userspace apps to use
these ?
This comment applies only for the first two structures as the third
one is never exchanged over virtio.
Yeah. Actually, the backend only needs "struct virtio_i2c_out_hdr out_hdr"
and "struct virtio_i2c_in_hdr in_hdr" for communication. So we only need
to keep
the first two in uapi and move "struct virtio_i2c_req" into the driver.
But Jason wanted to include "struct virtio_i2c_req" in uapi. He
explained in this link
https://lists.linuxfoundation.org/pipermail/virtualization/2020-October/050222.html.
Do you agree with that explanation ?
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On 2021/3/1 23:19, Arnd Bergmann wrote:
On Mon, Mar 1, 2021 at 7:41 AM Jie Deng wrote:
--- /dev/null
+++ b/include/uapi/linux/virtio_i2c.h
@@ -0,0 +1,56 @@
+/* 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
Why is this a uapi header? Can't this all be moved into the driver
itself?
+/**
+ * struct virtio_i2c_req - the virtio I2C request structure
+ * @out_hdr: the OUT header of the virtio I2C message
+ * @write_buf: contains one I2C segment being written to the device
+ * @read_buf: contains one I2C segment being read from the device
+ * @in_hdr: the IN header of the virtio I2C message
+ */
+struct virtio_i2c_req {
+ struct virtio_i2c_out_hdr out_hdr;
+ u8 *write_buf;
+ u8 *read_buf;
+ struct virtio_i2c_in_hdr in_hdr;
+};
In particular, this structure looks like it is only ever usable between
the transfer functions in the driver itself, it is shared with neither
user space nor the virtio host side.
Arnd
Please check this link.
https://lists.linuxfoundation.org/pipermail/virtualization/2020-October/050222.html
Jason/Stefan, could you please double confirm about the following ?
**
/I think following definition in uAPI for the status is enough. />/There is no need to provide a "u8" status in the structure. />//>//* The final status written by the device */ />/#define VIRTIO_I2C_MSG_OK 0 />/#define VIRTIO_I2C_MSG_ERR 1 />//>/You can see an example in virtio_blk. />//>/In the spec: />//>/struct virtio_blk_req { />/le32 type; />/le32 reserved; />/le64 sector; />/u8 data[]; />/u8 status; />/};
/>//>/In virtio_blk.h, there is only following definitions. />//>/#define VIRTIO_BLK_S_OK 0 />/#define VIRTIO_BLK_S_IOERR 1 />/#define VIRTIO_BLK_S_UNSUPP 2 />/////virtio-blk is a bad example, it's just too late to fix. For any new //introduced uAPI it should be a complete one. ////Thanks ///>>>/I checked a relatively new device "virtio_fs". />>>/I found following definition in spec but not in uAPI also. />>>//>>>/struct virtio_fs_req { />>>/// Device -readable part
/>>>/struct fuse_in_header in; />>>/u8 datain[]; />>>/// Device -writable part />>>/struct fuse_out_header out; />>>/u8 dataout[]; />>>/}; />>>//>>>/So is this also a bad example which has not been fixed yet. />>//>>//>>/Cc Stefan for the answer. />>//>>//>>>/Or what's your mean about "complete" here ? Is there any definition />>>/about "complete uAPI" ? />>//>>//>>/My understanding it should contain all the fields defined in the />>/virtio spec. />>//>>/Thanks />>//>/OK. I noticed this isn't strictly implemented in the current virtio />/codes. />/I'm not sure if this is
already a consensus. I will follow it if this />/is the opinion of the majority. /
Please do that, this forces us to maintain uABI compatibility which is
what Linux try to maintain for ever.
**
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On 2021/3/1 19:54, Viresh Kumar wrote:
On 01-03-21, 14:41, Jie Deng wrote:
+/**
+ * struct virtio_i2c_req - the virtio I2C request structure
+ * @out_hdr: the OUT header of the virtio I2C message
+ * @write_buf: contains one I2C segment being written to the device
+ * @read_buf: contains one I2C segment being read from the device
+ * @in_hdr: the IN header of the virtio I2C message
+ */
+struct virtio_i2c_req {
+ struct virtio_i2c_out_hdr out_hdr;
+ u8 *write_buf;
+ u8 *read_buf;
+ struct virtio_i2c_in_hdr in_hdr;
+};
I am not able to appreciate the use of write/read bufs here as we
aren't trying to read/write data in the same transaction. Why do we
have two bufs here as well as in specs ?
What about this on top of your patch ?
---
drivers/i2c/busses/i2c-virtio.c | 31 +++
include/uapi/linux/virtio_i2c.h | 3 +--
2 files changed, 12 insertions(+), 22 deletions(-)
diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
index 8c8bc9545418..e71ab1d2c83f 100644
--- a/drivers/i2c/busses/i2c-virtio.c
+++ b/drivers/i2c/busses/i2c-virtio.c
@@ -67,14 +67,13 @@ static int virtio_i2c_send_reqs(struct virtqueue *vq,
if (!buf)
break;
+ reqs[i].buf = buf;
+ sg_init_one(_buf, reqs[i].buf, msgs[i].len);
+
if (msgs[i].flags & I2C_M_RD) {
- reqs[i].read_buf = buf;
- sg_init_one(_buf, reqs[i].read_buf, msgs[i].len);
sgs[outcnt + incnt++] = _buf;
} else {
- reqs[i].write_buf = buf;
- memcpy(reqs[i].write_buf, msgs[i].buf, msgs[i].len);
- sg_init_one(_buf, reqs[i].write_buf, msgs[i].len);
+ memcpy(reqs[i].buf, msgs[i].buf, msgs[i].len);
sgs[outcnt++] = _buf;
}
@@ -84,13 +83,8 @@ static int virtio_i2c_send_reqs(struct virtqueue *vq,
err = virtqueue_add_sgs(vq, sgs, outcnt, incnt, [i],
GFP_KERNEL);
if (err < 0) {
pr_err("failed to add msg[%d] to virtqueue.\n", i);
- if (msgs[i].flags & I2C_M_RD) {
- kfree(reqs[i].read_buf);
- reqs[i].read_buf = NULL;
- } else {
- kfree(reqs[i].write_buf);
- reqs[i].write_buf = NULL;
- }
+ kfree(reqs[i].buf);
+ reqs[i].buf = NULL;
break;
}
}
@@ -118,14 +112,11 @@ static int virtio_i2c_complete_reqs(struct virtqueue *vq,
break;
}
- if (msgs[i].flags & I2C_M_RD) {
- memcpy(msgs[i].buf, req->read_buf, msgs[i].len);
- kfree(req->read_buf);
- req->read_buf = NULL;
- } else {
- kfree(req->write_buf);
- req->write_buf = NULL;
- }
+ if (msgs[i].flags & I2C_M_RD)
+ memcpy(msgs[i].buf, req->buf, msgs[i].len);
+
+ kfree(req->buf);
+ req->buf = NULL;
}
return i;
diff --git a/include/uapi/linux/virtio_i2c.h b/include/uapi/linux/virtio_i2c.h
index 92febf0c527e..61f0086ac75b 100644
--- a/include/uapi/linux/virtio_i2c.h
+++ b/include/uapi/linux/virtio_i2c.h
@@ -48,8 +48,7 @@ struct virtio_i2c_in_hdr {
*/
struct virtio_i2c_req {
struct virtio_i2c_out_hdr out_hdr;
- u8 *write_buf;
- u8 *read_buf;
+ u8 *buf;
struct virtio_i2c_in_hdr in_hdr;
};
That's my original proposal. I used to mirror this interface with
"struct i2c_msg".
But the design philosophy of virtio TC is that VIRTIO devices are not
specific to Linux
so the specs design should avoid the limitations of the current Linux
driver behavior.
We had some discussion about this. You may check these links to learn
the story.
https://lists.oasis-open.org/archives/virtio-comment/202010/msg00016.html
https://lists.oasis-open.org/archives/virtio-comment/202010/msg00033.html
https://lists.oasis-open.org/archives/virtio-comment/202011/msg00025.html
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On Mon, Mar 1, 2021 at 1:10 PM Andy Shevchenko
wrote:
> On Mon, Mar 01, 2021 at 02:09:25PM +0200, Andy Shevchenko wrote:
> > On Mon, Mar 01, 2021 at 05:24:41PM +0530, Viresh Kumar wrote:
> > > On 01-03-21, 14:41, Jie Deng wrote:
> > > > +/**
> > > > + * struct virtio_i2c_req - the virtio I2C request structure
> > > > + * @out_hdr: the OUT header of the virtio I2C message
> > > > + * @write_buf: contains one I2C segment being written to the device
> > > > + * @read_buf: contains one I2C segment being read from the device
> > > > + * @in_hdr: the IN header of the virtio I2C message
> > > > + */
> > > > +struct virtio_i2c_req {
> > > > + struct virtio_i2c_out_hdr out_hdr;
> > > > + u8 *write_buf;
> > > > + u8 *read_buf;
> > > > + struct virtio_i2c_in_hdr in_hdr;
> > > > +};
> > >
> > > I am not able to appreciate the use of write/read bufs here as we
> > > aren't trying to read/write data in the same transaction. Why do we
> > > have two bufs here as well as in specs ?
> >
> > I涎 and SMBus support bidirectional transfers as well. I think two buffers is
> > the right thing to do.
>
> Strictly speaking "half duplex".
But the driver does not support this at all: the sglist always has three
members as Viresh says: outhdr, msgbuf and inhdr. It then uses a
bounce buffer for the actual data transfer, and this always goes either
one way or the other.
I think the more important question is: does this driver actually need
the bounce buffer? It doesn't have to worry about adjacent stack
data being clobbered by cache maintenance because virtio is always
cache coherent and, so I suspect the bounce buffer can be left out.
It might actually be simpler to just have a fixed-length array of
headers on the stack and at most the corresponding number of
transfers for one virtqueue_kick().
Is there a reasonable limit on how many transfers we would
expect to handle at once? I see that most callers of i2c_transfer()
hardcode the number to '1' or '2', rarely '3' or '4', while the proposed
implementation seems to be optimized for much larger numbers.
Arnd
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On Mon, Mar 1, 2021 at 7:41 AM Jie Deng wrote:
> --- /dev/null
> +++ b/include/uapi/linux/virtio_i2c.h
> @@ -0,0 +1,56 @@
> +/* 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
Why is this a uapi header? Can't this all be moved into the driver
itself?
> +/**
> + * struct virtio_i2c_req - the virtio I2C request structure
> + * @out_hdr: the OUT header of the virtio I2C message
> + * @write_buf: contains one I2C segment being written to the device
> + * @read_buf: contains one I2C segment being read from the device
> + * @in_hdr: the IN header of the virtio I2C message
> + */
> +struct virtio_i2c_req {
> + struct virtio_i2c_out_hdr out_hdr;
> + u8 *write_buf;
> + u8 *read_buf;
> + struct virtio_i2c_in_hdr in_hdr;
> +};
In particular, this structure looks like it is only ever usable between
the transfer functions in the driver itself, it is shared with neither
user space nor the virtio host side.
Arnd
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On Mon, Mar 01, 2021 at 05:24:41PM +0530, Viresh Kumar wrote:
> On 01-03-21, 14:41, Jie Deng wrote:
> > +/**
> > + * struct virtio_i2c_req - the virtio I2C request structure
> > + * @out_hdr: the OUT header of the virtio I2C message
> > + * @write_buf: contains one I2C segment being written to the device
> > + * @read_buf: contains one I2C segment being read from the device
> > + * @in_hdr: the IN header of the virtio I2C message
> > + */
> > +struct virtio_i2c_req {
> > + struct virtio_i2c_out_hdr out_hdr;
> > + u8 *write_buf;
> > + u8 *read_buf;
> > + struct virtio_i2c_in_hdr in_hdr;
> > +};
>
> I am not able to appreciate the use of write/read bufs here as we
> aren't trying to read/write data in the same transaction. Why do we
> have two bufs here as well as in specs ?
I²C and SMBus support bidirectional transfers as well. I think two buffers is
the right thing to do.
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On Mon, Mar 01, 2021 at 02:41:35PM +0800, Jie Deng wrote:
> Add an I2C bus driver for virtio para-virtualization.
>
> The controller can be emulated by the backend driver in
> any device model software by following the virtio protocol.
>
> 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.
...
> + buf = kzalloc(msgs[i].len, GFP_KERNEL);
> + if (!buf)
> + break;
> +
> + if (msgs[i].flags & I2C_M_RD) {
kzalloc()
> + reqs[i].read_buf = buf;
> + sg_init_one(_buf, reqs[i].read_buf, msgs[i].len);
> + sgs[outcnt + incnt++] = _buf;
> + } else {
> + reqs[i].write_buf = buf;
> + memcpy(reqs[i].write_buf, msgs[i].buf, msgs[i].len);
kmemdup() ?
> + sg_init_one(_buf, reqs[i].write_buf, msgs[i].len);
> + sgs[outcnt++] = _buf;
> + }
...
> +
> +
One blank line is enough.
...
> + ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
> + if (ret == 0)
> + goto err_unlock_free;
> + else
Redundant.
> + nr = ret;
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On Mon, Mar 01, 2021 at 02:09:25PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 01, 2021 at 05:24:41PM +0530, Viresh Kumar wrote:
> > On 01-03-21, 14:41, Jie Deng wrote:
> > > +/**
> > > + * struct virtio_i2c_req - the virtio I2C request structure
> > > + * @out_hdr: the OUT header of the virtio I2C message
> > > + * @write_buf: contains one I2C segment being written to the device
> > > + * @read_buf: contains one I2C segment being read from the device
> > > + * @in_hdr: the IN header of the virtio I2C message
> > > + */
> > > +struct virtio_i2c_req {
> > > + struct virtio_i2c_out_hdr out_hdr;
> > > + u8 *write_buf;
> > > + u8 *read_buf;
> > > + struct virtio_i2c_in_hdr in_hdr;
> > > +};
> >
> > I am not able to appreciate the use of write/read bufs here as we
> > aren't trying to read/write data in the same transaction. Why do we
> > have two bufs here as well as in specs ?
>
> I²C and SMBus support bidirectional transfers as well. I think two buffers is
> the right thing to do.
Strictly speaking "half duplex".
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
