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

[Bough Chen]

> -----Original Message-----
> From: Jiawen Liu <1298662399@xxxxxx>
> Sent: 2026年7月28日 16:05
> To: Han Xu <han.xu@xxxxxxx>; Bough Chen <haibo.chen@xxxxxxx>; Yogesh
> Gaur <yogeshgaur.83@xxxxxxxxx>; Mark Brown <broonie@xxxxxxxxxx>
> Cc: linux-spi@xxxxxxxxxxxxxxx; imx@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; Jiawen Liu <1298662399@xxxxxx>
> Subject: [PATCH v2 3/3] spi: nxp-fspi: check runtime PM get in cleanup
> 
> nxp_fspi_cleanup() resumes the device before disabling the controller,
> because the cleanup path writes to controller registers. The return value
> from the runtime PM get is currently ignored, so a failed resume can be
> followed by MMIO access while the controller clock is still gated.
> 
> Use PM_RUNTIME_ACQUIRE() and check the acquire result before touching
> the
> registers. If the device cannot be resumed, skip the hardware-disable MMIO
> write and continue with the remaining cleanup.
> 
> Signed-off-by: Jiawen Liu <1298662399@xxxxxx>
> ---
>  drivers/spi/spi-nxp-fspi.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
> index 3f1c3ed748eb..2671ffb49096 100644
> --- a/drivers/spi/spi-nxp-fspi.c
> +++ b/drivers/spi/spi-nxp-fspi.c
> @@ -1268,14 +1268,21 @@ static const struct spi_controller_mem_caps
> nxp_fspi_mem_caps_disable_dtr = {
>  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);

Thanks for the patch, but please don't use PM_RUNTIME_ACQUIRE() in the cleanup/remove path here.

PM_RUNTIME_ACQUIRE() is a scope guard whose destructor implicitly calls pm_runtime_put() when it goes out of scope. pm_runtime_put() is an asynchronous idle put (__pm_runtime_idle(dev, RPM_GET_PUT | RPM_ASYNC)), so it may queue an idle/suspend that runs nxp_fspi_runtime_suspend(), which in turn calls nxp_fspi_clk_disable_unprep().

The remove path has strict ordering requirements that this implicit put cannot express:

pm_runtime_disable() must be called first, so the runtime PM core can no longer invoke nxp_fspi_runtime_suspend() on its own.

The put must be pm_runtime_put_noidle() - it only drops the usage count and must never queue an idle/suspend. The guard uses pm_runtime_put(), not pm_runtime_put_noidle(), so it breaks this.

nxp_fspi_clk_disable_unprep() is then done explicitly and safely, because the PM state machine is already stopped.

With the scope guard, the destructor fires at the end of the block and its ordering relative to pm_runtime_disable() is dictated by scope, not by us. Depending on the usage count, the guard's pm_runtime_put() may either race with the explicit nxp_fspi_clk_disable_unprep() (double clock disable -> CCF warning), or the pending async work may later be cancelled by __pm_runtime_barrier() during removal (clock leak / inconsistent PM state).

In short: the cleanup/remove path must keep explicit control over the pm_runtime_disable() / put ordering and must use pm_runtime_put_noidle(). A scope guard with an implicit pm_runtime_put() cannot express this.

If the goal is only to check the return value of the get operation, please keep the explicit form, e.g.:

static void nxp_fspi_cleanup(void *data)
{
	struct nxp_fspi *f = data;
	int ret;

	/* enable clock first since there is register access */
	ret = pm_runtime_get_sync(f->dev);
	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);

	pm_runtime_disable(f->dev);
	pm_runtime_put_noidle(f->dev);
	nxp_fspi_clk_disable_unprep(f);

	if (f->ahb_addr)
		iounmap(f->ahb_addr);
}
This keeps the required get_sync -> (conditional MMIO) -> disable -> put_noidle -> clk_disable_unprep ordering, while still checking the get result and skipping the register access when the resume fails.

Thanks, Haibo
> 
> -	/* 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);
> +		}
> +	}
> 
> -	pm_runtime_put_noidle(f->dev);
>  	nxp_fspi_clk_disable_unprep(f);
> 
>  	if (f->ahb_addr)
> --
> 2.34.1