Re: [PATCH v3 10/13] spi: cadence-quadspi: enable PHY for direct reads and indirect writes

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Integer underflow in `cqspi_memcpy_fromio()` leads to out-of-bounds read/write and potential kernel panic.
- [High] Direct dereferencing of potentially unaligned pointers as 16-bit words in `cqspi_memcpy_fromio()` causes alignment exceptions.
- [High] Failure to revert the hardware PHY state when `cqspi_phy_enable()` times out leaves the controller in a corrupted state.
- [High] Unconditionally decrementing the dummy cycle count in `cqspi_phy_enable()` underflows when 0 dummy cycles are configured.
- [High] Unaligned head and tail bytes are read without PHY at tuned high frequencies, risking silent data corruption.
- [High] Enabling the PHY pipeline after starting an indirect write transaction causes a hardware timeout.

Pre-existing issues:
- [High] Mixed usage of stream and register I/O accessors for the same FIFO leads to data corruption on big-endian architectures.
--

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

spi: cadence-quadspi: enable PHY for direct reads and indirect writes

After PHY tuning completes, data transfers still use the default
read-capture path. The PHY pipeline must be activated around each
eligible transfer to benefit from the calibrated delay settings.

> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index c368dff4f1ebf..a089f2f083bf4 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c

[ ... ]

> +static int cqspi_phy_enable(struct cqspi_flash_pdata *f_pdata, bool enable)
> +{
> +	struct cqspi_st *cqspi = f_pdata->cqspi;
> +	void __iomem *reg_base = cqspi->iobase;
> +	u32 reg;
> +	u8 dummy;
> +
> +	if (enable) {

[ ... ]

> +		/*
> +		 * The PHY data-capture pipeline absorbs one dummy cycle's
> +		 * worth of latency; reduce the count to avoid over-compensation.
> +		 */
> +		reg = readl(reg_base + CQSPI_REG_RD_INSTR);
> +		dummy = FIELD_GET(CQSPI_REG_RD_INSTR_DUMMY_MASK
> +					  << CQSPI_REG_RD_INSTR_DUMMY_LSB,
> +				  reg);
> +		dummy--;

[Severity: High]
Does this code underflow if the transaction has 0 dummy cycles configured?

If the dummy value is 0, it rolls over to 255. When masked back into the 5-bit
register field, this would truncate to 31 dummy cycles, potentially causing
the hardware to wait for non-existent cycles and corrupt captured data.

> +		reg &= ~(CQSPI_REG_RD_INSTR_DUMMY_MASK
> +			 << CQSPI_REG_RD_INSTR_DUMMY_LSB);
> +		reg |= FIELD_PREP(CQSPI_REG_RD_INSTR_DUMMY_MASK
> +					  << CQSPI_REG_RD_INSTR_DUMMY_LSB,
> +				  dummy);
> +		writel(reg, reg_base + CQSPI_REG_RD_INSTR);
> +	} else {

[ ... ]

> +	}
> +
> +	return cqspi_wait_idle(cqspi);
> +}

[Severity: High]
If cqspi_wait_idle() times out after the PHY is enabled in CQSPI_REG_CONFIG,
does the hardware get left in a corrupted state?

The function returns an error without reverting the register changes.
Callers like cqspi_direct_read_execute() return this error directly, which
could permanently leave PHY features enabled for subsequent non-PHY
transactions.

[ ... ]

> @@ -1226,6 +1282,15 @@ static int cqspi_indirect_write_execute(struct cqspi_flash_pdata *f_pdata,
>  	if (cqspi->apb_ahb_hazard)
>  		readl(reg_base + CQSPI_REG_INDIRECTWR);
>  
> +	/* Use PHY only for large writes where setup overhead is amortized */
> +	use_phy_write = n_tx >= SZ_1K && f_pdata->use_phy;
> +
> +	if (use_phy_write) {
> +		ret = cqspi_phy_enable(f_pdata, true);
> +		if (ret)
> +			goto failwr;
> +	}

[Severity: High]
Is the PHY being enabled after the indirect write transaction has already
been started?

Writing to the start register earlier in this function puts the controller in
an active state waiting for AHB FIFO data. Calling cqspi_phy_enable() then
polls cqspi_wait_idle(). Because the controller is actively waiting for data
that hasn't been written yet, the idle check will always time out.

>  	while (remaining > 0) {
>  		size_t write_words, mod_bytes;

[Severity: High]
This is a pre-existing issue, but does the driver use mixed accessors for the
same FIFO inside this loop?

The driver violates the subsystem I/O accessor guidelines by using stream
accessors for bulk data and register accessors for remainder bytes on the
same FIFO. On big-endian architectures, register accessors byte-swap the data,
while stream accessors do not, leading to silent data corruption for partial
transfers.

[ ... ]

> +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]
Does this direct cast to a 16-bit pointer cause unaligned access faults?

The destination pointer comes from the SPI framework and may lack strict
2-byte alignment. Dereferencing it directly will trigger an alignment fault
and kernel panic on architectures without unaligned access support. Should
put_unaligned() be used here?

> +			from += 2;
> +			to += 2;
> +			count -= 2;
> +		}

[Severity: Critical]
Can the count variable underflow to a large integer here?

If count is odd (for example, 1 byte), subtracting 2 will underflow an
unsigned integer. The subsequent check below for count >= 4 will then pass,
and memcpy_fromio() will attempt to copy an enormous amount of memory,
resulting in a buffer overflow.

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

[ ... ]

> +static int cqspi_direct_read_execute(struct cqspi_flash_pdata *f_pdata,
> +				     const struct spi_mem_op *op)
> +{

[ ... ]

> +	/* Split into unaligned head, aligned middle, unaligned tail */
> +	from_aligned = ALIGN(from, 16);
> +	to_aligned = ALIGN_DOWN(from + len, 16);
> +	len_aligned = to_aligned - from_aligned;
> +
> +	if (from != from_aligned) {
> +		ret = cqspi_direct_read_dma(f_pdata, buf, from,
> +					    from_aligned - from);
> +		if (ret)
> +			return ret;
> +		buf += from_aligned - from;
> +	}

[Severity: High]
Are the unaligned head and tail bytes being read at the tuned maximum
frequency without the PHY enabled?

Bypassing the PHY at high frequencies forces these bytes through the default
uncalibrated capture path. This violates hardware timing margins and can
result in silent data corruption.

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