Re: [PATCH v6 15/17] mtd: spinand: negotiate optimal controller operating point before dirmap creation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Ranked controller optimization downgrades the read variant on tuning failure but fails to update or disable continuous read, leading to potential controller lockups and data corruption.
- [High] The SPI NAND chip is unconditionally restored to ODTR mode upon system resume, even if controller optimization dynamically fell back to SSDR mode at probe time, breaking statically configured dirmaps.
- [High] SPI controller PHY tuning state is lost across suspend/resume cycles.
- [Medium] `spinand->max_write_op` is not reset between iterations in `spinand_try_ranked_variant()`, leaking `max_freq` modifications across tuning attempts.
--

--- Patch [15]: [PATCH v6 15/17] mtd: spinand: negotiate optimal controller operating point before dirmap creation ---
commit dc7c5dff1e36808c28c6f5690c48c5cbe8f6d925
Author: Santhosh Kumar K <s-k6@ti.com>
Subject: mtd: spinand: negotiate optimal controller operating point before dirmap creation

This commit moves dirmap creation after negotiating the optimal controller
operating point, so the validated frequency is embedded in the descriptors.
It introduces a ranked fallback mechanism to try variants in performance
order if the initial optimization fails.

> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index f839438a44ff2..e082e6233c7c1 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
[ ... ]
> @@ -2050,6 +2055,175 @@ static void spinand_cleanup(struct spinand_device *spinand)
> +static bool spinand_try_ranked_variant(struct spinand_device *spinand,
> +				       struct spi_mem *mem,
> +				       enum spinand_bus_interface iface,
> +				       u32 *tried_mask)
> +{
> +	const struct spinand_op_variants *variants = spinand->all_read_variants;
> +	const struct spi_mem_op *best;
> +	int ret;
> +
> +	if (!variants)
> +		return false;
> +
> +	while ((best = spinand_op_find_best_variant(spinand, variants, iface,
> +						    *tried_mask))) {
> +		*tried_mask |= BIT(best - variants->ops);
> +		spinand->max_read_op = *best;
> +		spinand->max_read_op.max_freq = 0;

[Severity: Medium]
Should spinand->max_write_op.max_freq also be reset to 0 here?

If the controller driver modifies the write op's max_freq during a failed
tuning attempt, the non-zero max_freq leaks into the next loop iteration. This
could limit the frequency negotiation artificially, causing subsequent valid
fallback variants to fail tuning or operate at suboptimal speeds.

> +		ret = spi_mem_execute_tuning(mem, &spinand->max_read_op,
> +					     &spinand->max_write_op);
> +		if (ret && ret != -EOPNOTSUPP)
> +			dev_dbg(&mem->spi->dev, "%s optimization failed: %d\n",
> +				iface == ODTR ? "ODTR" : "SSDR", ret);
> +		if (!ret && spinand->max_read_op.max_freq) {
> +			if (iface == ODTR)
> +				spinand->odtr_op_templates.read_cache = best;
> +			else
> +				spinand->ssdr_op_templates.read_cache = best;

[Severity: High]
Is it necessary to update or disable cont_read_cache to match the new
controller capabilities?

If spinand_optimize_controller() falls back to a slower ranked variant, it
only updates read_cache here, leaving cont_read_cache and cont_read_possible
unchanged. When spinand_create_dirmap() constructs the secondary read dirmap,
it will use the unmodified, incompatible cont_read_cache but incorrectly assign
it the max_freq of this slower fallback variant. 

When the kernel executes a continuous read, will it send an unsupported command
at an incompatible PHY frequency, potentially crashing the controller or
silently corrupting data?

> +			return true;
> +		}
> +	}
> +	return false;
> +}
[ ... ]
> +static void spinand_optimize_controller(struct spinand_device *spinand,
> +					struct spi_mem *mem)
> +{
[ ... ]
> +	/*
> +	 * Pass 2: switch to SSDR and try all SSDR variants in performance
> +	 * order. configure_chip is guaranteed non-NULL here: reaching ODTR
> +	 * mode requires it.
> +	 */
> +	if (WARN_ON(!spinand->configure_chip))
> +		goto use_odtr_fallback;
> +
> +	if (spinand->configure_chip(spinand, SSDR))
> +		goto use_odtr_fallback;
> +
> +	spinand->op_templates = &spinand->ssdr_op_templates;
> +	spinand->bus_iface = SSDR;

[Severity: High]
Does falling back to SSDR mode here require clearing the ODTR templates?

If ODTR tuning fails during probe, the device is switched to SSDR and dirmaps
are created using SSDR templates. However, because odtr_op_templates are not
cleared, when the system resumes from suspend, spinand_mtd_resume() calls
spinand_init_flash() -> spinand_configure_chip(). 

The configure chip logic will see that the ODTR templates exist and
unconditionally force the chip back into ODTR mode. Will subsequent I/O
operations then send SSDR-formatted SPI commands to a chip expecting ODTR
commands, resulting in data corruption?

> +	spinand->max_write_op = *spinand->ssdr_op_templates.write_cache;
> +	spinand->max_write_op.max_freq = 0;
[ ... ]
> @@ -2072,6 +2246,20 @@ static int spinand_probe(struct spi_mem *mem)
>  	if (ret)
>  		return ret;
>  
> +	/*
> +	 * Negotiate the best controller operating point before creating dirmaps
> +	 * so the validated frequency is available at dirmap construction time.
> +	 */
> +	spinand_optimize_controller(spinand, mem);

[Severity: High]
Will the PHY tuning state negotiated here get restored across suspend/resume
cycles?

During probe, spinand_optimize_controller() calibrates the controller PHY delay
registers via spi_mem_execute_tuning() and embeds the validated max_freq into
the dirmaps. During suspend, the SPI controller loses power.

On resume, neither the SPI core, spinand_mtd_resume(), nor the controller
driver appear to restore the PHY tuning registers or re-invoke
spi_mem_execute_tuning(). Could the dirmaps continue executing I/O at the high
max_freq with uncalibrated PHY timings, resulting in I/O errors or data
corruption?

> +	ret = spinand_create_dirmaps(spinand);
> +	if (ret) {
> +		dev_err(&mem->spi->dev,
> +			"Failed to create direct mappings for read/write operations (err = %d)\n",
> +			ret);
> +		goto err_spinand_cleanup;
> +	}

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