Re: [PATCH v3 2/2] spi: ma35d1-qspi: Add Nuvoton MA35D1 QSPI controller support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Inconsistent chip-select polarity logic between SPI core and spi-mem operations leads to inverted CS levels.
- [High] Data race on the Slave Select Control (SSCTL) register due to concurrent `spi_setup()` and transfers.
- [High] The PIO transfer loop uses atomic polling for bulk data transfers without yielding the CPU, risking soft lockups on large memory operations.
--

commit 870f4cc6d9d6ab3e7f64bb28977c07e0fcfd4027
Author: Chi-Wen Weng <cwweng@nuvoton.com>

spi: ma35d1-qspi: Add Nuvoton MA35D1 QSPI controller support

Add SPI controller driver support for the Nuvoton MA35D1 Quad SPI
controller. The controller supports standard SPI transfers and spi-mem
operations in single, dual, and quad modes.

diff --git a/drivers/spi/spi-ma35d1-qspi.c b/drivers/spi/spi-ma35d1-qspi.c
new file mode 100644
index 0000000000000..d3e687b6a06d7
--- /dev/null
+++ b/drivers/spi/spi-ma35d1-qspi.c
[ ... ]
> +static int nuvoton_qspi_txrx(struct nuvoton_qspi *qspi, const void *txbuf,
> +			     void *rxbuf, unsigned int len)
> +{
> +	unsigned int bytes_per_word = DIV_ROUND_UP(qspi->bits_per_word, 8);
> +	unsigned int words;
> +	u32 val;
> +	int ret;
> +	int i;
> +
> +	if (!len)
> +		return 0;
> +
> +	if (len % bytes_per_word)
> +		return -EINVAL;
> +
> +	words = len / bytes_per_word;
> +
> +	ret = nuvoton_qspi_reset_fifo(qspi);
> +	if (ret) {
> +		dev_err(qspi->dev, "FIFO reset timed out\n");
> +		return ret;
> +	}
> +
> +	/*
> +	 * Use conservative word-by-word PIO access. This keeps the initial driver
> +	 * simple and avoids relying on FIFO threshold interrupts or DMA support.
> +	 *
> +	 * The MA35D1 QSPI controller pushes one RX FIFO entry for each TX word in
> +	 * single, dual-output and quad-output modes. Drain RX after every TX word
> +	 * and discard the value for TX-only transfers to avoid RX FIFO overflow.
> +	 */
> +	for (i = 0; i < words; i++) {
> +		ret = nuvoton_qspi_wait_tx_not_full(qspi);
> +		if (ret) {
> +			dev_err(qspi->dev, "TX FIFO full timeout\n");
> +			return ret;
> +		}
> +
> +		nuvoton_qspi_write(qspi, nuvoton_qspi_tx_word(txbuf, i,
> +							      qspi->bits_per_word),
> +				   NUVOTON_QSPI_TX_OFFSET);
> +
> +		ret = nuvoton_qspi_wait_rx_not_empty(qspi);

[Severity: High]
Will this loop trigger kernel soft lockups on large memory operations?

The wait helpers nuvoton_qspi_wait_tx_not_full() and
nuvoton_qspi_wait_rx_not_empty() use readl_poll_timeout_atomic(), which
busy-waits without yielding the CPU.

Since the driver does not set max_transfer_size limits, the SPI framework
can pass down arbitrarily large buffers (e.g., several megabytes for firmware
flashes or filesystems).

Does this loop need a cond_resched(), or should the controller set
appropriate transfer size limits to prevent CPU stalls?

> +		if (ret) {
> +			dev_err(qspi->dev, "RX FIFO empty timeout\n");
> +			return ret;
> +		}
> +
> +		val = nuvoton_qspi_read(qspi, NUVOTON_QSPI_RX_OFFSET);
> +		if (rxbuf)
> +			nuvoton_qspi_rx_word(rxbuf, i, val, qspi->bits_per_word);
> +	}
[ ... ]
> +static void nuvoton_qspi_set_cs_level(struct nuvoton_qspi *qspi,
> +				      unsigned int cs, bool assert)
> +{
> +	u32 mask;
> +	u32 val;
> +
> +	switch (cs) {
> +	case 0:
> +		mask = NUVOTON_QSPI_SSCTL_SS0_MASK;
> +		break;
> +	case 1:
> +		mask = NUVOTON_QSPI_SSCTL_SS1_MASK;
> +		break;
> +	default:
> +		dev_warn(qspi->dev, "invalid chip select %u\n", cs);
> +		return;
> +	}
> +
> +	val = nuvoton_qspi_read(qspi, NUVOTON_QSPI_SSCTL_OFFSET);
> +
> +	if (assert)
> +		val |= mask;
> +	else
> +		val &= ~mask;
> +
> +	nuvoton_qspi_write(qspi, val, NUVOTON_QSPI_SSCTL_OFFSET);

[Severity: High]
Can this unprotected read-modify-write corrupt the active chip-select state?

The SPI core invokes spi_set_cs() directly during spi_setup() without holding
controller-level locks.

If a transfer is active for CS0, and a concurrent setup call occurs for CS1
(such as an ioctl(SPI_IOC_WR_MODE) from spidev), could this read-modify-write
on the shared SSCTL register race and cause silent data corruption or aborted
transfers?

> +}
> +
> +static void nuvoton_qspi_set_cs(struct spi_device *spi, bool enable)
> +{
> +	struct nuvoton_qspi *qspi = spi_controller_get_devdata(spi->controller);
> +
> +	nuvoton_qspi_set_cs_level(qspi, spi_get_chipselect(spi, 0), enable);
> +}
> +
> +static void nuvoton_qspi_mem_set_cs(struct spi_device *spi, bool enable)
> +{
> +	struct nuvoton_qspi *qspi = spi_controller_get_devdata(spi->controller);
> +	bool assert = enable;
> +
> +	if (spi->mode & SPI_CS_HIGH)
> +		assert = !assert;
> +
> +	nuvoton_qspi_set_cs_level(qspi, spi_get_chipselect(spi, 0), assert);

[Severity: High]
Does this cause contradictory chip-select polarity between standard SPI
transfers and SPI memory operations?

The SPI framework passes the physical level to drive to ctlr->set_cs(),
where false means drive low and true means drive high. nuvoton_qspi_set_cs()
passes this directly to nuvoton_qspi_set_cs_level().

However, nuvoton_qspi_mem_set_cs() treats its enable parameter as a logical
assertion flag and performs a manual inversion.

For an active-low device, will one of these paths end up driving the
chip-select line with the wrong polarity?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611091246.2070485-1-cwweng.linux@gmail.com?part=2