Re: [PATCH v2 5/7] spi: cadence-xspi: Add irq-less support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] spi: cadence-xspi: 1ms polling timeout is too short for large STIG transfers
- [High] spi: cadence-xspi: stale completion state after early abort or double-completion
- [High] spi: cadence-xspi: missing SDMA error check in polling mode
- [High] spi: cadence-xspi: spurious completions from uncleared polling status
--

commit 16ebab91423ce939f852c48607b518f29634b538
Author: Tanmay Jagdale <tanmay@marvell.com>

spi: cadence-xspi: Add irq-less support

Make the interrupt line optional. If not provided, the driver will
instead resort to polling registers.

> diff --git a/drivers/spi/spi-cadence-xspi.c b/drivers/spi/spi-cadence-xspi.c
> index 095bf640b379a..185dffda54bbd 100644
> --- a/drivers/spi/spi-cadence-xspi.c
> +++ b/drivers/spi/spi-cadence-xspi.c
> @@ -365,6 +365,39 @@ static int cdns_xspi_wait_for_controller_idle(struct cdns_xspi_dev *cdns_xspi)
>  					  100, 1000);
>  }
>  
> +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);

[Severity: High]
Does this polling loop leave the CDNS_XSPI_STIG_DONE bit uncleared in
INTR_STATUS_REG?

If a shared IRQ fires later, cdns_xspi_irq_handler() will see the uncleared
CDNS_XSPI_STIG_DONE bit and call complete(&cdns_xspi->cmd_complete). This can
corrupt the completion counter and cause subsequent commands to prematurely
return from wait_for_completion().

> +}
> +
> +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)) {

[Severity: High]
Does this polling loop need to check for CDNS_XSPI_SDMA_ERROR?

If a hardware SDMA error occurs and the trigger bit is never set, this will
loop unnecessarily until the timeout. Worse, if both the error and trigger bits
are set, this returns true and fails to clear the error bit, which could lead
to executing the DMA handler on corrupted data.

[Severity: High]
This hardcodes a 1ms polling timeout (MRVL_XSPI_POLL_TIMEOUT_US).

In cdns_xspi_send_stig_command(), this timeout is applied to STIG transfers
that can be up to sdmasize (often 4096 bytes or more). At typical SPI
frequencies, will large transfers take longer than 1ms and spuriously fail with
-EIO?

> +		return false;
> +	}
> +
> +	/* Clear the flag */
> +	writel(CDNS_XSPI_SDMA_TRIGGER,
> +	       cdns_xspi->iobase + CDNS_XSPI_INTR_STATUS_REG);
> +
> +	return true;
> +}
> +
> +

[ ... ]

> @@ -562,16 +596,25 @@ static int cdns_xspi_send_stig_command(struct cdns_xspi_dev *cdns_xspi,
>  
>  		cdns_xspi_trigger_command(cdns_xspi, cmd_regs);
>  
> -		wait_for_completion(&cdns_xspi->sdma_complete);
> -		if (cdns_xspi->sdma_error) {
> -			cdns_xspi->set_interrupts_handler(cdns_xspi, false);
> +		if (cdns_xspi->irq >= 0) {
> +			wait_for_completion(&cdns_xspi->sdma_complete);
> +			if (cdns_xspi->sdma_error) {
> +				cdns_xspi->set_interrupts_handler(cdns_xspi, false);
> +				return -EIO;
> +			}

[Severity: High]
If an sdma_error occurs, this path returns -EIO and abandons the wait on
cmd_complete.

Because set_interrupts_handler(..., false) disables interrupts without clearing
the hardware status, the hardware command may still finish and set the
CDNS_XSPI_STIG_DONE status.

When the next transfer re-enables interrupts, the stale status will immediately
trigger the IRQ handler and signal cmd_complete. Without reinitializing
cmd_complete, won't the new transfer falsely succeed before its hardware
operation actually finishes?

Also, if SDMA_ERROR and SDMA_TRIGGER both occur, won't the IRQ handler call
complete(&cdns_xspi->sdma_complete) twice, leaving another stale completion?

> +		} else if (!cdns_xspi_is_sdma_ready(cdns_xspi, true)) {
>  			return -EIO;
>  		}

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

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