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

Message ID 20180903215035.17265-2-linus.walleij@linaro.org
State New
Headers show
Series
  • SPI 3WIRE fixes and highz turnaround
Related show

Commit Message

Linus Walleij Sept. 3, 2018, 9:50 p.m.
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.

Transfers with TX and RX at the same time will not work
however (just one wire after all), so be sure to disallow
those.

Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
 drivers/spi/spi.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

-- 
2.17.1

Comments

Lorenzo Bianconi Sept. 4, 2018, 7:38 a.m. | #1
On Sep 03, 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.

> 

> Transfers with TX and RX at the same time will not work

> however (just one wire after all), so be sure to disallow

> those.

> 

> Cc: Andrzej Hajda <a.hajda@samsung.com>

> Cc: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> ---

>  drivers/spi/spi.c | 15 +++++++++++----

>  1 file changed, 11 insertions(+), 4 deletions(-)

> 

> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c

> index ec395a6baf9c..f6f9314e9a18 100644

> --- a/drivers/spi/spi.c

> +++ b/drivers/spi/spi.c

> @@ -2841,10 +2841,17 @@ static int __spi_validate(struct spi_device *spi, struct spi_message *message)

>  		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;

> +			/*

> +			 * 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;

> +			}

>  		}

>  	}

>  

> -- 

> 2.17.1


That is the way ST devices work: W(addr)R(data) 
I did not spot the issue since another 4-wire device was connected to the
controller (so MISO was connected).

Acked-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
Mark Brown Sept. 4, 2018, 5:22 p.m. | #2
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...

>  		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.
Linus Walleij Sept. 4, 2018, 8:47 p.m. | #3
On Tue, Sep 4, 2018 at 7:22 PM Mark Brown <broonie@kernel.org> 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
Linus Walleij Sept. 4, 2018, 8:55 p.m. | #4
On Tue, Sep 4, 2018 at 10:47 PM Linus Walleij <linus.walleij@linaro.org> wrote:

Ah
> > > +                     if (spi->mode & SPI_CONTROLLER_HALF_DUPLEX) {


Hm that should have rather been flags... not spi->mode.

OK I think I have the patch in a better state now. Bear
with me.

Yours,
Linus Walleij
Mark Brown Sept. 5, 2018, 9:39 a.m. | #5
On Tue, Sep 04, 2018 at 10:47:14PM +0200, Linus Walleij wrote:

> 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.


AFAICT from the rest of the series the root cause here is that you're
trying to work around the GPIO controller setting the wrong flags rather
than an actual fix here - there's no need for any change that I can see.
Linus Walleij Sept. 5, 2018, 1:09 p.m. | #6
On Wed, Sep 5, 2018 at 11:39 AM Mark Brown <broonie@kernel.org> wrote:
> On Tue, Sep 04, 2018 at 10:47:14PM +0200, Linus Walleij wrote:

>

> > 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.

>

> AFAICT from the rest of the series the root cause here is that you're

> trying to work around the GPIO controller setting the wrong flags rather

> than an actual fix here - there's no need for any change that I can see.


I think that may be up to the interpretation of
SPI_[MASTER|CONTROLLER]_NO_[RX|TX] flags.

From the code and the bitbanging inlines it is clear that
the actual semantics of these flags are:
SPI_MASTER_NO_RX == does not have a MISO line
SPI_MASTER_NO_TX == does not have a MOSI line

But these names are pretty confusing, since a 3WIRE
only has a single line (MISO) but can do both RX and
TX transfers.

Maybe I should make a patch renaming the flags
as SPI_*_NO_MISO, SPI_*_NO_MOSI
as that is how they are used in the code.

Yours,
Linus Walleij

Patch

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index ec395a6baf9c..f6f9314e9a18 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2841,10 +2841,17 @@  static int __spi_validate(struct spi_device *spi, struct spi_message *message)
 		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;
+			/*
+			 * 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;
+			}
 		}
 	}