Re: [PATCH v8 16/18] mtd: spinand: negotiate optimal controller operating point before dirmap creation

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] `spinand_try_ranked_variant()` unconditionally overwrites `spinand->cont_read_possible`, bypassing SPI controller capability constraints.
- [Medium] Inconsistent tuning error handling in `spinand_optimize_controller()` prevents ranked fallback when tuning fails.
--

--- Patch [16]: [PATCH v8 16/18] mtd: spinand: negotiate optimal controller operating point before dirmap creation ---
commit 1ac90d4e8f96020586049b7af66688b1ba996146
Author: Santhosh Kumar K <[email protected]>

mtd: spinand: negotiate optimal controller operating point before dirmap creation

This commit negotiates the optimal SPI controller operating point before
creating dirmaps. It introduces spinand_optimize_controller() to evaluate
variants in descending performance order if the initial tuning attempt fails.

> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index f839438a44ff2..cd353eb8b5374 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c

[ ... ]

> +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;
> +	struct spinand_mem_ops *templates = iface == ODTR ?
> +		&spinand->odtr_op_templates : &spinand->ssdr_op_templates;
> +	const struct spi_mem_op *best;
> +	int ret;
> +
> +	if (!variants)
> +		return false;
> +
> +	while ((best = spinand_op_find_best_variant(spinand, variants, iface,
> +						    *tried_mask, NULL))) {
> +		*tried_mask |= BIT(best - variants->ops);
> +		spinand->max_read_op = *best;
> +		spinand->max_read_op.max_freq = 0;
> +		spinand->max_write_op.max_freq = 0;
> +		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) {
> +			templates->read_cache = best;
> +			if (spinand->all_cont_read_variants) {
> +				templates->cont_read_cache =
> +					spinand_find_cont_read_variant(spinand, best, iface);
> +				spinand->cont_read_possible =
> +					!!templates->cont_read_cache;

[Severity: High]
Does this unconditionally overwrite spinand->cont_read_possible and bypass the
controller capability constraints evaluated earlier in
spinand_cont_read_init()?

Forcing continuous read on a SPI controller or ECC engine that does not
support maintaining chip select assertion across operations could cause the
controller to prematurely de-assert CS during a read. Could this lead to
corrupted data when a fallback variant is selected?

> +			}
> +			return true;
> +		}
> +	}
> +	return false;
> +}

[ ... ]

> +static void spinand_optimize_controller(struct spinand_device *spinand,
> +					struct spi_mem *mem)
> +{
> +	u32 tried_mask;
> +	int ret;
> +
> +	/* Skip entirely when no post-config target is configured. */
> +	if (!mem->spi->post_config_max_speed_hz)
> +		return;
> +
> +	spinand_reset_max_freq_ops(spinand, spinand->op_templates);
> +
> +	ret = spi_mem_execute_tuning(mem, &spinand->max_read_op,
> +				     &spinand->max_write_op);
> +	if (ret && ret != -EOPNOTSUPP)
> +		dev_dbg(&mem->spi->dev, "Controller optimization failed: %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: Medium]
Does this explicit early return abort the ranked fallback process if the
first tuning attempt returns an error (such as -EINVAL)?

In spinand_try_ranked_variant(), tuning errors on subsequent variants allow
the loop to continue falling back to slower variants. By aborting the fallback
process entirely here on the first error, does it leave the device operating
in the fastest mode at base frequency without tuning? Could this result in
unstable transfers and data corruption?

> +
> +	/* SSDR-only devices have no ranked ODTR fallback available. */
> +	if (spinand->bus_iface == SSDR || !spinand->all_read_variants)
> +		return;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=16