Re: [PATCH v6 05/17] spi: spi-mem: add execute_tuning callback and spi_mem_execute_tuning()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] spi_mem_execute_tuning() directly invokes the controller callback without acquiring the SPI bus locks or ensuring the controller is powered on.
- [High] spi_mem_execute_tuning() does not check if the SPI device is using a GPIO chip select before invoking the hardware tuning callback.
--

--- Patch [5]: [PATCH v6 05/17] spi: spi-mem: add execute_tuning callback and spi_mem_execute_tuning() ---
commit 74cda1abf92afd30bb8cd6cbe4cb6c5ffa5027f5
Author: Santhosh Kumar K <s-k6@ti.com>

spi: spi-mem: add execute_tuning callback and spi_mem_execute_tuning()

This commit adds an execute_tuning callback to struct spi_controller_mem_ops.
It also introduces the spi_mem_execute_tuning() wrapper function to check
callback availability and invoke it.

> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> index e20eca1b82457..ae9e2d7ac6f3c 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c

[ ... ]

> @@ -660,6 +660,37 @@ u64 spi_mem_calc_op_duration(struct spi_mem *mem, struct spi_mem_op *op)
>  }
>  EXPORT_SYMBOL_GPL(spi_mem_calc_op_duration);
>  
> +/**
> + * spi_mem_execute_tuning() - Execute controller tuning procedure
> + * @mem: the SPI memory device
> + * @read_op: read operation template (mandatory)
> + * @write_op: write operation template (optional, may be NULL)
> + *
> + * Requests the controller to perform tuning for high-speed operation
> + * using the provided op templates. On success the controller callback
> + * sets @read_op->max_freq (and @write_op->max_freq when non-NULL) to
> + * the validated clock rate.
> + *
> + * Return: 0 on success, -EINVAL if @mem or @read_op is NULL,
> + *         -EOPNOTSUPP if the controller doesn't support tuning,
> + *         or a negative error code on failure.
> + */
> +int spi_mem_execute_tuning(struct spi_mem *mem, struct spi_mem_op *read_op,
> +			   struct spi_mem_op *write_op)
> +{
> +	struct spi_controller *ctlr;
> +
> +	if (!mem || !read_op)
> +		return -EINVAL;
> +
> +	ctlr = mem->spi->controller;

[Severity: High]
Does this code execute hardware tuning operations without acquiring the SPI
bus locks or ensuring the controller is powered on?

Other standard SPI memory operations use spi_mem_access_start() and
spi_mem_access_end() to take the runtime PM reference and provide mutual
exclusion against other ongoing SPI transfers. Since those functions are
internal, external callers cannot acquire these necessary protections
themselves.

Could this lead to hardware lockups or data corruption on the SPI bus?

> +	if (!ctlr->mem_ops || !ctlr->mem_ops->execute_tuning)
> +		return -EOPNOTSUPP;
> +
> +	return ctlr->mem_ops->execute_tuning(mem, read_op, write_op);

[Severity: High]
Should this check if the SPI device is using a GPIO chip select before
delegating to the controller's hardware tuning callback?

Standard SPI memory operations (like spi_mem_exec_op) explicitly check
!spi_get_csgpiod(mem->spi, 0) before delegating to ctlr->mem_ops, because
hardware-level memory controllers operate the native chip select.

If a device is configured with a GPIO chip select, the hardware engine will
toggle the native CS (which might be disconnected or connected to a different
device) instead of the GPIO CS. Does this need a check similar to other
spi_mem operations?

> +}
> +EXPORT_SYMBOL_GPL(spi_mem_execute_tuning);

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