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

[PATCH V2] 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>
---

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 | 60 +++++++++++++++++++++++++-------------------------
 1 file changed, 30 insertions(+), 30 deletions(-)

diff --git a/virtio-i2c.tex b/virtio-i2c.tex
index 949d75f44158..ae344b2bc822 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,10 @@ \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{read_buf} if the final \field{status} returned
-from the device is VIRTIO_I2C_MSG_ERR.
+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 queue the requests in order if multiple requests are going to
 be sent at a time.
@@ -160,11 +158,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

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

[Michael S. Tsirkin]

On Mon, Aug 16, 2021 at 03:17:23PM +0530, Viresh Kumar 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.

So I wonder. What if we allow zero-length buffers in virtio? Would that
address the need?

> 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>
> ---
> 
> 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 | 60 +++++++++++++++++++++++++-------------------------
>  1 file changed, 30 insertions(+), 30 deletions(-)
> 
> diff --git a/virtio-i2c.tex b/virtio-i2c.tex
> index 949d75f44158..ae344b2bc822 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,10 @@ \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{read_buf} if the final \field{status} returned
> -from the device is VIRTIO_I2C_MSG_ERR.
> +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 queue the requests in order if multiple requests are going to
>  be sent at a time.
> @@ -160,11 +158,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

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

[Viresh Kumar]

On 16-08-21, 10:45, Michael S. Tsirkin wrote:
> On Mon, Aug 16, 2021 at 03:17:23PM +0530, Viresh Kumar 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.
> 
> So I wonder. What if we allow zero-length buffers in virtio? Would that
> address the need?

There are three things this patch does, all of which are important
IMO:

1. Drop the need of passing both write and read buffers, as that isn't
   required really since we already have another way of linking
   different transfers using the fail-next flag.

2. Make the buffer optional, for zero-length transfers.

3. Pass the r/w flag separately, which is kind of relevant because of
   2 (zero-length transfers) only.


I think (1) can be done separately anyway, and we may be able to solve
(2) and (3) if we allow zero-length buffers in virtio itself.

-- 
viresh

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

[Michael S. Tsirkin]

On Tue, Aug 17, 2021 at 09:23:24AM +0530, Viresh Kumar wrote:
> On 16-08-21, 10:45, Michael S. Tsirkin wrote:
> > On Mon, Aug 16, 2021 at 03:17:23PM +0530, Viresh Kumar 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.
> > 
> > So I wonder. What if we allow zero-length buffers in virtio? Would that
> > address the need?
> 
> There are three things this patch does, all of which are important
> IMO:
> 
> 1. Drop the need of passing both write and read buffers, as that isn't
>    required really since we already have another way of linking
>    different transfers using the fail-next flag.
> 
> 2. Make the buffer optional, for zero-length transfers.
> 
> 3. Pass the r/w flag separately, which is kind of relevant because of
>    2 (zero-length transfers) only.
> 
> 
> I think (1) can be done separately anyway, and we may be able to solve
> (2) and (3) if we allow zero-length buffers in virtio itself.

OK, so yea. Let's split this up please.

> -- 
> viresh

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

[Viresh Kumar]

On 17-08-21, 02:47, Michael S. Tsirkin wrote:
> On Tue, Aug 17, 2021 at 09:23:24AM +0530, Viresh Kumar wrote:
> > On 16-08-21, 10:45, Michael S. Tsirkin wrote:
> > > So I wonder. What if we allow zero-length buffers in virtio? Would that
> > > address the need?

> OK, so yea. Let's split this up please.

Just to make sure I understand what you asked for, we are looking for
something on these lines ? And the kernel side will still send an 'sg'
element and call sg_init_one(sg, NULL, 0) ?

diff --git a/packed-ring.tex b/packed-ring.tex
index a9e6c162fe12..cf3af6f42fbd 100644
--- a/packed-ring.tex
+++ b/packed-ring.tex
@@ -163,7 +163,9 @@ \subsection{Element Address and Length}

 In an available descriptor, Element Address corresponds to the
 physical address of the buffer element. The length of the element assumed
