All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Michael S. Tsirkin" <mst@redhat.com>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: "Cornelia Huck" <cohuck@redhat.com>,
	"Jie Deng" <jie.deng@intel.com>, "Wolfram Sang" <wsa@kernel.org>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Vincent Guittot" <vincent.guittot@linaro.org>,
	"Jason Wang" <jasowang@redhat.com>,
	"Bill Mills" <bill.mills@linaro.org>,
	"Alex Bennée" <alex.bennee@linaro.org>,
	virtio-dev@lists.oasis-open.org, stratos-dev@op-lists.linaro.org,
	"Gerd Hoffmann" <kraxel@redhat.com>
Subject: Re: [PATCH V2] virtio: i2c: Allow zero-length transactions
Date: Mon, 16 Aug 2021 10:45:58 -0400	[thread overview]
Message-ID: <20210816104430-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <bd0f81517c064b333ea5263bacbb74385e0d9b3a.1629107000.git.viresh.kumar@linaro.org>

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


  reply	other threads:[~2021-08-16 14:45 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-16  9:47 [PATCH V2] virtio: i2c: Allow zero-length transactions Viresh Kumar
2021-08-16 14:45 ` Michael S. Tsirkin [this message]
2021-08-17  3:53   ` Viresh Kumar
2021-08-17  6:47     ` Michael S. Tsirkin
2021-08-17 10:43       ` Viresh Kumar
2021-08-18  2:38         ` [virtio-dev] " Jie Deng
2021-08-18  3:26           ` Viresh Kumar
2021-08-18  8:03             ` Arnd Bergmann
2021-08-18  8:09               ` Viresh Kumar
2021-08-23  7:31                 ` Viresh Kumar
2021-09-01  0:03 ` Michael S. Tsirkin
2021-09-01  6:22   ` Viresh Kumar
2021-09-01  7:22     ` Michael S. Tsirkin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210816104430-mutt-send-email-mst@kernel.org \
    --to=mst@redhat.com \
    --cc=alex.bennee@linaro.org \
    --cc=bill.mills@linaro.org \
    --cc=cohuck@redhat.com \
    --cc=jasowang@redhat.com \
    --cc=jie.deng@intel.com \
    --cc=kraxel@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=stratos-dev@op-lists.linaro.org \
    --cc=vincent.guittot@linaro.org \
    --cc=viresh.kumar@linaro.org \
    --cc=virtio-dev@lists.oasis-open.org \
    --cc=wsa@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.