[PATCH V5 0/2] virtio: i2c: Allow zero-length transactions

[PATCH V5 0/2] virtio: i2c: Allow zero-length transactions

[Viresh Kumar]

Hi,

Arnd suggested (over IRC) to split this into two patches for better readability
and so here is a resend. The eventual specification hasn't changed at all.

V4->V5:
- Split into two patches.

V3->V4:
- Add a new mandatory feature flag.

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.

------

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.

Viresh Kumar (2):
  virtio: i2c: No need to have separate read-write buffers
  virtio: i2c: Allow zero-length transactions

 virtio-i2c.tex | 76 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 45 insertions(+), 31 deletions(-)

-- 
2.31.1.272.g89b43f80a514

[PATCH V5 1/2] virtio: i2c: No need to have separate read-write buffers

[Viresh Kumar]

The virtio I2C protocol allows to contain multiple read-write requests
in a single I2C transaction using the VIRTIO_I2C_FLAGS_FAIL_NEXT flag,
where each request contains a header, buffer and status.

There is no need to pass both read and write buffers in a single
request, as we have a better way of combining requests into a single I2C
transaction. Moreover, this also limits the transactions to two buffers,
one for read operation and one for write. By using
VIRTIO_I2C_FLAGS_FAIL_NEXT, we don't have any such limits.

Remove support for multiple buffers within a single request.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/112
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 virtio-i2c.tex | 54 +++++++++++++++++++-------------------------------
 1 file changed, 20 insertions(+), 34 deletions(-)

diff --git a/virtio-i2c.tex b/virtio-i2c.tex
index 949d75f44158..5d634aec6e7c 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}
@@ -89,11 +88,8 @@ \subsubsection{Device Operation: Request Queue}\label{sec:Device Types / I2C Ada
 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 contains one segment of an I2C transaction
+being read from or written to 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.
@@ -103,27 +99,18 @@ \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 ``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.
+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 +128,8 @@ \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 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 +144,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 V5 1/2] virtio: i2c: No need to have separate read-write buffers

[Arnd Bergmann]

On Tue, Oct 12, 2021 at 1:23 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> The virtio I2C protocol allows to contain multiple read-write requests
> in a single I2C transaction using the VIRTIO_I2C_FLAGS_FAIL_NEXT flag,
> where each request contains a header, buffer and status.
>
> There is no need to pass both read and write buffers in a single
> request, as we have a better way of combining requests into a single I2C
> transaction. Moreover, this also limits the transactions to two buffers,
> one for read operation and one for write. By using
> VIRTIO_I2C_FLAGS_FAIL_NEXT, we don't have any such limits.
>
> Remove support for multiple buffers within a single request.
>
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/112
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Thanks for splitting up the two patches, I think this is much clearer now.

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

[PATCH V5 2/2] 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.

Add a new feature flag for zero length requests and make it mandatory
for it to be implemented, so we don't need to drag the old
implementation around.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/112
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 virtio-i2c.tex | 34 +++++++++++++++++++++++++++++++---
 1 file changed, 31 insertions(+), 3 deletions(-)

