* [PATCH v3 0/6] spi: spi-sun4i: assorted fixes
@ 2026-09-14 15:35 Jonas Rebmann
2026-09-14 15:35 ` [PATCH v3 1/6] spi: spi-sun4i: sun4i_spi_transfer_one(): fix transfers for SUN4I_FIFO_DEPTH Jonas Rebmann
` (6 more replies)
0 siblings, 7 replies; 13+ messages in thread
From: Jonas Rebmann @ 2026-09-14 15:35 UTC (permalink / raw)
To: Mark Brown, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland,
Maxime Ripard, Alexandru Gagniuc, Olliver Schinagl
Cc: linux-spi, linux-arm-kernel, linux-sunxi, linux-kernel, kernel,
Marc Kleine-Budde, Jonas Rebmann, Tobias Schramm
This is a series of patches for the spi-sun4i driver, some of which have
been sitting around for a while.
1-4 are fixes and 5 is a small patch that introduces reporting of the
effective SPI speed.
Signed-off-by: Jonas Rebmann <jre@pengutronix.de>
---
Changes in v3:
- 4/6: Explain omission of superfluous TC bit clear in commit message
- Link to v2: https://patch.msgid.link/20260907-spi-sun4i-fixes-v2-0-7e805662b9bd@pengutronix.de
Changes in v2:
- Add Patch 5/6 to fix division by zero potential in 6/6
- 3/6: Correctly subtract 1 to gain the register value for SUN4I_CLK_CTL_CDR1
- Mitigate race condition potential in 4/6 by disabling and syncing
interrupts before sun4i_spi_drain_fifo() in the timeout path
- Link to v1: https://patch.msgid.link/20260902-spi-sun4i-fixes-v1-0-19985ef75673@pengutronix.de
---
Jonas Rebmann (1):
spi: spi-sun4i: sun4i_spi_transfer_one(): add error handling at clock setup
Marc Kleine-Budde (5):
spi: spi-sun4i: sun4i_spi_transfer_one(): fix transfers for SUN4I_FIFO_DEPTH
spi: spi-sun4i: sun4i_spi_fill_fifo(): always apply A10 FIFO fill level workaround
spi: spi-sun4i: sun4i_spi_transfer_one(): fix setting of clock rate
spi: spi-sun4i: sun4i_spi_handler(): fix race condition between transfer completion and RX FIFO interrupt
spi: spi-sun4i: sun4i_spi_transfer_one(): report effectively used speed_hz of transfer
drivers/spi/spi-sun4i.c | 68 +++++++++++++++++++++++++++++--------------------
1 file changed, 40 insertions(+), 28 deletions(-)
---
base-commit: 89a312991dc6e638a36adc43ccb91dbc25504c04
change-id: 20260902-spi-sun4i-fixes-1a50e880dc7b
Best regards,
--
Jonas Rebmann <jre@pengutronix.de>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 1/6] spi: spi-sun4i: sun4i_spi_transfer_one(): fix transfers for SUN4I_FIFO_DEPTH
2026-09-14 15:35 [PATCH v3 0/6] spi: spi-sun4i: assorted fixes Jonas Rebmann
@ 2026-09-14 15:35 ` Jonas Rebmann
2026-09-14 15:35 ` [PATCH v3 2/6] spi: spi-sun4i: sun4i_spi_fill_fifo(): always apply A10 FIFO fill level workaround Jonas Rebmann
` (5 subsequent siblings)
6 siblings, 0 replies; 13+ messages in thread
From: Jonas Rebmann @ 2026-09-14 15:35 UTC (permalink / raw)
To: Mark Brown, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland,
Maxime Ripard, Alexandru Gagniuc, Olliver Schinagl
Cc: linux-spi, linux-arm-kernel, linux-sunxi, linux-kernel, kernel,
Marc Kleine-Budde, Jonas Rebmann
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(-)
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 */
--
2.55.0.806.gb8242b093d
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 2/6] spi: spi-sun4i: sun4i_spi_fill_fifo(): always apply A10 FIFO fill level workaround
2026-09-14 15:35 [PATCH v3 0/6] spi: spi-sun4i: assorted fixes Jonas Rebmann
2026-09-14 15:35 ` [PATCH v3 1/6] spi: spi-sun4i: sun4i_spi_transfer_one(): fix transfers for SUN4I_FIFO_DEPTH Jonas Rebmann
@ 2026-09-14 15:35 ` Jonas Rebmann
2026-09-14 15:50 ` sashiko-bot
2026-09-14 15:35 ` [PATCH v3 3/6] spi: spi-sun4i: sun4i_spi_transfer_one(): fix setting of clock rate Jonas Rebmann
` (4 subsequent siblings)
6 siblings, 1 reply; 13+ messages in thread
From: Jonas Rebmann @ 2026-09-14 15:35 UTC (permalink / raw)
To: Mark Brown, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland,
Maxime Ripard, Alexandru Gagniuc, Olliver Schinagl
Cc: linux-spi, linux-arm-kernel, linux-sunxi, linux-kernel, kernel,
Marc Kleine-Budde, Jonas Rebmann
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(-)
diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
index 2e2324453905..3649bcabcc9a 100644
--- a/drivers/spi/spi-sun4i.c
+++ b/drivers/spi/spi-sun4i.c
@@ -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 */
--
2.55.0.806.gb8242b093d
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 3/6] spi: spi-sun4i: sun4i_spi_transfer_one(): fix setting of clock rate
2026-09-14 15:35 [PATCH v3 0/6] spi: spi-sun4i: assorted fixes Jonas Rebmann
2026-09-14 15:35 ` [PATCH v3 1/6] spi: spi-sun4i: sun4i_spi_transfer_one(): fix transfers for SUN4I_FIFO_DEPTH Jonas Rebmann
2026-09-14 15:35 ` [PATCH v3 2/6] spi: spi-sun4i: sun4i_spi_fill_fifo(): always apply A10 FIFO fill level workaround Jonas Rebmann
@ 2026-09-14 15:35 ` Jonas Rebmann
2026-09-14 15:35 ` [PATCH v3 4/6] spi: spi-sun4i: sun4i_spi_handler(): fix race condition between transfer completion and RX FIFO interrupt Jonas Rebmann
` (3 subsequent siblings)
6 siblings, 0 replies; 13+ messages in thread
From: Jonas Rebmann @ 2026-09-14 15:35 UTC (permalink / raw)
To: Mark Brown, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland,
Maxime Ripard, Alexandru Gagniuc, Olliver Schinagl
Cc: linux-spi, linux-arm-kernel, linux-sunxi, linux-kernel, kernel,
Marc Kleine-Budde, Jonas Rebmann
From: Marc Kleine-Budde <mkl@pengutronix.de>
A SPI transfer defines the _maximum_ speed of the SPI transfer. However
the driver doesn't take into account that the clock divider is always
rounded down (due to integer arithmetic). This results in a too high
clock rate for the SPI transfer.
E.g.: with an mclk_rate of 24 MHz and an SPI transfer speed of 10 MHz,
the original code calculates a reg of "0", which results in an effective
divider of "2" and a 12 MHz clock for the SPI transfer.
Use DIV_ROUND_UP() instead of a plain integer division to fix the
problem.
While there simplify the divider calculation for the CDR1 case, use
order_base_2() instead of two ilog2() calculations.
Fixes: b5f6517948cc ("spi: sunxi: Add Allwinner A10 SPI controller driver")
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Jonas Rebmann <jre@pengutronix.de>
---
drivers/spi/spi-sun4i.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
index 3649bcabcc9a..8a9dcd3b6b8f 100644
--- a/drivers/spi/spi-sun4i.c
+++ b/drivers/spi/spi-sun4i.c
@@ -212,7 +212,7 @@ static int sun4i_spi_transfer_one(struct spi_controller *host,
struct spi_transfer *tfr)
{
struct sun4i_spi *sspi = spi_controller_get_devdata(host);
- unsigned int mclk_rate, div;
+ unsigned int mclk_rate, div, div_cdr1, div_cdr2;
unsigned long time_left;
unsigned int start, end, tx_time;
unsigned int tx_len = 0;
@@ -296,15 +296,13 @@ static int sun4i_spi_transfer_one(struct spi_controller *host,
* First try CDR2, and if we can't reach the expected
* frequency, fall back to CDR1.
*/
- div = mclk_rate / (2 * tfr->speed_hz);
- if (div <= (SUN4I_CLK_CTL_CDR2_MASK + 1)) {
- if (div > 0)
- div--;
-
- reg = SUN4I_CLK_CTL_CDR2(div) | SUN4I_CLK_CTL_DRS;
+ div_cdr1 = DIV_ROUND_UP(mclk_rate, tfr->speed_hz);
+ div_cdr2 = DIV_ROUND_UP(div_cdr1, 2);
+ if (div_cdr2 <= (SUN4I_CLK_CTL_CDR2_MASK + 1)) {
+ reg = SUN4I_CLK_CTL_CDR2(div_cdr2 - 1) | SUN4I_CLK_CTL_DRS;
} else {
- div = ilog2(mclk_rate) - ilog2(tfr->speed_hz);
- reg = SUN4I_CLK_CTL_CDR1(div);
+ div = min(SUN4I_CLK_CTL_CDR1_MASK + 1, order_base_2(div_cdr1));
+ reg = SUN4I_CLK_CTL_CDR1(div - 1);
}
sun4i_spi_write(sspi, SUN4I_CLK_CTL_REG, reg);
--
2.55.0.806.gb8242b093d
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 4/6] spi: spi-sun4i: sun4i_spi_handler(): fix race condition between transfer completion and RX FIFO interrupt
2026-09-14 15:35 [PATCH v3 0/6] spi: spi-sun4i: assorted fixes Jonas Rebmann
` (2 preceding siblings ...)
2026-09-14 15:35 ` [PATCH v3 3/6] spi: spi-sun4i: sun4i_spi_transfer_one(): fix setting of clock rate Jonas Rebmann
@ 2026-09-14 15:35 ` Jonas Rebmann
2026-09-14 15:50 ` sashiko-bot
2026-09-14 15:35 ` [PATCH v3 5/6] spi: spi-sun4i: sun4i_spi_transfer_one(): add error handling at clock setup Jonas Rebmann
` (2 subsequent siblings)
6 siblings, 1 reply; 13+ messages in thread
From: Jonas Rebmann @ 2026-09-14 15:35 UTC (permalink / raw)
To: Mark Brown, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland,
Maxime Ripard, Alexandru Gagniuc, Olliver Schinagl
Cc: linux-spi, linux-arm-kernel, linux-sunxi, linux-kernel, kernel,
Marc Kleine-Budde, Jonas Rebmann, Tobias Schramm
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.
With all interrupts disabled once the transfer completed, omit the
clearing of the Transfer Complete bit in sun4i_spi_handler. This is safe
because sun4i_spi_transfer_one() takes care of this before enabling any
interrupts again.
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(-)
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;
--
2.55.0.806.gb8242b093d
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 5/6] spi: spi-sun4i: sun4i_spi_transfer_one(): add error handling at clock setup
2026-09-14 15:35 [PATCH v3 0/6] spi: spi-sun4i: assorted fixes Jonas Rebmann
` (3 preceding siblings ...)
2026-09-14 15:35 ` [PATCH v3 4/6] spi: spi-sun4i: sun4i_spi_handler(): fix race condition between transfer completion and RX FIFO interrupt Jonas Rebmann
@ 2026-09-14 15:35 ` Jonas Rebmann
2026-09-14 15:36 ` [PATCH v3 6/6] spi: spi-sun4i: sun4i_spi_transfer_one(): report effectively used speed_hz of transfer Jonas Rebmann
2026-10-06 15:31 ` [PATCH v3 0/6] spi: spi-sun4i: assorted fixes Mark Brown
6 siblings, 0 replies; 13+ messages in thread
From: Jonas Rebmann @ 2026-09-14 15:35 UTC (permalink / raw)
To: Mark Brown, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland,
Maxime Ripard, Alexandru Gagniuc, Olliver Schinagl
Cc: linux-spi, linux-arm-kernel, linux-sunxi, linux-kernel, kernel,
Marc Kleine-Budde, Jonas Rebmann
Bail out if clk_get_rate() or clk_set_rate() fail during
sun4i_spi_transfer_one(). This might happen if the clock isn't known yet
or fails to set the requested rate.
This also ensures that mclk_rate is nonzero and can be safely divided by
in a future commit.
Signed-off-by: Jonas Rebmann <jre@pengutronix.de>
---
drivers/spi/spi-sun4i.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
index ea8be0170fbf..94e1c7bbd632 100644
--- a/drivers/spi/spi-sun4i.c
+++ b/drivers/spi/spi-sun4i.c
@@ -279,9 +279,17 @@ static int sun4i_spi_transfer_one(struct spi_controller *host,
/* Ensure that we have a parent clock fast enough */
mclk_rate = clk_get_rate(sspi->mclk);
+ if (!mclk_rate)
+ return -EINVAL;
+
if (mclk_rate < (2 * tfr->speed_hz)) {
- clk_set_rate(sspi->mclk, 2 * tfr->speed_hz);
+ ret = clk_set_rate(sspi->mclk, 2 * tfr->speed_hz);
+ if (ret)
+ return ret;
+
mclk_rate = clk_get_rate(sspi->mclk);
+ if (!mclk_rate)
+ return -EINVAL;
}
/*
--
2.55.0.806.gb8242b093d
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 6/6] spi: spi-sun4i: sun4i_spi_transfer_one(): report effectively used speed_hz of transfer
2026-09-14 15:35 [PATCH v3 0/6] spi: spi-sun4i: assorted fixes Jonas Rebmann
` (4 preceding siblings ...)
2026-09-14 15:35 ` [PATCH v3 5/6] spi: spi-sun4i: sun4i_spi_transfer_one(): add error handling at clock setup Jonas Rebmann
@ 2026-09-14 15:36 ` Jonas Rebmann
2026-09-14 15:48 ` sashiko-bot
2026-10-06 15:31 ` [PATCH v3 0/6] spi: spi-sun4i: assorted fixes Mark Brown
6 siblings, 1 reply; 13+ messages in thread
From: Jonas Rebmann @ 2026-09-14 15:36 UTC (permalink / raw)
To: Mark Brown, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland,
Maxime Ripard, Alexandru Gagniuc, Olliver Schinagl
Cc: linux-spi, linux-arm-kernel, linux-sunxi, linux-kernel, kernel,
Marc Kleine-Budde, Jonas Rebmann
From: Marc Kleine-Budde <mkl@pengutronix.de>
Report the effectively used speed_hz for the transfer by setting
tfr->effective_speed_hz.
For more information see commit 5d7e2b5ed585 ("spi: core: allow
reporting the effectivly used speed_hz for a transfer"), which adds this
feature to the SPI core.
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, 2 insertions(+)
diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
index 94e1c7bbd632..59941b38b757 100644
--- a/drivers/spi/spi-sun4i.c
+++ b/drivers/spi/spi-sun4i.c
@@ -310,9 +310,11 @@ static int sun4i_spi_transfer_one(struct spi_controller *host,
div_cdr2 = DIV_ROUND_UP(div_cdr1, 2);
if (div_cdr2 <= (SUN4I_CLK_CTL_CDR2_MASK + 1)) {
reg = SUN4I_CLK_CTL_CDR2(div_cdr2 - 1) | SUN4I_CLK_CTL_DRS;
+ tfr->effective_speed_hz = mclk_rate / (2 * div_cdr2);
} else {
div = min(SUN4I_CLK_CTL_CDR1_MASK + 1, order_base_2(div_cdr1));
reg = SUN4I_CLK_CTL_CDR1(div - 1);
+ tfr->effective_speed_hz = mclk_rate / (1 << div);
}
sun4i_spi_write(sspi, SUN4I_CLK_CTL_REG, reg);
--
2.55.0.806.gb8242b093d
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 6/6] spi: spi-sun4i: sun4i_spi_transfer_one(): report effectively used speed_hz of transfer
2026-09-14 15:36 ` [PATCH v3 6/6] spi: spi-sun4i: sun4i_spi_transfer_one(): report effectively used speed_hz of transfer Jonas Rebmann
@ 2026-09-14 15:48 ` sashiko-bot
0 siblings, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-09-14 15:48 UTC (permalink / raw)
To: Jonas Rebmann; +Cc: linux-sunxi
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] The SPI controller is not properly stopped or reset on transfer timeout, leading to state corruption for subsequent transfers.
- [High] Potential out-of-bounds write in `sun4i_spi_drain_fifo` due to missing bounds check against the receive buffer length.
--
commit 6f977d6ebfc9cf20201026442f97064e4b99df02
Author: Marc Kleine-Budde <mkl@pengutronix.de>
spi: spi-sun4i: sun4i_spi_transfer_one(): report effectively used speed_hz of transfer
Report the effectively used speed_hz for the transfer by setting
tfr->effective_speed_hz. This allows the SPI core to track the actual speed
used.
> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
> index 94e1c7bbd6325..59941b38b7574 100644
> --- a/drivers/spi/spi-sun4i.c
> +++ b/drivers/spi/spi-sun4i.c
> @@ -310,9 +310,11 @@ static int sun4i_spi_transfer_one(struct spi_controller *host,
> div_cdr2 = DIV_ROUND_UP(div_cdr1, 2);
> if (div_cdr2 <= (SUN4I_CLK_CTL_CDR2_MASK + 1)) {
> reg = SUN4I_CLK_CTL_CDR2(div_cdr2 - 1) | SUN4I_CLK_CTL_DRS;
> + tfr->effective_speed_hz = mclk_rate / (2 * div_cdr2);
> } else {
> div = min(SUN4I_CLK_CTL_CDR1_MASK + 1, order_base_2(div_cdr1));
> reg = SUN4I_CLK_CTL_CDR1(div - 1);
> + tfr->effective_speed_hz = mclk_rate / (1 << div);
> }
>
> sun4i_spi_write(sspi, SUN4I_CLK_CTL_REG, reg);
[Severity: High]
This is a pre-existing issue, but I noticed a potential problem in the
timeout handling further down in sun4i_spi_transfer_one(). When a timeout
occurs, does the error path properly stop the hardware transfer?
In sun4i_spi_transfer_one():
time_left = wait_for_completion_timeout(&sspi->done,
msecs_to_jiffies(tx_time));
...
if (!time_left) {
dev_warn(&host->dev, ...);
ret = -ETIMEDOUT;
sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, 0);
synchronize_irq(sspi->irq);
}
It appears this path disables interrupts and returns but fails to clear the
SUN4I_CTL_XCH bit or issue a soft-reset to the controller. Could this leave
the hardware active and corrupt the state machine for subsequent transfers,
especially since the next transfer might read back and write XCH=1 during
setup?
[Severity: High]
This is also a pre-existing issue, but could the lack of a hardware reset
during a timeout trigger an out-of-bounds write in sun4i_spi_drain_fifo()?
If a timeout leaves XCH=1, the next transfer might start prematurely and
load data based on the previous burst count into the RX FIFO. When
sun4i_spi_drain_fifo() runs, it appears to blindly trust the hardware FIFO
count without verifying it against the size of the receive buffer:
static inline void sun4i_spi_drain_fifo(struct sun4i_spi *sspi, int len)
{
...
reg = sun4i_spi_read(sspi, SUN4I_FIFO_STA_REG);
reg &= SUN4I_FIFO_STA_RF_CNT_MASK;
cnt = reg >> SUN4I_FIFO_STA_RF_CNT_BITS;
if (len > cnt)
len = cnt;
while (len--) {
byte = readb(sspi->base_addr + SUN4I_RXDATA_REG);
if (sspi->rx_buf)
*sspi->rx_buf++ = byte;
}
}
Since sun4i_spi_drain_fifo() is called with len = SUN4I_FIFO_DEPTH, if the
hardware provides more bytes than tfr->len, will this loop unconditionally
write past the end of sspi->rx_buf?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260914-spi-sun4i-fixes-v3-0-9fe919d5cecf@pengutronix.de?part=6
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 4/6] spi: spi-sun4i: sun4i_spi_handler(): fix race condition between transfer completion and RX FIFO interrupt
2026-09-14 15:35 ` [PATCH v3 4/6] spi: spi-sun4i: sun4i_spi_handler(): fix race condition between transfer completion and RX FIFO interrupt Jonas Rebmann
@ 2026-09-14 15:50 ` sashiko-bot
2026-09-16 11:40 ` Marc Kleine-Budde
0 siblings, 1 reply; 13+ messages in thread
From: sashiko-bot @ 2026-09-14 15:50 UTC (permalink / raw)
To: Jonas Rebmann; +Cc: linux-sunxi
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [Medium] The interrupt handler reads the interrupt status without masking it with the interrupt enable register. Combined with the removal of the status bit clearing, this exposes the driver to spurious interrupts causing destructive double-completions.
Pre-existing issues:
- [High] SMP read-modify-write data race on `SUN4I_INT_CTL_REG` between the timeout path and the IRQ handler can re-enable disabled interrupts, leading to Use-After-Free.
- [High] The SPI interrupt is requested before the completion structure is initialized, exposing the driver to a NULL pointer dereference if an early interrupt fires.
--
commit a883667a9345430d63d8a01e0fab4daa8e1e23c6
Author: Marc Kleine-Budde <mkl@pengutronix.de>
spi: spi-sun4i: sun4i_spi_handler(): fix race condition between transfer completion and RX FIFO interrupt
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.
> 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
> @@ -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);
> }
[Severity: High]
This is a pre-existing issue, but does this timeout path create an SMP read-
modify-write data race with the IRQ handler?
If a timeout occurs, CPU0 executes sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, 0).
Concurrently on CPU1, if the TF_E34 interrupt fires, the IRQ handler calls
sun4i_spi_disable_interrupt(), which performs a read-modify-write:
drivers/spi/spi-sun4i.c:sun4i_spi_disable_interrupt() {
u32 reg = sun4i_spi_read(sspi, SUN4I_INT_CTL_REG);
reg &= ~mask;
sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, reg);
}
If CPU1 reads the register before CPU0 writes 0, and writes its value back
afterwards, it could unintentionally re-enable the TC and RF_F34 interrupts.
Could this lead to a use-after-free if the re-enabled interrupt later fires
and writes to a freed SPI buffer?
[ ... ]
> @@ -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;
> }
[Severity: Medium]
Will omitting the clearing of the status bit expose the driver to spurious
interrupts causing destructive double-completions?
The interrupt handler reads the status directly from the hardware without
masking it against the enabled interrupts in SUN4I_INT_CTL_REG:
drivers/spi/spi-sun4i.c:sun4i_spi_handler() {
u32 status = sun4i_spi_read(sspi, SUN4I_INT_STA_REG);
/* Transfer complete */
if (status & SUN4I_INT_CTL_TC) {
...
}
Because the pending interrupts are cleared at the start of a transfer in
sun4i_spi_transfer_one():
drivers/spi/spi-sun4i.c:sun4i_spi_transfer_one() {
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);
...
}
There is a narrow race window here. If a spurious interrupt fires right after
reinit_completion() but before the status register is cleared, the unmasked
status check in the handler could see an old TC flag. Could this cause
complete(&sspi->done) to be called prematurely, leading the SPI core to
tear down an active transfer and result in data corruption?
[ ... ]
> @@ -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;
[Severity: High]
This is a pre-existing issue, but is the IRQ requested before the completion
structure is initialized?
Earlier in sun4i_spi_probe(), the interrupt is requested:
drivers/spi/spi-sun4i.c:sun4i_spi_probe() {
...
ret = devm_request_irq(&pdev->dev, irq, sun4i_spi_handler,
0, "sun4i-spi", sspi);
...
}
However, init_completion(&sspi->done) is only called later in the probe
sequence. If the bootloader leaves the SPI controller active with a pending
interrupt, it will be unmasked at the GIC and fire immediately when requested.
The IRQ handler would then call complete(&sspi->done) on a zero-allocated,
uninitialized completion structure. Could this result in a NULL pointer
dereference and a kernel panic on boot?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260914-spi-sun4i-fixes-v3-0-9fe919d5cecf@pengutronix.de?part=4
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/6] spi: spi-sun4i: sun4i_spi_fill_fifo(): always apply A10 FIFO fill level workaround
2026-09-14 15:35 ` [PATCH v3 2/6] spi: spi-sun4i: sun4i_spi_fill_fifo(): always apply A10 FIFO fill level workaround Jonas Rebmann
@ 2026-09-14 15:50 ` sashiko-bot
2026-09-16 11:36 ` Marc Kleine-Budde
0 siblings, 1 reply; 13+ messages in thread
From: sashiko-bot @ 2026-09-14 15:50 UTC (permalink / raw)
To: Jonas Rebmann; +Cc: linux-sunxi
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [High] Integer underflow in `sun4i_spi_fill_fifo()` causes an infinite loop and buffer overflow when calculating available TX FIFO space.
Pre-existing issues:
- [High] Unbounded writes to the receive buffer in `sun4i_spi_drain_fifo` can cause a heap buffer overflow.
--
commit 293e40dd169fc9a0b7d04d7abe8b6d6292a688f1
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
directly limits the TX FIFO fill level internally to work around timeouts
on A10s SoCs. It ensures the workaround is always applied regardless of
where the FIFO is refilled.
> 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
> @@ -142,15 +142,21 @@ static inline void sun4i_spi_drain_fifo(struct sun4i_spi *sspi, int len)
> }
> }
[Severity: High]
This is a pre-existing issue, but does sun4i_spi_drain_fifo() lack bounds
checking against the allocated receive buffer size?
Looking at sun4i_spi_drain_fifo():
static inline void sun4i_spi_drain_fifo(struct sun4i_spi *sspi, int len)
{
...
if (len > cnt)
len = cnt;
while (len--) {
byte = readb(sspi->base_addr + SUN4I_RXDATA_REG);
if (sspi->rx_buf)
*sspi->rx_buf++ = byte;
}
}
Data from the hardware is read based on what the hardware reports as
available, and unconditionally written to sspi->rx_buf. It doesn't appear
to track the total bytes written against the transfer length limit.
If the hardware asserts spurious interrupts or reports more bytes than
requested (e.g., when called from sun4i_spi_handler with SUN4I_FIFO_DEPTH),
couldn't this blindly advance the pointer past the end of the allocation
and cause a heap buffer overflow?
>
> -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;
[Severity: High]
Can this subtraction underflow if the hardware reports a full FIFO?
If sun4i_spi_get_tx_fifo_count(sspi) returns 64 (the physical maximum
depth), cnt will evaluate to 63 - 64 = -1. Because cnt is a u32, this
underflows to 0xFFFFFFFF.
The min_t() macro then compares (int)0xFFFFFFFF (which is -1) against
sspi->len. Assuming sspi->len is greater than zero, min_t() returns -1.
The subsequent while (len--) loop will post-decrement on the negative
integer and could spin indefinitely, leading to out-of-bounds reads from
sspi->tx_buf and continuous writes to the memory-mapped register.
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260914-spi-sun4i-fixes-v3-0-9fe919d5cecf@pengutronix.de?part=2
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/6] spi: spi-sun4i: sun4i_spi_fill_fifo(): always apply A10 FIFO fill level workaround
2026-09-14 15:50 ` sashiko-bot
@ 2026-09-16 11:36 ` Marc Kleine-Budde
0 siblings, 0 replies; 13+ messages in thread
From: Marc Kleine-Budde @ 2026-09-16 11:36 UTC (permalink / raw)
To: sashiko-bot; +Cc: Jonas Rebmann, linux-sunxi
[-- Attachment #1: Type: text/plain, Size: 4198 bytes --]
Dear humans,
On 14.09.2026 15:50:08, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [High] Integer underflow in `sun4i_spi_fill_fifo()` causes an infinite loop and buffer overflow when calculating available TX FIFO space.
>
> Pre-existing issues:
> - [High] Unbounded writes to the receive buffer in `sun4i_spi_drain_fifo` can cause a heap buffer overflow.
> --
>
> commit 293e40dd169fc9a0b7d04d7abe8b6d6292a688f1
> 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
> directly limits the TX FIFO fill level internally to work around timeouts
> on A10s SoCs. It ensures the workaround is always applied regardless of
> where the FIFO is refilled.
>
> > 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
> > @@ -142,15 +142,21 @@ static inline void sun4i_spi_drain_fifo(struct sun4i_spi *sspi, int len)
> > }
> > }
>
> [Severity: High]
> This is a pre-existing issue, but does sun4i_spi_drain_fifo() lack bounds
> checking against the allocated receive buffer size?
>
> Looking at sun4i_spi_drain_fifo():
>
> static inline void sun4i_spi_drain_fifo(struct sun4i_spi *sspi, int len)
> {
> ...
> if (len > cnt)
> len = cnt;
>
> while (len--) {
> byte = readb(sspi->base_addr + SUN4I_RXDATA_REG);
> if (sspi->rx_buf)
> *sspi->rx_buf++ = byte;
> }
> }
>
> Data from the hardware is read based on what the hardware reports as
> available, and unconditionally written to sspi->rx_buf. It doesn't appear
> to track the total bytes written against the transfer length limit.
>
> If the hardware asserts spurious interrupts or reports more bytes than
> requested (e.g., when called from sun4i_spi_handler with SUN4I_FIFO_DEPTH),
> couldn't this blindly advance the pointer past the end of the allocation
> and cause a heap buffer overflow?
Neither spurious interrupts nor more bytes than requested have been
observed. So far we trust the hardware.
> >
> > -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;
>
> [Severity: High]
> Can this subtraction underflow if the hardware reports a full FIFO?
With this change the hardware TX buffer is only filled up to
SUN4I_FIFO_DEPTH - 1, so it will never report to be filled with
SUN4I_FIFO_DEPTH.
> If sun4i_spi_get_tx_fifo_count(sspi) returns 64 (the physical maximum
> depth), cnt will evaluate to 63 - 64 = -1. Because cnt is a u32, this
> underflows to 0xFFFFFFFF.
>
> The min_t() macro then compares (int)0xFFFFFFFF (which is -1) against
> sspi->len. Assuming sspi->len is greater than zero, min_t() returns -1.
>
> The subsequent while (len--) loop will post-decrement on the negative
> integer and could spin indefinitely, leading to out-of-bounds reads from
> sspi->tx_buf and continuous writes to the memory-mapped register.
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 4/6] spi: spi-sun4i: sun4i_spi_handler(): fix race condition between transfer completion and RX FIFO interrupt
2026-09-14 15:50 ` sashiko-bot
@ 2026-09-16 11:40 ` Marc Kleine-Budde
0 siblings, 0 replies; 13+ messages in thread
From: Marc Kleine-Budde @ 2026-09-16 11:40 UTC (permalink / raw)
To: sashiko-bot; +Cc: Jonas Rebmann, linux-sunxi
[-- Attachment #1: Type: text/plain, Size: 5330 bytes --]
Dear humans,
On 14.09.2026 15:50:02, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>
> New issues:
> - [Medium] The interrupt handler reads the interrupt status without
> masking it with the interrupt enable register. Combined with the
> removal of the status bit clearing, this exposes the driver to
> spurious interrupts causing destructive double-completions.
Spurious interrupts have not been observed with this hardware.
> Pre-existing issues:
> - [High] SMP read-modify-write data race on `SUN4I_INT_CTL_REG` between the timeout path and the IRQ handler can re-enable disabled interrupts, leading to Use-After-Free.
> - [High] The SPI interrupt is requested before the completion structure is initialized, exposing the driver to a NULL pointer dereference if an early interrupt fires.
> --
>
> commit a883667a9345430d63d8a01e0fab4daa8e1e23c6
> Author: Marc Kleine-Budde <mkl@pengutronix.de>
>
> spi: spi-sun4i: sun4i_spi_handler(): fix race condition between transfer completion and RX FIFO interrupt
>
> 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.
>
> > 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
> > @@ -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);
> > }
>
> [Severity: High]
> This is a pre-existing issue, but does this timeout path create an SMP read-
> modify-write data race with the IRQ handler?
>
> If a timeout occurs, CPU0 executes sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, 0).
> Concurrently on CPU1, if the TF_E34 interrupt fires, the IRQ handler calls
> sun4i_spi_disable_interrupt(), which performs a read-modify-write:
>
> drivers/spi/spi-sun4i.c:sun4i_spi_disable_interrupt() {
> u32 reg = sun4i_spi_read(sspi, SUN4I_INT_CTL_REG);
>
> reg &= ~mask;
> sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, reg);
> }
>
> If CPU1 reads the register before CPU0 writes 0, and writes its value back
> afterwards, it could unintentionally re-enable the TC and RF_F34 interrupts.
> Could this lead to a use-after-free if the re-enabled interrupt later fires
> and writes to a freed SPI buffer?
>
> [ ... ]
>
> > @@ -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;
> > }
>
> [Severity: Medium]
> Will omitting the clearing of the status bit expose the driver to spurious
> interrupts causing destructive double-completions?
Spurious interrupts have not been observed with this hardware.
> The interrupt handler reads the status directly from the hardware without
> masking it against the enabled interrupts in SUN4I_INT_CTL_REG:
>
> drivers/spi/spi-sun4i.c:sun4i_spi_handler() {
> u32 status = sun4i_spi_read(sspi, SUN4I_INT_STA_REG);
>
> /* Transfer complete */
> if (status & SUN4I_INT_CTL_TC) {
> ...
> }
>
> Because the pending interrupts are cleared at the start of a transfer in
> sun4i_spi_transfer_one():
>
> drivers/spi/spi-sun4i.c:sun4i_spi_transfer_one() {
> 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);
> ...
> }
>
> There is a narrow race window here. If a spurious interrupt fires right after
> reinit_completion() but before the status register is cleared, the unmasked
> status check in the handler could see an old TC flag. Could this cause
> complete(&sspi->done) to be called prematurely, leading the SPI core to
> tear down an active transfer and result in data corruption?
Spurious interrupts have not been observed with this hardware.
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 0/6] spi: spi-sun4i: assorted fixes
2026-09-14 15:35 [PATCH v3 0/6] spi: spi-sun4i: assorted fixes Jonas Rebmann
` (5 preceding siblings ...)
2026-09-14 15:36 ` [PATCH v3 6/6] spi: spi-sun4i: sun4i_spi_transfer_one(): report effectively used speed_hz of transfer Jonas Rebmann
@ 2026-10-06 15:31 ` Mark Brown
6 siblings, 0 replies; 13+ messages in thread
From: Mark Brown @ 2026-10-06 15:31 UTC (permalink / raw)
To: Chen-Yu Tsai, Jernej Skrabec, Samuel Holland, Maxime Ripard,
Alexandru Gagniuc, Olliver Schinagl, Jonas Rebmann
Cc: linux-spi, linux-arm-kernel, linux-sunxi, linux-kernel, kernel,
Marc Kleine-Budde, Tobias Schramm
On Mon, 14 Sep 2026 17:35:54 +0200, Jonas Rebmann wrote:
> spi: spi-sun4i: assorted fixes
>
> This is a series of patches for the spi-sun4i driver, some of which have
> been sitting around for a while.
>
> 1-4 are fixes and 5 is a small patch that introduces reporting of the
> effective SPI speed.
>
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-7.4
Thanks!
[1/6] spi: spi-sun4i: sun4i_spi_transfer_one(): fix transfers for SUN4I_FIFO_DEPTH
https://git.kernel.org/broonie/spi/c/f3ce7a3ca82b
[2/6] spi: spi-sun4i: sun4i_spi_fill_fifo(): always apply A10 FIFO fill level workaround
https://git.kernel.org/broonie/spi/c/ee109e2bbea0
[3/6] spi: spi-sun4i: sun4i_spi_transfer_one(): fix setting of clock rate
https://git.kernel.org/broonie/spi/c/672ba6d88324
[4/6] spi: spi-sun4i: sun4i_spi_handler(): fix race condition between transfer completion and RX FIFO interrupt
https://git.kernel.org/broonie/spi/c/b8f09251da78
[5/6] spi: spi-sun4i: sun4i_spi_transfer_one(): add error handling at clock setup
https://git.kernel.org/broonie/spi/c/a3df049084c7
[6/6] spi: spi-sun4i: sun4i_spi_transfer_one(): report effectively used speed_hz of transfer
https://git.kernel.org/broonie/spi/c/b42ff8dc7e54
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
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2026-10-08 7:14 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-14 15:35 [PATCH v3 0/6] spi: spi-sun4i: assorted fixes Jonas Rebmann
2026-09-14 15:35 ` [PATCH v3 1/6] spi: spi-sun4i: sun4i_spi_transfer_one(): fix transfers for SUN4I_FIFO_DEPTH Jonas Rebmann
2026-09-14 15:35 ` [PATCH v3 2/6] spi: spi-sun4i: sun4i_spi_fill_fifo(): always apply A10 FIFO fill level workaround Jonas Rebmann
2026-09-14 15:50 ` sashiko-bot
2026-09-16 11:36 ` Marc Kleine-Budde
2026-09-14 15:35 ` [PATCH v3 3/6] spi: spi-sun4i: sun4i_spi_transfer_one(): fix setting of clock rate Jonas Rebmann
2026-09-14 15:35 ` [PATCH v3 4/6] spi: spi-sun4i: sun4i_spi_handler(): fix race condition between transfer completion and RX FIFO interrupt Jonas Rebmann
2026-09-14 15:50 ` sashiko-bot
2026-09-16 11:40 ` Marc Kleine-Budde
2026-09-14 15:35 ` [PATCH v3 5/6] spi: spi-sun4i: sun4i_spi_transfer_one(): add error handling at clock setup Jonas Rebmann
2026-09-14 15:36 ` [PATCH v3 6/6] spi: spi-sun4i: sun4i_spi_transfer_one(): report effectively used speed_hz of transfer Jonas Rebmann
2026-09-14 15:48 ` sashiko-bot
2026-10-06 15:31 ` [PATCH v3 0/6] spi: spi-sun4i: assorted fixes Mark Brown
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox