Re: [PATCH v5 17/17] mtd: spi-nor: run controller optimization before dirmap creation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Tuned max_freq is unconditionally applied to all read operations, causing PHY mismatch and data corruption during temporary protocol downgrades like 1-1-1 OTP reads.
- [High] An uninitialized variable in the CQSPI controller's tuning implementation can incorrectly report success when tuning is skipped, causing the SPI clock to run at high frequencies without PHY calibration.
--

--- Patch [17]: [PATCH v5 17/17] mtd: spi-nor: run controller optimization before dirmap creation ---
commit 2bd8b43f8358132165659bf7cc2e06b7b61244b6
Author: Santhosh Kumar K <s-k6@ti.com>

mtd: spi-nor: run controller optimization before dirmap creation

This commit runs controller optimization in spi_nor_probe() before creating
dirmaps so the validated frequency is available at construction time. It
stores the configured read op template in nor->max_read_op and propagates
the validated max_freq into every read op.

> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index b683c077a233d..2efcdc7f637fb 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -216,6 +216,9 @@ static struct spi_mem_op spi_nor_spimem_get_read_op(struct spi_nor *nor)
>  	if (spi_nor_protocol_is_dtr(nor->read_proto))
>  		op.dummy.nbytes *= 2;
>  
> +	/* Propagate the validated frequency; zero before tuning. */
> +	op.max_freq = nor->max_read_op.max_freq;
> +

[Severity: High]
Could this cause issues when read operations temporarily downgrade the
protocol?

If a read operation temporarily downgrades to 1-1-1, it looks like this will
unconditionally apply the DTR-tuned max_freq.

For example, in spi_nor_otp_read_secr():

    nor->read_proto = SNOR_PROTO_1_1_1;
    ...
    ret = spi_nor_read_data(nor, addr, len, buf);

Could propagating the DTR-calibrated frequency into an SDR 1-1-1 read op
cause the SPI controller to erroneously engage a DTR-calibrated PHY and
corrupt the data?

>  	return op;
>  }
>  

[ ... ]

> @@ -3843,6 +3846,17 @@ static int spi_nor_probe(struct spi_mem *spimem)
>  			return -ENOMEM;
>  	}
>  
> +	/*
> +	 * Populate the persistent template and run PHY tuning before dirmap
> +	 * creation so the validated frequency feeds into the dirmap op.
> +	 * Tuning failure is non-fatal; the device operates at base speed.
> +	 */
> +	nor->max_read_op = spi_nor_spimem_get_read_op(nor);
> +
> +	ret = spi_mem_execute_tuning(spimem, &nor->max_read_op, NULL);

[Severity: High]
Does passing NULL for the write_op here expose an uninitialized return value
in some SPI controllers?

Looking at cqspi_am654_ospi_execute_tuning() in the Cadence QSPI driver,
if write_op is NULL and the pattern partition isn't found, it skips tuning:

drivers/spi/spi-cadence-quadspi.c:cqspi_am654_ospi_execute_tuning() {
    int ret;
    ...
    if (!f_pdata->phy_pattern_partition_offset) {
        dev_warn(dev, "pattern partition not found, skipping tuning\n");
        goto out;
    }
    ...
out:
    if (!ret) {
        read_op->max_freq = mem->spi->post_config_max_speed_hz;
}

Since ret is uninitialized when the partition is missing, could it happen
to be 0 and falsely report success? If this happens, it looks like it would
enable high-speed operations without a tuned PHY.

> +	if (ret && ret != -EOPNOTSUPP)
> +		dev_dbg(dev, "Controller optimization failed: %d\n", ret);
> +
>  	ret = spi_nor_create_read_dirmap(nor);

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