Re: [PATCH v2 2/5] spi: atmel-quadspi: add controller init callback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Medium] The custom `.init` callback is evaluated unconditionally in `atmel_qspi_resume()`, bypassing required PM state restoration for non-gclk variants.
- [Medium] Unbalanced clock prepare operations on the error path of the newly added `.init()` callback in `atmel_qspi_resume()`.

Pre-existing issues:
- [High] Pre-existing bugs in the `has_gclk` suspend/resume paths cause a PM runtime usage counter leak and perform hardware register accesses with disabled clocks.
- [High] The generic clock (`gclk`) enable count leaks continuously because `clk_prepare_enable()` is invoked unconditionally during every SPI setup.
--

--- Patch [2]: [PATCH v2 2/5] spi: atmel-quadspi: add controller init callback ---
commit 6c4a6f692bd1bda4ee70cd3b53efc7320ebf6191
Author: Robert Marko <robert.marko@sartura.hr>

spi: atmel-quadspi: add controller init callback

Allow controller variants to provide a custom initialization callback
through their capability data. This prepares the driver for variants
which require a different hardware initialization sequence without
adding SoC checks to the common path.

> diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
> index aaf7f4c46b22f..62ea84d234d02 100644
> --- a/drivers/spi/atmel-quadspi.c
> +++ b/drivers/spi/atmel-quadspi.c

[ ... ]

> @@ -1156,6 +1159,9 @@ static int atmel_qspi_sama7g5_setup(struct spi_device *spi)
>  	/* The controller can communicate with a single peripheral device (target). */
>  	aq->target_max_speed_hz = spi->max_speed_hz;
>  
> +	if (aq->caps->init)
> +		return aq->caps->init(aq);
> +

[Severity: High]
This is a pre-existing issue, but does invoking initialization callbacks
from atmel_qspi_sama7g5_setup() cause a generic clock enable count leak?

The generic clock enable count appears to leak continuously because
atmel_qspi_set_gclk() unconditionally calls clk_prepare_enable().

Since spi_setup() can be called multiple times during the device lifecycle,
such as when the SPI core probes child devices or adjusts transfer speeds,
the clock's enable count repeatedly increments:

drivers/spi/atmel-quadspi.c:atmel_qspi_set_gclk() {
    ...
	ret = clk_prepare_enable(aq->gclk);
    ...
}

Because clk_disable_unprepare() is only called once during system
suspend, the clock becomes severely unbalanced and might never be fully
shut down.

>  	return atmel_qspi_sama7g5_init(aq);
>  }

[ ... ]

> @@ -1570,6 +1576,9 @@ static int __maybe_unused atmel_qspi_resume(struct device *dev)
>  		return ret;
>  	}
>  
> +	if (aq->caps->init)
> +		return aq->caps->init(aq);
> +

[Severity: Medium]
Will this unconditional early return bypass required power management state
restoration for non-gclk variants?

If a non-gclk variant defines an init callback, it will intercept the
system resume path and completely bypass pm_runtime_force_resume() and the
default non-gclk hardware restoration sequence that occurs further down in
atmel_qspi_resume().

[Severity: Medium]
Does this also create a clock prepare count leak on the error path?

If the newly added init callback fails and returns a non-zero error code
in atmel_qspi_resume(), the function returns immediately.

This skips the necessary clk_unprepare() calls for aq->pclk and aq->qspick
that were successfully prepared earlier in atmel_qspi_resume().

>  	if (aq->caps->has_gclk)
>  		return atmel_qspi_sama7g5_init(aq);

[Severity: High]
This is a pre-existing issue, but does returning early for has_gclk
devices leak a power management runtime usage counter and access hardware
with disabled clocks?

In atmel_qspi_suspend(), pm_runtime_resume_and_get() increments the power
management usage count. For has_gclk devices, it returns early, skipping
pm_runtime_force_suspend() and missing a balancing pm_runtime_put():

drivers/spi/atmel-quadspi.c:atmel_qspi_suspend() {
    ...
	if (aq->caps->has_gclk) {
		ret = atmel_qspi_sama7g5_suspend(aq);
		clk_disable_unprepare(aq->pclk);
		return ret;
	}
    ...
}

In atmel_qspi_resume(), it calls clk_prepare() for aq->pclk but misses
clk_enable(). It then returns early via atmel_qspi_sama7g5_init(),
completely skipping pm_runtime_put_autosuspend().

As a result, every system suspend/resume cycle permanently leaks a power
management usage count.

Furthermore, because clk_enable() is never called, the hardware
initialization callback accesses registers with a disabled peripheral clock,
which could cause an asynchronous external abort or bus fault on resume:

drivers/spi/atmel-quadspi.c:atmel_qspi_lan969x_init() {
    ...
	atmel_qspi_write(QSPI_CR_DLLOFF, aq, QSPI_CR);
    ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709112006.390742-1-robert.marko@sartura.hr?part=2