Re: [PATCH v2 08/11] spi: dw: use irq handler for enhanced spi

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The SPI controller's FIFO threshold registers are modified while the controller is active, and a hardcoded delay introduces a race condition leading to silent RX FIFO overflows.
- [High] dw_spi_exec_enh_mem_op() unconditionally relies on interrupts, breaking SPI operations on systems without an IRQ line.
--

--- Patch [8]: [PATCH v2 08/11] spi: dw: use irq handler for enhanced spi ---
commit e91a45b5a4fd49e726c98b12e74d1f4306f85fe7
Author: Sudip Mukherjee <[email protected]>

spi: dw: use irq handler for enhanced spi

This commit introduces an interrupt handler for enhanced SPI read/write
operations. It utilizes the controller's xfer_completion to wait for
transfer completion or timeout from the IRQ handler. It also adjusts
RXFTLR dynamically based on the expected data length.

> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c
[ ... ]
> @@ -920,11 +979,54 @@ static int dw_spi_exec_enh_mem_op(...)
>  
>  	dw_spi_update_config(dws, mem->spi, &cfg, &enh_cfg);
>  
> +	dw_spi_mask_intr(dws, 0xff);
> +	reinit_completion(&ctlr->xfer_completion);
>  	dw_spi_enable_chip(dws, 1);
>  
>  	dw_spi_enh_write_cmd_addr(dws, op);
> +	dw_spi_set_cs(mem->spi, false);
>  
> -	return 0;
> +	/*
> +	 * FIXME: The exact reason for this delay is not fully understood,
> +	 * but empirical testing shows it significantly improves the stability
> +	 * of read/write operations. Without this delay, occasional transfer
> +	 * errors or timeouts may occur under certain conditions.
> +	 * Keeping it as a safeguard based on practical validation.
> +	 */
> +	udelay(5);
> +
> +	dw_spi_enh_irq_setup(dws);

[Severity: High]
Does modifying the SPI controller's FIFO threshold registers inside
dw_spi_enh_irq_setup() while the controller is active (after calling
dw_spi_enable_chip()) violate the DW APB SSI hardware specifications?

Additionally, with this 5us delay between starting the transfer and unmasking
interrupts, could the RX FIFO overflow before dw_spi_enh_irq_setup() prepares
the IRQ handler to drain it?

At typical SPI frequencies (e.g., 50MHz), 5us is enough time to clock in
more data than a small RX FIFO can hold, which might lead to silent data loss
and immediate -EIO timeouts.

> +
> +	/* Use timeout calculation from spi_transfer_wait() */
> +	ms = 8LL * MSEC_PER_SEC * (dws->rx_len ? dws->rx_len : dws->tx_len);
> +	do_div(ms, dws->current_freq);
> +
> +	/*
> +	 * Increase it twice and add 200 ms tolerance, use
> +	 * predefined maximum in case of overflow.
> +	 */
> +	ms += ms + 200;
> +	if (ms > UINT_MAX)
> +		ms = UINT_MAX;
> +
> +	ms = wait_for_completion_timeout(&ctlr->xfer_completion,
> +					 msecs_to_jiffies(ms));
> +	if (ms == 0) {

[Severity: High]
Will this wait unconditionally time out on platforms without a connected IRQ?

Unlike the standard dw_spi_exec_mem_op() which falls back to polling, this
appears to rely solely on the interrupt handler to signal completion. If a
system uses polling mode (no dedicated SPI IRQ), it seems enhanced memory
operations will always result in an -EIO timeout here.

> +		dw_spi_mask_intr(dws, 0xff);
> +		synchronize_irq(dws->irq);
> +		dws->rx = NULL;
> +		dws->tx = NULL;
> +		dws->rx_len = 0;
> +		dws->tx_len = 0;
> +		dw_spi_stop_mem_op(dws, mem->spi);
> +		return -EIO;
> +	}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8