| Message ID | 20210211180820.25757-1-nsaenzjulienne@suse.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b306320322c9cfaa465bc2c7367acf6072b1ac0e |
| Headers | show |
| Series | spi: Skip zero-length transfers in spi_transfer_one_message() | expand |
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.
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
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.
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
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:
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(-)