Re: [PATCH] spi: Skip zero-length transfers in spi_transfer_one_message()
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
Re: [PATCH] spi: Skip zero-length transfers in spi_transfer_one_message()
On Fri, Feb 12, 2021 at 01:57:24PM +0100, Geert Uytterhoeven wrote: > On Fri, Feb 12, 2021 at 1:55 PM Mark Brown 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. signature.asc Description: PGP signature
Re: [PATCH] spi: Skip zero-length transfers in spi_transfer_one_message()
Hi Mark,
On Fri, Feb 12, 2021 at 1:55 PM Mark Brown wrote:
> On Fri, Feb 12, 2021 at 01:48:21PM +0100, Nicolas Saenz Julienne wrote:
> > On Fri, 2021-02-12 at 12:31 +, 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 -- ge...@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
Re: [PATCH] spi: Skip zero-length transfers in spi_transfer_one_message()
On Fri, Feb 12, 2021 at 01:48:21PM +0100, Nicolas Saenz Julienne wrote:
> On Fri, 2021-02-12 at 12:31 +, 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.
signature.asc
Description: PGP signature
Re: [PATCH] spi: Skip zero-length transfers in spi_transfer_one_message()
On Fri, 2021-02-12 at 12:31 +, 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
signature.asc
Description: This is a digitally signed message part
Re: [PATCH] spi: Skip zero-length transfers in spi_transfer_one_message()
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.
signature.asc
Description: PGP signature
