Re: [PATCH v3 04/13] spi: spi-mem: add spi_mem_apply_base_freq_cap()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] The base frequency cap is omitted from validation, duration calculation, and native execution paths, leading to bypassed limits and inconsistent behavior.
- [High] spi_mem_apply_base_freq_cap() incorrectly overrides explicitly requested intermediate frequencies, heavily throttling them to the base speed.
--

commit 6ec1a9a4c4cab8ccebd526a9ea576bc96e8ba16e
Author: Santhosh Kumar K <s-k6@ti.com>

spi: spi-mem: add spi_mem_apply_base_freq_cap()

This patch introduces spi_mem_apply_base_freq_cap() to cap operation
frequencies to a conservative base speed when they are not explicitly
configured for maximum speed. It aims to prevent unconfigured operations
from being silently raised to the device's maximum speed.

> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> index f64eda9bbd9f0..37d10c4e614b2 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -391,7 +391,11 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>  	u8 *tmpbuf;
>  	int ret;
>  
> -	/* Make sure the operation frequency is correct before going futher */
> +	/*
> +	 * Ops not configured for maximum speed are limited to the conservative
> +	 * base speed; spi_mem_adjust_op_freq() then caps to the device maximum.
> +	 */
> +	spi_mem_apply_base_freq_cap(mem, (struct spi_mem_op *)op);

[Severity: Critical]
Are we omitting the base frequency cap from validation, duration calculation,
and native execution paths, which could lead to bypassed limits and
inconsistent behavior?

For instance, spi_mem_supports_op() evaluates templates during initialization
and mutates their max_freq to max_speed_hz, which modifies the underlying
static .data arrays. When these mutated ops are later executed, the equality
check op->max_freq == max_speed_hz evaluates as true and bypasses the cap
entirely before phy tuning occurs.

Native direct mapping paths like dirmap_read or dirmap_write completely bypass
spi_mem_exec_op(). The spi_mem_dirmap_create() function uses
spi_mem_adjust_op_freq(), locking templates to max_speed_hz without applying
the base cap. This causes untuned reads to run at uncapped maximum speeds.

Also, spi_mem_calc_op_duration() calculates timeouts based on max_speed_hz
instead of applying the base cap. This drastically underestimates the duration
for operations actually executed at base_speed_hz, potentially causing
premature timeouts.

[ ... ]

> @@ -592,6 +596,26 @@ void spi_mem_adjust_op_freq(struct spi_mem *mem, struct spi_mem_op *op)
>  }
>  EXPORT_SYMBOL_GPL(spi_mem_adjust_op_freq);
>  
> +/**
> + * spi_mem_apply_base_freq_cap() - Enforce the conservative base speed for
> + *				   operations that are not explicitly validated
> + * @mem: the SPI memory
> + * @op: the operation to adjust
> + *
> + * When @mem->spi->base_speed_hz is non-zero, caps @op->max_freq to that
> + * value unless @op->max_freq is already set to @mem->spi->max_speed_hz,
> + * which signals the operation has been configured for max-speed use.
> + */
> +void spi_mem_apply_base_freq_cap(struct spi_mem *mem, struct spi_mem_op *op)
> +{
> +	if (!mem->spi->base_speed_hz || op->max_freq == mem->spi->max_speed_hz)
> +		return;
> +
> +	if (!op->max_freq || op->max_freq > mem->spi->base_speed_hz)
> +		op->max_freq = mem->spi->base_speed_hz;

[Severity: High]
Could this logic unintentionally override explicitly requested intermediate
frequencies, heavily throttling them to the base speed?

If a driver intentionally limits specific operations to a safe intermediate
frequency, such as 86 MHz for read_cache_octal_variants in the Winbond
driver, the exact equality check for max_speed_hz will fail.

If base_speed_hz < op->max_freq < max_speed_hz, this function will discard
the driver's intentional constraint, aggressively downgrading op->max_freq
to base_speed_hz even after phy tuning.

Would it be better to apply the cap only to unconfigured operations where
op->max_freq == 0?

> +}
> +EXPORT_SYMBOL_GPL(spi_mem_apply_base_freq_cap);

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