-to be physically contiguous is stored in Element Length.
+to be physically contiguous is stored in Element Length. Virtio also
+allows zero-length descriptors, where both address and length of the
+element is set to zero by the driver.

 In a used descriptor, Element Address is unused. Element Length
 specifies the length of the buffer that has been initialized
diff --git a/split-ring.tex b/split-ring.tex
index bfef62d18dab..baf478e4998e 100644
--- a/split-ring.tex
+++ b/split-ring.tex
@@ -175,6 +175,8 @@ \subsection{The Virtqueue Descriptor Table}\label{sec:Basic Facilities of a Virt
 can be chained via \field{next}. Each descriptor describes a
 buffer which is read-only for the device (``device-readable'') or write-only for the device (``device-writable''), but a chain of
 descriptors can contain both device-readable and device-writable buffers.
+Virtio also allows zero-length descriptors, where both \field{addr} and
+\field{len} is set to zero by the driver.

 The actual contents of the memory offered to the device depends on the
 device type.  Most common is to begin the data with a header

-- 
viresh

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

[Jie Deng]

On 2021/8/17 18:43, Viresh Kumar wrote:
> On 17-08-21, 02:47, Michael S. Tsirkin wrote:
>> On Tue, Aug 17, 2021 at 09:23:24AM +0530, Viresh Kumar wrote:
>>> On 16-08-21, 10:45, Michael S. Tsirkin wrote:
>>>> So I wonder. What if we allow zero-length buffers in virtio? Would that
>>>> address the need?
>> OK, so yea. Let's split this up please.
> Just to make sure I understand what you asked for, we are looking for
> something on these lines ? And the kernel side will still send an 'sg'
> element and call sg_init_one(sg, NULL, 0) ?


 From the perspective of specification，I think we can allow zero-length 
buffers in virtio.

we can use the len of descriptor to see if it is a zero-length buffer.

But for a specific implementation, I don't think "NULL" can be passed to 
this API.

There is a check "BUG_ON(!virt_addr_valid(buf))" in it.




---------------------------------------------------------------------
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 V2] virtio: i2c: Allow zero-length transactions

[Viresh Kumar]

+Arnd

On 18-08-21, 10:38, Jie Deng wrote:
> 
> On 2021/8/17 18:43, Viresh Kumar wrote:
> > On 17-08-21, 02:47, Michael S. Tsirkin wrote:
> > > On Tue, Aug 17, 2021 at 09:23:24AM +0530, Viresh Kumar wrote:
> > > > On 16-08-21, 10:45, Michael S. Tsirkin wrote:
> > > > > So I wonder. What if we allow zero-length buffers in virtio? Would that
> > > > > address the need?
> > > OK, so yea. Let's split this up please.
> > Just to make sure I understand what you asked for, we are looking for
> > something on these lines ? And the kernel side will still send an 'sg'
> > element and call sg_init_one(sg, NULL, 0) ?
> 
> 
> From the perspective of specification，I think we can allow zero-length
> buffers in virtio.
> 
> we can use the len of descriptor to see if it is a zero-length buffer.
> 
> But for a specific implementation, I don't think "NULL" can be passed to
> this API.
> 
> There is a check "BUG_ON(!virt_addr_valid(buf))" in it.

I tried to look at implementations of virt_addr_valid() and it doesn't
check for NULL specifically (for the ones I looked at). I haven't
tested it though.

Though I am not sure what's better here, remove the need of sending
buffer altogether, the way this patch proposed initially or what
Michael has suggested. And all that to prevent just a single bit to be
used in flags field, which will likely be used for more things later
on.

-- 
viresh

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

[Arnd Bergmann]

On Wed, Aug 18, 2021 at 5:26 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 18-08-21, 10:38, Jie Deng wrote:
> >
> > From the perspective of specification，I think we can allow zero-length
> > buffers in virtio.
> >
> > we can use the len of descriptor to see if it is a zero-length buffer.
> >
> > But for a specific implementation, I don't think "NULL" can be passed to
> > this API.
> >
> > There is a check "BUG_ON(!virt_addr_valid(buf))" in it.
>
> I tried to look at implementations of virt_addr_valid() and it doesn't
> check for NULL specifically (for the ones I looked at). I haven't
> tested it though.
>
> Though I am not sure what's better here, remove the need of sending
> buffer altogether, the way this patch proposed initially or what
> Michael has suggested. And all that to prevent just a single bit to be
> used in flags field, which will likely be used for more things later
> on.

