Re: [PATCH v4 6/7] spi: cadence-xspi: Add support for panic writes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] spi: cadence-xspi: missing interrupt status clear causes premature SDMA readiness
- [High] spi: cadence-xspi: shared IRQ steals SDMA_TRIGGER from polling loop
- [High] spi: cadence-xspi: 1ms timeout causes panic writes to fail if controller is busy
- [High] spi: cadence-xspi: missing reinit_completion() allows premature completion
- [Medium] spi: cadence-xspi: SDMA polling ignores error status
--

commit 7f5b0e01fa6683f0eb21af13e7014a84bdbb381d
Author: Tanmay Jagdale <tanmay@marvell.com>

spi: cadence-xspi: Add support for panic writes

Advertise support for panic writes. When a memory op is received with
the 'panic' flag set, the driver will then work without relying on
interrupts and without sleeping.

> diff --git a/drivers/spi/spi-cadence-xspi.c b/drivers/spi/spi-cadence-xspi.c
> index 2a2a40bf0a4c4..a7fd715c45d5f 100644
> --- a/drivers/spi/spi-cadence-xspi.c
> +++ b/drivers/spi/spi-cadence-xspi.c
> @@ -353,40 +357,33 @@ struct cdns_xspi_dev {
>  	int current_xfer_qword;
>  };
>  
> -static int cdns_xspi_wait_for_controller_idle(struct cdns_xspi_dev *cdns_xspi)
> +static int cdns_xspi_wait_for_controller_idle(struct cdns_xspi_dev *cdns_xspi, bool sleep)
>  {
>  	u32 ctrl_stat;
>  
> -	return readl_relaxed_poll_timeout(cdns_xspi->iobase +
> -					  CDNS_XSPI_CTRL_STATUS_REG,
> -					  ctrl_stat,
> -					  ((ctrl_stat &
> -					    CDNS_XSPI_CTRL_BUSY) == 0),
> -					  100, 1000);
> +	return cdns_readl_poll(cdns_xspi->iobase + CDNS_XSPI_CTRL_STATUS_REG,
> +			       ctrl_stat, !(ctrl_stat & CDNS_XSPI_CTRL_BUSY),
> +			       100, 1000, sleep);

[Severity: High]
Does this 1000us timeout cause panic writes to fail if the controller is busy?

If a kernel panic occurs while the controller is executing a large data
transfer, the hardware might still be busy finishing the interrupted transfer.

The 1ms timeout here will cause the function to fail with -ETIMEDOUT, instantly
aborting the panic write.

>  }
>  
>  static bool cdns_xspi_is_stig_ready(struct cdns_xspi_dev *cdns_xspi, bool sleep)
>  {
>  	u32 ctrl_stat;
>  
> -	return !readl_relaxed_poll_timeout
> -		(cdns_xspi->iobase + CDNS_XSPI_CTRL_STATUS_REG,
> -		ctrl_stat,
> -		((ctrl_stat & BIT(3)) == 0),
> -		sleep ? MRVL_XSPI_POLL_DELAY_US : 0,
> -		sleep ? MRVL_XSPI_POLL_TIMEOUT_US : 0);
> +	return !cdns_readl_poll(cdns_xspi->iobase + CDNS_XSPI_CTRL_STATUS_REG,
> +				ctrl_stat, !(ctrl_stat & BIT(3)),
> +				MRVL_XSPI_POLL_DELAY_US,
> +				MRVL_XSPI_POLL_TIMEOUT_US, sleep);

[Severity: High]
Does this timeout also cause issues for panic writes?

STIG command completion also relies on a 1000us timeout (via
MRVL_XSPI_POLL_TIMEOUT_US), which might be insufficient for large data
transfers during a panic write.

>  }
>  
>  static bool cdns_xspi_is_sdma_ready(struct cdns_xspi_dev *cdns_xspi, bool sleep)
>  {
>  	u32 ctrl_stat;
>  
> -	if (readl_relaxed_poll_timeout
> -	    (cdns_xspi->iobase + CDNS_XSPI_INTR_STATUS_REG,
> -	     ctrl_stat,
> -	     (ctrl_stat & CDNS_XSPI_SDMA_TRIGGER),
> -	     sleep ? MRVL_XSPI_POLL_DELAY_US : 0,
> -	     sleep ? MRVL_XSPI_POLL_TIMEOUT_US : 0)) {
> +	if (cdns_readl_poll(cdns_xspi->iobase + CDNS_XSPI_INTR_STATUS_REG,
> +			    ctrl_stat, (ctrl_stat & CDNS_XSPI_SDMA_TRIGGER),
> +			    MRVL_XSPI_POLL_DELAY_US, MRVL_XSPI_POLL_TIMEOUT_US,
> +			    sleep)) {

[Severity: High]
Does this polling loop rely on a hardware bit that could be stolen by the
shared IRQ handler?

If a concurrent interrupt from another device on the same shared IRQ line
invokes cdns_xspi_irq_handler(), it unconditionally clears the raw interrupt
status:

cdns_xspi_irq_handler()
    irq_status = readl(cdns_xspi->iobase + CDNS_XSPI_INTR_STATUS_REG);
    writel(irq_status, cdns_xspi->iobase + CDNS_XSPI_INTR_STATUS_REG);

This steals the SDMA_TRIGGER bit without verifying if interrupts were actually
enabled, which would cause this active polling loop to time out and fail the
transfer.

[Severity: Medium]
Does this SDMA polling ignore the error status?

This logic explicitly checks only for the trigger bit and ignores
CDNS_XSPI_SDMA_ERROR. If the hardware asserts only the error bit on failure,
the driver will needlessly wait for the full timeout instead of failing fast.

Additionally, if the hardware asserts both the trigger and error bits
simultaneously on a failure, the driver will match the trigger bit, return
true, and treat the failed transaction as successful, potentially leading to
silent data corruption.

>  		return false;
>  	}

[ ... ]

> @@ -557,14 +554,14 @@ static int cdns_xspi_send_stig_command(struct cdns_xspi_dev *cdns_xspi,
>  	int ret;
>  	int dummybytes = op->dummy.nbytes;
>  
> -	ret = cdns_xspi_wait_for_controller_idle(cdns_xspi);
> +	ret = cdns_xspi_wait_for_controller_idle(cdns_xspi, !op->panic_write);
>  	if (ret < 0)
>  		return -EIO;
>  
>  	writel(FIELD_PREP(CDNS_XSPI_CTRL_WORK_MODE, CDNS_XSPI_WORK_MODE_STIG),
>  	       cdns_xspi->iobase + CDNS_XSPI_CTRL_CONFIG_REG);
>  
> -	if (cdns_xspi->irq >= 0)
> +	if (cdns_xspi->irq >= 0 && !op->panic_write)
>  		cdns_xspi->set_interrupts_handler(cdns_xspi, true);

[Severity: High]
Does this miss clearing stale interrupt status bits left behind from an
interrupted command?

During a panic write, normal interrupt handling is bypassed. If a panic occurs
while a previous SPI command was executing, the CDNS_XSPI_INTR_STATUS_REG will
have stale flags set.

When the panic write issues a new command, cdns_xspi_is_sdma_ready() will
immediately read this stale flag, clear it, and return true.

This causes the driver to prematurely interact with the SDMA buffer before the
new command's SDMA phase is actually ready, which could corrupt the panic write
data.

>  	cdns_xspi->sdma_error = false;
>  
> @@ -595,23 +592,23 @@ static int cdns_xspi_send_stig_command(struct cdns_xspi_dev *cdns_xspi,
>  
>  		cdns_xspi_trigger_command(cdns_xspi, cmd_regs);
>  
> -		if (cdns_xspi->irq >= 0) {
> +		if (cdns_xspi->irq >= 0 && !op->panic_write) {
>  			wait_for_completion(&cdns_xspi->sdma_complete);
>  			if (cdns_xspi->sdma_error) {
>  				cdns_xspi->set_interrupts_handler(cdns_xspi, false);
>  				return -EIO;
>  			}

[Severity: High]
Are we missing a reinit_completion() call here?

When this error path is taken, cmd_complete is not waited upon. If the hardware
subsequently asserts the STIG_DONE interrupt, cmd_complete is completed,
leaving the completion count at 1.

> -		} else if (!cdns_xspi_is_sdma_ready(cdns_xspi, true)) {
> +		} else if (!cdns_xspi_is_sdma_ready(cdns_xspi, !op->panic_write)) {
>  			return -EIO;
>  		}
>  
>  		cdns_xspi->sdma_handler(cdns_xspi);
>  	}
>  
> -	if (cdns_xspi->irq >= 0) {
> +	if (cdns_xspi->irq >= 0 && !op->panic_write) {
>  		wait_for_completion(&cdns_xspi->cmd_complete);

[Severity: High]
This wait will return immediately if a previous aborted command left a lingering
completion count, causing the new command to prematurely continue execution.

Should we call reinit_completion() before triggering commands?

>  		cdns_xspi->set_interrupts_handler(cdns_xspi, false);
> -	} else if (!cdns_xspi_is_stig_ready(cdns_xspi, true)) {
> +	} else if (!cdns_xspi_is_stig_ready(cdns_xspi, !op->panic_write)) {
>  		return -EIO;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016102917.171095-1-paul.cercueil@bootlin.com?part=6

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/