[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-comment] [PATCH v5] virtio-i2c: add the device specification
On Tue, 22 Dec 2020 14:11:24 +0800 Jie Deng <jie.deng@intel.com> wrote: > On 2020/12/20 3:05, Michael S. Tsirkin wrote: > > On Fri, Dec 18, 2020 at 10:06:45AM +0800, Jie Deng wrote: > >> On 2020/12/17 18:26, Stefan Hajnoczi wrote: > >>> On Thu, Dec 17, 2020 at 03:00:55AM -0500, Michael S. Tsirkin wrote: > >>>> On Thu, Dec 17, 2020 at 03:08:07PM +0800, Jie Deng wrote: > >>>>> +The \field{flags} of the request is currently reserved as zero for future > >>>>> +feature extensibility. > >>>>> + > >>>>> +The \field{written} of the request is the number of data bytes in the \field{write_buf} > >>>>> +being written to the I2C slave address. > >>>>> > >>>>> This field seems redundant since the device can determine the size of > >>>>> write_buf implicitly from the total out buffer size. virtio-blk takes > >>>>> this approach. > >>>>> > >>>>> The read/write are the actual number of data bytes being read from or written > >>>>> to the device > >>>>> which is not determined by the device. So I don't think it is redundant. > >>>> I am still not sure I understand the difference. > >>>> This point is unclear to multiple people. > >>> I think I get it now. This is made clear by splitting the struct: > >>> > >>> /* Driver->device fields */ > >>> struct virtio_i2c_out_hdr > >>> { > >>> le16 addr; > >>> le16 padding; > >>> le32 flags; > >>> }; > >>> > >>> /* Device->driver fields */ > >>> struct virtio_i2c_in_hdr > >>> { > >>> le16 written; > >>> le16 read; > >>> u8 status; > >>> }; > >> written/read are not device->driver fields. They are driver->device fields. > >> They are not determined by the device but the driver(user). > >> > >> However, Michael said that the two fields may duplicate buf size available > >> in the descriptor. He intended to remove them. > >> > >> " > >> I note that read and written actually duplicate buf size > >> available in the descriptor. > >> Given we no longer mirror i2c_msg 1:1 do we still want to do this? > >> It will be trivial for the host device to populate these fields > >> correctly for linux. > >> Duplication of information iten leads to errors ... > >> " > >> > >> But there is a corner case I'm not sure if you have noticed. > >> > >> read and written can be 0. I think we may not put a buf with size 0 into the > >> virtqueue. > > You always have the header and the status, right? > > E.g. with the below, the total buffer size is virtio_i2c_out_hdr size + > > write size for writes and read size + virtio_i2c_in_hdr size for reads. > > Neither result is ever 0. > > Then how to distinguish the request type the buffer contains. I have read through the thread and I remain confused. > > Each type will have both virtio_i2c_out_hdr and virtio_i2c_in_hdr. > the backend can know the type by checking the read/written. > > If the read=0 and the written>0, the request is a write request > The buffer may contains 3 scatterlist: > > virtio_i2c_out_hdr // scatterlist[0] So, what does virtio_i2c_{out,in}_hdr contain here? If it is the one from above, ... > > ÂÂÂ buf[/* this is write data, since read = 0 */] // scatterlist[1] > > ÂÂÂ virtio_i2c_in_hdr // scatterlist[2] ...we do not know whether there's read data, write data, or what their length is, until we've actually consumed the whole buffer, and then we have to go backwards. > > If the read>0 and the written=0, the request is a read request. > The buffer may contains 3 scatterlist: > > virtio_i2c_out_hdr // scatterlist[0] > > ÂÂÂ buf[/* This is read data, since written = 0 */] // scatterlist[1] > > ÂÂÂ virtio_i2c_in_hdr // scatterlist[2] > > If the read>0 and the written>0, the request is a write-read request. > The buffer may contains 4 scatterlist: > > virtio_i2c_out_hdrÂÂ // scatterlist[0] > > ÂÂÂ buf[/*write data*/]Â // scatterlist[1] > > ÂÂÂ buf[/*read data*/] // scatterlist[2] > > ÂÂÂ virtio_i2c_in_hdr // scatterlist[3] Is there any reason why we need to infer the type of the request by checking some lengths? Can't we just specify explicit flags for read and write? What am I missing? > > >> @Stefan @Paolo > >> > >> So what's your opinion about these two fields ? > >> > >>> /* > >>> * Virtqueue element layout looks like this: > >>> * > >>> * struct virtio_i2c_out_hdr out_hdr; /* OUT */ > >>> * u8 write_buf[]; /* OUT */ > >>> * u8 read_buf[]; /* IN */ > >>> * struct virtio_i2c_in_hdr in_hdr; /* IN */ > >>> */ > >>> > >>> This makes sense to me: a bi-directional request has both write_buf[] > >>> and read_buf[] so the vring used.len field is not enough to report back > >>> how many bytes were written and read. The virtio_i2c_in_hdr fields are > >>> really needed. > >>> > >>> Please split the struct in the spec so it's clear how this works.
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]