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.
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
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.
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:
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.
Make the buffer optional, for zero-length transfers.
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
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
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.
+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.
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
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.
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 ?
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
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.
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.
stratos-dev@op-lists.linaro.org
-
Arnd Bergmann -
Jie Deng -
Michael S. Tsirkin -
Viresh Kumar