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

[Viresh Kumar <viresh.kumar@linaro.org>]

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