OASIS Mailing List ArchivesView the OASIS mailing list archive below
or browse/search using MarkMail.

 


Help: OASIS Mailing Lists Help | MarkMail Help

virtio-dev message

[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 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.

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]

ÂÂÂ buf[/* this is write data, since read = 0 */]Â // scatterlist[1]

ÂÂÂ virtio_i2c_in_hdrÂÂ // scatterlist[2]

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]


      
@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]