diff mbox series

[v2,4/6] spi: spi-sun4i: sun4i_spi_handler(): fix race condition between transfer completion and RX FIFO interrupt

Message ID 20260907-spi-sun4i-fixes-v2-4-7e805662b9bd@pengutronix.de (mailing list archive)
State New
Headers
Series spi: spi-sun4i: assorted fixes |

Commit Message

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

In commit 196737912da5 ("spi: sun4i: Allow transfers larger than FIFO
size"), support for transfers larger than the FIFO size was added.

This commit moves the draining of the RX-FIFO from
sun4i_spi_transfer_one() (after completion of the transfer) to the IRQ
handler when the IRQ "transfer complete" is handled. In addition, the
IRQ "RX-FIFO 3/4 full" is activated for all transfers.

However, this does not take into account that the RX-IRQ for transfers
that exceed 3/4 of the FIFO size is still pending after the IRQ
"transfer complete" has been processed. All interrupt sources are only
deactivated after the wait_for_completion_timeout() in
sun4i_spi_transfer_one().

This opens a race window for "RX-FIFO 3/4 full" interrupts to come.
The sequence is as follows:

| sun4i_spi_transfer_one()
|     sun4i_spi_fill_fifo()       // fill TX-FIFO with 48 bytes
|     // enable RX-FIFO 3/4 full IRQ
|     wait_for_completion_timeout();
|
| // SPI controller transfers 48 bytes
| // SPI controller issues "transfer complete" and "RX-FIFO 3/4 full" IRQ
|
| // IRQ handler start
|     sun4i_spi_handler()
|         // ACK "transfer complete" IRQ
|         sun4i_spi_drain_fifo();
|         complete();                         ----.
|         return IRQ_HANDLED;                      \
| // IRQ handler end                                \__ race
|                                                   /   window
|     // wait_for_completion_timeout() continues   /
|     // disable all IRQ sources              ----'

Avoid the race condition by disabling all interrupts when handling the
"transfer complete" IRQ and before calling complete(). Also move the
draining of the RX-FIFO back into sun4i_spi_transfer_one() where it
was before commit 196737912da5 ("spi: sun4i: Allow transfers larger than
FIFO size").

This has the added benefit of spending a little less time in the IRQ
handler.

Cc: Tobias Schramm <t.schramm@manjaro.org>
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 | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

Comments

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

New issues:
- [High] Missing Interrupt Acknowledgment causes race condition and data corruption.

Pre-existing issues:
- [High] Interrupt requested before clocks enabled and synchronization primitives initialized.
--

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

spi: spi-sun4i: sun4i_spi_handler(): fix race condition between transfer completion and RX FIFO interrupt

This commit moves RX-FIFO draining back to sun4i_spi_transfer_one() and
disables all interrupts in the IRQ handler when processing the transfer
complete interrupt to close a race window.

> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
> index 8a9dcd3b6b8f4..ea8be0170fbfd 100644
> --- a/drivers/spi/spi-sun4i.c
> +++ b/drivers/spi/spi-sun4i.c

[ ... ]

> @@ -357,8 +359,7 @@ static irqreturn_t sun4i_spi_handler(int irq, void *dev_id)
>  
>  	/* Transfer complete */
>  	if (status & SUN4I_INT_CTL_TC) {
> -		sun4i_spi_write(sspi, SUN4I_INT_STA_REG, SUN4I_INT_CTL_TC);
> -		sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
> +		sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, 0);

[Severity: High]
Does this change leave the Transfer Complete (TC) bit active in the status
register?

It looks like replacing the write to SUN4I_INT_STA_REG with a write to
SUN4I_INT_CTL_REG masks the interrupt, but fails to acknowledge and clear
the pending status bit.

If a spurious interrupt fires during the setup of a new transfer, could this
cause the new transfer to complete prematurely? For example, in
sun4i_spi_transfer_one(), there is a window between reinitializing the
completion and clearing the status register:

	reinit_completion(&sspi->done);
	sspi->tx_buf = tfr->tx_buf;
	sspi->rx_buf = tfr->rx_buf;
	sspi->len = tfr->len;

	/* Clear pending interrupts */
	sun4i_spi_write(sspi, SUN4I_INT_STA_REG, ~0);

If a spurious interrupt arrives just before SUN4I_INT_STA_REG is cleared,
the handler will read the stale TC bit and call complete(&sspi->done). The
wait_for_completion_timeout() would then return immediately, potentially
leading to use-after-free of DMA buffers or silent data corruption since the
hardware might still be active.

>  		complete(&sspi->done);
>  		return IRQ_HANDLED;
>  	}

[ ... ]

> @@ -456,6 +457,7 @@ static int sun4i_spi_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	sspi->irq = irq;

[Severity: High]
This is a pre-existing issue, but looking at the surrounding probe function,
is it safe to request the IRQ before the clocks are enabled and the
completion is initialized?

In sun4i_spi_probe(), devm_request_irq() is called just above this hunk,
enabling the IRQ line. However, the completion structure is initialized much
later via init_completion(&sspi->done), and the AHB/module clocks required
for MMIO access are only enabled during sun4i_spi_runtime_resume().

If the bootloader left the interrupt asserted (e.g., during kexec) or a
spurious interrupt fires before initialization is complete, the handler
sun4i_spi_handler() could execute early. Accessing MMIO registers without
the AHB clock enabled often causes a synchronous external abort on ARM.
Additionally, calling complete() on an uninitialized completion structure
dereferences an invalid waitqueue lock, resulting in an Oops.

>  	sspi->host = host;
>  	host->max_speed_hz = 100 * 1000 * 1000;
>  	host->min_speed_hz = 3 * 1000;
diff mbox series

Patch

diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
index 8a9dcd3b6b8f..ea8be0170fbf 100644
--- a/drivers/spi/spi-sun4i.c
+++ b/drivers/spi/spi-sun4i.c
@@ -82,6 +82,8 @@  struct sun4i_spi {
 
 	struct completion	done;
 
+	int			irq;
+
 	const u8		*tx_buf;
 	u8			*rx_buf;
 	int			len;
@@ -333,6 +335,7 @@  static int sun4i_spi_transfer_one(struct spi_controller *host,
 	start = jiffies;
 	time_left = wait_for_completion_timeout(&sspi->done,
 						msecs_to_jiffies(tx_time));
+
 	end = jiffies;
 	if (!time_left) {
 		dev_warn(&host->dev,
@@ -340,12 +343,11 @@  static int sun4i_spi_transfer_one(struct spi_controller *host,
 			 dev_name(&spi->dev), tfr->len, tfr->speed_hz,
 			 jiffies_to_msecs(end - start), tx_time);
 		ret = -ETIMEDOUT;
-		goto out;
+		sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, 0);
+		synchronize_irq(sspi->irq);
 	}
 
-
-out:
-	sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, 0);
+	sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
 
 	return ret;
 }
@@ -357,8 +359,7 @@  static irqreturn_t sun4i_spi_handler(int irq, void *dev_id)
 
 	/* Transfer complete */
 	if (status & SUN4I_INT_CTL_TC) {
-		sun4i_spi_write(sspi, SUN4I_INT_STA_REG, SUN4I_INT_CTL_TC);
-		sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
+		sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, 0);
 		complete(&sspi->done);
 		return IRQ_HANDLED;
 	}
@@ -456,6 +457,7 @@  static int sun4i_spi_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	sspi->irq = irq;
 	sspi->host = host;
 	host->max_speed_hz = 100 * 1000 * 1000;
 	host->min_speed_hz = 3 * 1000;