[PATCH] spi: cadence: recover the controller after a failed transfer

[PATCH] spi: cadence: recover the controller after a failed transfer

[Mario Behn]

When a transfer times out, the bytes it clocked in are still in the RX
FIFO: the interrupt that would have read them did not run in time, and
the host path never drains the FIFO between transfers (only the target
path does, in cdns_unprepare_transfer_hardware).  Every later transfer
then reads the stale bytes first and returns shifted data, and the
device behind the controller looks dead until the system is rebooted.

Seen on a Zynq-7010 with an AD9361 on spi0 under PREEMPT_RT: after one
"SPI transfer timed out" every register read returned 0xFF, the status
register showed RX FIFO not empty while the bus was idle, and a later
sample rate change failed with a calibration timeout.

Add a handle_err callback that masks the interrupts, drops the stale
bytes and brings the controller back to its initial state.  The next
message sets the clock mode, the clock rate and the chip select again
through the usual prepare_message, transfer_one and set_cs path.

Signed-off-by: Mario Behn <mario.behn@gmail.com>
---
 drivers/spi/spi-cadence.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/spi/spi-cadence.c b/drivers/spi/spi-cadence.c
index 50ef3a7871c2..474ce14a59b3 100644
--- a/drivers/spi/spi-cadence.c
+++ b/drivers/spi/spi-cadence.c
@@ -542,6 +542,30 @@ static int cdns_transfer_one(struct spi_controller *ctlr,
 	return transfer->len;
 }
 
