Re: [PATCH v5 10/17] spi: cadence-quadspi: refactor direct read path for PHY support

[sashiko-bot@kernel.org]

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

New issues:
- [High] In 8D-8D-8D mode, `cqspi_memcpy_fromio()` performs an unaligned 16-bit I/O read (`__raw_readw()`) if the requested SPI read offset (`op->addr.val`) is not 2-byte aligned. This causes a synchronous Alignment Fault (kernel panic) on architectures with strict I/O alignment rules.
- [High] Directly casting the generic destination buffer `to` to a `u16 *` and dereferencing it causes an unaligned memory store if the buffer is not 2-byte aligned.
- [High] Relying on `memcpy_fromio()` to perform 4-byte hardware reads in 8D-8D-8D mode violates hardware access width requirements, as `memcpy_fromio()` does not guarantee 32-bit I/O transactions.

Pre-existing issues:
- [High] When the `CQSPI_NO_INDIRECT_MODE` quirk is set, `cqspi_read()` and `cqspi_write()` bypass the `ahb_size` boundary check, allowing out-of-bounds MMIO accesses.
--

--- Patch [10]: [PATCH v5 10/17] spi: cadence-quadspi: refactor direct read path for PHY support ---
commit 2b5af9ab10a299ef73d9e0a5cb2201de2b8ee054
Author: Santhosh Kumar K <s-k6@ti.com>
Subject: spi: cadence-quadspi: refactor direct read path for PHY support

This patch extracts the DMA transfer code from cqspi_direct_read_execute()
into a new cqspi_direct_read_dma() helper. It adds cqspi_memcpy_fromio() to
handle non-DMA transfers, with 2-byte-aligned I/O accesses for 8D-8D-8D
mode.

> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index 5070ffc53d5e8..322e95b3bc17d 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c

[ ... ]

> @@ -1580,8 +1577,61 @@ static int cqspi_direct_read_execute(struct cqspi_flash_pdata *f_pdata,
> +static void cqspi_memcpy_fromio(const struct spi_mem_op *op, void *to,
> +				const void __iomem *from, size_t count)
> +{
> +	if (op->data.buswidth == 8 && op->data.dtr) {
> +		unsigned long from_addr = (unsigned long)from;
> +
> +		/* Handle unaligned start with 2-byte read */
> +		if (count && !IS_ALIGNED(from_addr, 4)) {
> +			*(u16 *)to = __raw_readw(from);

[Severity: High]
Could this cause a synchronous alignment fault on architectures with strict
I/O alignment rules? If the requested SPI read offset (from_addr) is an odd
address, !IS_ALIGNED(from_addr, 4) evaluates to true, which results in
__raw_readw() executing on an unaligned I/O address.

Additionally, does casting the generic destination buffer 'to' to a u16
pointer risk unaligned memory stores? If the buffer provided by userspace or
another subsystem is not 2-byte aligned, this might cause undefined behavior.
Would it be safer to use put_unaligned() here?

> +			from += 2;
> +			to += 2;
> +			count -= 2;
> +		}
> +
> +		/* Use 4-byte reads for aligned bulk (no readq for 32-bit) */
> +		if (count >= 4) {
> +			size_t len = round_down(count, 4);
> +
> +			memcpy_fromio(to, from, len);

[Severity: High]
Can we rely on memcpy_fromio() to guarantee 32-bit hardware transactions on
all architectures? The comment mentions using 4-byte reads for aligned bulk,
but on some platforms memcpy_fromio() falls back to byte-by-byte reads. Should
this use ioread32_rep() or a manual readl() loop to enforce the hardware
access width requirements for 8D-8D-8D mode?

> +			from += len;
> +			to += len;
> +			count -= len;
> +		}
> +
> +		/* Handle remaining 2 bytes */
> +		if (count)
> +			*(u16 *)to = __raw_readw(from);

[Severity: High]
Similarly to the unaligned start logic, does this risk unaligned I/O reads or
unaligned memory stores if the trailing bytes involve an odd start address or
an unaligned destination buffer?

> +
> +		return;
> +	}
> +
> +	memcpy_fromio(to, from, count);
> +}

[ ... ]

> @@ -1597,7 +1647,8 @@ static ssize_t cqspi_read(struct cqspi_flash_pdata *f_pdata,
>  
>  	if ((cqspi->use_direct_mode && ((from + len) <= cqspi->ahb_size)) ||
>  	    (cqspi->ddata && cqspi->ddata->quirks & CQSPI_NO_INDIRECT_MODE))
> -		return cqspi_direct_read_execute(f_pdata, buf, from, len);
> +		return cqspi_direct_read_execute(f_pdata, op,
> +						 post_config_max_speed_hz);

[Severity: High]
This is a pre-existing issue, but does the CQSPI_NO_INDIRECT_MODE quirk
bypass the ahb_size boundary check here? Because of the logical OR condition,
an SPI read with an offset and length exceeding the controller's memory-mapped
AHB window might proceed and access out-of-bounds MMIO.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717185116.2065505-1-s-k6@ti.com?part=10