Re: [PATCH v3 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 can cause unkillable task hang
- [High] spi: cadence-xspi: missing reinit_completion() unblocks stale transfers
- [High] spi: cadence-xspi: SDMA errors ignored and uncleared in polling mode
- [High] spi: cadence-xspi: hardcoded 1ms timeout breaks large polling transfers
- [High] spi: cadence-xspi: shared IRQ handler steals polled SDMA_TRIGGER status
--

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

spi: cadence-xspi: Add irq-less support

This patch makes the interrupt line optional, adding support for polling
registers instead. The cdns_xspi_is_sdma_ready() and cdns_xspi_is_stig_ready()
functions are updated and moved to handle the polling paths.

> 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]
Does this hardcoded MRVL_XSPI_POLL_TIMEOUT_US timeout break large polling
transfers?

Enforcing a strict 1ms limit on STIG command completion could cause spurious
-EIO aborts for transfers that take longer than 1ms to complete on the SPI
bus (e.g., when operating at low bus frequencies).

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

[Severity: High]
Is the CDNS_XSPI_SDMA_ERROR mask omitted here?

If the hardware asserts CDNS_XSPI_SDMA_ERROR without CDNS_XSPI_SDMA_TRIGGER,
the driver will wait the full timeout, return false, and fail to clear the
error status, leaving the controller in an unclean state. If both are
asserted, the polling loop will incorrectly return true, causing the driver
to proceed and perform I/O on a failed DMA transaction.

Also, looking at cdns_xspi_irq_handler(), it unconditionally clears all
pending interrupts (including SDMA_TRIGGER) even if the controller's
interrupts are disabled. If a stray interrupt on a shared IRQ line triggers
the handler during this polling phase, it steals the status bits. Could this
cause the polling loop to falsely time out and return -EIO?

> +	     sleep ? MRVL_XSPI_POLL_DELAY_US : 0,
> +	     sleep ? MRVL_XSPI_POLL_TIMEOUT_US : 0)) {

[Severity: High]
Similarly, does this hardcoded MRVL_XSPI_POLL_TIMEOUT_US timeout break large
SDMA data transfers?

Large multi-kilobyte transfers bounded by cdns_xspi->sdmasize may take longer
than 1ms, resulting in spurious -EIO aborts.

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

[Severity: High]
Does this leave the error bit uncleared?

Since only the trigger bit is cleared, the error bit could remain set in this
write-1-to-clear register if CDNS_XSPI_SDMA_ERROR was also asserted.

> +
> +	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]
Does this unbounded wait_for_completion() cause an unkillable task hang?

If the hardware drops the completion interrupt or hangs, wait_for_completion()
blocks indefinitely in TASK_UNINTERRUPTIBLE mode. Should this use
wait_for_completion_timeout() instead, matching the bounds used in the
polling path?

> +			if (cdns_xspi->sdma_error) {
> +				cdns_xspi->set_interrupts_handler(cdns_xspi, false);
> +				return -EIO;

[Severity: High]
Can returning early here leave cmd_complete unbalanced?

If the hardware later finishes the command and signals CDNS_XSPI_STIG_DONE,
the IRQ handler will complete cmd_complete.

Also, looking at cdns_xspi_irq_handler(), if both CDNS_XSPI_SDMA_ERROR and
CDNS_XSPI_SDMA_TRIGGER are set in the IRQ status register, sdma_complete is
double-completed.

Without a corresponding reinit_completion() before reusing these completions,
both scenarios leave the internal done counters > 0, erroneously satisfying
future transfers.

> +			}
> +		} 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]
Is it possible for this wait_for_completion() to cause an unkillable task
hang?

Similar to sdma_complete, if the hardware fails to assert the STIG_DONE
interrupt, the thread blocks 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/20261014154301.157755-1-paul.cercueil@bootlin.com?part=5

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