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 <[email protected]> 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/[email protected]?part=9
