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.
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
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.
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
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? #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 ? -- viresh
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
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. -- viresh
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
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
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) :).
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
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 ? > 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) :). -- viresh
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;
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.
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.
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.
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On 02-03-21, 13:21, Jie Deng wrote: > > 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. Okay, fair enough. Please add such details somewhere in the code so people can understand it. You can very well add these at the top of the file as well, in the general header. -- viresh
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
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. 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. -- viresh
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 ?
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
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} -- viresh
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
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. -- viresh
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
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 ? -- viresh
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On 01-03-21, 16:46, Arnd Bergmann wrote: > 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. Yes and if the driver doesn't support the specification fully then it should say so in a comment at least and also fail in case someone tries a full duplex transfer.. -- viresh
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On 01-03-21, 14:10, 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²C and SMBus support bidirectional transfers as well. I think two buffers > > is > > the right thing to do. > > Strictly speaking "half duplex". Half duplex is what this driver implemented, i.e. only one side can send a msg at once, we don't need two buffers for that for sure. Though we need two buffers if we are ever going to support full duplex. -- viresh
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
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 ? -- viresh
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
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
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
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
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
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
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
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; }; -- viresh
[PATCH v5] 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 | 281 include/uapi/linux/virtio_i2c.h | 56 include/uapi/linux/virtio_ids.h | 1 + 5 files changed, 352 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..8c8bc95 --- /dev/null +++ b/drivers/i2c/busses/i2c-virtio.c @@ -0,0 +1,281 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Virtio I2C Bus Driver + * + * 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; +}; + +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; + + 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