Re: [PATCH v2 3/3] spi: nxp-fspi: check runtime PM get in cleanup

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Asynchronous runtime PM put in cleanup path causes a race condition leading to double clock disable.
--

--- Patch [3]: [PATCH v2 3/3] spi: nxp-fspi: check runtime PM get in cleanup ---
commit 7a9a4bc911b3e8e33c657c1f4c72809d1937328c
Author: Jiawen Liu <[email protected]>

spi: nxp-fspi: check runtime PM get in cleanup

This commit updates nxp_fspi_cleanup() to use PM_RUNTIME_ACQUIRE() to
safely check the result of the runtime PM get operation. If the device
fails to resume, it skips the MMIO write that disables the hardware.

> diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
> index 3f1c3ed748eba..2671ffb490968 100644
> --- a/drivers/spi/spi-nxp-fspi.c
> +++ b/drivers/spi/spi-nxp-fspi.c
> @@ -1268,14 +1268,21 @@ static void nxp_fspi_cleanup(void *data)
>  {
>  	struct nxp_fspi *f = data;
> +	int ret;
>  
> -	/* enable clock first since there is register access */
> -	pm_runtime_get_sync(f->dev);
> +	{
> +		/* enable clock first since there is register access */
> +		PM_RUNTIME_ACQUIRE(f->dev, pm);
>  
> -	/* disable the hardware */
> -	fspi_writel(f, FSPI_MCR0_MDIS, f->iobase + FSPI_MCR0);
> +		ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +		if (ret < 0) {
> +			dev_warn(f->dev, "Failed to enable clock for cleanup: %d\n", ret);
> +		} else {
> +			/* disable the hardware */
> +			fspi_writel(f, FSPI_MCR0_MDIS, f->iobase + FSPI_MCR0);
> +		}
> +	}

[Severity: High]
Could this scope-based PM_RUNTIME_ACQUIRE() introduce a race condition?

The PM_RUNTIME_ACQUIRE() macro acts as a guard that calls an asynchronous
pm_runtime_put() when it goes out of scope at the end of this block.

Because it is asynchronous, the queued suspend could execute concurrently
and call nxp_fspi_runtime_suspend(), which disables the clocks.

Meanwhile, the cleanup thread immediately proceeds to explicitly call
nxp_fspi_clk_disable_unprep(f), which could result in a concurrent double
clock disable and trigger a common clock framework warning.

Additionally, using an asynchronous put just before device removal or
pm_runtime_disable() means the pending suspend work might be cancelled
by __pm_runtime_barrier(). Does this leave the PM core thinking the device
is active while its hardware clocks have been manually turned off?

> -	pm_runtime_put_noidle(f->dev);
>  	nxp_fspi_clk_disable_unprep(f);
>  
>  	if (f->ahb_addr)

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