Re: [PATCH AUTOSEL 6.18] spi: fsl-dspi: clean up after failed suspend and resume

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] spi: fsl-dspi: premature disable_irq() and unroll gap in dspi_suspend()
--

commit 7e693362fffc058a6e36cb3136ecf18f7eb1c968
Author: Jiawen Liu <1298662399@qq.com>

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

This commit checks the return values of spi_controller_suspend() in
dspi_suspend() and spi_controller_resume() in dspi_resume(), restoring the
IRQ or disabling the clock if an error occurs.

Link: https://patch.msgid.link/tencent_427FA55E3D59112524886E9C931CA0F92F06@qq.com

> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> index 83ea296597e94..1283a193b6fcc 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);
> +	if (ret) {
> +		if (dspi->irq)
> +			enable_irq(dspi->irq);
> +		return ret;
> +	}

[Severity: High]
When dspi_suspend() takes this new error return path after
spi_controller_suspend() fails, does it leave SPI_CONTROLLER_SUSPENDED
permanently set on dspi->ctlr->flags?

In drivers/spi/spi-fsl-dspi.c, dspi_suspend() calls disable_irq(dspi->irq)
before calling spi_controller_suspend(dspi->ctlr). On platforms using
DSPI_XSPI_MODE with an IRQ, in-flight or queued transfers in
dspi_transfer_one_message() wait for dspi_interrupt() to signal
dspi->xfer_done:

drivers/spi/spi-fsl-dspi.c:dspi_transfer_one_message() {
    ...
			if (dspi->irq)
				reinit_completion(&dspi->xfer_done);

			dspi_fifo_write(dspi);

			if (dspi->irq)
				wait_for_completion(&dspi->xfer_done);
			else
				dspi_poll(dspi);
    ...
}

Because disable_irq(dspi->irq) has already disabled the interrupt line, an
in-flight IRQ-driven transfer cannot complete while spi_controller_suspend()
waits in spi_stop_queue() for the queue to drain, causing spi_stop_queue()
to time out after 5 seconds with -EBUSY.

In drivers/spi/spi.c, spi_controller_suspend() still calls
__spi_mark_suspended(ctlr) unconditionally even when spi_stop_queue() fails:

drivers/spi/spi.c:spi_controller_suspend() {
    ...
	if (ctlr->queued) {
		ret = spi_stop_queue(ctlr);
		if (ret)
			dev_err(&ctlr->dev, "queue stop failed\n");
	}

	__spi_mark_suspended(ctlr);
	return ret;
}

When dspi_suspend() returns -EBUSY to device_suspend(),
dev->power.is_suspended is not set, so device_resume() skips calling
dspi_resume() during suspend-abort recovery. With nothing calling
spi_controller_resume(dspi->ctlr) to clear SPI_CONTROLLER_SUSPENDED,
wouldn't all subsequent synchronous SPI transfers in __spi_sync() fail with
-ESHUTDOWN?

Should disable_irq(dspi->irq) be called after spi_controller_suspend()
succeeds, or should the failure path also call spi_controller_resume()?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019135441.1420968-426-sashal@kernel.org?part=1