Re: [virtio-comment] [PATCH v6] virtio-i2c: add the device specification

[Stefan Hajnoczi <stefanha@redhat.com>]

On Thu, Dec 31, 2020 at 10:50:13AM +0800, Jie Deng wrote:
> 
> On 2020/12/30 18:20, Stefan Hajnoczi wrote:
> > On Wed, Dec 30, 2020 at 10:17:23AM +0800, Jie Deng wrote:
> > > On 2020/12/29 19:09, Stefan Hajnoczi wrote:
> > > 
> > > > On Thu, Dec 24, 2020 at 04:10:43PM +0800, Jie Deng wrote:
> > > > > +The \field{padding} is used to pad to full dword.
> > > > > +
> > > > > +The \field{flags} of the request is currently reserved as zero for future
> > > > > +feature extensibility.
> > > > > +
> > > > > +The \field{write_buf} of the request contains one segment of an I2C transaction
> > > > > +being written to the device.
> > > > > +
> > > > > +The \field{read_buf} of the request contains one segment of an I2C transaction
> > > > > +being read from the device.
> > > > > +
> > > > > +The final \field{status} byte of the request is written by the device: either
> > > > > +VIRTIO_I2C_MSG_OK for success or VIRTIO_I2C_MSG_ERR for error.
> > > > > +
> > > > > +\begin{lstlisting}
> > > > > +#define VIRTIO_I2C_MSG_OK     0
> > > > > +#define VIRTIO_I2C_MSG_ERR    1
> > > > > +\end{lstlisting}
> > > > > +
> > > > > +If ``length of \field{read_buf}''=0 and ``length of \field{write_buf}''>0,
> > > > > +the request is called write request.
> > > > > +
> > > > > +If ``length of \field{read_buf}''>0 and ``length of \field{write_buf}''=0,
> > > > > +the request is called read request.
> > > > > +
> > > > > +If ``length of \field{read_buf}''>0 and ``length of \field{write_buf}''>0,
> > > > > +the request is called write-read request. It means an I2C write segment followed
> > > > > +by a read segment. Usually, the write segment provides the number of an I2C
> > > > > +controlled device register to be read.
> > > > > +
> > > > > +The case when ``length of \field{write_buf}''=0, and at the same time,
> > > > > +``length of \field{read_buf}''=0 doesn't make any sense.
> > > > > +
> > > > > +\subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C Adapter Device / Device Operation: Operation Status}
> > > > > +
> > > > > +\field{addr}, ``length of \field{write_buf}'' and ``length of \field{read_buf}''
> > > > > +are determined by the driver, while \field{status} is determined by the processing
> > > > > +of the devcie. A driver puts the data written to the device into \field{write_buf}, while
> > > > s/devcie/device/
> > > Will fix this typo.
> > > > > +a device puts the data of the corresponding length into \field{read_buf} according the
> > > > > +request of the driver.
> > > > > +
> > > > > +A driver may send one request or multiple requests to the device at a time.
> > > > > +The requests in the virtqueue are both queued and processed in order.
> > > > > +
> > > > > +If a driver sends multiple requests at a time and a device fails to process
> > > > > +some of them, then the first failed request will have its \field{status}
> > > > > +being set to VIRTIO_I2C_MSG_ERR by the device and the requests after the first
> > > > > +failed one will also be treated as VIRTIO_I2C_MSG_ERR by the driver,
> > > > > +no matter what \field{status} of them. In this case, the number of successfully
> > > > > +sent requests this time is the number of the last request being successfully
> > > > > +processed.
> > > > If I understand correctly all requests submitted in a single available
> > > > buffer notification are grouped together. Once the first request fails
> > > > the driver must treat all remaining requests as failed?
> > > Right. That's how it works.
> > > > Devices may poll vrings instead of using available buffer notifications.
> > > > Polled devices cannot provide the grouping semantics described above.
> > > Poll is not recommended for this device. But if poll is used, the device
> > > must notify
> > > the driver once it encounters an error request.
> > > 
> > > So if poll is used and the virtqueue has following requests (May be
> > > submitted by many kick operation, no need to group)
> > > 
> > > Y Y Y Y N X X X X
> > > 
> > > (Y: OK,ÂÂ N: error,ÂÂ X: no need to care about, must be treated as error.)
> > > 
> > > The device must notify the driver at the first "N" and driver must treat
> > > the N and "X X X X" as error.
> > > 
> > > > If you want to group requests then it is necessary to include a
> > > > per-request group ID or continuation/end-of-group field. virtio-net
> > > > VIRTIO_NET_F_MRG_RXBUF is an example of this.
> > > > 
> > > > Additionally, this paragraph does not specify whether the device
> > > > executes the remaining requests or not. This seems problematic to me
> > > > because the driver does not know which requests actually executed when
> > > > a request in a group of requests fails?
> > > The device must not execute the remaining requests.
> > Please add that to the spec so it's clear the device must fail remaining
> > requests insteads of attempting to execute them.
> Sure. Will do that.
> > This approach causes a bottleneck when there are multiple I2C devices.
> > It requires exclusive access to the virtqueue and virtual I2C bus,
> > slowing down applications that are accessing separate I2C devices.
> > 
> > This is inefficient, especially if the I2C devices are emulated or on
> > separate physical I2C busses where request concurrent processing is
> > possible.
> > 
> > This also creates starvation/denial-of-service problems in the driver
> > because an application can submit many requests and needs exclusive
> > access to the virtqueue until all requests have completed.
> > 
> > I suggest defining a VIRTIO_I2C_FLAGS_CONTINUATION bit in struct
> > virtio_i2c_out_hdr::flags. When a request fails and this flag is set
> > then the device fails all later requests until it reaches a request
> > where the flag is zero. This allows the driver to enqueue a sequence of
> > requests that fail as a group without waiting for an empty virtqueue.
> The flags is designed to be a "driver->device" field for extensibility. It
> is read-onlyï to the device.
> So when a request fails, the device can't set this read-only field.

Sorry, "and this flag is set" means "and this flag was set by the
driver". My intention was that the driver marks a request so the device
knows that it needs to fail the next request too when this request
fails.

Thinking about it again, VIRTIO_I2C_FLAGS_FAIL_NEXT would be a clearer
name for this flag.

> We just need to make sure that once the driver adds some requests to the
> virtqueue,
> it should complete them (either success or fail) before adding new requests.
> I think this
> is a behavior of physical I2C adapter drivers and we can follow.

Existing VIRTIO devices do not have special empty queue semantics and I
don't think virtio-i2c should introduce a special case here. The
VIRTIO_I2C_FLAGS_FAIL_NEXT flag is simple to implement, allows drivers
more freedom to enqueue requests, and eliminates the race condition with
polled device implementations (they could complete failed request #1
before request #2 is enqueued and then request #2 would not be
automatically failed).

Stefan

Attachment: signature.asc
Description: PGP signature