On 2021/3/1 23:19, Arnd Bergmann wrote:> On Mon, Mar 1, 2021 at 7:41 AM Jie Deng <jie.deng at intel.com> 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. > > ArndPlease 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. ****************************************************************************** -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20210302/a00bcd34/attachment.html>
Stefan Hajnoczi
2021-Mar-02 09:51 UTC
[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 <jie.deng at intel.com> 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 <linux/virtio_net.h> or <linux/virtio_blk.h>.> > > +/* > > > + * 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_<device>_F_<feature>) 2. VIRTIO Configuration Space layout (struct virtio_<device>_config) 3. Virtqueue request layout (struct virtio_<device>_<req/resp>) For examples, see <linux/virtio_net.h> and <linux/virtio_blk.h>.> > > +/** > > > + * 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 -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 488 bytes Desc: not available URL: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20210302/f801cc42/attachment.sig>