[PATCH V3] virtio: i2c: Allow zero-length transactions

[PATCH V3] virtio: i2c: Allow zero-length transactions

[Viresh Kumar]

The I2C protocol allows zero-length requests with no data, like the
SMBus Quick command, where the command is inferred based on the
read/write flag itself.

In order to allow such a request, allocate another bit,
VIRTIO_I2C_FLAGS_M_RD(1), in the flags to pass the request type, as read
or write. This was earlier done using the read/write permission to the
buffer itself.

This still won't work well if multiple buffers are passed for the same
request, i.e. the write-read requests, as the VIRTIO_I2C_FLAGS_M_RD flag
can only be used with a single buffer.

Coming back to it, there is no need to send multiple buffers with a
single request. All we need, is a way to group several requests
together, which we can already do based on the
VIRTIO_I2C_FLAGS_FAIL_NEXT flag.

Remove support for multiple buffers within a single request.

Since we are at very early stage of development currently, we can do
these modifications without addition of new features or versioning of
the protocol.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V2->V3:
- Add conformance clauses that require that the flag is consistent with the
  buffer.

V1->V2:
- Name the buffer-less request as zero-length request.

Hi Guys,

I did try to follow the discussion you guys had during V4, where we added
support for multiple buffers for the same request, which I think is unnecessary
now, after introduction of the VIRTIO_I2C_FLAGS_FAIL_NEXT flag.

https://lists.oasis-open.org/archives/virtio-comment/202011/msg00005.html

And so starting this discussion again, because we need to support stuff
like: i2cdetect -q <i2c-bus-number>, which issues a zero-length SMBus
Quick command.
---
 virtio-i2c.tex | 66 +++++++++++++++++++++++++++-----------------------
 1 file changed, 36 insertions(+), 30 deletions(-)

diff --git a/virtio-i2c.tex b/virtio-i2c.tex
index 949d75f44158..c7335372a8bb 100644
--- a/virtio-i2c.tex
+++ b/virtio-i2c.tex
@@ -54,8 +54,7 @@ \subsubsection{Device Operation: Request Queue}\label{sec:Device Types / I2C Ada
 \begin{lstlisting}
 struct virtio_i2c_req {
         struct virtio_i2c_out_hdr out_hdr;
-        u8 write_buf[];
-        u8 read_buf[];
+        u8 buf[];
         struct virtio_i2c_in_hdr in_hdr;
 };
 \end{lstlisting}
@@ -84,16 +83,16 @@ \subsubsection{Device Operation: Request Queue}\label{sec:Device Types / I2C Ada
     and sets it on the other requests. If this bit is set and a device fails
     to process the current request, it needs to fail the next request instead
     of attempting to execute it.
+
+\item[VIRTIO_I2C_FLAGS_M_RD(1)] is used to mark the request as READ or WRITE.
 \end{description}
 
 Other bits of \field{flags} are 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 \field{buf} of the request is optional and contains one segment of an I2C
+transaction being read from or written to the device, based on the value of the
+\field{VIRTIO_I2C_FLAGS_M_RD} bit in the \field{flags} field.
 
 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.
@@ -103,27 +102,27 @@ \subsubsection{Device Operation: Request Queue}\label{sec:Device Types / I2C Ada
 #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 \field{VIRTIO_I2C_FLAGS_M_RD} bit is set in the \field{flags}, then the
+request is called a read request.
 
-If ``length of \field{read_buf}''>0 and ``length of \field{write_buf}''=0,
-the request is called read request.
+If \field{VIRTIO_I2C_FLAGS_M_RD} bit is unset in the \field{flags}, then the
+request is called a write 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 \field{buf} is optional and will not be present for a zero-length request,
+like SMBus Quick.
 
-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.
+The virtio I2C protocol supports write-read requests, i.e. 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), by grouping a list of requests
+together using the \field{VIRTIO_I2C_FLAGS_FAIL_NEXT} flag.
 
 \subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C Adapter Device / Device Operation: Operation Status}
 
-\field{addr}, \field{flags}, ``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 device. A driver puts the data written to the device into \field{write_buf}, while
-a device puts the data of the corresponding length into \field{read_buf} according to the
-request of the driver.
+\field{addr}, \field{flags}, and ``length of \field{buf}'' are determined by the
+driver, while \field{status} is determined by the processing of the device.  A
+driver, for a write request, puts the data to be written to the device into the
+\field{buf}, while a device, for a read request, puts the data read from device
+into the \field{buf} according to the request from 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.
@@ -141,11 +140,16 @@ \subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C
 
 A driver MUST set the reserved bits of \field{flags} to be zero.
 
-The driver MUST NOT send a request with ``length of \field{write_buf}''=0 and
-``length of \field{read_buf}''=0 at the same time.
+A driver MUST NOT send the \field{buf}, for a zero-length request.
+
+A driver MUST NOT use \field{buf}, for a read request, if the final
+\field{status} returned from the device is VIRTIO_I2C_MSG_ERR.
 
-A driver MUST NOT use \field{read_buf} if the final \field{status} returned
-from the device is VIRTIO_I2C_MSG_ERR.
+A driver MUST set the \field{VIRTIO_I2C_FLAGS_M_RD} flag for a read operation,
+where the buffer is write-only for the device.
+
+A driver MUST NOT set the \field{VIRTIO_I2C_FLAGS_M_RD} flag for a write
+operation, where the buffer is read-only for the device.
 
 A driver MUST queue the requests in order if multiple requests are going to
 be sent at a time.
@@ -160,11 +164,13 @@ \subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C
 A device SHOULD keep consistent behaviors with the hardware as described in
 \hyperref[intro:I2C]{I2C}.
 
-A device MUST NOT change the value of \field{addr}, reserved bits of \field{flags}
-and \field{write_buf}.
+A device MUST NOT change the value of \field{addr}, and reserved bits of
+\field{flags}.
+
+A device MUST not change the value of the \field{buf} for a write request.
 
-A device MUST place one I2C segment of the corresponding length into \field{read_buf}
-according the driver's request.
+A device MUST place one I2C segment of the ``length of \field{buf}'', for the
+read request, into the \field{buf} according the driver's request.
 
 A device MUST guarantee the requests in the virtqueue being processed in order
 if multiple requests are received at a time.
-- 
2.31.1.272.g89b43f80a514

[virtio-dev] Re: [PATCH V3] virtio: i2c: Allow zero-length transactions

[Cornelia Huck]

On Wed, Sep 01 2021, Viresh Kumar <viresh.kumar@linaro.org> wrote:

> The I2C protocol allows zero-length requests with no data, like the
> SMBus Quick command, where the command is inferred based on the
> read/write flag itself.
>
> In order to allow such a request, allocate another bit,
> VIRTIO_I2C_FLAGS_M_RD(1), in the flags to pass the request type, as read
> or write. This was earlier done using the read/write permission to the
> buffer itself.
>
> This still won't work well if multiple buffers are passed for the same
> request, i.e. the write-read requests, as the VIRTIO_I2C_FLAGS_M_RD flag
> can only be used with a single buffer.
>
> Coming back to it, there is no need to send multiple buffers with a
> single request. All we need, is a way to group several requests
> together, which we can already do based on the
> VIRTIO_I2C_FLAGS_FAIL_NEXT flag.
>
> Remove support for multiple buffers within a single request.
>
> Since we are at very early stage of development currently, we can do
> these modifications without addition of new features or versioning of
> the protocol.

I'm a bit worried about this statement. While we have not done a formal
release of the standard since i2c has been added, it still has been out
there already, and this is an incompatible change (even if it is one
that makes sense.)

So, I'm a bit torn between just going with this and adding a feature bit
(that can be mandatory so we don't need to drag the old implementation
around.) But I'm also worried whether there are more incompatible
changes expected in the near future; if there are, I'd be more
comfortable if we could just hold off until everyone agrees things are
stable.


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] Re: [PATCH V3] virtio: i2c: Allow zero-length transactions

