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

[sashiko-bot@kernel.org]

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.
--

--- Patch [1]: [PATCH] spi: nxp-fspi: fix runtime PM cleanup on probe failure ---
commit 8285d0b4a15f14a23608e3bcf58977aed6bff9b3
Author: Jiawen Liu <1298662399@qq.com>

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://sashiko.dev/#/patchset/tencent_D74B48085D60A729C0BE5FD64A87FAB9A207@qq.com?part=1