Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver

[Jie Deng <jie.deng@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3480 bytes --]


On 2021/3/1 23:19, Arnd Bergmann wrote:
> On Mon, Mar 1, 2021 at 7:41 AM Jie Deng <jie.deng@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.
>
>         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.

******************************************************************************


[-- Attachment #1.2: Type: text/html, Size: 5932 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization