Re: [PATCH 2/5] spi: spi-sun4i: sun4i_spi_fill_fifo(): always apply A10 FIFO fill level workaround

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Race condition on the SPI transfer timeout path due to missing interrupt disable/locking, leading to concurrent FIFO access.
--

commit 1dffb9065c85ba457023e16b227c366bf84a8ee4
Author: Marc Kleine-Budde <mkl@pengutronix.de>

spi: spi-sun4i: sun4i_spi_fill_fifo(): always apply A10 FIFO fill level workaround

This commit removes the length argument from sun4i_spi_fill_fifo() and updates
it to always apply the A10 FIFO fill level workaround internally. This ensures
the TX FIFO is limited to SUN4I_FIFO_DEPTH - 1 bytes regardless of where the
function is called.

> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
> index 2e23244539053..3649bcabcc9a7 100644
> --- a/drivers/spi/spi-sun4i.c
> +++ b/drivers/spi/spi-sun4i.c

[ ... ]

> @@ -311,12 +317,8 @@ static int sun4i_spi_transfer_one(struct spi_controller *host,
>  	sun4i_spi_write(sspi, SUN4I_BURST_CNT_REG, SUN4I_BURST_CNT(tfr->len));
>  	sun4i_spi_write(sspi, SUN4I_XMIT_CNT_REG, SUN4I_XMIT_CNT(tx_len));
>  
> -	/*
> -	 * Fill the TX FIFO
> -	 * Filling the FIFO fully causes timeout for some reason
> -	 * at least on spi2 on A10s
> -	 */
> -	sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH - 1);
> +	/* Fill the TX FIFO */
> +	sun4i_spi_fill_fifo(sspi);
>  
>  	/* Enable the interrupts */
>  	sun4i_spi_enable_interrupt(sspi, SUN4I_INT_CTL_TC |

[Severity: High]
Does this code expose a race condition on the SPI transfer timeout path due to
missing interrupt disable or locking?

If wait_for_completion_timeout() times out later in sun4i_spi_transfer_one(),
the code calls sun4i_spi_drain_fifo() while SPI interrupts are still enabled:

sun4i_spi_transfer_one() {
    ...
    time_left = wait_for_completion_timeout(&sspi->done,
                                            msecs_to_jiffies(tx_time));

    sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
    ...
}

If the SUN4I_INT_CTL_RF_F34 interrupt fires concurrently, the IRQ handler in
sun4i_spi_handler() will also execute sun4i_spi_drain_fifo() on another CPU
without synchronization:

sun4i_spi_handler() {
    ...
    /* Receive FIFO 3/4 full */
    if (status & SUN4I_INT_CTL_RF_F34) {
        sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH);
        /* Only clear the interrupt _after_ draining the FIFO */
        sun4i_spi_write(sspi, SUN4I_INT_STA_REG, SUN4I_INT_CTL_RF_F34);
        return IRQ_HANDLED;
    }
    ...
}

Both threads will read from the hardware FIFO and increment sspi->rx_buf
locklessly. Can this cause a data race, duplicate bytes, and a buffer overflow
on sspi->rx_buf? 

This would be reachable under error conditions or interrupt storms if a
hardware timeout occurs during an SPI transfer followed by an interrupt.

Note that this race was introduced by a subsequent patch in this series
('spi: spi-sun4i: sun4i_spi_handler(): fix race condition between transfer
completion and RX FIFO interrupt') which moved the drain call outside the
timeout condition without disabling interrupts first.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902-spi-sun4i-fixes-v1-0-19985ef75673@pengutronix.de?part=2