Re: [PATCH 4/4] spi: nxp-fspi: disable runtime PM on probe failures

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Probe error paths leave the hardware powered on and clocks leaked because the newly added cleanup path cancels the autosuspend timer without suspending the device.

Pre-existing issues:
- [High] Unchecked pm_runtime_get_sync() failure in nxp_fspi_cleanup() can cause a synchronous external abort (bus fault).
- [High] The ignored return value of nxp_fspi_default_setup() corrupts the clock enable count and causes a kernel panic.
--

commit f84d61f342ed9ba0fef9a6eeb2fb3c7129154065
Author: Jiawen Liu <1298662399@qq.com>

spi: nxp-fspi: disable runtime PM on probe failures

nxp_fspi_probe() enables runtime PM and autosuspend before several operations
that can fail. Some failure paths returned directly before the devm cleanup
action was installed, leaving runtime PM enabled. Route those failures through
a common runtime PM cleanup path. Use pm_runtime_resume_and_get() for the
initial clock enable.

> diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
> index 1e36ae084dd86..d94a2a7b98d44 100644
> --- a/drivers/spi/spi-nxp-fspi.c
> +++ b/drivers/spi/spi-nxp-fspi.c
[ ... ]
> @@ -1362,18 +1364,24 @@ static int nxp_fspi_probe(struct platform_device *pdev)
>  	nxp_fspi_default_setup(f);

[Severity: High]
This is a pre-existing issue, but does nxp_fspi_probe() need to check the
return value of nxp_fspi_default_setup()?

If nxp_fspi_default_setup() fails, it leaves the clocks disabled. Since this
return value is ignored, the driver proceeds to register the SPI controller.

When the device autosuspends, nxp_fspi_runtime_suspend() will disable the
already-disabled clock, which drives the clock enable count negative.

A subsequent access to the SPI bus will increment the count back to 0 in
nxp_fspi_runtime_resume(), leaving the clock physically OFF.

Would this lead to reading/writing unclocked MMIO registers and trigger a
synchronous external abort?

>  
>  	ret = pm_runtime_put_sync(dev);
[ ... ]
> @@ -1389,6 +1397,11 @@ static int nxp_fspi_probe(struct platform_device *pdev)
>  		return ret;
>  
>  	return devm_spi_register_controller(&pdev->dev, ctlr);
> +
> +err_disable_pm:
> +	pm_runtime_dont_use_autosuspend(dev);
> +	pm_runtime_disable(dev);

[Severity: Medium]
When execution jumps here due to a failure (e.g., in devm_request_irq()),
the device is still in the RPM_ACTIVE state because pm_runtime_put_sync()
only scheduled an autosuspend timer.

Will calling pm_runtime_disable() cancel the pending autosuspend timer
and freeze the device in RPM_ACTIVE?

Since the devm cleanup action (nxp_fspi_cleanup) has not been registered yet,
does this leave the hardware powered on and permanently leak the clocks?

> +	return ret;
>  }

[Severity: High]
This is a pre-existing issue, but there appears to be another unchecked
clock enablement in nxp_fspi_cleanup():

drivers/spi/spi-nxp-fspi.c:nxp_fspi_cleanup() {
	...
	/* enable clock first since there is register access */
	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 enable the clocks, the hardware remains
unclocked.

Since the return value is ignored, the code unconditionally proceeds to
execute fspi_writel().

Can accessing unclocked MMIO registers trigger a synchronous external
abort and crash the system during teardown or module unload?

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