spi: spi-gpio: Don't set MOSI as an input if not 3WIRE mode

  The addition of 3WIRE support would affect MOSI direction even
when still in standard (4 wire) mode. This can lead to MOSI being
at an invalid logic level when a device driver sets an SPI
message with a NULL tx_buf.

spi.h states that if tx_buf is NULL then "zeros will be shifted
out ... " If MOSI is tristated then the data shifted out is subject
to pull resistors, keepers, or in the absence of those, noise.

This issue came to light when using spi-gpio connected to an
ADS7843 touchscreen controller. MOSI pulled high when clocking
MISO data in caused the SPI device to interpret this as a command
which would put the device in an unexpected and non-functional
state.

Fixes: 4b859db2c606 ("spi: spi-gpio: add SPI_3WIRE support")
Fixes: 5132b3d28371 ("spi: gpio: Support 3WIRE high-impedance turn-around")
Signed-off-by: Kris Bahnsen <kris@embeddedTS.com>
---

As an aside, I wasn't sure how to best put down the Fixes: tags.
4b859db2c606 ("spi: spi-gpio: add SPI_3WIRE support") introduced the
actual bug, but 5132b3d28371 ("spi: gpio: Support 3WIRE high-impedance turn-around")
modified that commit slightly and is what this patch actually applies
to. Let me know if marking both as fixes is incorrect and I can
create another patch.

 drivers/spi/spi-gpio.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)
  

On Wed, Dec 07, 2022 at 03:08:53PM -0800, Kris Bahnsen wrote:
> The addition of 3WIRE support would affect MOSI direction even
> when still in standard (4 wire) mode. This can lead to MOSI being
> at an invalid logic level when a device driver sets an SPI
> message with a NULL tx_buf.
> 
> spi.h states that if tx_buf is NULL then "zeros will be shifted
> out ... " If MOSI is tristated then the data shifted out is subject
> to pull resistors, keepers, or in the absence of those, noise.
> 
> This issue came to light when using spi-gpio connected to an
> ADS7843 touchscreen controller. MOSI pulled high when clocking
> MISO data in caused the SPI device to interpret this as a command
> which would put the device in an unexpected and non-functional
> state.

A cleaner fix which is probably marginally more performant would be to
make the setting of spi_gpio_set_direction() conditional on SPI_3WIRE -
then we won't call into the function at all when not doing 3 wire,
avoiding the issue entirely.

> As an aside, I wasn't sure how to best put down the Fixes: tags.
> 4b859db2c606 ("spi: spi-gpio: add SPI_3WIRE support") introduced the
> actual bug, but 5132b3d28371 ("spi: gpio: Support 3WIRE high-impedance turn-around")
> modified that commit slightly and is what this patch actually applies
> to. Let me know if marking both as fixes is incorrect and I can
> create another patch.

That's fine, it doesn't really matter either way.
  

On Wed, 2022-12-07 at 23:44 +0000, Mark Brown wrote:
> On Wed, Dec 07, 2022 at 03:08:53PM -0800, Kris Bahnsen wrote:
> > The addition of 3WIRE support would affect MOSI direction even
> > when still in standard (4 wire) mode. This can lead to MOSI being
> > at an invalid logic level when a device driver sets an SPI
> > message with a NULL tx_buf.
> > 
> > spi.h states that if tx_buf is NULL then "zeros will be shifted
> > out ... " If MOSI is tristated then the data shifted out is subject
> > to pull resistors, keepers, or in the absence of those, noise.
> > 
> > This issue came to light when using spi-gpio connected to an
> > ADS7843 touchscreen controller. MOSI pulled high when clocking
> > MISO data in caused the SPI device to interpret this as a command
> > which would put the device in an unexpected and non-functional
> > state.
> 
> A cleaner fix which is probably marginally more performant would be to
> make the setting of spi_gpio_set_direction() conditional on SPI_3WIRE -
> then we won't call into the function at all when not doing 3 wire,
> avoiding the issue entirely.

