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

> -----Original Message-----
> From: [email protected] <[email protected]>
> Sent: 2026年7月28日 16:19
> To: Jiawen Liu <[email protected]>
> Cc: [email protected]; [email protected]
> Subject: 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.

These are all pre-existing issues. To keep things simple, I'll send another patch set to address them.

Regards
Haibo Chen
> --
> 
> --- 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F
> %2Fsashiko.dev%2F%23%2Fpatchset%2F20260728080443.153827-1-12986
> 62399%40qq.com%3Fpart%3D2&data=05%7C02%7Chaibo.chen%40nxp.com
> %7Cd9dc3eb6701f494aa6bc08deec81bb08%7C686ea1d3bc2b4c6fa92cd99c
> 5c301635%7C0%7C0%7C639208239104394557%7CUnknown%7CTWFpbGZ
> sb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIs
> IkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=gojbFGHTm2l
> LqDrBAH7cl%2FDtoHHsATysYve%2BsTxoTLk%3D&reserved=0