Re: [PATCH 5/5] spi: cadence-xspi: Add support for panic writes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] spi: cadence-xspi: missing PHY wakeup in panic write path
- [High] spi: cadence-xspi: deadlock in panic write via ktime_get()
- [High] spi: cadence-xspi: unmasked interrupts leak in polling mode
--

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

spi: cadence-xspi: Add support for panic writes
    
This commit adds support for panic writes to the Cadence XSPI driver by
checking the panic flag on memory operations. When a panic write is requested,
the driver operates without relying on interrupts or sleeping.

> diff --git a/drivers/spi/spi-cadence-xspi.c b/drivers/spi/spi-cadence-xspi.c
> index 09ed2afbd4af3..0b7162740940f 100644
> --- a/drivers/spi/spi-cadence-xspi.c
> +++ b/drivers/spi/spi-cadence-xspi.c
> @@ -350,10 +350,11 @@ struct cdns_xspi_dev {
>  	void (*set_interrupts_handler)(struct cdns_xspi_dev *cdns_xspi, bool enabled);
>  
>  	bool xfer_in_progress;
> +	bool panic_write;
>  	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;
>  
> @@ -362,7 +363,7 @@ static int cdns_xspi_wait_for_controller_idle(struct cdns_xspi_dev *cdns_xspi)
>  					  ctrl_stat,
>  					  ((ctrl_stat &
>  					    CDNS_XSPI_CTRL_BUSY) == 0),
> -					  100, 1000);
> +					  sleep ? 100 : 0, 1000);

[Severity: High]
Does this code cause a deadlock during a kernel panic?

readl_relaxed_poll_timeout() expands to read_poll_timeout(), which evaluates
ktime_get() to enforce the timeout, even when the delay is 0. If another CPU
was halted by an NMI or IPI while holding the write lock for the timekeeping
sequence lock (tk_core.seq) during the panic, calling ktime_get() will hang
the system.

Should this use readl_relaxed_poll_timeout_atomic() instead to avoid
timekeeping functions during a panic?

[ ... ]
> @@ -586,23 +587,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;
>  			}
> -		} 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);
>  		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;
>  	}

[Severity: High]
Does this leak unmasked interrupts in polling mode?

Earlier in cdns_xspi_send_stig_command(), interrupts are unconditionally
enabled:

    cdns_xspi->set_interrupts_handler(cdns_xspi, true);

If the controller is in polling mode (irq < 0), this code takes the else-if
branch. Whether cdns_xspi_is_stig_ready() succeeds or fails, the function
returns without calling cdns_xspi->set_interrupts_handler(cdns_xspi, false).

Will this cause an unhandled interrupt storm if the hardware IRQ line
is physically shared with other devices?

[ ... ]
> @@ -622,6 +623,8 @@ static int cdns_xspi_mem_op(struct cdns_xspi_dev *cdns_xspi,
>  	if (cdns_xspi->cur_cs != spi_get_chipselect(mem->spi, 0))
>  		cdns_xspi->cur_cs = spi_get_chipselect(mem->spi, 0);
>  
> +	cdns_xspi->panic_write = op->panic_write;
> +
>  	return cdns_xspi_send_stig_command(cdns_xspi, op,
>  					   (dir != SPI_MEM_NO_DATA));
>  }

[Severity: High]
Does this entry point fail to wake up the PHY from suspend during a panic
write?

If the system panics while the device is suspended, the controller will
attempt to execute STIG commands while the PHY is still suspended, which
could lead to bus faults or hardware lockups.

Should this path call cdns_xspi_configure_phy() to wake up the PHY before
sending commands during a panic write?

[ ... ]
> @@ -831,9 +834,11 @@ static bool cdns_xspi_is_dll_locked(struct cdns_xspi_dev *cdns_xspi)
>  {
>  	u32 dll_lock;
>  
> +	/* Busy-wait in case we wake up the PHY from suspend to do a panic write */
>  	return !readl_relaxed_poll_timeout(cdns_xspi->iobase +
>  		CDNS_XSPI_INTR_STATUS_REG,
> -		dll_lock, ((dll_lock & CDNS_XSPI_DLL_LOCK) == 1), 10, 10000);
> +		dll_lock, ((dll_lock & CDNS_XSPI_DLL_LOCK) == 1),
> +		cdns_xspi->panic_write ? 0 : 10, 10000);
>  }

[Severity: High]
Is this code reachable during a panic write?

The comment indicates this handles waking up the PHY during a panic write.
However, this function is only called from cdns_xspi_configure_phy(), which
is entirely omitted from the cdns_xspi_mem_op() panic write execution path.

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