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

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 <[email protected]>
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/[email protected]?part=2