Re: [PATCH v2 09/14] spi: stm32-qspi: Open-code message transfer walk

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Early returns in `stm32_qspi_transfer_one_message` fail to call `spi_finalize_current_message()`, leaking the SPI message and hanging the caller.
- [High] The driver mishandles SPI transfers where both `tx_buf` and `rx_buf` are NULL but `len > 0`, causing a NULL pointer dereference.
--

commit 0feb4c5cf513d6c2af724ffe0a636c0f0b92a880
Author: Kaitao Cheng <chengkaitao@kylinos.cn>

spi: stm32-qspi: Open-code message transfer walk

A later change will make list_for_each_entry() cache the next element
before entering the loop body. stm32_qspi_transfer_one_message() can
consume the following transfer as part of the current operation and then
advance the loop cursor to that entry.

Keep the transfer walk open-coded so the loop step observes that cursor
update and skips the consumed transfer. This preserves the existing
message sequencing semantics and prepares the code for the list iterator
update.

> diff --git a/drivers/spi/spi-stm32-qspi.c b/drivers/spi/spi-stm32-qspi.c
> index ea69fe25686f3..e264fee2bd2d7 100644
> --- a/drivers/spi/spi-stm32-qspi.c
> +++ b/drivers/spi/spi-stm32-qspi.c
> @@ -576,7 +576,10 @@ static int stm32_qspi_transfer_one_message(struct spi_controller *ctrl,
>  
>  	gpiod_set_value_cansleep(spi_get_csgpiod(spi, 0), true);
>  
> -	list_for_each_entry(transfer, &msg->transfers, transfer_list) {
> +	for (transfer = list_first_entry(&msg->transfers,
> +					 typeof(*transfer), transfer_list);
> +	     !list_entry_is_head(transfer, &msg->transfers, transfer_list);
> +	     transfer = list_next_entry(transfer, transfer_list)) {
>  		u8 dummy_bytes = 0;
>  
>  		memset(&op, 0, sizeof(op));

[Severity: High]
This is a pre-existing issue, but I noticed early returns in
stm32_qspi_transfer_one_message() might fail to call
spi_finalize_current_message() and leak the SPI message:

	if (!spi_get_csgpiod(spi, 0))
		return -EOPNOTSUPP;

	ret = pm_runtime_resume_and_get(qspi->dev);
	if (ret < 0)
		return ret;

Could these early returns cause spi_sync() to hang indefinitely on
wait_for_completion() and permanently lock the SPI bus? Should we arrange for
spi_finalize_current_message() to be called in these error paths?


[Severity: High]
This isn't a bug introduced by this patch, but there might be an issue with
how the driver handles SPI transfers where both tx_buf and rx_buf are NULL
but len > 0. In stm32_qspi_transfer_one_message():

		} else {
			qspi->fmode = CCR_FMODE_INDW;
			op.data.buswidth = transfer->tx_nbits;
			op.data.dir = SPI_MEM_DATA_OUT;
			op.data.buf.out = transfer->tx_buf;
		}

If a local user sends a message via spidev with tx_buf = 0, rx_buf = 0, and
len > 0, the driver assumes it is a transmit operation and assigns
op.data.buf.out to NULL.

Does the operation then fall back to stm32_qspi_tx_poll() which loops over
len, and since the transmit FIFO is empty, immediately satisfies the FIFO
Threshold Flag?

If so, could this result in dereferencing the NULL buf pointer in
stm32_qspi_tx_poll():

		fifo(buf, qspi->io_base + QSPI_DR, step);
		len -= step;
		buf += step;

and triggering a kernel panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609064122.95825-1-kaitao.cheng@linux.dev?part=9