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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] spi: cadence-xspi: unhandled interrupt storm in polling mode
- [High] spi: cadence-xspi: SDMA W1C trigger bit not cleared in polling mode
--

commit 85002136114ad45055d6ac972dbcbdf8c8e3d6b2
Author: Tanmay Jagdale <tanmay@xxxxxxxxxxx>

spi: cadence-xspi: Add irq-less support

This commit makes the interrupt line optional. If an interrupt is not
provided, the driver resorts to polling registers. It modifies
cdns_xspi_send_stig_command() and probe functions to handle environments
without a dedicated IRQ.

> diff --git a/drivers/spi/spi-cadence-xspi.c b/drivers/spi/spi-cadence-xspi.c
> index 39c868a5b171c..7cf52bcb05df6 100644
> --- a/drivers/spi/spi-cadence-xspi.c
> +++ b/drivers/spi/spi-cadence-xspi.c
> @@ -365,6 +365,30 @@ static int cdns_xspi_wait_for_controller_idle(struct cdns_xspi_dev *cdns_xspi)
[ ... ]
> +static bool cdns_xspi_is_sdma_ready(struct cdns_xspi_dev *cdns_xspi, bool sleep)
> +{
> +	u32 ctrl_stat;
> +
> +	return !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 leave the W1C (Write-1-to-Clear) trigger bit set?

It appears CDNS_XSPI_INTR_STATUS_REG is a latching W1C register (as seen in
cdns_xspi_irq_handler() which reads and writes back to clear latched events).

In polling mode (when irq < 0), this function only reads the register. For
base Cadence hardware, cdns_xspi_set_interrupts() doesn't clear this either.

Could this cause the CDNS_XSPI_SDMA_TRIGGER bit to remain permanently set
after the first SDMA transfer, leading subsequent transfers to falsely see a
ready state and potentially access memory prematurely?

[ ... ]
> @@ -562,16 +586,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;
> +			}
> +		} 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);
> +		cdns_xspi->set_interrupts_handler(cdns_xspi, false);
> +	} else if (!cdns_xspi_is_stig_ready(cdns_xspi, true)) {
> +		return -EIO;
> +	}

[Severity: High]
Does this leave hardware interrupts permanently unmasked in polling mode?

At the start of cdns_xspi_send_stig_command(), interrupts are unmasked
unconditionally:

    cdns_xspi->set_interrupts_handler(cdns_xspi, true);

However, when irq < 0, the corresponding teardown call:

    cdns_xspi->set_interrupts_handler(cdns_xspi, false);

is skipped because it is guarded by if (cdns_xspi->irq >= 0). 

If the physical IRQ line is connected or shared, could this cause the
controller to assert it indefinitely, triggering an unhandled interrupt storm
that forces the kernel to disable the IRQ line?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005081141.33688-1-paul.cercueil@xxxxxxxxxxx?part=3