Re: [PATCH v10 08/19] spi: cadence-quadspi: add PHY tuning support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] spi: cadence-quadspi: infinite loop in cqspi_find_rx_high_sdr() on underflow
- [Medium] spi: cadence-quadspi: masking of -ENOMEM in PHY tuning loops
--

commit 62a3174b365e4277190116eb19dbdefea81fa88c
Author: Santhosh Kumar K <s-k6@ti.com>
spi: cadence-quadspi: add PHY tuning support

This commit adds PHY tuning support for the Cadence QSPI controller. It
introduces a calibration sequence that reads a known data pattern to determine
optimal RX and TX delays for DDR and SDR protocols. The tuning ensures
reliable high-speed operation by selecting the midpoint of the passing timing
window.

> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index 046fa5d69b0f7..e21164419b00f 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
[ ... ]
> @@ -1559,10 +1718,1572 @@ static bool cqspi_supports_mem_op(struct spi_mem *mem,
[ ... ]
> +static int cqspi_phy_check_pattern(struct cqspi_flash_pdata *f_pdata,
> +				   struct spi_mem *mem)
> +{
> +	struct spi_controller *ctlr = mem->spi->controller;
> +	struct spi_mem_op op;
> +	u8 *read_data;
> +	int ret;
> +
> +	read_data = kmalloc_array(ARRAY_SIZE(phy_tuning_pattern),
> +				  sizeof(phy_tuning_pattern[0]), GFP_KERNEL);
> +	if (!read_data)
> +		return -ENOMEM;
[ ... ]
> +static int cqspi_find_rx_low_ddr(struct cqspi_flash_pdata *f_pdata,
> +				 struct spi_mem *mem, struct cqspi_phy_setting *phy)
> +{
> +	struct device *dev = &f_pdata->cqspi->pdev->dev;
> +	int ret;
> +
> +	do {
> +		phy->rx = CQSPI_PHY_RX_LOW_SEARCH_START;
> +		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 += CQSPI_PHY_DDR_SEARCH_STEP;
> +		} while (phy->rx <= CQSPI_PHY_RX_LOW_SEARCH_END);

[Severity: Medium]
Will this correctly handle and propagate memory allocation failures?

The function cqspi_phy_check_pattern() returns -ENOMEM if kmalloc_array()
fails. However, this tuning loop (along with the other tuning loops) only
evaluates if (!ret). 

If an -ENOMEM error is returned, the loop advances to the next delay tap and
pointlessly retries the allocation instead of aborting. Can we differentiate
between a pattern mismatch (-EAGAIN) and a fatal allocation failure (-ENOMEM)
so the controller avoids looping over the entire tuning grid under memory
pressure?

[ ... ]
> +static int cqspi_find_rx_high_sdr(struct cqspi_flash_pdata *f_pdata,
> +				  struct spi_mem *mem, struct cqspi_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);

[Severity: High]
Does this loop terminate if lowerbound is 0 and the pattern is never found?

Since phy->rx is an unsigned 8-bit integer, if it reaches 0 and the pattern
check fails, the phy->rx-- decrement will underflow to 255. Because 255 >= 0
is always true, won't this result in an infinite loop? 

This sequence seems reachable during hardware probe from cqspi_phy_tuning_sdr()
where cqspi_find_rx_high_sdr() is called with rxlow.rx as the lowerbound,
which can be 0.

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