diff mbox series

[1/5] spi: spi-sun4i: sun4i_spi_transfer_one(): fix transfers for SUN4I_FIFO_DEPTH

Message ID 20260902-spi-sun4i-fixes-v1-1-19985ef75673@pengutronix.de (mailing list archive)
State New
Headers
Series spi: spi-sun4i: assorted fixes |

Commit Message

Jonas Rebmann Sept. 2, 2026, 3:35 p.m. UTC 
From: Marc Kleine-Budde <mkl@pengutronix.de>

In commit 6d9fe44bd73d ("spi: sun4i: fix FIFO limit"), the TX-FIFO is
filled max to SUN4I_FIFO_DEPTH - 1 (= 63) bytes to work around timeouts
observed on A10s SoCs.

Commit 196737912da5 ("spi: sun4i: Allow transfers larger than FIFO
size") added support for transfers larger than the FIFO size. But this
commit only enabled the TX-FIFO empty interrupt for transfers larger
than the FIFO (= 64) bytes.

This breaks transfers with exactly 64 bytes: the TX-FIFO is only filled
with 63 bytes but the interrupt to refill the FIFO is not triggered. The
problem can be reproduced with the following command:

| spidev_test -D /dev/spidev0.1 -S 64 -s 20000000 -I 1
|
| [ 7797.548745] spi_master spi0: spi0.1: timeout transferring 64 bytes@20000000Hz for 110(100)ms
| [ 7797.557237] spidev spi0.1: SPI transfer failed: -110
| [ 7797.562308] spi_master spi0: failed to transfer one message from queue
| [ 7797.568936] spi_master spi0: noqueue transfer failed

To fix the problem enable the TX-FIFO interrupt if the total TX length
is larger than SUN4I_FIFO_DEPTH - 1 (= 63) bytes.

Fixes: 196737912da5 ("spi: sun4i: Allow transfers larger than FIFO size")
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Jonas Rebmann <jre@pengutronix.de>
---
 drivers/spi/spi-sun4i.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

sashiko-bot@kernel.org Sept. 2, 2026, 3:49 p.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] If the hardware reports a TX FIFO count greater than SUN4I_FIFO_DEPTH - 1 (e.g., a full FIFO of 64), sun4i_spi_fill_fifo encounters an integer underflow and enters a nearly infinite loop.
--

commit bfe90ee7eb547aed612b50ceef43a3538623eb4d
Author: Marc Kleine-Budde <mkl@pengutronix.de>

spi: spi-sun4i: sun4i_spi_transfer_one(): fix transfers for SUN4I_FIFO_DEPTH

This commit fixes SPI transfers with exactly 64 bytes by enabling the
TX-FIFO empty interrupt when the transfer length is greater than
SUN4I_FIFO_DEPTH - 1. Previously, the interrupt was only enabled for
transfers strictly larger than the 64-byte FIFO, causing timeouts since
only 63 bytes are initially filled.

> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
> index d5c16392cd4d7..2e23244539053 100644
> --- a/drivers/spi/spi-sun4i.c
> +++ b/drivers/spi/spi-sun4i.c
> @@ -322,7 +322,7 @@ static int sun4i_spi_transfer_one(struct spi_controller *host,
>  	sun4i_spi_enable_interrupt(sspi, SUN4I_INT_CTL_TC |
>  					 SUN4I_INT_CTL_RF_F34);
>  	/* Only enable Tx FIFO interrupt if we really need it */
> -	if (tx_len > SUN4I_FIFO_DEPTH)
> +	if (tx_len > SUN4I_FIFO_DEPTH - 1)
>  		sun4i_spi_enable_interrupt(sspi, SUN4I_INT_CTL_TF_E34);

[Severity: High]
This isn't a bug introduced by this patch, but does the hardware reporting a
TX FIFO count greater than SUN4I_FIFO_DEPTH - 1 cause an integer underflow?

If sun4i_spi_get_tx_fifo_count(sspi) returns a value >= 64 (which is
structurally possible since SUN4I_FIFO_DEPTH is 64 and the register mask is
0x7f), the unsigned subtraction in sun4i_spi_fill_fifo() underflows:

drivers/spi/spi-sun4i.c:sun4i_spi_fill_fifo() {
	cnt = SUN4I_FIFO_DEPTH - 1 - sun4i_spi_get_tx_fifo_count(sspi);

	len = min_t(int, cnt, sspi->len);

	while (len--) {
		...
}

This underflow produces 0xFFFFFFFF, which min_t(int, cnt, sspi->len) casts
to -1.

This negative length could cause the while (len--) loop to evaluate -1 as
true, turning it into an infinite loop that executes ~4 billion times and
overruns sspi->tx_buf.

This problem appears to be made more likely by the subsequent commit "spi:
spi-sun4i: sun4i_spi_fill_fifo(): always apply A10 FIFO fill level
workaround" which lowered the underflow threshold from > 64 to >= 64.

Can this out-of-bounds memory access occur if the hardware FIFO is completely
full (64 bytes) or reports spurious values?

>  
>  	/* Start the transfer */
diff mbox series

Patch

diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
index d5c16392cd4d..2e2324453905 100644
--- a/drivers/spi/spi-sun4i.c
+++ b/drivers/spi/spi-sun4i.c
@@ -322,7 +322,7 @@  static int sun4i_spi_transfer_one(struct spi_controller *host,
 	sun4i_spi_enable_interrupt(sspi, SUN4I_INT_CTL_TC |
 					 SUN4I_INT_CTL_RF_F34);
 	/* Only enable Tx FIFO interrupt if we really need it */
-	if (tx_len > SUN4I_FIFO_DEPTH)
+	if (tx_len > SUN4I_FIFO_DEPTH - 1)
 		sun4i_spi_enable_interrupt(sspi, SUN4I_INT_CTL_TF_E34);
 
 	/* Start the transfer */