Re: [PATCH v4 14/16] mtd: spinand: negotiate optimal PHY operating point before dirmap creation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Fallback to SSDR during PHY tuning does not invalidate ODTR templates, causing a mode mismatch and persistent flash failure upon system resume.
- [High] Fallback PHY tuning downgrades only the primary read operation, leaving write, update, and continuous read operations at their original unvalidated high-speed variants.
- [High] Missing reset of `max_freq` on PHY tuning error paths and across tuning loops violates the zero-frequency invariant, leading to operations at unvalidated frequencies.
--

commit 0f63572c37a35621b6ca824014ba943d4c511e93
Author: Santhosh Kumar K <s-k6@ti.com>
mtd: spinand: negotiate optimal PHY operating point before dirmap creation

This commit moves dirmap creation to spinand_probe() and introduces PHY
tuning negotiation before dirmaps are created. It iterates through DTR
and SDR variants in descending performance order to find the best PHY
operating point, and falls back to non-PHY modes on complete failure.

> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index b678d05342979..5dcfaabaf2cc5 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -1294,9 +1300,11 @@ static int spinand_create_dirmap(struct spinand_device *spinand,
>  	/* Read descriptor */
>  	info.primary_op_tmpl = *spinand->op_templates->read_cache;
>  	info.primary_op_tmpl.data.ecc = enable_ecc;
> +	info.primary_op_tmpl.max_freq = spinand->max_read_op.max_freq;
>  	if (secondary_op) {
>  		info.secondary_op_tmpl = *spinand->op_templates->cont_read_cache;
>  		info.secondary_op_tmpl.data.ecc = enable_ecc;
> +		info.secondary_op_tmpl.max_freq = spinand->max_read_op.max_freq;

[Severity: High]
When the fallback tuning negotiates a slower read variant, the read_cache
is updated but the cont_read_cache, update_cache, and write_cache are not
downgraded.

Does this result in the uncalibrated high-speed cont_read_cache and
update_cache variants being embedded into the dirmaps using the max_freq
of the downgraded read variant?

Could this lead to I/O failures or data corruption when the controller
executes these uncalibrated high-speed variants?

>  	}
>  	desc = spinand_create_rdesc(spinand, &info);

[ ... ]

> +static bool spinand_try_phy_ranked(struct spinand_device *spinand,
> +				   struct spi_mem *mem, bool odtr,
> +				   u32 *tried_mask)
> +{
> +	const struct spinand_op_variants *variants = spinand->phy_read_variants;
> +	const struct spi_mem_op *best;
> +	int ret;
> +
> +	if (!variants)
> +		return false;
> +
> +	while ((best = spinand_op_find_best(spinand, variants, odtr,
> +					    *tried_mask))) {
> +		*tried_mask |= BIT(best - variants->ops);
> +		spinand->max_read_op = *best;
> +		spinand->max_read_op.max_freq = 0;
> +		ret = spi_mem_execute_tuning(mem, &spinand->max_read_op,
> +					     &spinand->max_write_op);

[Severity: High]
The loop zeroes out spinand->max_read_op.max_freq before each iteration,
but it does not reset spinand->max_write_op.max_freq.

If spi_mem_execute_tuning() mutates max_write_op.max_freq before failing,
does this leaked non-zero frequency propagate to subsequent iterations?

> +		if (ret && ret != -EOPNOTSUPP)
> +			dev_warn(&mem->spi->dev, "%s PHY tuning failed: %d\n",
> +				 odtr ? "ODTR" : "SSDR", ret);
> +		if (!ret && spinand->max_read_op.max_freq) {
> +			if (odtr)
> +				spinand->odtr_op_templates.read_cache = best;
> +			else
> +				spinand->ssdr_op_templates.read_cache = best;
> +			return true;
> +		}
> +	}
> +	return false;
> +}

[ ... ]

> +static void spinand_configure_phy(struct spinand_device *spinand,
> +				  struct spi_mem *mem)
> +{
> +	u32 tried_mask;
> +	int ret;
> +
> +	spinand_reset_max_ops(spinand, spinand->op_templates);
> +
> +	ret = spi_mem_execute_tuning(mem, &spinand->max_read_op,
> +				     &spinand->max_write_op);
> +	if (ret && ret != -EOPNOTSUPP)
> +		dev_warn(&mem->spi->dev, "Failed to execute PHY tuning: %d\n",
> +			 ret);
> +
> +	/*
> +	 * Any non-zero return or a set max_freq means we are done (error,
> +	 * unsupported, or success). Fallback only for the op-specific "skip"
> +	 * signal: ret == 0 with max_freq still 0.
> +	 */
> +	if (ret || spinand->max_read_op.max_freq)
> +		return;

[Severity: High]
If the initial tuning call fails and returns early here, does it leave the
potentially mutated max_write_op.max_freq without resetting it?

Could this leaked frequency be embedded into dirmap descriptors and cause
operations to run at unvalidated frequencies?

> +
> +	if (!mem->spi->post_config_max_speed_hz || spinand->bus_iface == SSDR ||
> +	    !spinand->phy_read_variants)
> +		return;

[ ... ]

> +	/*
> +	 * Pass 2: switch to SSDR and try all SSDR variants in performance
> +	 * order.
> +	 *
> +	 * Only enter if we actually have SSDR support and a reconfigure
> +	 * callback. The hardware is still in ODTR mode here so no
> +	 * configure_chip call is needed to undo; just set up the ODTR non-PHY
> +	 * fallback and return.
> +	 */
> +	if (!spinand->ssdr_op_templates.read_cache ||
> +	    !spinand->ssdr_op_templates.write_cache ||
> +	    !spinand->configure_chip)
> +		goto use_odtr_non_phy;
> +
> +	if (spinand->configure_chip(spinand, SSDR))
> +		goto use_odtr_non_phy;
> +
> +	spinand->op_templates = &spinand->ssdr_op_templates;
> +	spinand->bus_iface = SSDR;
> +	spinand->max_write_op = *spinand->ssdr_op_templates.write_cache;
> +	spinand->max_write_op.max_freq = 0;

[Severity: Critical]
When dynamically falling back to SSDR mode, spinand->odtr_op_templates is
not cleared.

When the system resumes from suspend, spinand_configure_chip() uses the
uncleared ODTR templates to determine the mode:

    if (spinand->odtr_op_templates.read_cache &&
        spinand->odtr_op_templates.write_cache &&
        spinand->odtr_op_templates.update_cache)
        odtr = true;

Will this cause the flash chip to be incorrectly reconfigured back to ODTR
mode upon resume, while the statically created dirmaps still contain SSDR
commands?

Can this command desynchronization cause I/O failures after the device
resumes?

> +
> +	/*
> +	 * Only ODTR variants were candidates in Pass 1; SSDR bit positions
> +	 * are clear
> +	 */
> +	if (spinand_try_phy_ranked(spinand, mem, false, &tried_mask))
> +		return;

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