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

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

[Jiawen Liu]

nxp_fspi_probe() enables runtime PM and then gets the device active for
the initial register setup. If that get fails, the old open-coded
pm_runtime_get_sync() path needs manual cleanup of both the usage counter
and runtime PM enablement.

Follow the newer spi-nxp-xspi pattern instead: use
devm_pm_runtime_enable() for managed runtime PM disablement, and use
PM_RUNTIME_ACQUIRE_AUTOSUSPEND() for the initial active window. This keeps
the error path simple and avoids leaving runtime PM state behind when the
initial resume fails.

Signed-off-by: Jiawen Liu <1298662399@qq.com>
---
 drivers/spi/spi-nxp-fspi.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
index 1e36ae084dd8..6be78712b534 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);
 
@@ -1345,12 +1344,15 @@ static int nxp_fspi_probe(struct platform_device *pdev)
 	if (irq < 0)
 		return dev_err_probe(dev, irq, "Failed to get irq source");
 
-	pm_runtime_enable(dev);
 	pm_runtime_set_autosuspend_delay(dev, FSPI_RPM_TIMEOUT);
 	pm_runtime_use_autosuspend(dev);
+	ret = devm_pm_runtime_enable(dev);
+	if (ret)
+		return ret;
 
 	/* enable clock */
-	ret = pm_runtime_get_sync(f->dev);
+	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(dev, pm);
+	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
 	if (ret < 0)
 		return dev_err_probe(dev, ret, "Failed to enable clock");
 
@@ -1361,10 +1363,6 @@ static int nxp_fspi_probe(struct platform_device *pdev)
 
 	nxp_fspi_default_setup(f);
 
-	ret = pm_runtime_put_sync(dev);
-	if (ret < 0)
-		return dev_err_probe(dev, ret, "Failed to disable clock");
-
 	init_completion(&f->c);
 	ret = devm_request_irq(dev, irq,
 			nxp_fspi_irq_handler, 0, pdev->name, f);
-- 
2.34.1

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

[sashiko-bot]

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 <1298662399@qq.com>

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?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728080443.153827-1-1298662399@qq.com?part=1

[PATCH v2 2/3] spi: nxp-fspi: propagate default setup failures

[Jiawen Liu]

nxp_fspi_default_setup() may fail while setting the controller clock rate
or while preparing and enabling the clock. The probe and runtime-resume
paths currently ignore that return value and continue as if the controller
was usable.

Return the setup error from probe and runtime resume so the driver does not
leave the PM core believing the controller is active after setup failed.

Signed-off-by: Jiawen Liu <1298662399@qq.com>
---
 drivers/spi/spi-nxp-fspi.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
index 6be78712b534..3f1c3ed748eb 100644
--- a/drivers/spi/spi-nxp-fspi.c
+++ b/drivers/spi/spi-nxp-fspi.c
@@ -1361,7 +1361,9 @@ static int nxp_fspi_probe(struct platform_device *pdev)
 	if (reg)
 		fspi_writel(f, reg, f->iobase + FSPI_INTR);
 
