RE: [PATCH] spi: nxp-fspi: fix runtime PM cleanup on probe failure

> -----Original Message-----
> From: [email protected] <[email protected]>
> Sent: 2026年7月26日 14:04
> To: Jiawen Liu <[email protected]>
> Cc: [email protected]; [email protected]
> Subject: Re: [PATCH] spi: nxp-fspi: fix runtime PM cleanup on probe failure
> 
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s)
> to consider:
> 
> Pre-existing issues:
> - [High] The return value of `nxp_fspi_default_setup()` is ignored, which can
> leave the device in an unclocked state and cause a system crash upon register
> access.
> - [High] Unchecked return value of `pm_runtime_get_sync()` in
> `nxp_fspi_cleanup()` can lead to a bus fault during driver removal.
> - [High] `nxp_fspi_irq_handler` accesses hardware registers without verifying
> the device's PM state, risking a kernel panic on late or spurious interrupts.

Hi Jiawen

The three issues raised by Sashiko are all valid. It would be good to address them in the next patch set as well. If you're not interested in working on them, I can help follow up on them.

Regards
Haibo Chen

> --
> 
> --- Patch [1]: [PATCH] spi: nxp-fspi: fix runtime PM cleanup on probe failure ---
> commit 8285d0b4a15f14a23608e3bcf58977aed6bff9b3
> Author: Jiawen Liu <[email protected]>
> 
> spi: nxp-fspi: fix runtime PM cleanup on probe failure
> 
> This commit cleans up the runtime PM state by dropping the usage counter
> and disabling runtime PM if pm_runtime_get_sync() fails during probe.
> 
> > diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
> > index 1e36ae084dd86..c6037f3209c5a 100644
> > --- a/drivers/spi/spi-nxp-fspi.c
> > +++ b/drivers/spi/spi-nxp-fspi.c
> > @@ -1352,7 +1352,7 @@ static int nxp_fspi_probe(struct platform_device
> *pdev)
> >  	/* enable clock */
> >  	ret = pm_runtime_get_sync(f->dev);
> >  	if (ret < 0)
> > -		return dev_err_probe(dev, ret, "Failed to enable clock");
> > +		goto err_pm_runtime;
> >
> >  	/* Clear potential interrupts */
> >  	reg = fspi_readl(f, f->iobase + FSPI_INTR);
> 
> [ ... ]
> 
> >  	if (reg)
> >  		fspi_writel(f, reg, f->iobase + FSPI_INTR);
> >
> >  	nxp_fspi_default_setup(f);
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but is it safe to ignore the return
> value of nxp_fspi_default_setup() here and in nxp_fspi_runtime_resume()?
> 
> nxp_fspi_default_setup() disables the peripheral clocks at the beginning.
> If it subsequently fails to set the rate or prep/enable the clock, it returns a
> negative error code with the clocks still physically disabled.
> 
> Because nxp_fspi_probe() and nxp_fspi_runtime_resume() ignore this return
> value, the SPI core might later call nxp_fspi_exec_op() while the PM core
> believes the device is active. Subsequent MMIO accesses to the unclocked
> controller will trigger a synchronous external abort (bus fault) and panic.
> 
> [ ... ]
> 
> > +err_pm_runtime:
> > +	pm_runtime_put_noidle(dev);
> > +	pm_runtime_dont_use_autosuspend(dev);
> > +	pm_runtime_disable(dev);
> > +
> > +	return dev_err_probe(dev, ret, "Failed to enable clock");
> >  }
> 
> 
> Also, looking at the driver removal path:
> 
> nxp_fspi_cleanup() {
> 	...
> 	pm_runtime_get_sync(f->dev);
> 
> 	/* disable the hardware */
> 	fspi_writel(f, FSPI_MCR0_MDIS, f->iobase + FSPI_MCR0);
> 	...
> }
> 
> [Severity: High]
> This is a pre-existing issue, but could this cause a bus fault during driver
> removal?
> 
> If pm_runtime_get_sync() fails, the clock is not enabled. The driver ignores
> the return value and immediately performs an MMIO write to the
> FSPI_MCR0 register. Accessing the hardware with disabled clocks will trigger a
> synchronous external abort.
> 
> 
> Finally, regarding the interrupt handler:
> 
> nxp_fspi_do_op() {
> 	...
> 	if (!wait_for_completion_timeout(&f->c, msecs_to_jiffies(1000)))
> 		err = -ETIMEDOUT;
> 	...
> }
> 
> nxp_fspi_irq_handler() {
> 	...
> 	/* clear interrupt */
> 	reg = fspi_readl(f, f->iobase + FSPI_INTR);
> 	...
> }
> 
> [Severity: High]
> This is a pre-existing issue, but does the IRQ handler need to verify the
> device's PM state?
> 
> During a SPI operation, if wait_for_completion_timeout() in
> nxp_fspi_do_op() expires, it abandons the operation without masking the
> controller's interrupt. nxp_fspi_exec_op() then places the device into
> autosuspend.
> 
> If the hardware is slow and finishes the operation after the device has
> suspended and its clocks are gated, it will assert the interrupt.
> The nxp_fspi_irq_handler() reads the FSPI_INTR register directly without
> checking pm_runtime_get_if_active(). Reading unclocked registers causes a
> bus fault.
> 
> --
> Sashiko AI
> review ・ https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F
> %2Fsashiko.dev%2F%23%2Fpatchset%2Ftencent_D74B48085D60A729C0BE5
> FD64A87FAB9A207%40qq.com%3Fpart%3D1&data=05%7C02%7Chaibo.chen
> %40nxp.com%7C2250421a98754f52851108deeadc0357%7C686ea1d3bc2b
> 4c6fa92cd99c5c301635%7C0%7C0%7C639206427830880359%7CUnknown
> %7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAi
> OiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=
> N3roQX8MLnggUpWZOm436nKrcHwd1DzpmfdFFiKhK%2B0%3D&reserved=0