Re: [PATCH v11 04/19] spi: spi-mem: teach spi_mem_adjust_op_freq() about post-config ops

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] spi: spi-mem: false per_op_freq failure when post_config equals max_speed_hz
- [Medium] spi: spi-mem: spi_mem_adjust_op_freq() conflates max_freq with post-config
- [Medium] spi: spi-mem: missing controller max_speed_hz check for post-config ops
--

commit 69ed76e4edf90b58e3385f80fa249633f3ea7445
Author: Santhosh Kumar K <s-k6@ti.com>

spi: spi-mem: teach spi_mem_adjust_op_freq() about post-config ops

This commit extends spi_mem_adjust_op_freq() to bypass the max_speed_hz cap
when op->max_freq matches post_config_max_speed_hz, and updates
spi_mem_default_supports_op() to require the per_op_freq controller
capability for post-config operations.

> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> index b94680b60af95..7916387a4f1d0 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -206,7 +206,8 @@ bool spi_mem_default_supports_op(struct spi_mem *mem,
>  		return false;
>  
>  	if (op->max_freq &&
> -	    op->max_freq < mem->spi->max_speed_hz) {
> +	    (op->max_freq < mem->spi->max_speed_hz ||
> +	     op->max_freq == mem->spi->post_config_max_speed_hz)) {
>  		if (!spi_mem_controller_is_capable(ctlr, per_op_freq))
>  			return false;
>  	}

[Severity: Medium]
As reported by Sashiko on v10 [1], what happens in
spi_mem_default_supports_op() when mem->spi->post_config_max_speed_hz equals
mem->spi->max_speed_hz on a controller that does not advertise per_op_freq in
ctlr->mem_caps?

Both spi_mem_supports_op() and spi_mem_exec_op() call
spi_mem_adjust_op_freq() before calling spi_mem_internal_supports_op() ->
spi_mem_default_supports_op(). For any standard untuned operation where
op->max_freq is 0 or exceeds mem->spi->max_speed_hz, spi_mem_adjust_op_freq()
sets:

	op->max_freq = mem->spi->max_speed_hz;

If spi-max-frequency and spi-max-post-config-frequency-hz are configured to
the same rate in Device Tree (or if __spi_setup() clamps spi->max_speed_hz to
ctlr->max_speed_hz matching post_config_max_speed_hz), then
op->max_freq == mem->spi->post_config_max_speed_hz evaluates to true for
every operation even though op->max_freq is equal to the base rate
mem->spi->max_speed_hz and no per-op frequency switching is needed.

Wouldn't this cause spi_mem_default_supports_op() to reject all SPI memory
operations with -EOPNOTSUPP on controllers without per_op_freq, and would
checking op->max_freq != mem->spi->max_speed_hz avoid this regression?

> @@ -627,9 +628,18 @@ EXPORT_SYMBOL_GPL(spi_mem_adjust_op_size);
>   * Some chips have per-op frequency limitations and must adapt the maximum
>   * speed. This function allows SPI mem drivers to set @op->max_freq to the
>   * maximum supported value.
> + *
> + * When @mem->spi->post_config_max_speed_hz is set, ops with @op->max_freq
> + * equal to that value are treated as post-configuration ops (e.g. PHY-tuned)
> + * and are allowed to run at the full post-config rate. All other ops are
> + * capped to @mem->spi->max_speed_hz, the always-reachable base rate.
>   */
>  void spi_mem_adjust_op_freq(struct spi_mem *mem, struct spi_mem_op *op)
>  {
> +	if (mem->spi->post_config_max_speed_hz &&
> +	    op->max_freq == mem->spi->post_config_max_speed_hz)
> +		return;

[Severity: Medium]
As reported by Sashiko on v6 [2], can identifying post-configuration
operations in spi_mem_adjust_op_freq() purely by comparing
op->max_freq == mem->spi->post_config_max_speed_hz conflate static datasheet
frequency limits with tuned post-config operations?

In drivers/spi/spi.c, of_spi_parse_dt() populates
spi->post_config_max_speed_hz unconditionally during Device Tree parsing,
before any SPI memory driver probes or runs controller tuning:

	if (!of_property_read_u32(nc, "spi-max-post-config-frequency-hz", &value))
		spi->post_config_max_speed_hz = value;

Meanwhile, flash drivers such as drivers/mtd/nand/spi/winbond.c define static
op variant tables (e.g., read_cache_octal_variants) with non-zero datasheet
max_freq values alongside unrestricted variants with max_freq == 0:

	SPINAND_PAGE_READ_FROM_CACHE_8D_8D_8D_OP(0, 24, NULL, 0, 120 * HZ_PER_MHZ),
	SPINAND_PAGE_READ_FROM_CACHE_8D_8D_8D_OP(0, 16, NULL, 0, 86 * HZ_PER_MHZ),
	SPINAND_PAGE_READ_FROM_CACHE_1S_1D_8D_OP(0, 3, NULL, 0, 120 * HZ_PER_MHZ),
	SPINAND_PAGE_READ_FROM_CACHE_1S_1D_8D_OP(0, 2, NULL, 0, 105 * HZ_PER_MHZ),
	SPINAND_PAGE_READ_FROM_CACHE_1S_8S_8S_OP(0, 20, NULL, 0, 0),

During initial chip detection in spinand_match_and_init() ->
spinand_select_op_variant() -> spinand_op_find_best_variant() in
drivers/mtd/nand/spi/core.c, each static variant is evaluated through
spi_mem_adjust_op_freq(), spi_mem_supports_op(), and
spi_mem_calc_op_duration() before tuning has occurred:

	spi_mem_adjust_op_freq(spinand->spimem, &op);

	if (!spi_mem_supports_op(spinand->spimem, &op))
		break;

	nbytes -= op.data.nbytes;

	op_duration_ns += spi_mem_calc_op_duration(spinand->spimem, &op);

If a board configures spi-max-post-config-frequency-hz to a value that
happens to match a static variant's datasheet max_freq while
spi-max-frequency is set to a lower base rate, won't spi_mem_adjust_op_freq()
leave that variant uncapped while capping unrestricted variants (max_freq ==
0) and other frequency-limited variants to max_speed_hz? That can skew
spi_mem_calc_op_duration() into selecting a suboptimal variant, cause
spi_mem_default_supports_op() to reject matching variants on controllers
without per_op_freq, or allow an untuned op to execute at the post-config
rate before tuning has succeeded.

[Severity: Medium]
As reported by Sashiko on v10 [3], does returning early in
spi_mem_adjust_op_freq() when op->max_freq equals
mem->spi->post_config_max_speed_hz bypass the controller's hardware maximum
frequency limit (mem->spi->controller->max_speed_hz)?

In drivers/spi/spi.c, __spi_setup() clamps spi->max_speed_hz to
spi->controller->max_speed_hz:

	if (spi->controller->max_speed_hz &&
	    (!spi->max_speed_hz ||
	     spi->max_speed_hz > spi->controller->max_speed_hz))
		spi->max_speed_hz = spi->controller->max_speed_hz;

However, spi->post_config_max_speed_hz is never clamped to
spi->controller->max_speed_hz, and spi_mem_default_supports_op() only checks
mem->spi->controller->min_speed_hz:

	if (op->max_freq && mem->spi->controller->min_speed_hz &&
	    op->max_freq < mem->spi->controller->min_speed_hz)
		return false;

If spi-max-post-config-frequency-hz in Device Tree exceeds
mem->spi->controller->max_speed_hz, won't this early return pass an
op->max_freq above the controller's maximum supported speed to
spi_mem_supports_op(), spi_mem_calc_op_duration(), and
ctlr->mem_ops->exec_op()?

[1] https://lore.kernel.org/r/20260929063529.1235417-5-s-k6@ti.com
[2] https://lore.kernel.org/r/20260723113443.06C2D1F000E9@smtp.kernel.org
[3] https://lore.kernel.org/r/20260929063529.1235417-5-s-k6@ti.com

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