Re: [PATCH 3/4] spi: fsl-dspi: clean up after failed suspend and resume

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Deadlock and guaranteed 5-second suspend timeout due to incorrect lifecycle ordering in dspi_suspend().
- [High] Race condition and potential deadlock in dspi_resume() due to resuming the SPI queue before hardware initialization.
--

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

spi: fsl-dspi: clean up after failed suspend and resume

This patch updates the suspend and resume routines to better handle failures.
It ensures the IRQ is re-enabled if spi_controller_suspend() fails, and
properly unwinds clock preparation if controller resume or hardware
initialization fails.

> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> index 019d05cdefe69..c2d283876ef80 100644
> --- a/drivers/spi/spi-fsl-dspi.c
> +++ b/drivers/spi/spi-fsl-dspi.c
> @@ -1464,10 +1464,18 @@ static int dspi_init(struct fsl_dspi *dspi)
>  static int dspi_suspend(struct device *dev)
>  {
>  	struct fsl_dspi *dspi = dev_get_drvdata(dev);
> +	int ret;
>  
>  	if (dspi->irq)
>  		disable_irq(dspi->irq);
> -	spi_controller_suspend(dspi->ctlr);
> +
> +	ret = spi_controller_suspend(dspi->ctlr);

[Severity: High]
This is a pre-existing issue, but does disabling the IRQ before suspending
the controller cause active transfers to hang?

Looking at dspi_suspend(), disable_irq() is called before
spi_controller_suspend(). When spi_controller_suspend() stops the queue, it
waits up to 5 seconds for the active transfer to finish.

However, if the active transfer requires the hardware interrupt handler to
complete, it will never finish because the IRQ is already disabled. This
seems like it would guarantee a 5-second timeout, causing
spi_controller_suspend() to return -EBUSY and abort the suspend.

Should the controller be suspended before the IRQ is disabled?

> +	if (ret) {
> +		if (dspi->irq)
> +			enable_irq(dspi->irq);
> +		return ret;
> +	}
> +
>  	clk_disable_unprepare(dspi->clk);

[ ... ]

> @@ -1485,12 +1493,15 @@ static int dspi_resume(struct device *dev)
>  	ret = clk_prepare_enable(dspi->clk);
>  	if (ret)
>  		return ret;
> -	spi_controller_resume(dspi->ctlr);
> +
> +	ret = spi_controller_resume(dspi->ctlr);
> +	if (ret)
> +		goto disable_clk;
>  
>  	ret = dspi_init(dspi);

[Severity: High]
This isn't a bug introduced by this patch, but does calling
spi_controller_resume() before dspi_init() and enable_irq() create a race
window?

spi_controller_resume() unblocks the message pump, allowing the worker
thread to immediately dispatch pending transfers. If a transfer starts
before dspi_init() initializes the hardware state, it might interact with
uninitialized registers.

Additionally, dspi_init() clears the status registers:

	regmap_write(dspi->regmap, SPI_SR, SPI_SR_CLEAR);

If a racing transfer sets completion flags in the hardware right as
dspi_init() clears them, the interrupt could be permanently lost. The
worker thread would then deadlock waiting for the xfer_done completion
event.

Should the hardware and IRQs be fully restored before resuming the queue?

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