[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-comment] [PATCH v6] virtio-i2c: add the device specification
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
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]