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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The custom memory-mapped I/O read loop in cqspi_memcpy_fromio() byteswaps and corrupts data on Big Endian architectures.
- [High] Unaligned I/O memory accesses in cqspi_memcpy_fromio() cause hardware exceptions (Alignment Faults) on architectures like ARM/ARM64.

Pre-existing issues:
- [High] Mixed I/O accessors in cqspi_indirect_write_execute() corrupt partial FIFO writes on big-endian architectures.
--

commit ece753244f464ecb928c830b6abeb4d3b99224f1
Author: Santhosh Kumar K <s-k6@ti.com>

spi: cadence-quadspi: refactor direct read path for PHY support

Extract the DMA transfer code from cqspi_direct_read_execute() into a
new cqspi_direct_read_dma() helper. Add cqspi_memcpy_fromio() to handle
non-DMA transfers, using 2-byte-aligned I/O reads for 8D-8D-8D mode.

> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index fc33bbf56dd61..58bca15dbd6ec 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c

[ ... ]

> @@ -1588,8 +1586,63 @@ static int cqspi_direct_read_execute(struct cqspi_flash_pdata *f_pdata,
>  	return ret;
>  }
>  
> +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)) {
> +			put_unaligned_le16(__raw_readw(from), to);

[Severity: High]
Can these MMIO reads cause alignment faults on architectures like ARM/ARM64?

If a user or filesystem requests a SPI read at an unaligned odd byte offset,
the start address from_addr could be odd. If it is odd, !IS_ALIGNED() is true,
and the code executes __raw_readw(from) from an odd device memory address.
Unaligned accesses to device I/O memory are forbidden on some architectures
and can trigger a data abort panic.

> +			from += 2;
> +			to += 2;
> +			count -= 2;
> +		}
> +
> +		/*
> +		 * Use an explicit 32-bit read loop for the bulk; this
> +		 * guarantees the minimum 16-bit AHB transaction width required
> +		 * in 8D-8D-8D mode on all architectures.
> +		 */
> +		while (count >= 4) {
> +			put_unaligned_le32(__raw_readl(from), to);

[Severity: High]
Does combining native-endian __raw_readl() with little-endian
put_unaligned_le32() cause unwanted byte-swapping of stream data on
big-endian platforms?

Because __raw_readl() natively loads a 32-bit word from the bus, and
put_unaligned_le32() forces it into little-endian byte order in memory, the
byte sequence within each chunk gets reversed on big-endian architectures.
This reverses the exact byte stream from device memory, breaking the semantics
guaranteed by the standard memcpy_fromio() fallback.

Additionally, if the initial offset was odd, adding 2 in the block above means
from remains odd. Does executing __raw_readl(from) here also trigger an
unaligned device memory access fault during the 32-bit read?

[ ... ]


[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the I/O
accessors, I noticed a pre-existing issue in cqspi_indirect_write_execute():

drivers/spi/spi-cadence-quadspi.c:cqspi_indirect_write_execute() {
    ...
		if (write_words) {
			iowrite32_rep(cqspi->ahb_base, txbuf, write_words);
			txbuf += (write_words * 4);
		}
		if (mod_bytes) {
			unsigned int temp = 0xFFFFFFFF;

			memcpy(&temp, txbuf, mod_bytes);
			iowrite32(temp, cqspi->ahb_base);
			txbuf += mod_bytes;
		}
    ...
}

Does using iowrite32() for the remainder bytes corrupt partial FIFO writes
on big-endian systems?

The bulk data loop uses iowrite32_rep(), which is a stream accessor that
preserves byte semantics. However, the remainder writes use iowrite32(),
which byte-swaps on big-endian architectures. This mixes stream and register
accessors for the same FIFO base, which can corrupt the final 1-3 bytes
of an indirect write transfer.

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