RE: [PATCH v2 1/3] spi: nxp-fspi: use devm runtime PM enablement

> -----Original Message-----
> From: [email protected] <[email protected]>
> Sent: 2026年7月28日 16:21
> To: Jiawen Liu <[email protected]>
> Cc: [email protected]; [email protected]
> Subject: Re: [PATCH v2 1/3] spi: nxp-fspi: use devm runtime PM enablement
> 
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s)
> to consider:
> - [High] Removing pm_runtime_disable() from nxp_fspi_cleanup() while
> keeping manual clock disablement causes a deterministic double clock disable
> (reference count underflow) on driver unbind and probe error paths.
> --
> 
> --- Patch [1]: [PATCH v2 1/3] spi: nxp-fspi: use devm runtime PM enablement
> --- commit 274e174a430a74bbdd5c40d6aaca37aea56ac764
> Author: Jiawen Liu <[email protected]>
> 
> spi: nxp-fspi: use devm runtime PM enablement
> 
> This commit replaces open-coded runtime PM enablement with
> devm_pm_runtime_enable() and uses
> PM_RUNTIME_ACQUIRE_AUTOSUSPEND() to simplify the error path and
> avoid leaving runtime PM state behind on failure.
> 
> > diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
> > index 1e36ae084dd86..6be78712b5343 100644
> > --- a/drivers/spi/spi-nxp-fspi.c
> > +++ b/drivers/spi/spi-nxp-fspi.c
> > @@ -1275,7 +1275,6 @@ static void nxp_fspi_cleanup(void *data)
> >  	/* 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);
> 
> [Severity: High]
> Does removing pm_runtime_disable() here cause a double clock disable
> during driver unbind or late probe failure?
> 
> When devres executes nxp_fspi_cleanup(), it drops the usage count via
> pm_runtime_put_noidle() and manually disables the clocks with
> nxp_fspi_clk_disable_unprep(). However, the device remains in the
> RPM_ACTIVE state.
> 
> Later in the devres cleanup phase, the action registered by
> devm_pm_runtime_enable() executes and clears autosuspend. Because the
> PM usage count is 0 and autosuspend is now disabled, the PM core
> synchronously transitions the device to RPM_SUSPENDED by executing the
> driver's runtime suspend callback.
> 
> Will the runtime suspend callback then blindly invoke
> nxp_fspi_clk_disable_unprep() a second time, triggering a clock reference
> count underflow?

I believe this is a false positive.

The finding assumes that when the devm_pm_runtime_enable() cleanup action runs, it will "clear autosuspend and synchronously execute the runtime suspend callback," resulting in a second call to nxp_fspi_clk_disable_unprep(). That is not how it works.

devm_pm_runtime_enable() registers pm_runtime_disable() as its devres teardown action. pm_runtime_disable() does not invoke the runtime suspend callback ― it disables the runtime PM framework and cancels any pending requests. It would only force a synchronous suspend if one were already in progress, which is not the case here.

Looking at nxp_fspi_cleanup():

pm_runtime_get_sync(f->dev);      /* usage_count = 1, RPM_ACTIVE, clocks on */
fspi_writel(f, FSPI_MCR0_MDIS, f->iobase + FSPI_MCR0);
pm_runtime_put_noidle(f->dev);    /* usage_count = 0, but *_noidle does NOT queue a suspend */
nxp_fspi_clk_disable_unprep(f);   /* clocks disabled manually */
pm_runtime_put_noidle() decrements the usage count without scheduling an idle/suspend request. So no suspend is queued, and when the devm_pm_runtime_enable() teardown later calls pm_runtime_disable(), the runtime suspend callback is never executed. There is no second nxp_fspi_clk_disable_unprep() call and therefore no clock reference count underflow.

Removing pm_runtime_disable() from nxp_fspi_cleanup() is in fact required by this change: devm_pm_runtime_enable() now owns that call and performs it during its own devres teardown. Keeping the explicit pm_runtime_disable() would cause a double pm_runtime_disable() instead.

So the two changes are correctly paired and the patch is fine as-is.

Regards
Haibo Chen
> 
> [ ... ]
> 
> --
> Sashiko AI
> review ・ https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F
> %2Fsashiko.dev%2F%23%2Fpatchset%2F20260728080443.153827-1-12986
> 62399%40qq.com%3Fpart%3D1&data=05%7C02%7Chaibo.chen%40nxp.com
> %7Cc2a2eed4f99b49b0839308deec821919%7C686ea1d3bc2b4c6fa92cd99c
> 5c301635%7C0%7C0%7C639208240688136932%7CUnknown%7CTWFpbGZ
> sb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIs
> IkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=W%2FNs0USv
> BAOYBLNqjFOpjk8AR%2Bjq7uHSBdBKqY7qX98%3D&reserved=0