I'd prefer your earlier approach.

My feeling is that changing the virtqueue code to allow zero-length
buffers is more fragile than having something in the virtio-i2c code
that has a special case for leaving out both read_buf and write_buf.

Even if we fix the linux virtqueue code to deal with zero-length
NULL buffers, there are other implementations that will require
similar workarounds for existing sanity checks.

       Arnd

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

[Viresh Kumar]

On 18-08-21, 10:03, Arnd Bergmann wrote:
> I'd prefer your earlier approach.
> 
> My feeling is that changing the virtqueue code to allow zero-length
> buffers is more fragile than having something in the virtio-i2c code
> that has a special case for leaving out both read_buf and write_buf.

Exactly what I think about it and so my hesitation for the same :)

> Even if we fix the linux virtqueue code to deal with zero-length
> NULL buffers, there are other implementations that will require
> similar workarounds for existing sanity checks.

Lets say what Michael thinks about it.

-- 
viresh

---------------------------------------------------------------------
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 V2] virtio: i2c: Allow zero-length transactions

[Viresh Kumar]

On 18-08-21, 13:39, Viresh Kumar wrote:
> On 18-08-21, 10:03, Arnd Bergmann wrote:
> > I'd prefer your earlier approach.
> > 
> > My feeling is that changing the virtqueue code to allow zero-length
> > buffers is more fragile than having something in the virtio-i2c code
> > that has a special case for leaving out both read_buf and write_buf.
> 
> Exactly what I think about it and so my hesitation for the same :)
> 
> > Even if we fix the linux virtqueue code to deal with zero-length
> > NULL buffers, there are other implementations that will require
> > similar workarounds for existing sanity checks.
> 
> Lets say what Michael thinks about it.

Michael, do you want to re comment on this ?

-- 
viresh

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

[Michael S. Tsirkin]

On Mon, Aug 16, 2021 at 03:17:23PM +0530, Viresh Kumar 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.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

I think zero length buffers are useful in their own right.
But if you are doing it like this, then I think we should
have conformance clauses that require that the flag
is consistent with the buffer.

Paolo what are your thoughts on VIRTIO_I2C_FLAGS_FAIL_NEXT?

> ---
> 
> 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 | 60 +++++++++++++++++++++++++-------------------------
>  1 file changed, 30 insertions(+), 30 deletions(-)
> 
> diff --git a/virtio-i2c.tex b/virtio-i2c.tex
> index 949d75f44158..ae344b2bc822 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,10 @@ \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{read_buf} if the final \field{status} returned
> -from the device is VIRTIO_I2C_MSG_ERR.
> +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 queue the requests in order if multiple requests are going to
>  be sent at a time.
> @@ -160,11 +158,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

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

[Viresh Kumar]

On 31-08-21, 20:03, Michael S. Tsirkin wrote:
> But if you are doing it like this, then I think we should
> have conformance clauses that require that the flag
> is consistent with the buffer.

Something like this will work ?

diff --git a/virtio-i2c.tex b/virtio-i2c.tex
index ae344b2bc822..c7335372a8bb 100644
--- a/virtio-i2c.tex
+++ b/virtio-i2c.tex
@@ -145,6 +145,12 @@ \subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C
 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 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.

-- 
viresh

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

[Michael S. Tsirkin]

On Wed, Sep 01, 2021 at 11:52:31AM +0530, Viresh Kumar wrote:
> On 31-08-21, 20:03, Michael S. Tsirkin wrote:
> > But if you are doing it like this, then I think we should
> > have conformance clauses that require that the flag
> > is consistent with the buffer.
> 
> Something like this will work ?
> 
> diff --git a/virtio-i2c.tex b/virtio-i2c.tex
> index ae344b2bc822..c7335372a8bb 100644
> --- a/virtio-i2c.tex
> +++ b/virtio-i2c.tex
> @@ -145,6 +145,12 @@ \subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C
>  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 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.
> 
> -- 
> viresh

Something like this, yes.

-- 
MST