diff --git a/virtio-i2c.tex b/virtio-i2c.tex
index 5d634aec6e7c..0e73348963ce 100644
--- a/virtio-i2c.tex
+++ b/virtio-i2c.tex
@@ -17,7 +17,11 @@ \subsection{Virtqueues}\label{sec:Device Types / I2C Adapter Device / Virtqueues
 
 \subsection{Feature bits}\label{sec:Device Types / I2C Adapter Device / Feature bits}
 
-None currently defined.
+\begin{description}
+\item[VIRTIO_I2C_F_ZERO_LENGTH_REQUEST (0)] The device supports zero-length I2C
+request and \field{VIRTIO_I2C_FLAGS_M_RD} flag. It is mandatory to implement
+this feature.
+\end{description}
 
 \subsection{Device configuration layout}\label{sec:Device Types / I2C Adapter Device / Device configuration layout}
 
@@ -83,13 +87,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{buf} of the request contains one segment of an I2C transaction
-being read from or written to 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.
@@ -99,6 +106,15 @@ \subsubsection{Device Operation: Request Queue}\label{sec:Device Types / I2C Ada
 #define VIRTIO_I2C_MSG_ERR    1
 \end{lstlisting}
 
+If \field{VIRTIO_I2C_FLAGS_M_RD} bit is set in the \field{flags}, then the
+request is called a read request.
+
+If \field{VIRTIO_I2C_FLAGS_M_RD} bit is unset in the \field{flags}, then the
+request is called a write request.
+
+The \field{buf} is optional and will not be present for a zero-length request,
+like SMBus Quick.
+
 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
@@ -124,13 +140,23 @@ \subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C
 
 \drivernormative{\subsubsection}{Device Operation}{Device Types / I2C Adapter Device / Device Operation}
 
+A driver SHOULD implement the VIRTIO_I2C_F_ZERO_LENGTH_REQUEST feature.
+
 A driver MUST set \field{addr} and \field{flags} before sending the request.
 
 A driver MUST set the reserved bits of \field{flags} to be zero.
 
+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 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.
 
@@ -141,6 +167,8 @@ \subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C
 
 \devicenormative{\subsubsection}{Device Operation}{Device Types / I2C Adapter Device / Device Operation}
 
+A device SHOULD implement the VIRTIO_I2C_F_ZERO_LENGTH_REQUEST feature.
+
 A device SHOULD keep consistent behaviors with the hardware as described in
 \hyperref[intro:I2C]{I2C}.
 
-- 
2.31.1.272.g89b43f80a514

Re: [PATCH V5 2/2] virtio: i2c: Allow zero-length transactions

[Arnd Bergmann]

On Tue, Oct 12, 2021 at 1:23 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:

> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/112
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---

I have a few very minor comments, regardless of how you address those:

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

> diff --git a/virtio-i2c.tex b/virtio-i2c.tex
> index 5d634aec6e7c..0e73348963ce 100644
> --- a/virtio-i2c.tex
> +++ b/virtio-i2c.tex
> @@ -17,7 +17,11 @@ \subsection{Virtqueues}\label{sec:Device Types / I2C Adapter Device / Virtqueues
>
>  \subsection{Feature bits}\label{sec:Device Types / I2C Adapter Device / Feature bits}
>
> -None currently defined.
> +\begin{description}
> +\item[VIRTIO_I2C_F_ZERO_LENGTH_REQUEST (0)] The device supports zero-length I2C
> +request and \field{VIRTIO_I2C_FLAGS_M_RD} flag. It is mandatory to implement
> +this feature.
> +\end{description}

I'm not quite sure what it means to have a mandatory feature flag,
or what the driver is expected to do if it finds the flag to be missing.

Assuming this is something we do in other virtio specs, I see nothing
wrong here though.

> @@ -99,6 +106,15 @@ \subsubsection{Device Operation: Request Queue}\label{sec:Device Types / I2C Ada
>  #define VIRTIO_I2C_MSG_ERR    1
>  \end{lstlisting}
>
> +If \field{VIRTIO_I2C_FLAGS_M_RD} bit is set in the \field{flags}, then the
> +request is called a read request.
> +
> +If \field{VIRTIO_I2C_FLAGS_M_RD} bit is unset in the \field{flags}, then the
> +request is called a write request.
> +
> +The \field{buf} is optional and will not be present for a zero-length request,
> +like SMBus Quick.

Maybe write this as

... like the SMBus "Quick" command.

> @@ -124,13 +140,23 @@ \subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C
>
>  \drivernormative{\subsubsection}{Device Operation}{Device Types / I2C Adapter Device / Device Operation}
>
> +A driver SHOULD implement the VIRTIO_I2C_F_ZERO_LENGTH_REQUEST feature.

I don't think this needs to be "SHOULD", as a driver may be written to only
talk to certain i2c clients on the device side, and they do not need zero
length requests. Maybe this could be

"A driver MAY assume that the VIRTIO_I2C_F_ZERO_LENGTH_REQUEST
feature is available".

> @@ -141,6 +167,8 @@ \subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C
>
>  \devicenormative{\subsubsection}{Device Operation}{Device Types / I2C Adapter Device / Device Operation}
>
> +A device SHOULD implement the VIRTIO_I2C_F_ZERO_LENGTH_REQUEST feature.

On the device side it seems reasonable.

         Arnd

Re: [virtio-dev] Re: [PATCH V5 2/2] virtio: i2c: Allow zero-length transactions

[Jie Deng]

On 2021/10/12 22:09, Arnd Bergmann wrote:
>
>> @@ -124,13 +140,23 @@ \subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C
>>
>>   \drivernormative{\subsubsection}{Device Operation}{Device Types / I2C Adapter Device / Device Operation}
>>
>> +A driver SHOULD implement the VIRTIO_I2C_F_ZERO_LENGTH_REQUEST feature.
> I don't think this needs to be "SHOULD", as a driver may be written to only
> talk to certain i2c clients on the device side, and they do not need zero
> length requests. Maybe this could be
>
> "A driver MAY assume that the VIRTIO_I2C_F_ZERO_LENGTH_REQUEST
> feature is available".

Agree. "MAY" is better.

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

Re: [PATCH V5 2/2] virtio: i2c: Allow zero-length transactions

[Viresh Kumar]

On 12-10-21, 16:09, Arnd Bergmann wrote:
> On Tue, Oct 12, 2021 at 1:23 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > +\begin{description}
> > +\item[VIRTIO_I2C_F_ZERO_LENGTH_REQUEST (0)] The device supports zero-length I2C
> > +request and \field{VIRTIO_I2C_FLAGS_M_RD} flag. It is mandatory to implement
> > +this feature.
> > +\end{description}
> 
> I'm not quite sure what it means to have a mandatory feature flag,

Cornelia suggested this option earlier:

https://lists.oasis-open.org/archives/virtio-dev/202109/msg00087.html

> or what the driver is expected to do if it finds the flag to be missing.

I think the driver is expected to fail in that case.

> Assuming this is something we do in other virtio specs, I see nothing
> wrong here though.

I am not sure if it is used anywhere else.

> > @@ -99,6 +106,15 @@ \subsubsection{Device Operation: Request Queue}\label{sec:Device Types / I2C Ada
> >  #define VIRTIO_I2C_MSG_ERR    1
> >  \end{lstlisting}
> >
> > +If \field{VIRTIO_I2C_FLAGS_M_RD} bit is set in the \field{flags}, then the
> > +request is called a read request.
> > +
> > +If \field{VIRTIO_I2C_FLAGS_M_RD} bit is unset in the \field{flags}, then the
> > +request is called a write request.
> > +
> > +The \field{buf} is optional and will not be present for a zero-length request,
> > +like SMBus Quick.
> 
> Maybe write this as
> 
> ... like the SMBus "Quick" command.

Sure.

> > @@ -124,13 +140,23 @@ \subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C
> >
> >  \drivernormative{\subsubsection}{Device Operation}{Device Types / I2C Adapter Device / Device Operation}
> >
> > +A driver SHOULD implement the VIRTIO_I2C_F_ZERO_LENGTH_REQUEST feature.
> 
> I don't think this needs to be "SHOULD", as a driver may be written to only
> talk to certain i2c clients on the device side, and they do not need zero
> length requests. Maybe this could be
> 
> "A driver MAY assume that the VIRTIO_I2C_F_ZERO_LENGTH_REQUEST
> feature is available".

The problem here is that "VIRTIO_I2C_FLAGS_M_RD" is supported only
with VIRTIO_I2C_F_ZERO_LENGTH_REQUEST feature. If the feature isn't
available, then the device/driver need to encode/decode the direction
of the transfer (read/write) to/from the permissions of the buffer.

This is exactly what we are looking to avoid here with Mandatory
feature, i.e. not to drag the old implementation around. And so it
really needs to be SHOULD instead.

-- 
viresh

[virtio-dev] Re: [PATCH V5 2/2] virtio: i2c: Allow zero-length transactions

[Jie Deng]

[-- Attachment #1: Type: text/plain, Size: 1214 bytes --]


On 2021/10/13 11:39, Viresh Kumar wrote:
>
>>> @@ -124,13 +140,23 @@ \subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C
>>>
>>>   \drivernormative{\subsubsection}{Device Operation}{Device Types / I2C Adapter Device / Device Operation}
>>>
>>> +A driver SHOULD implement the VIRTIO_I2C_F_ZERO_LENGTH_REQUEST feature.
>> I don't think this needs to be "SHOULD", as a driver may be written to only
>> talk to certain i2c clients on the device side, and they do not need zero
>> length requests. Maybe this could be
>>
>> "A driver MAY assume that the VIRTIO_I2C_F_ZERO_LENGTH_REQUEST
>> feature is available".
> The problem here is that "VIRTIO_I2C_FLAGS_M_RD" is supported only
> with VIRTIO_I2C_F_ZERO_LENGTH_REQUEST feature. If the feature isn't
> available, then the device/driver need to encode/decode the direction
> of the transfer (read/write) to/from the permissions of the buffer.


The "VIRTIO_I2C_FLAGS_M_RD" is consistent with the permissions of the 
buffer.

When this flag is set, the buffer is also marked as device write -only 
with VIRTQ_DESC_F_WRITE set.

So if this feature isn't available we can use the the latter. We only 
need this flag when there is no buffer.


[-- Attachment #2: Type: text/html, Size: 2285 bytes --]

Re: [PATCH V5 2/2] virtio: i2c: Allow zero-length transactions

[Viresh Kumar]

On 13-10-21, 12:07, Jie Deng wrote:
> The "VIRTIO_I2C_FLAGS_M_RD" is consistent with the permissions of the
> buffer.

Yes.

> When this flag is set, the buffer is also marked as device write -only with
> VIRTQ_DESC_F_WRITE set.

Yes.

> So if this feature isn't available we can use the the latter. We only need
> this flag when there is no buffer.

Which means that we need to carry extra piece of code for the same
work (with no benefit as it will never happen, everyone will support
this flag as all the implementations are new currently). This is
exactly what we are trying to avoid by making this feature mandatory,
drivers will be expected to set/clear this flag all the time.

We don't want to have code like this anywhere:

if (feature)
        read flag...
else
        read permissions...

-- 
viresh

[virtio-dev] Re: [PATCH V5 2/2] virtio: i2c: Allow zero-length transactions

[Cornelia Huck]

On Tue, Oct 12 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.
>
> Add a new feature flag for zero length requests and make it mandatory
> for it to be implemented, so we don't need to drag the old
> implementation around.
>
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/112
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  virtio-i2c.tex | 34 +++++++++++++++++++++++++++++++---
>  1 file changed, 31 insertions(+), 3 deletions(-)
>
> diff --git a/virtio-i2c.tex b/virtio-i2c.tex
> index 5d634aec6e7c..0e73348963ce 100644
> --- a/virtio-i2c.tex
> +++ b/virtio-i2c.tex
> @@ -17,7 +17,11 @@ \subsection{Virtqueues}\label{sec:Device Types / I2C Adapter Device / Virtqueues
>  
>  \subsection{Feature bits}\label{sec:Device Types / I2C Adapter Device / Feature bits}
>  
> -None currently defined.
> +\begin{description}
> +\item[VIRTIO_I2C_F_ZERO_LENGTH_REQUEST (0)] The device supports zero-length I2C
> +request and \field{VIRTIO_I2C_FLAGS_M_RD} flag. It is mandatory to implement
> +this feature.

Maybe add a footnote, explaining that VIRTIO_I2C_FLAGS_M_RD had not been
specified initially, and that we need an easy way to detect incompatible
implementations?

> +\end{description}
>  
>  \subsection{Device configuration layout}\label{sec:Device Types / I2C Adapter Device / Device configuration layout}
>  

(...)

> @@ -124,13 +140,23 @@ \subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C
>  
>  \drivernormative{\subsubsection}{Device Operation}{Device Types / I2C Adapter Device / Device Operation}
>  
> +A driver SHOULD implement the VIRTIO_I2C_F_ZERO_LENGTH_REQUEST feature.

Make this MUST, or maybe "MUST accept"? The central point of the feature
flag (at least for me) was to be able to change the semantics and format
without things breaking silently. I don't think we need to keep old
driver revisions compliant?

> +
>  A driver MUST set \field{addr} and \field{flags} before sending the request.
>  
>  A driver MUST set the reserved bits of \field{flags} to be zero.
>  
> +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 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.
>  
> @@ -141,6 +167,8 @@ \subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C
>  
>  \devicenormative{\subsubsection}{Device Operation}{Device Types / I2C Adapter Device / Device Operation}
>  
> +A device SHOULD implement the VIRTIO_I2C_F_ZERO_LENGTH_REQUEST feature.

Same here, I would make this "MUST offer", and simply make old device
implementations non-compliant. The device should also reject any driver
that does not negotiate the flag, I think?

> +
>  A device SHOULD keep consistent behaviors with the hardware as described in
>  \hyperref[intro:I2C]{I2C}.
>  

No objections from my side to the reset of the changes.


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

Re: [PATCH V5 2/2] virtio: i2c: Allow zero-length transactions

[Viresh Kumar]

On 13-10-21, 11:26, Cornelia Huck wrote:
> On Tue, Oct 12 2021, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > @@ -124,13 +140,23 @@ \subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C
> >  
> >  \drivernormative{\subsubsection}{Device Operation}{Device Types / I2C Adapter Device / Device Operation}
> >  
> > +A driver SHOULD implement the VIRTIO_I2C_F_ZERO_LENGTH_REQUEST feature.
> 
> Make this MUST, or maybe "MUST accept"? The central point of the feature
> flag (at least for me) was to be able to change the semantics and format
> without things breaking silently.

Right.

> I don't think we need to keep old
> driver revisions compliant?

No, we don't.

> > +
> >  A driver MUST set \field{addr} and \field{flags} before sending the request.
> >  
> >  A driver MUST set the reserved bits of \field{flags} to be zero.
> >  
> > +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 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.
> >  
> > @@ -141,6 +167,8 @@ \subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C
> >  
> >  \devicenormative{\subsubsection}{Device Operation}{Device Types / I2C Adapter Device / Device Operation}
> >  
> > +A device SHOULD implement the VIRTIO_I2C_F_ZERO_LENGTH_REQUEST feature.
> 
> Same here, I would make this "MUST offer", and simply make old device
> implementations non-compliant. The device should also reject any driver
> that does not negotiate the flag, I think?

Right.

-- 
viresh

[virtio-dev] Re: [PATCH V5 2/2] virtio: i2c: Allow zero-length transactions

[Cornelia Huck]

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

> On 13-10-21, 11:26, Cornelia Huck wrote:
>> On Tue, Oct 12 2021, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> > @@ -124,13 +140,23 @@ \subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C
>> >  
>> >  \drivernormative{\subsubsection}{Device Operation}{Device Types / I2C Adapter Device / Device Operation}
>> >  
>> > +A driver SHOULD implement the VIRTIO_I2C_F_ZERO_LENGTH_REQUEST feature.
>> 
>> Make this MUST, or maybe "MUST accept"? The central point of the feature
>> flag (at least for me) was to be able to change the semantics and format
>> without things breaking silently.
>
> Right.
>
>> I don't think we need to keep old
>> driver revisions compliant?
>
> No, we don't.

Thinking some more, we'll also probably want the driver to reject any
device that does not offer the feature. Just keep those old device/driver
revisions to themselves :)


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

Re: [PATCH V5 2/2] virtio: i2c: Allow zero-length transactions

[Viresh Kumar]

On 13-10-21, 11:26, Cornelia Huck wrote:
> On Tue, Oct 12 2021, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > +A device SHOULD implement the VIRTIO_I2C_F_ZERO_LENGTH_REQUEST feature.
> 
> Same here, I would make this "MUST offer", and simply make old device
> implementations non-compliant.

> The device should also reject any driver that does not negotiate the
> flag, I think?

Thinking about this a bit more, in practice the device doesn't get to
know about what the driver does with the flag. So how can a device
reject the driver ?

-- 
viresh

[virtio-dev] Re: [PATCH V5 2/2] virtio: i2c: Allow zero-length transactions

[Cornelia Huck]

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

> On 13-10-21, 11:26, Cornelia Huck wrote:
>> On Tue, Oct 12 2021, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> > +A device SHOULD implement the VIRTIO_I2C_F_ZERO_LENGTH_REQUEST feature.
>> 
>> Same here, I would make this "MUST offer", and simply make old device
>> implementations non-compliant.
>
>> The device should also reject any driver that does not negotiate the
>> flag, I think?
>
> Thinking about this a bit more, in practice the device doesn't get to
> know about what the driver does with the flag. So how can a device
> reject the driver ?

It can fail setting FEATURES_OK, if the driver does not set the feature.


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

Re: [PATCH V5 0/2] virtio: i2c: Allow zero-length transactions

[Jie Deng]

On 2021/10/12 19:23, Viresh Kumar wrote:
> Hi,
>
> Arnd suggested (over IRC) to split this into two patches for better readability
> and so here is a resend. The eventual specification hasn't changed at all.
>
> V4->V5:
> - Split into two patches.
>
> V3->V4:
> - Add a new mandatory feature flag.
>
> 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.
>
> ------
>
> 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.
>
> Viresh Kumar (2):
>    virtio: i2c: No need to have separate read-write buffers
>    virtio: i2c: Allow zero-length transactions

LGTM. Thanks Viresh.

Reviewed-by: Jie Deng <jie.deng@intel.com>