* [PATCH] spi: Skip zero-length transfers in spi_transfer_one_message() @ 2021-02-11 18:08 Nicolas Saenz Julienne 2021-02-12 12:31 ` Mark Brown 2021-02-12 14:01 ` Mark Brown 0 siblings, 2 replies; 7+ messages in thread From: Nicolas Saenz Julienne @ 2021-02-11 18:08 UTC (permalink / raw) To: Mark Brown, Robin Murphy Cc: Nicolas Saenz Julienne, Phil Elwell, linux-spi, linux-kernel 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 <phil@raspberrypi.com> Fixes: 26751de25d25 ("spi: bcm2835: Micro-optimise FIFO loops") Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> [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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] spi: Skip zero-length transfers in spi_transfer_one_message() 2021-02-11 18:08 [PATCH] spi: Skip zero-length transfers in spi_transfer_one_message() Nicolas Saenz Julienne @ 2021-02-12 12:31 ` Mark Brown 2021-02-12 12:48 ` Nicolas Saenz Julienne 2021-02-12 14:01 ` Mark Brown 1 sibling, 1 reply; 7+ messages in thread From: Mark Brown @ 2021-02-12 12:31 UTC (permalink / raw) To: Nicolas Saenz Julienne; +Cc: Robin Murphy, Phil Elwell, linux-spi, linux-kernel [-- Attachment #1: Type: text/plain, Size: 432 bytes --] 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. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] spi: Skip zero-length transfers in spi_transfer_one_message() 2021-02-12 12:31 ` Mark Brown @ 2021-02-12 12:48 ` Nicolas Saenz Julienne 2021-02-12 12:52 ` Mark Brown 0 siblings, 1 reply; 7+ messages in thread From: Nicolas Saenz Julienne @ 2021-02-12 12:48 UTC (permalink / raw) To: Mark Brown; +Cc: Robin Murphy, Phil Elwell, linux-spi, linux-kernel [-- Attachment #1: Type: text/plain, Size: 645 bytes --] 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 [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] spi: Skip zero-length transfers in spi_transfer_one_message() 2021-02-12 12:48 ` Nicolas Saenz Julienne @ 2021-02-12 12:52 ` Mark Brown 2021-02-12 12:57 ` Geert Uytterhoeven 0 siblings, 1 reply; 7+ messages in thread From: Mark Brown @ 2021-02-12 12:52 UTC (permalink / raw) To: Nicolas Saenz Julienne; +Cc: Robin Murphy, Phil Elwell, linux-spi, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1052 bytes --] 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. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] spi: Skip zero-length transfers in spi_transfer_one_message() 2021-02-12 12:52 ` Mark Brown @ 2021-02-12 12:57 ` Geert Uytterhoeven 2021-02-12 13:05 ` Mark Brown 0 siblings, 1 reply; 7+ messages in thread From: Geert Uytterhoeven @ 2021-02-12 12:57 UTC (permalink / raw) To: Mark Brown Cc: Nicolas Saenz Julienne, Robin Murphy, Phil Elwell, linux-spi, Linux Kernel Mailing List Hi Mark, On Fri, Feb 12, 2021 at 1:55 PM Mark Brown <broonie@kernel.org> 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 -- geert@linux-m68k.org 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] spi: Skip zero-length transfers in spi_transfer_one_message() 2021-02-12 12:57 ` Geert Uytterhoeven @ 2021-02-12 13:05 ` Mark Brown 0 siblings, 0 replies; 7+ messages in thread From: Mark Brown @ 2021-02-12 13:05 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Nicolas Saenz Julienne, Robin Murphy, Phil Elwell, linux-spi, Linux Kernel Mailing List [-- Attachment #1: Type: text/plain, Size: 870 bytes --] On Fri, Feb 12, 2021 at 01:57:24PM +0100, Geert Uytterhoeven wrote: > On Fri, Feb 12, 2021 at 1:55 PM Mark Brown <broonie@kernel.org> 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. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] spi: Skip zero-length transfers in spi_transfer_one_message() 2021-02-11 18:08 [PATCH] spi: Skip zero-length transfers in spi_transfer_one_message() Nicolas Saenz Julienne 2021-02-12 12:31 ` Mark Brown @ 2021-02-12 14:01 ` Mark Brown 1 sibling, 0 replies; 7+ messages in thread From: Mark Brown @ 2021-02-12 14:01 UTC (permalink / raw) To: Robin Murphy, Nicolas Saenz Julienne; +Cc: linux-spi, linux-kernel, Phil Elwell 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-02-12 14:04 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-11 18:08 [PATCH] spi: Skip zero-length transfers in spi_transfer_one_message() Nicolas Saenz Julienne 2021-02-12 12:31 ` Mark Brown 2021-02-12 12:48 ` Nicolas Saenz Julienne 2021-02-12 12:52 ` Mark Brown 2021-02-12 12:57 ` Geert Uytterhoeven 2021-02-12 13:05 ` Mark Brown 2021-02-12 14:01 ` Mark Brown
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.