-	nxp_fspi_default_setup(f);
+	ret = nxp_fspi_default_setup(f);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to setup controller");
 
 	init_completion(&f->c);
 	ret = devm_request_irq(dev, irq,
@@ -1408,7 +1410,10 @@ static int nxp_fspi_runtime_resume(struct device *dev)
 		return ret;
 
 	if (f->flags & FSPI_NEED_INIT) {
-		nxp_fspi_default_setup(f);
+		ret = nxp_fspi_default_setup(f);
+		if (ret)
+			return ret;
+
 		ret = pinctrl_pm_select_default_state(dev);
 		if (ret)
 			dev_err(dev, "select flexspi default pinctrl failed!\n");
-- 
2.34.1

Re: [PATCH v2 2/3] spi: nxp-fspi: propagate default setup failures

[sashiko-bot]

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

New issues:
- [High] Failure in `nxp_fspi_default_setup()` during runtime resume leaks the hardware clocks.

Pre-existing issues:
- [High] Probe error paths following PM runtime auto-suspend guard acquisition cancel the pending suspend, leaving clocks permanently enabled.
- [High] The PM runtime usage counter is leaked in `nxp_fspi_exec_op()` if `pm_runtime_get_sync()` fails.
- [High] Missing error check for `pm_runtime_get_sync()` in `nxp_fspi_cleanup()` can cause a system crash and unbalanced clock disables.
--

--- Patch [2]: [PATCH v2 2/3] spi: nxp-fspi: propagate default setup failures ---
commit aea7011ea2d11337a5d6f21669233c5dbd4bf29b
Author: Jiawen Liu <1298662399@qq.com>
Subject: spi: nxp-fspi: propagate default setup failures

nxp_fspi_default_setup() may fail while setting the controller clock rate
or while preparing and enabling the clock. The probe and runtime-resume
paths currently ignore that return value and continue as if the controller
was usable.

> diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
> index 6be78712b5343..3f1c3ed748eba 100644
> --- a/drivers/spi/spi-nxp-fspi.c
> +++ b/drivers/spi/spi-nxp-fspi.c
> @@ -1361,7 +1361,9 @@ static int nxp_fspi_probe(struct platform_device *pdev)
>  	if (reg)
>  		fspi_writel(f, reg, f->iobase + FSPI_INTR);
>  
> -	nxp_fspi_default_setup(f);
> +	ret = nxp_fspi_default_setup(f);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to setup controller");

[Severity: High]
This isn't a bug introduced by this patch, but does returning an error here
leak the hardware clocks?

Earlier in nxp_fspi_probe(), the PM guard is acquired:

	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(dev, pm);

If probe fails and we return an error, the guard goes out of scope and
queues an asynchronous suspend.

Devres then unwinds devm_pm_runtime_enable(), which calls
pm_runtime_disable().

According to PM subsystem guidelines, pm_runtime_disable() cancels any pending
async suspend requests, which might leave the device powered on indefinitely.

>  	init_completion(&f->c);
>  	ret = devm_request_irq(dev, irq,

[ ... ]

> @@ -1408,7 +1410,10 @@ static int nxp_fspi_runtime_resume(struct device *dev)
>  		return ret;
>  
>  	if (f->flags & FSPI_NEED_INIT) {
> -		nxp_fspi_default_setup(f);
> +		ret = nxp_fspi_default_setup(f);
> +		if (ret)
> +			return ret;

[Severity: High]
If nxp_fspi_default_setup() fails here, does this regression leave the hardware
clocks enabled permanently?

nxp_fspi_clk_prep_enable(f) was called earlier in nxp_fspi_runtime_resume()
to enable the clocks.

If this returns an error, the PM core aborts the resume and the device stays
in the RPM_SUSPENDED state, but it doesn't look like
nxp_fspi_clk_disable_unprep() is called to disable the clocks before returning.


[Severity: High]
This is a pre-existing issue, but I noticed a potential leak of the PM
runtime usage counter in nxp_fspi_exec_op() when pm_runtime_get_sync() fails:

drivers/spi/spi-nxp-fspi.c:nxp_fspi_exec_op() {
    ...
	err = pm_runtime_get_sync(f->dev);
	if (err < 0) {
		dev_err(f->dev, "Failed to enable clock %d\n", __LINE__);
		return err;
	}
    ...
}

Because pm_runtime_get_sync() increments the usage counter regardless of
success, returning directly on failure without calling pm_runtime_put_noidle()
might permanently leak the counter and prevent the device from entering runtime
suspend again.

Could this prevent the device from suspending in the future?


[Severity: High]
This is a pre-existing issue, but is it safe to proceed without checking the
return value of pm_runtime_get_sync() in nxp_fspi_cleanup()?

drivers/spi/spi-nxp-fspi.c:nxp_fspi_cleanup() {
    ...
	pm_runtime_get_sync(f->dev);

	/* disable the hardware */
	fspi_writel(f, FSPI_MCR0_MDIS, f->iobase + FSPI_MCR0);
    ...
}

If pm_runtime_get_sync() fails to resume the device and enable clocks, the
driver immediately writes to the FSPI_MCR0 register on unpowered hardware.

Could this cause a synchronous external abort on ARM SoCs, and subsequently
cause unbalanced clock disables when nxp_fspi_clk_disable_unprep() is called?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728080443.153827-1-1298662399@qq.com?part=2

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

[Jiawen Liu]

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@qq.com>
---
 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);
 
-	/* 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

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

[sashiko-bot]

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 <1298662399@qq.com>

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/20260728080443.153827-1-1298662399@qq.com?part=3