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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] spi: cadence-xspi: unbounded wait for completion on hardware interrupts
- [High] spi: cadence-xspi: missing reinit_completion() causes completion state leak
- [High] spi: cadence-xspi: shared IRQ handler steals status bits breaking polling
- [High] spi: cadence-xspi: SDMA error ignored and uncleared in polling mode
- [High] spi: cadence-xspi: 1ms poll timeout is too short for large transfers
--

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

spi: cadence-xspi: Add irq-less support

This patch makes the interrupt line optional. If an interrupt is not provided,
the driver will fall back to polling the controller registers for completion.

> diff --git a/drivers/spi/spi-cadence-xspi.c b/drivers/spi/spi-cadence-xspi.c
> index 095bf640b379a..2a2a40bf0a4c4 100644
> --- a/drivers/spi/spi-cadence-xspi.c
> +++ b/drivers/spi/spi-cadence-xspi.c
> @@ -365,6 +365,38 @@ 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]
Is MRVL_XSPI_POLL_TIMEOUT_US (1ms) long enough for large transfers?

When irq < 0 or during a panic write, cdns_xspi_send_stig_command() uses
this function to poll. The driver allows transfers up to sdmasize (often
64KB), which can take ~21ms at 25MHz. The 1ms timeout will systematically
fail these larger transfers, causing hardware state corruption.

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

[Severity: High]
If an SDMA transaction encounters an error while in polling mode,
will this loop ignore CDNS_XSPI_SDMA_ERROR?

If CDNS_XSPI_SDMA_ERROR is asserted alongside CDNS_XSPI_SDMA_TRIGGER, the
function returns true despite the error. If only the error is asserted,
it will wait the full timeout and return false, but it never clears the W1C
CDNS_XSPI_SDMA_ERROR bit, permanently wedging the DMA hardware state machine.

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

[Severity: High]
Could the shared IRQ handler steal the trigger bit from this polling loop?

In cdns_xspi_transfer_one_message_b0(), the driver performs normal SPI
transfers by polling cdns_xspi_is_sdma_ready(). If another device sharing
the same IRQ line fires an interrupt, cdns_xspi_irq_handler() will read
CDNS_XSPI_INTR_STATUS_REG and unconditionally write it back, clearing all
pending bits. This can cause the polling loop to time out and the transfer
to incorrectly fail with -EIO.

> +
> +	return true;
> +}

[ ... ]

> @@ -562,16 +595,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);

[Severity: High]
This problem wasn't introduced by this patch, but does the completion
state need to be reinitialized via reinit_completion() before the hardware
command is triggered?

If an error occurs or a spurious interrupt fires, cdns_xspi_irq_handler()
could call complete() multiple times for cdns_xspi->sdma_complete. Without
reinit_completion(), leftover completion tokens will cause subsequent
wait_for_completion() calls to return immediately while the hardware is
still transferring data, leading to memory and state corruption.

[Severity: High]
This problem wasn't introduced by this patch, but does this wait need a
timeout?

If the hardware fails to generate the interrupt, the calling thread will
hang indefinitely in TASK_UNINTERRUPTIBLE, creating a Denial of Service
condition.

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

[Severity: High]
This problem wasn't introduced by this patch, but should this wait also
have a timeout?

Just like with sdma_complete, if the hardware fails to generate the STIG
completion interrupt, the calling thread will hang indefinitely.

> +		cdns_xspi->set_interrupts_handler(cdns_xspi, false);
> +	} else if (!cdns_xspi_is_stig_ready(cdns_xspi, true)) {
> +		return -EIO;
> +	}
>  
>  	cmd_status = cdns_xspi_check_command_status(cdns_xspi);
>  	if (cmd_status)

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

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