[2/5] spi: spi-sun4i: sun4i_spi_fill_fifo(): always apply A10 FIFO fill level workaround
Commit Message
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. This
commit did not consider the A10 workaround (limit the TX FIFO fill
size to SUN4I_FIFO_DEPTH - 1 (= 63) bytes) for refilling the TX FIFO
in the IRQ handler.
To apply the workaround independent from where sun4i_spi_fill_fifo()
is called, remove the length argument from the function and directly
take the max fill level of SUN4I_FIFO_DEPTH - 1 into account when
calculating the free space in the FIFO.
Due to the lack of HW this patch has not been tested on an A10 SoC,
but on an A20 SoC. It was not possible to reproduce the timeout on the
A20.
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 | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
Comments
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Race condition on the SPI transfer timeout path due to missing interrupt disable/locking, leading to concurrent FIFO access.
--
commit 1dffb9065c85ba457023e16b227c366bf84a8ee4
Author: Marc Kleine-Budde <mkl@pengutronix.de>
spi: spi-sun4i: sun4i_spi_fill_fifo(): always apply A10 FIFO fill level workaround
This commit removes the length argument from sun4i_spi_fill_fifo() and updates
it to always apply the A10 FIFO fill level workaround internally. This ensures
the TX FIFO is limited to SUN4I_FIFO_DEPTH - 1 bytes regardless of where the
function is called.
> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
> index 2e23244539053..3649bcabcc9a7 100644
> --- a/drivers/spi/spi-sun4i.c
> +++ b/drivers/spi/spi-sun4i.c
[ ... ]
> @@ -311,12 +317,8 @@ static int sun4i_spi_transfer_one(struct spi_controller *host,
> sun4i_spi_write(sspi, SUN4I_BURST_CNT_REG, SUN4I_BURST_CNT(tfr->len));
> sun4i_spi_write(sspi, SUN4I_XMIT_CNT_REG, SUN4I_XMIT_CNT(tx_len));
>
> - /*
> - * Fill the TX FIFO
> - * Filling the FIFO fully causes timeout for some reason
> - * at least on spi2 on A10s
> - */
> - sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH - 1);
> + /* Fill the TX FIFO */
> + sun4i_spi_fill_fifo(sspi);
>
> /* Enable the interrupts */
> sun4i_spi_enable_interrupt(sspi, SUN4I_INT_CTL_TC |
[Severity: High]
Does this code expose a race condition on the SPI transfer timeout path due to
missing interrupt disable or locking?
If wait_for_completion_timeout() times out later in sun4i_spi_transfer_one(),
the code calls sun4i_spi_drain_fifo() while SPI interrupts are still enabled:
sun4i_spi_transfer_one() {
...
time_left = wait_for_completion_timeout(&sspi->done,
msecs_to_jiffies(tx_time));
sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
...
}
If the SUN4I_INT_CTL_RF_F34 interrupt fires concurrently, the IRQ handler in
sun4i_spi_handler() will also execute sun4i_spi_drain_fifo() on another CPU
without synchronization:
sun4i_spi_handler() {
...
/* Receive FIFO 3/4 full */
if (status & SUN4I_INT_CTL_RF_F34) {
sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
/* Only clear the interrupt _after_ draining the FIFO */
sun4i_spi_write(sspi, SUN4I_INT_STA_REG, SUN4I_INT_CTL_RF_F34);
return IRQ_HANDLED;
}
...
}
Both threads will read from the hardware FIFO and increment sspi->rx_buf
locklessly. Can this cause a data race, duplicate bytes, and a buffer overflow
on sspi->rx_buf?
This would be reachable under error conditions or interrupt storms if a
hardware timeout occurs during an SPI transfer followed by an interrupt.
Note that this race was introduced by a subsequent patch in this series
('spi: spi-sun4i: sun4i_spi_handler(): fix race condition between transfer
completion and RX FIFO interrupt') which moved the drain call outside the
timeout condition without disabling interrupts first.
@@ -142,15 +142,21 @@ static inline void sun4i_spi_drain_fifo(struct sun4i_spi *sspi, int len)
}
}
-static inline void sun4i_spi_fill_fifo(struct sun4i_spi *sspi, int len)
+static inline void sun4i_spi_fill_fifo(struct sun4i_spi *sspi)
{
u32 cnt;
+ int len;
u8 byte;
- /* See how much data we can fit */
- cnt = SUN4I_FIFO_DEPTH - sun4i_spi_get_tx_fifo_count(sspi);
+ /*
+ * See how much data we can fit
+ *
+ * Filling the FIFO fully causes timeout for some reason
+ * at least on spi2 on A10s
+ */
+ cnt = SUN4I_FIFO_DEPTH - 1 - sun4i_spi_get_tx_fifo_count(sspi);
- len = min3(len, (int)cnt, sspi->len);
+ len = min_t(int, cnt, sspi->len);
while (len--) {
byte = sspi->tx_buf ? *sspi->tx_buf++ : 0;
@@ -311,12 +317,8 @@ static int sun4i_spi_transfer_one(struct spi_controller *host,
sun4i_spi_write(sspi, SUN4I_BURST_CNT_REG, SUN4I_BURST_CNT(tfr->len));
sun4i_spi_write(sspi, SUN4I_XMIT_CNT_REG, SUN4I_XMIT_CNT(tx_len));
- /*
- * Fill the TX FIFO
- * Filling the FIFO fully causes timeout for some reason
- * at least on spi2 on A10s
- */
- sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH - 1);
+ /* Fill the TX FIFO */
+ sun4i_spi_fill_fifo(sspi);
/* Enable the interrupts */
sun4i_spi_enable_interrupt(sspi, SUN4I_INT_CTL_TC |
@@ -373,7 +375,7 @@ static irqreturn_t sun4i_spi_handler(int irq, void *dev_id)
/* Transmit FIFO 3/4 empty */
if (status & SUN4I_INT_CTL_TF_E34) {
- sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH);
+ sun4i_spi_fill_fifo(sspi);
if (!sspi->len)
/* nothing left to transmit */