That makes sense to me. I was operating under the assumption that 3WIRE
mode could be switched in to at a later time via ioctl(), but with the
death of spidev that is presumably no longer a concern.

I'll get a v2 put together and probably sent in tomorrow. Thanks.

> 
> > As an aside, I wasn't sure how to best put down the Fixes: tags.
> > 4b859db2c606 ("spi: spi-gpio: add SPI_3WIRE support") introduced the
> > actual bug, but 5132b3d28371 ("spi: gpio: Support 3WIRE high-impedance turn-around")
> > modified that commit slightly and is what this patch actually applies
> > to. Let me know if marking both as fixes is incorrect and I can
> > create another patch.
> 
> That's fine, it doesn't really matter either way.
  

On Wed, Dec 07, 2022 at 04:36:41PM -0800, Kris Bahnsen wrote:
> On Wed, 2022-12-07 at 23:44 +0000, Mark Brown wrote:

> > A cleaner fix which is probably marginally more performant would be to
> > make the setting of spi_gpio_set_direction() conditional on SPI_3WIRE -
> > then we won't call into the function at all when not doing 3 wire,
> > avoiding the issue entirely.

> That makes sense to me. I was operating under the assumption that 3WIRE
> mode could be switched in to at a later time via ioctl(), but with the
> death of spidev that is presumably no longer a concern.

Ugh, right, spidev.  Really even with spidev devices should probably
have the mode configured beforehand (I'm not sure pinmux will do the
right thing on most platforms...) but now I check it's part of the ABI
so we can't get rid of it and therefore your current patch probably is
what we need.  No need to reroll, sorry for the noise :/

I'm not sure why you think spidev is dying, it does still exist and
devices use it?
  

On Thu, 2022-12-08 at 00:42 +0000, Mark Brown wrote:
> On Wed, Dec 07, 2022 at 04:36:41PM -0800, Kris Bahnsen wrote:
> > On Wed, 2022-12-07 at 23:44 +0000, Mark Brown wrote:
> > > A cleaner fix which is probably marginally more performant would be to
> > > make the setting of spi_gpio_set_direction() conditional on SPI_3WIRE -
> > > then we won't call into the function at all when not doing 3 wire,
> > > avoiding the issue entirely.
> > That makes sense to me. I was operating under the assumption that 3WIRE
> > mode could be switched in to at a later time via ioctl(), but with the
> > death of spidev that is presumably no longer a concern.
> 
> Ugh, right, spidev.  Really even with spidev devices should probably
> have the mode configured beforehand (I'm not sure pinmux will do the
> right thing on most platforms...) but now I check it's part of the ABI
> so we can't get rid of it and therefore your current patch probably is
> what we need.  No need to reroll, sorry for the noise :/

No need to apologize, thanks for the followup. I'm not terribly
familiar with SPI internals in Linux so I'm not sure how deep that
rabbit hole goes. Let me know if you change your mind, I will happily
whip something else up.

> 
> I'm not sure why you think spidev is dying, it does still exist and
> devices use it?

"The death of spidev" _as a generic interface_.

A number of our products provide a generic pin header with SPI
available for customer use. We've been told when we RFC'ed dts files
to support our platforms that spidev isn't acceptable on these
headers and the downstream developer must add their own as needed.
Which, many of our customers use devices that don't have drivers
anyway, so we still have to assist them in getting spidev functional
in one way or another. It's just a sore spot for us.
  

On Wed, 07 Dec 2022 15:08:53 -0800, Kris Bahnsen wrote:
> The addition of 3WIRE support would affect MOSI direction even
> when still in standard (4 wire) mode. This can lead to MOSI being
> at an invalid logic level when a device driver sets an SPI
> message with a NULL tx_buf.
> 
> spi.h states that if tx_buf is NULL then "zeros will be shifted
> out ... " If MOSI is tristated then the data shifted out is subject
> to pull resistors, keepers, or in the absence of those, noise.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: spi-gpio: Don't set MOSI as an input if not 3WIRE mode
      commit: 3a6f994f848a69deb2bf3cd9d130dd0c09730e55

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark