Re: [PATCH v5 11/17] spi: cadence-quadspi: enable PHY for direct reads

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Splitting the direct read into adjacent unaligned DMA mappings violates the DMA API cacheline alignment rules, causing memory corruption.
- [Critical] Direct reads for unaligned buffers, small lengths, or vmalloc addresses bypass PHY tuning at high speeds, causing silent data corruption.
- [High] Integer underflow when decrementing the dummy cycle count for 0-dummy cycle commands, resulting in 31 dummy cycles being programmed.
- [High] The return value of `cqspi_tune_phy(..., false)` is completely ignored, masking hardware timeouts.
- [High] Hardware state modifications are not reverted if `cqspi_wait_idle()` fails in `cqspi_tune_phy(..., true)`, leaving the PHY permanently enabled.
- [High] If `cqspi_resync_dll()` fails, the controller retains the target device's settings in `CQSPI_REG_PHY_CONFIG` but the DLL remains unlocked, causing silent data corruption on subsequent reads.
- [High] The `rx_dma_complete` completion is reinitialized after the DMA transaction is submitted, creating a TOCTOU race condition.
- [High] Modifying PHY configuration and dummy cycle registers while the controller may not be fully idle corrupts the hardware state machine.

Pre-existing issues:
- [High] Inconsistent MMIO accessor semantics between bulk transfers and partial remainder transfers, leading to data corruption on big-endian architectures.
--

--- Patch [11]: [PATCH v5 11/17] spi: cadence-quadspi: enable PHY for direct reads ---
commit c88f8eecc883007f79c6f7679e88e02531e508ae
Author: Santhosh Kumar K <s-k6@ti.com>

spi: cadence-quadspi: enable PHY for direct reads
    
Add cqspi_tune_phy() to toggle PHY mode. Enabling sets the calibrated
read-capture delay, asserts PHY_EN and PHY_PIPELINE, and decrements the
dummy cycle count by one since the PHY pipeline absorbs that latency.
When multiple chip selects have different calibration results, the DLL
is reprogrammed on each enable to match the active device's settings.
Disabling reverses all three.

For direct reads, split the transfer into an unaligned head, a
16-byte-aligned middle section with PHY active, and an unaligned tail.
PHY is used when tuning completed successfully and the transfer is at
the calibrated frequency.

> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index 322e95b3bc17d..35a4000739449 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
> @@ -1067,6 +1067,82 @@ static int cqspi_resync_dll(struct cqspi_st *cqspi)
>  	return ret;
>  }
>  
> +static int cqspi_tune_phy(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;
> +	int ret;
> +
> +	if (enable) {
[ ... ]
> +		reg = readl(reg_base + CQSPI_REG_PHY_CONFIG);
> +		if (((reg >> CQSPI_REG_PHY_CONFIG_RX_DEL_LSB) &
> +		     CQSPI_REG_PHY_CONFIG_RX_DEL_MASK) != f_pdata->phy_setting.rx ||
> +		    ((reg >> CQSPI_REG_PHY_CONFIG_TX_DEL_LSB) &
> +		     CQSPI_REG_PHY_CONFIG_TX_DEL_MASK) != f_pdata->phy_setting.tx) {
> +			cqspi_set_dll(reg_base, f_pdata->phy_setting.rx,
> +				      f_pdata->phy_setting.tx);
> +			ret = cqspi_resync_dll(cqspi);
> +			if (ret)
> +				return ret;

[Severity: High]
Does returning early here if cqspi_resync_dll() fails leave the controller in
an inconsistent state? cqspi_set_dll() has already updated CQSPI_REG_PHY_CONFIG
with the new settings. If we return here, on the next read, the driver might
skip cqspi_resync_dll() entirely because the settings appear to match, and
proceed to read with the PHY enabled but the DLL unlocked or out-of-sync.
Could this cause silent data corruption?

> +		}
> +
> +		cqspi_readdata_capture(cqspi, true, f_pdata->use_dqs,
> +				       f_pdata->phy_setting.read_delay);
> +
> +		reg = readl(reg_base + CQSPI_REG_CONFIG);
> +		reg |= CQSPI_REG_CONFIG_PHY_EN | CQSPI_REG_CONFIG_PHY_PIPELINE;
> +		writel(reg, reg_base + CQSPI_REG_CONFIG);

[Severity: High]
Is it safe to modify CQSPI_REG_CONFIG and CQSPI_REG_RD_INSTR here without
first ensuring the SPI controller is fully idle?

In cqspi_direct_read_execute(), cqspi_tune_phy() is called immediately after a
DMA segment completes. The DMA completion indicates that the AHB memory writes
finished, but not necessarily that the QSPI controller has fully de-asserted CS
and returned to idle. Could modifying these registers without a prior
cqspi_wait_idle() check race with the controller's shutdown sequence and
corrupt the hardware state machine?

> +
> +		/*
> +		 * 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--;
> +		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);

[Severity: High]
Can this decrement cause an integer underflow if the initial dummy count is 0?

If a read command requires 0 dummy cycles, dummy-- would underflow to 255.
When masked to 5 bits via FIELD_PREP, it would write 31 to the hardware
register instead of 0, which might misalign sampling and corrupt data.

> +		writel(reg, reg_base + CQSPI_REG_RD_INSTR);
> +	} else {
[ ... ]
> +		writel(reg, reg_base + CQSPI_REG_RD_INSTR);
> +	}
> +
> +	return cqspi_wait_idle(cqspi);

[Severity: High]
If cqspi_wait_idle() times out during the enable path, the function returns an
error, but it does not seem to revert the hardware state modifications
(like PHY_EN and the dummy cycle decrement). Since the caller will abort
without calling cqspi_tune_phy(..., false), does this leave the PHY permanently
enabled and break subsequent operations?

> +}
> +
[ ... ]
> @@ -1617,8 +1701,11 @@ static int cqspi_direct_read_execute(struct cqspi_flash_pdata *f_pdata,
>  {
>  	struct cqspi_st *cqspi = f_pdata->cqspi;
>  	loff_t from = op->addr.val;
> +	loff_t from_aligned, to_aligned;
>  	size_t len = op->data.nbytes;
> +	size_t len_aligned;
>  	u_char *buf = op->data.buf.in;
> +	int ret;
>  
>  	if (!cqspi->rx_chan || !virt_addr_valid(buf) ||
>  	    len < CQSPI_PHY_MIN_DIRECT_READ_LEN) {
> @@ -1626,7 +1713,42 @@ static int cqspi_direct_read_execute(struct cqspi_flash_pdata *f_pdata,
>  		return 0;
>  	}
>  
> -	return cqspi_direct_read_dma(f_pdata, buf, from, len);
> +	if (!cqspi_use_tuned_phy(f_pdata, op, post_config_max_speed_hz))
> +		return cqspi_direct_read_dma(f_pdata, buf, from, len);
> +
> +	/* 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);

[Severity: Critical]
By splitting the DMA mappings at 16-byte boundaries instead of the CPU's
cacheline size (typically 64 bytes), do these adjacent sub-buffers end up
sharing cachelines?

When the middle section is mapped for DMA, could the cache maintenance
operations destroy the data just written to memory by the head section's
DMA, or write back stale CPU cache data over it, causing memory corruption
on architectures with non-coherent DMA?

[Severity: High]
Additionally, I noticed a race condition inside cqspi_direct_read_dma() when
submitting the transaction:

    cookie = tx->tx_submit(tx);
    reinit_completion(&cqspi->rx_dma_complete);

If the DMA engine processes the descriptor immediately after tx_submit, the
callback might execute and call complete() before reinit_completion() clears it,
causing wait_for_completion_timeout() to incorrectly hang.

[Severity: Critical]
For unaligned buffers, short lengths, or vmalloc addresses, it appears we bypass
PHY tuning and read directly via AHB. However, the SPI clock frequency is still
running at post_config_max_speed_hz.

If this high frequency requires PHY tuning for proper data capture, will reading
without the PHY enabled violate setup/hold times and sample garbage data?

> +		if (ret)
> +			return ret;
> +		buf += from_aligned - from;
> +	}
> +
> +	if (len_aligned) {
> +		ret = cqspi_tune_phy(f_pdata, true);
> +		if (ret)
> +			return ret;
> +		ret = cqspi_direct_read_dma(f_pdata, buf, from_aligned,
> +					    len_aligned);
> +		cqspi_tune_phy(f_pdata, false);

[Severity: High]
Does this code ignore the return value of cqspi_tune_phy(..., false)?

If the preceding DMA operation succeeded but disabling the PHY fails
due to a hardware timeout in cqspi_wait_idle(), the return value is lost.
Could this mask hardware timeouts and leave the controller broken while the SPI
subsystem assumes success?

> +		if (ret)
> +			return ret;
> +		buf += len_aligned;
> +	}
> +
> +	if (to_aligned != (from + len)) {
> +		ret = cqspi_direct_read_dma(f_pdata, buf, to_aligned,
> +					    (from + len) - to_aligned);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
>  }

I also noticed an issue in the indirect transfer functions that is not
introduced by this patch, but might be worth addressing.

[Severity: High]
This is a pre-existing issue, but does mixing ioread32_rep() with ioread32()
cause data corruption on big-endian architectures?

In cqspi_indirect_read_execute():

    ioread32_rep(ahb_base, rxbuf, (bytes_to_read / 4));
    } else if (!word_remain && mod_bytes) {
        unsigned int temp = ioread32(ahb_base);

The bulk data is read using ioread32_rep() (which preserves FIFO byte order
without byteswapping), while the remainder bytes use ioread32() (which performs
CPU-to-device byteswapping on big-endian systems). The trailing bytes of the
payload will be byte-swapped relative to the bulk data. A similar pattern
exists in cqspi_indirect_write_execute() with iowrite32_rep() and iowrite32().

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