[Viresh Kumar]

On 22-09-21, 10:09, Cornelia Huck wrote:
> On Wed, Sep 01 2021, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> 
> > The I2C protocol allows zero-length requests with no data, like the
> > SMBus Quick command, where the command is inferred based on the
> > read/write flag itself.
> >
> > In order to allow such a request, allocate another bit,
> > VIRTIO_I2C_FLAGS_M_RD(1), in the flags to pass the request type, as read
> > or write. This was earlier done using the read/write permission to the
> > buffer itself.
> >
> > This still won't work well if multiple buffers are passed for the same
> > request, i.e. the write-read requests, as the VIRTIO_I2C_FLAGS_M_RD flag
> > can only be used with a single buffer.
> >
> > Coming back to it, there is no need to send multiple buffers with a
> > single request. All we need, is a way to group several requests
> > together, which we can already do based on the
> > VIRTIO_I2C_FLAGS_FAIL_NEXT flag.
> >
> > Remove support for multiple buffers within a single request.
> >
> > Since we are at very early stage of development currently, we can do
> > these modifications without addition of new features or versioning of
> > the protocol.
> 
> I'm a bit worried about this statement. While we have not done a formal
> release of the standard since i2c has been added, it still has been out
> there already,

As far as I know, there are only two users of this stuff for now, Linaro and
Intel (Jie). And both of us are okay to go ahead with this change.

> and this is an incompatible change (even if it is one
> that makes sense.)

Yes, it is incompatible for sure.

> So, I'm a bit torn between just going with this and adding a feature bit
> (that can be mandatory so we don't need to drag the old implementation
> around.)

I would be fine with that as it would solve the problem at hand.

> But I'm also worried whether there are more incompatible
> changes expected in the near future; if there are, I'd be more
> comfortable if we could just hold off until everyone agrees things are
> stable.

Both me and Jie are done with our developments of backends and Linux driver and
this is the only thing that came up during reviews. So there is nothing which is
already known.

-- 
viresh