Re: [PATCH 1/4] spi: core: Allow both TX and RX transfers in 3WIRE

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Tue, Sep 4, 2018 at 7:22 PM Mark Brown <broonie@xxxxxxxxxx> wrote:
> On Mon, Sep 03, 2018 at 11:50:32PM +0200, Linus Walleij wrote:
> > The SPI message validation code in __spi_validate() is too
> > restrictive on 3WIRE transfers: the core bitbanging code,
> > for example, will gladly switch direction of the line
> > inbetween transfers.
>
> > Allow 3WIRE messages even if there is both TX and RX
> > transfers in the message.
>
> *Any* SPI message is likely to have mixes of TX and RX only transfers,
> that's an incredibly normal mode of operation...

OK I fixed the wording.

> >               list_for_each_entry(xfer, &message->transfers, transfer_list) {
> >                       if (xfer->rx_buf && xfer->tx_buf)
> >                               return -EINVAL;
> > -                     if ((flags & SPI_CONTROLLER_NO_TX) && xfer->tx_buf)
> > -                             return -EINVAL;
> > -                     if ((flags & SPI_CONTROLLER_NO_RX) && xfer->rx_buf)
> > -                             return -EINVAL;
>
> The above check doesn't check over the entire message, it checks that
> the individual transfer can be physically supported by the device.
>
> > +                     /*
> > +                      * 3WIRE can indeed do a write message followed by a
> > +                      * read message, the direction of the line will be
> > +                      * switched between the two messages.
> > +                      */
> > +                     if (spi->mode & SPI_CONTROLLER_HALF_DUPLEX) {
> > +                             if ((flags & SPI_CONTROLLER_NO_TX) && xfer->tx_buf)
> > +                                     return -EINVAL;
> > +                             if ((flags & SPI_CONTROLLER_NO_RX) && xfer->rx_buf)
> > +                                     return -EINVAL;
> > +                     }
>
> This is broken, we now no longer check that transfers are valid for
> single duplex devices.

The whole loop is inside this (outside of patch context):

if ((ctlr->flags & SPI_CONTROLLER_HALF_DUPLEX) ||
        (spi->mode & SPI_3WIRE)) {
        (...)
        list_for_each_entry(xfer, &message->transfers, transfer_list) {
        (...)

So the only thing that is excluded by the if() is actually the 3WIRE
mode.

But it's confusing and fragile, I've heard this way of coding has
a name (probably not a pretty one) and should be avoided. Geert
had a better idea on how to do it so I rewrote it in a cleaner way.

Yours,
Linus Walleij