| Message ID | 20180903215035.17265-2-linus.walleij@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | SPI 3WIRE fixes and highz turnaround | expand |
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>
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.
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
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
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.
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
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; + } } }
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