virtio: i2c: Allow zero-length transactions
v5 cover virtio: i2c: Allow zero-length transactions
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(-)
Signature · 1 line
2.31.1.272.g89b43f80a514
PATCH v5 1/2 virtio: i2c: No need to have separate read-write buffers
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.
--- virtio-i2c.tex | 54 +++++++++++++++++++------------------------------- 1 file changed, 20 insertions(+), 34 deletions(-)
virtio-i2c.tex +20 -34
@@ -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.
Signature · 1 line
2.31.1.272.g89b43f80a514
PATCH v5 2/2 virtio: i2c: Allow zero-length transactions
In reply to Viresh KumarThe 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.
--- virtio-i2c.tex | 34 +++++++++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 3 deletions(-)
virtio-i2c.tex +31 -3
@@ -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}.
Signature · 1 line
2.31.1.272.g89b43f80a514
Re: [PATCH V5 1/2] virtio: i2c: No need to have separate read-write buffers
In reply to Viresh KumarQuoted 11 lines · Viresh Kumar
The virtio I2C protocol allows to contain multiple read-write requests in a single I2C transaction using the
VIRTIO_I2C_FLAGS_FAIL_NEXTflag, 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.
Quoted 4 lines
Remove support for multiple buffers within a single request.
Thanks for splitting up the two patches, I think this is much clearer now.
Quoted 3 lines · Viresh Kumar
Signed-off-by Viresh Kumar <[email protected]>---
I have a few very minor comments, regardless of how you address those:
Quoted 5 lines
virtio-i2c.tex +0
-0
virtio-i2c.tex \subsection{Virtqueues}\label{sec:Device Types / I2C Adapter Device / Virtqueues · line 24
Quoted 9 lines
virtio-i2c.tex +5
-1@@ -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.
virtio-i2c.tex \subsubsection{Device Operation: Request Queue}\label{sec:Device Types / I2C Ada · line 116
Quoted 12 lines
virtio-i2c.tex +8
-0@@ -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.
virtio-i2c.tex \subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C · line 143
Quoted 5 lines
virtio-i2c.tex +1
-0@@ -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".
virtio-i2c.tex \subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C · line 170
Quoted 5 lines
virtio-i2c.tex +1
-0@@ -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
Quoted 26 lines · 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_NEXTflag.https://lists.oasis-open.org/archives/virtio-comment/202011/msg00005.html
Quoted 7 lines
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.
Quoted 5 lines · Arnd Bergmann
Quoted 5 lines
virtio-i2c.tex +0
-0@@ -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}
Quoted 7 lines
virtio-i2c.tex \subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C · line 143
Quoted 1 line
virtio-i2c.tex +1
-0+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_REQUESTfeature is available".
Agree. "MAY" is better.
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Quoted 8 lines · Arnd Bergmann
virtio-i2c.tex \subsection{Virtqueues}\label{sec:Device Types / I2C Adapter Device / Virtqueues · line 24
Quoted 5 lines · Viresh Kumar
+\begin{description}
+\item[VIRTIO_I2C_F_ZERO_LENGTH_REQUEST (0)] The device supports zero-length I2Cvirtio-i2c.tex +3
-0+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
Quoted 1 line
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.
Quoted 2 lines
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.
Quoted 15 lines
virtio-i2c.tex \subsubsection{Device Operation: Request Queue}\label{sec:Device Types / I2C Ada · line 116
Quoted 12 lines
virtio-i2c.tex +8
-0@@ -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
Quoted 1 line
... like the SMBus "Quick" command.
Sure.
Quoted 4 lines
Quoted 4 lines
virtio-i2c.tex +0
-0@@ -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}
Quoted 8 lines
virtio-i2c.tex \subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C · line 143
Quoted 1 line
virtio-i2c.tex +1
-0+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_REQUESTfeature 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.
Signature · 1 line
viresh
Quoted 10 lines · Viresh Kumar
Quoted 10 lines
virtio-i2c.tex \subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C · line 143
Quoted 6 lines
virtio-i2c.tex +1
-0@@ -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
Quoted 6 lines
Quoted 2 lines
"A driver MAY assume that the
VIRTIO_I2C_F_ZERO_LENGTH_REQUESTfeature is available".The problem here is that "
VIRTIO_I2C_FLAGS_M_RD" is supported only withVIRTIO_I2C_F_ZERO_LENGTH_REQUESTfeature. 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.
Quoted 2 lines · Jie Deng
The "
VIRTIO_I2C_FLAGS_M_RD" is consistent with the permissions of the buffer.
Yes.
Quoted 2 lines
When this flag is set, the buffer is also marked as device write -only with
VIRTQ_DESC_F_WRITEset.
Yes.
Quoted 2 lines
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...
Signature · 1 line
viresh
Quoted 24 lines · 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.
Signed-off-by Viresh Kumar <[email protected]>--- virtio-i2c.tex | 34 +++++++++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 3 deletions(-)virtio-i2c.tex +0
-0
virtio-i2c.tex \subsection{Virtqueues}\label{sec:Device Types / I2C Adapter Device / Virtqueues · line 23
Quoted 8 lines
virtio-i2c.tex +4
-1@@ -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?
Quoted 4 lines
virtio-i2c.tex +1
-0+\end{description} \subsection{Device configuration layout}\label{sec:Device Types / I2C Adapter Device / Device configuration layout}
(...)
virtio-i2c.tex \subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C · line 143
Quoted 5 lines
virtio-i2c.tex +1
-0@@ -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?
Quoted 20 lines
virtio-i2c.tex +9
-0+ 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.
virtio-i2c.tex \subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C · line 170
Quoted 4 lines
virtio-i2c.tex +1
-0@@ -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?
virtio-i2c.tex \subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C · line 174
Quoted 4 lines
virtio-i2c.tex +1
-0+ 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: [email protected] For additional commands, e-mail: [email protected]
Quoted 10 lines · Cornelia Huck
virtio-i2c.tex \subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C · line 143
Quoted 5 lines · Viresh Kumar
virtio-i2c.tex +1
-0@@ -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.
Quoted 2 lines
I don't think we need to keep old
driver revisions compliant?
No, we don't.
Quoted 21 lines
Quoted 21 lines
virtio-i2c.tex +9
-0+ 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
Quoted 7 lines
virtio-i2c.tex \subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C · line 170
Quoted 3 lines
virtio-i2c.tex +1
-0\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.
Signature · 1 line
viresh
Quoted 12 lines · Viresh Kumar
Quoted 10 lines · Cornelia Huck
virtio-i2c.tex \subsubsection{Device Operation: Operation Status}\label{sec:Device Types / I2C · line 143
Quoted 5 lines · Viresh Kumar
virtio-i2c.tex +1
-0@@ -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.
Quoted 6 lines
Right.
Quoted 2 lines
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: [email protected] For additional commands, e-mail: [email protected]
Quoted 8 lines · Cornelia Huck
Quoted 1 line · Viresh Kumar
+A device SHOULD implement the
VIRTIO_I2C_F_ZERO_LENGTH_REQUESTfeature.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 ?
Signature · 1 line
viresh
Quoted 7 lines · Viresh Kumar
Quoted 5 lines · Cornelia Huck
Quoted 1 line · Viresh Kumar
+A device SHOULD implement the
VIRTIO_I2C_F_ZERO_LENGTH_REQUESTfeature.Same here, I would make this "MUST offer", and simply make old device implementations non-compliant.
Quoted 6 lines
Quoted 2 lines
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: [email protected] For additional commands, e-mail: [email protected]