Re: [PATCH v3 08/13] spi: cadence-quadspi: add PHY tuning support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The final search point calculation for dual-region DDR tuning lacks bounds clamping, causing integer underflow/overflow on u8 types.
- [High] Outer TX sweep loops unconditionally apply an extra step increment after a successful pattern check, corrupting the discovered TX coordinate.
- [High] The execute_tuning callback performs unlocked read-modify-write operations on shared controller registers, risking race conditions with concurrent SPI transfers.
- [High] cqspi_find_rx_high_sdr() incorrectly returns -ENOENT and aborts tuning if the valid RX window is exactly one tap wide.
--

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

spi: cadence-quadspi: add PHY tuning support

This commit adds an execute_tuning callback to perform delay-line PHY
calibration using a known data pattern. It performs either a 2D sweep for
DDR protocols or a 1D sweep for SDR protocols to find optimal delay points.

> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index f4efb92c9446a..013fd50e416bd 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c

[ ... ]

> @@ -1559,10 +1641,1687 @@ static bool cqspi_supports_mem_op(struct spi_mem *mem,
>  	return spi_mem_default_supports_op(mem, op);
>  }

[ ... ]

> +static int cqspi_find_rx_high_sdr(struct cqspi_flash_pdata *f_pdata,
> +				  struct spi_mem *mem, struct phy_setting *phy,
> +				  u8 lowerbound)
> +{
> +	struct device *dev = &f_pdata->cqspi->pdev->dev;
> +	int ret;
> +
> +	phy->rx = CQSPI_PHY_MAX_DELAY;
> +	do {
> +		ret = cqspi_phy_apply_setting(f_pdata, phy);
> +		if (!ret) {
> +			ret = cqspi_phy_check_pattern(f_pdata, mem);
> +			if (!ret)
> +				return 0;
> +		}
> +		phy->rx--;
> +	} while (phy->rx > lowerbound);
> +
> +	dev_dbg(dev, "Unable to find RX high\n");
> +	return -ENOENT;
> +}

[Severity: High]
If the valid passing window is exactly 1 tap wide, no value strictly greater
than lowerbound will pass the pattern check. Would this loop decrement
phy->rx down to lowerbound and then exit returning -ENOENT? Since lowerbound
is already known to be a passing value, should this function instead
successfully return lowerbound to avoid aborting the SDR PHY tuning process?

[ ... ]

> +	/* Primary rxlow: Sweep TX window to find valid RX lower bound */
> +
> +	rxlow.tx = CQSPI_PHY_TX_LOOKUP_LOW_START;
> +	do {
> +		dev_dbg(dev, "Searching for Golden Primary rxlow on TX = %d\n",
> +			rxlow.tx);
> +		rxlow.read_delay = CQSPI_PHY_INIT_RD;
> +		ret = cqspi_find_rx_low_ddr(f_pdata, mem, &rxlow);
> +		rxlow.tx += CQSPI_PHY_DDR_SEARCH_STEP;
> +	} while (ret && rxlow.tx <= CQSPI_PHY_TX_LOOKUP_LOW_END);

[Severity: High]
Does this unconditionally add CQSPI_PHY_DDR_SEARCH_STEP even if
cqspi_find_rx_low_ddr() returns success? If ret == 0, rxlow.tx gets
incremented right before the loop exits, which might shift the recorded tx
coordinate away from the actual successful coordinate.

[ ... ]

> +		/* Compare Manhattan distances: choose corner furthest from gap */
> +		if ((abs(gaplow.tx - bottomleft.tx) +
> +		     abs(gaplow.rx - bottomleft.rx)) <
> +		    (abs(gaphigh.tx - topright.tx) +
> +		     abs(gaphigh.rx - topright.rx))) {
> +			/* Topright further: Use Region 2, 16 taps inward */
> +			searchpoint = topright;
> +			searchpoint.tx -= 16;
> +			searchpoint.rx -= (16 * (topright.rx - bottomleft.rx)) /
> +					  (topright.tx - bottomleft.tx);
> +		} else {
> +			/* Bottomleft further: Use Region 1, 16 taps inward */
> +			searchpoint = bottomleft;
> +			searchpoint.tx += 16;
> +			searchpoint.rx += (16 * (topright.rx - bottomleft.rx)) /
> +					  (topright.tx - bottomleft.tx);
> +		}

[Severity: High]
Since searchpoint.tx and searchpoint.rx are u8 types, could subtracting 16
cause an underflow if topright.tx or topright.rx is less than 16? This
might result in wrapping around to a large positive value. Should these
calculations be clamped to ensure they stay within valid bounds?

[ ... ]

> +static int cqspi_am654_ospi_execute_tuning(struct spi_mem *mem,
> +					   struct spi_mem_op *read_op,
> +					   struct spi_mem_op *write_op)
> +{
> +	struct cqspi_st *cqspi =
> +		spi_controller_get_devdata(mem->spi->controller);
> +	struct cqspi_flash_pdata *f_pdata;

[Severity: High]
Does execute_tuning() run without holding the SPI controller's bus lock
mutex? If so, could this race with concurrent SPI transfers from other devices
on the same controller, potentially causing issues when tuning functions like
cqspi_resync_dll() perform unlocked read-modify-write operations on shared
global registers like CQSPI_REG_CONFIG?

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