Re: [PATCH v1 07/11] spi: dw: use irq handler for enhanced spi

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Integer underflow when writing to the DW_SPI_RXFTLR hardware register causes 0xFFFFFFFF to be written.
- [High] A hardcoded 5 microsecond delay before unmasking interrupts can cause RX FIFO overflow during high-speed SPI reads.
- [High] Enhanced SPI memory operations for write and no-data commands are prematurely aborted, discarding data still in the TX FIFO.
- [High] Hardware errors (FIFO underflows/overflows) during enhanced SPI memory operations are silently ignored, reporting success.
- [High] Missing interrupt mask on the timeout path of `dw_spi_exec_enh_mem_op` leads to a Use-After-Free and interrupt storm.
--

--- Patch [7]: [PATCH v1 07/11] spi: dw: use irq handler for enhanced spi ---
commit 788453296e7040a4cdd9daf4c44b6d7c00eb97d0
Author: Sudip Mukherjee <sudip.mukherjee@sifive.com>

spi: dw: use irq handler for enhanced spi

This commit introduces an interrupt handler for enhanced SPI to perform
reads and writes based on generated IRQs. It also utilizes the
xfer_completion from spi_controller to wait for a timeout or completion
from the new IRQ handler.

> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c
> @@ -248,6 +248,34 @@ static irqreturn_t dw_spi_transfer_handler(struct dw_spi *dws)
>  	return IRQ_HANDLED;
>  }
>  
> +static irqreturn_t dw_spi_enh_handler(struct dw_spi *dws)
> +{
> +	u16 irq_status = dw_readl(dws, DW_SPI_ISR);
> +
> +	if (dw_spi_check_status(dws, false)) {
> +		spi_finalize_current_transfer(dws->ctlr);
> +		return IRQ_HANDLED;
> +	}

[Severity: High]
Does this silently ignore hardware errors during memory operations?

When a hardware error like an RX FIFO overflow occurs, dw_spi_check_status()
attempts to record the error by setting dws->ctlr->cur_msg->status.
However, memory operations bypass the standard spi_message queue, so cur_msg
is NULL and the error code is discarded.

The handler then signals completion, and the caller sees a successful wait
without knowing the data might be corrupted.

> +
> +	if (irq_status & DW_SPI_INT_RXFI) {
> +		dw_reader(dws);
> +		if (dws->rx_len <= dw_readl(dws, DW_SPI_RXFTLR))
> +			dw_writel(dws, DW_SPI_RXFTLR, dws->rx_len - 1);
> +	}

[Severity: High]
Can this cause an integer underflow when writing to the hardware register?

When a read operation finishes and dws->rx_len reaches 0, the check evaluates
to true, and this writes 0xFFFFFFFF to DW_SPI_RXFTLR.

> +
> +	if (irq_status & DW_SPI_INT_TXEI)
> +		dw_writer(dws);
> +
> +	if (!dws->tx_len && dws->rx_len) {
> +		dw_spi_mask_intr(dws, DW_SPI_INT_TXEI);
> +	} else if (!dws->rx_len && !dws->tx_len) {
> +		dw_spi_mask_intr(dws, 0xff);
> +		spi_finalize_current_transfer(dws->ctlr);
> +	}
> +
> +	return IRQ_HANDLED;
> +}

[Severity: High]
Does this prematurely abort write operations?

For write-only operations like flashing memory, dws->tx_len drops to 0 as soon
as all bytes are pushed into the TX FIFO. This signals completion immediately,
before the hardware actually transmits the bytes over the wire.

The upstream caller will then disable the SPI chip, which flushes the FIFOs
and discards the trailing bytes.

[ ... ]

> @@ -399,6 +434,34 @@ static void dw_spi_irq_setup(struct dw_spi *dws)
>  	dw_spi_umask_intr(dws, imask);
>  }
>  
> +static void dw_spi_enh_irq_setup(struct dw_spi *dws)
> +{
> +	u16 level;
> +	u8 imask;
> +
> +	/*
> +	 * Originally Tx and Rx data lengths match. Rx FIFO Threshold level
> +	 * will be adjusted at the final stage of the IRQ-based SPI transfer
> +	 * execution so not to lose the leftover of the incoming data.
> +	 */
> +	level = min_t(unsigned int, dws->fifo_len / 2, dws->tx_len);
> +	dw_writel(dws, DW_SPI_TXFTLR, level);
> +
> +	/*
> +	 * In enhanced mode if we are reading then tx_len is 0 as we
> +	 * have nothing to transmit. Calculate DW_SPI_RXFTLR with
> +	 * rx_len.
> +	 */
> +	level = min_t(unsigned int, dws->fifo_len / 2, dws->rx_len);
> +	dw_writel(dws, DW_SPI_RXFTLR, level - 1);

[Severity: High]
Will this write 0xFFFFFFFF to DW_SPI_RXFTLR during TX-only operations?

For any TX-only memory operation like a flash write, dws->rx_len is 0. This
results in level becoming 0, causing an unconditional write of 0xFFFFFFFF
to the hardware register.

[ ... ]

> @@ -906,9 +970,36 @@ static int dw_spi_exec_enh_mem_op(struct spi_mem *mem, const struct spi_mem_op *
>  
>  	dw_spi_update_config(dws, mem->spi, &cfg, &enh_cfg);
>  
> +	dw_spi_mask_intr(dws, 0xff);
> +	reinit_completion(&ctlr->xfer_completion);
>  	dw_spi_enable_chip(dws, 1);
>  
>  	dw_spi_enh_write_cmd_addr(dws, op);
> +	dw_spi_set_cs(mem->spi, false);
> +
> +	udelay(5);
> +
> +	dw_spi_enh_irq_setup(dws);

[Severity: High]
Can this delay cause an RX FIFO overflow?

The hardware transfer begins when CS is asserted, but the CPU blocks on this
udelay(5) before unmasking the interrupts.

During these 5 microseconds, the hardware is actively running and clocking in
data, but the CPU is prevented from draining the RX FIFO. At higher SPI clock
speeds with smaller FIFOs, this can result in the hardware receiving more data
than the FIFO can hold within 5us, leading to an RX overflow and data loss.

> +
> +	/* Use timeout calculation from spi_transfer_wait() */
> +	ms = 8LL * MSEC_PER_SEC * (dws->rx_len ? dws->rx_len : dws->tx_len);
> +	do_div(ms, dws->current_freq);
> +
> +	/*
> +	 * Increase it twice and add 200 ms tolerance, use
> +	 * predefined maximum in case of overflow.
> +	 */
> +	ms += ms + 200;
> +	if (ms > UINT_MAX)
> +		ms = UINT_MAX;
> +
> +	ms = wait_for_completion_timeout(&ctlr->xfer_completion,
> +					 msecs_to_jiffies(ms));
> +
> +	dw_spi_stop_mem_op(dws, mem->spi);
> +
> +	if (ms == 0)
> +		return -EIO;

[Severity: High]
Does this timeout path leave interrupts unmasked and cause a use-after-free?

If wait_for_completion_timeout() times out, this returns -EIO without
masking the interrupts via dw_spi_mask_intr().

Calling dw_spi_stop_mem_op() disables and re-enables the SPI chip, which
flushes the TX FIFO. Because the TXEI interrupt was never masked, re-enabling
the chip instantly triggers the interrupt.

The unmasked dw_spi_enh_handler() will fire asynchronously and call
dw_writer() to push more data, dereferencing dws->tx. Since this function
already returned -EIO, the upstream caller likely freed or popped the buffer
off the stack.

>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709055204.138168-1-changhuang.liang@starfivetech.com?part=7