With the introduction of 26751de25d25 ("spi: bcm2835: Micro-optimise
FIFO loops") it has become apparent that some users might initiate
zero-length SPI transfers. A fact the micro-optimization omitted, and
which turned out to cause crashes[1].
Instead of changing the micro-optimization itself, use a bigger hammer
and skip zero-length transfers altogether for drivers using the default
transfer_one_message() implementation.
Reported-by: Phil Elwell <[email protected]>
Fixes: 26751de25d25 ("spi: bcm2835: Micro-optimise FIFO loops")
Signed-off-by: Nicolas Saenz Julienne <[email protected]>
[1] https://github.com/raspberrypi/linux/issues/4100
---
NOTE: I've reviewed a bunch of drivers and couldn't find a compelling
reason why zero-length transfers should be passed into them. But I
might be missing something.
drivers/spi/spi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 7745eec994fd..b08efe88ccd6 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1269,7 +1269,7 @@ static int spi_transfer_one_message(struct spi_controller *ctlr,
ptp_read_system_prets(xfer->ptp_sts);
}
- if (xfer->tx_buf || xfer->rx_buf) {
+ if ((xfer->tx_buf || xfer->rx_buf) && xfer->len) {
reinit_completion(&ctlr->xfer_completion);
fallback_pio:
--
2.30.0
On Thu, Feb 11, 2021 at 07:08:20PM +0100, Nicolas Saenz Julienne wrote:
> - if (xfer->tx_buf || xfer->rx_buf) {
> + if ((xfer->tx_buf || xfer->rx_buf) && xfer->len) {
I think the issue here is more that some users were passing in buffers
with zero length transfers, the above check was already intended to
catch this case but was working on the assumption that if there was
nothing to transfer then no buffer would be provided.
On Fri, 2021-02-12 at 12:31 +0000, Mark Brown wrote:
> On Thu, Feb 11, 2021 at 07:08:20PM +0100, Nicolas Saenz Julienne wrote:
>
> > - if (xfer->tx_buf || xfer->rx_buf) {
> > + if ((xfer->tx_buf || xfer->rx_buf) && xfer->len) {
>
> I think the issue here is more that some users were passing in buffers
> with zero length transfers, the above check was already intended to
> catch this case but was working on the assumption that if there was
> nothing to transfer then no buffer would be provided.
Fair enough, maybe it makes sense to move the check into __spi_validate() and
propagate an error upwards?
Regads,
Nicolas
On Fri, Feb 12, 2021 at 01:48:21PM +0100, Nicolas Saenz Julienne wrote:
> On Fri, 2021-02-12 at 12:31 +0000, Mark Brown wrote:
> > On Thu, Feb 11, 2021 at 07:08:20PM +0100, Nicolas Saenz Julienne wrote:
> > > - if (xfer->tx_buf || xfer->rx_buf) {
> > > + if ((xfer->tx_buf || xfer->rx_buf) && xfer->len) {
> > I think the issue here is more that some users were passing in buffers
> > with zero length transfers, the above check was already intended to
> > catch this case but was working on the assumption that if there was
> > nothing to transfer then no buffer would be provided.
> Fair enough, maybe it makes sense to move the check into __spi_validate() and
> propagate an error upwards?
No, I think it's fine - there's probably some sensible use case with
drivers reusing a statically allocated transfer/buffer set for multiple
operations and just tweaking the length as needed which seems a bit
weird but I can't think of a reason not to allow it. Your patch is
currently queued, all being well it'll get tested & pushed out later
today.
On Fri, Feb 12, 2021 at 01:57:24PM +0100, Geert Uytterhoeven wrote:
> On Fri, Feb 12, 2021 at 1:55 PM Mark Brown <[email protected]> wrote:
> > No, I think it's fine - there's probably some sensible use case with
> > drivers reusing a statically allocated transfer/buffer set for multiple
> > operations and just tweaking the length as needed which seems a bit
> > weird but I can't think of a reason not to allow it. Your patch is
> > currently queued, all being well it'll get tested & pushed out later
> > today.
> Aren't the zero-length transfers also used to do tricks with the CS signal,
> e.g. combined with cs_change?
The issue wasn't that things were using zero length transfers, the issue
was that drivers were doing zero length transfers but also passing data
buffers which isn't an obvious thing to do given that there will be no
data in those buffers.
Hi Mark,
On Fri, Feb 12, 2021 at 1:55 PM Mark Brown <[email protected]> wrote:
> On Fri, Feb 12, 2021 at 01:48:21PM +0100, Nicolas Saenz Julienne wrote:
> > On Fri, 2021-02-12 at 12:31 +0000, Mark Brown wrote:
> > > On Thu, Feb 11, 2021 at 07:08:20PM +0100, Nicolas Saenz Julienne wrote:
>
> > > > - if (xfer->tx_buf || xfer->rx_buf) {
> > > > + if ((xfer->tx_buf || xfer->rx_buf) && xfer->len) {
>
> > > I think the issue here is more that some users were passing in buffers
> > > with zero length transfers, the above check was already intended to
> > > catch this case but was working on the assumption that if there was
> > > nothing to transfer then no buffer would be provided.
>
> > Fair enough, maybe it makes sense to move the check into __spi_validate() and
> > propagate an error upwards?
>
> No, I think it's fine - there's probably some sensible use case with
> drivers reusing a statically allocated transfer/buffer set for multiple
> operations and just tweaking the length as needed which seems a bit
> weird but I can't think of a reason not to allow it. Your patch is
> currently queued, all being well it'll get tested & pushed out later
> today.
Aren't the zero-length transfers also used to do tricks with the CS signal,
e.g. combined with cs_change?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Thu, 11 Feb 2021 19:08:20 +0100, Nicolas Saenz Julienne wrote:
> With the introduction of 26751de25d25 ("spi: bcm2835: Micro-optimise
> FIFO loops") it has become apparent that some users might initiate
> zero-length SPI transfers. A fact the micro-optimization omitted, and
> which turned out to cause crashes[1].
>
> Instead of changing the micro-optimization itself, use a bigger hammer
> and skip zero-length transfers altogether for drivers using the default
> transfer_one_message() implementation.
>
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
Thanks!
[1/1] spi: Skip zero-length transfers in spi_transfer_one_message()
commit: b306320322c9cfaa465bc2c7367acf6072b1ac0e
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