+/**
+ * cdns_spi_handle_err - Recover the controller after a failed transfer
+ * @ctlr:	Pointer to the spi_controller structure
+ * @msg:	Pointer to the spi_message that failed
+ *
+ * A transfer that timed out leaves the bytes it clocked in sitting in the
+ * RX FIFO, because the interrupt that reads them did not run in time and
+ * the host path never drains the FIFO between transfers.  Every later
+ * transfer would then read those stale bytes first and return shifted
+ * data.  Mask the interrupts, drop the stale bytes and bring the
+ * controller back to its initial state; the next message sets the clock
+ * and the chip select again on its way through transfer_one.
+ */
+static void cdns_spi_handle_err(struct spi_controller *ctlr,
+				struct spi_message *msg)
+{
+	struct cdns_spi *xspi = spi_controller_get_devdata(ctlr);
+
+	cdns_spi_write(xspi, CDNS_SPI_IDR, CDNS_SPI_IXR_ALL);
+	xspi->tx_bytes = 0;
+	xspi->rx_bytes = 0;
+	cdns_spi_init_hw(xspi, spi_controller_is_target(ctlr));
+}
+
 /**
  * cdns_prepare_transfer_hardware - Prepares hardware for transfer.
  * @ctlr:	Pointer to the spi_controller structure which provides
@@ -727,6 +751,7 @@ static int cdns_spi_probe(struct platform_device *pdev)
 	ctlr->prepare_message = cdns_prepare_message;
 	ctlr->transfer_one = cdns_transfer_one;
 	ctlr->unprepare_transfer_hardware = cdns_unprepare_transfer_hardware;
+	ctlr->handle_err = cdns_spi_handle_err;
 	ctlr->mode_bits = SPI_CPOL | SPI_CPHA;
 	ctlr->bits_per_word_mask = SPI_BPW_MASK(8);
 	ctlr->flags = SPI_CONTROLLER_MUST_TX;
-- 
2.53.0

Re: [PATCH] spi: cadence: recover the controller after a failed transfer

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] spi: cadence: unreset speed_hz in cdns_spi_handle_err() runs bus at max clk
- [High] spi: cadence: missing IRQ sync in cdns_spi_handle_err() causes buffer UAF
- [Medium] spi: cadence: unreset CDNS_SPI_THLD in cdns_spi_handle_err() breaks target
--

commit ac57da57c19b61a2d1f323d2cee0fc683cff3b77
Author: Mario Behn <mario.behn@gmail.com>

spi: cadence: recover the controller after a failed transfer

This commit adds a handle_err callback to the Cadence spi controller driver
to mask interrupts, drain stale bytes from the receive fifo, and reinitialize
the controller hardware after a transfer times out or fails.

> diff --git a/drivers/spi/spi-cadence.c b/drivers/spi/spi-cadence.c
> index 50ef3a7871c22..474ce14a59b3e 100644
> --- a/drivers/spi/spi-cadence.c
> +++ b/drivers/spi/spi-cadence.c
> @@ -542,6 +542,30 @@ static int cdns_transfer_one(struct spi_controller *ctlr,
>  	return transfer->len;
>  }
>  
> +/**
> + * cdns_spi_handle_err - Recover the controller after a failed transfer
> + * @ctlr:	Pointer to the spi_controller structure
> + * @msg:	Pointer to the spi_message that failed
> + *
> + * A transfer that timed out leaves the bytes it clocked in sitting in the
> + * RX FIFO, because the interrupt that reads them did not run in time and
> + * the host path never drains the FIFO between transfers.  Every later
> + * transfer would then read those stale bytes first and return shifted
> + * data.  Mask the interrupts, drop the stale bytes and bring the
> + * controller back to its initial state; the next message sets the clock
> + * and the chip select again on its way through transfer_one.
> + */
> +static void cdns_spi_handle_err(struct spi_controller *ctlr,
> +				struct spi_message *msg)
> +{
> +	struct cdns_spi *xspi = spi_controller_get_devdata(ctlr);
> +
> +	cdns_spi_write(xspi, CDNS_SPI_IDR, CDNS_SPI_IXR_ALL);
> +	xspi->tx_bytes = 0;
> +	xspi->rx_bytes = 0;

[Severity: High]
Can cdns_spi_handle_err() race with an in-flight or already-woken
cdns_spi_irq() handler when a transfer times out in spi_transfer_wait()?

In drivers/spi/spi-cadence.c, cdns_spi_probe() registers cdns_spi_irq() via
devm_request_irq() with flags set to 0, which runs as a forced threaded
handler on preempt-rt or concurrently on another processor. Writing to
CDNS_SPI_IDR masks peripheral interrupt generation, but cdns_spi_handle_err()
does not synchronize with the irq handler, nor does it clear xspi->rxbuf or
xspi->txbuf before spi_transfer_one_message() finalizes the message and the
caller frees transfer->rx_buf and transfer->tx_buf.

If cdns_spi_irq() is already running—for example, across the udelay(10) call
or inside cdns_spi_process_fifo() with local ntx and nrx counts—could it read
or write freed transfer buffers via cdns_spi_reader() and cdns_spi_writer(),
or race with cdns_spi_init_hw() while accessing hardware fifos?

In addition, cdns_spi_irq() reads raw status bits without masking against
CDNS_SPI_IMR:

drivers/spi/spi-cadence.c:cdns_spi_irq() {
	intr_status = cdns_spi_read(xspi, CDNS_SPI_ISR);
	cdns_spi_write(xspi, CDNS_SPI_ISR, intr_status);
	...
	} else if (intr_status & CDNS_SPI_IXR_TXOW) {
		...
		if (xspi->tx_bytes) {
			cdns_spi_process_fifo(ctlr, xspi, trans_cnt, trans_cnt);
		} else {
			udelay(10);
			cdns_spi_process_fifo(ctlr, xspi, 0, trans_cnt);
			cdns_spi_write(xspi, CDNS_SPI_IDR,
				       CDNS_SPI_IXR_DEFAULT);
			spi_finalize_current_transfer(ctlr);
		}
}

Because cdns_spi_init_hw() re-enables the controller (CDNS_SPI_ER_ENABLE)
with an empty transmit fifo, CDNS_SPI_IXR_TXOW becomes set in CDNS_SPI_ISR.
If a delayed irq thread runs after cdns_spi_handle_err() finishes, wouldn't
it see CDNS_SPI_IXR_TXOW set and xspi->tx_bytes == 0, disable interrupts, and
call spi_finalize_current_transfer() prematurely on the next transfer?

> +	cdns_spi_init_hw(xspi, spi_controller_is_target(ctlr));

[Severity: High]
Does calling cdns_spi_init_hw() in drivers/spi/spi-cadence.c without
resetting xspi->speed_hz leave the bus running at the maximum clock rate on
subsequent transfers?

In host mode, cdns_spi_init_hw() writes CDNS_SPI_CR_DEFAULT to CDNS_SPI_CR,
which resets the hardware baud rate divisor bits (CDNS_SPI_CR_BAUD_DIV) back
to CDNS_SPI_CR_BAUD_DIV_4 (xspi->clk_rate / 4, i.e., ctlr->max_speed_hz):

drivers/spi/spi-cadence.c:cdns_spi_init_hw() {
	if (!is_target)
		ctrl_reg |= CDNS_SPI_CR_DEFAULT;
	...
	cdns_spi_write(xspi, CDNS_SPI_CR, ctrl_reg);
}

When the next transfer runs cdns_transfer_one() -> cdns_spi_setup_transfer()
-> cdns_spi_config_clock_freq(), it only reprograms the divisor if
xspi->speed_hz differs from transfer->speed_hz:

drivers/spi/spi-cadence.c:cdns_spi_config_clock_freq() {
	ctrl_reg = cdns_spi_read(xspi, CDNS_SPI_CR);

	/* Set the clock frequency */
	if (xspi->speed_hz != transfer->speed_hz) {
		...
		ctrl_reg &= ~CDNS_SPI_CR_BAUD_DIV;
		ctrl_reg |= baud_rate_val << CDNS_SPI_BAUD_DIV_SHIFT;

		xspi->speed_hz = frequency / (2 << baud_rate_val);
	}
	cdns_spi_write(xspi, CDNS_SPI_CR, ctrl_reg);
}

If the next transfer requests the same frequency that was cached in
xspi->speed_hz before the error, won't cdns_spi_config_clock_freq() skip
updating CDNS_SPI_CR_BAUD_DIV and leave the hardware running at clk_rate / 4
instead of the requested speed?

[Severity: Medium]
What happens in target mode if a transfer with xspi->tx_bytes >
xspi->tx_fifo_depth fails while another message is already queued on the
controller?

In drivers/spi/spi-cadence.c, cdns_transfer_one() raises CDNS_SPI_THLD for
large target transfers:

drivers/spi/spi-cadence.c:cdns_transfer_one() {
	if (!spi_controller_is_target(ctlr)) {
		cdns_spi_setup_transfer(spi, transfer);
	} else {
		if (xspi->tx_bytes > xspi->tx_fifo_depth)
			cdns_spi_write(xspi, CDNS_SPI_THLD, xspi->tx_fifo_depth >> 1);
	}
}

Normally CDNS_SPI_THLD is reset to 0x1 either in cdns_spi_irq() when
xspi->tx_bytes drops below half the fifo depth or in
cdns_unprepare_transfer_hardware() when the queue becomes idle. Neither
cdns_spi_handle_err() nor cdns_spi_init_hw() resets CDNS_SPI_THLD to 0x1.

If a large target transfer is interrupted in spi_transfer_wait() and another
message is queued behind it, __spi_pump_messages() starts the next message
without calling cdns_unprepare_transfer_hardware(). If the next transfer has
tx_bytes <= tx_fifo_depth, should cdns_spi_handle_err() or cdns_spi_init_hw()
reset CDNS_SPI_THLD to 0x1 so that CDNS_SPI_IXR_TXOW does not fire
immediately with xspi->tx_bytes == 0 and complete the transfer prematurely?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105203703.979067-1-mario.behn@gmail.com?part=1