Re: [PATCH v3 6/6] spi: spi-sun4i: sun4i_spi_transfer_one(): report effectively used speed_hz of transfer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The SPI controller is not properly stopped or reset on transfer timeout, leading to state corruption for subsequent transfers.
- [High] Potential out-of-bounds write in `sun4i_spi_drain_fifo` due to missing bounds check against the receive buffer length.
--

commit 6f977d6ebfc9cf20201026442f97064e4b99df02
Author: Marc Kleine-Budde <mkl@pengutronix.de>

spi: spi-sun4i: sun4i_spi_transfer_one(): report effectively used speed_hz of transfer

Report the effectively used speed_hz for the transfer by setting
tfr->effective_speed_hz. This allows the SPI core to track the actual speed
used.

> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
> index 94e1c7bbd6325..59941b38b7574 100644
> --- a/drivers/spi/spi-sun4i.c
> +++ b/drivers/spi/spi-sun4i.c
> @@ -310,9 +310,11 @@ static int sun4i_spi_transfer_one(struct spi_controller *host,
>  	div_cdr2 = DIV_ROUND_UP(div_cdr1, 2);
>  	if (div_cdr2 <= (SUN4I_CLK_CTL_CDR2_MASK + 1)) {
>  		reg = SUN4I_CLK_CTL_CDR2(div_cdr2 - 1) | SUN4I_CLK_CTL_DRS;
> +		tfr->effective_speed_hz = mclk_rate / (2 * div_cdr2);
>  	} else {
>  		div = min(SUN4I_CLK_CTL_CDR1_MASK + 1, order_base_2(div_cdr1));
>  		reg = SUN4I_CLK_CTL_CDR1(div - 1);
> +		tfr->effective_speed_hz = mclk_rate / (1 << div);
>  	}
>  
>  	sun4i_spi_write(sspi, SUN4I_CLK_CTL_REG, reg);

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem in the
timeout handling further down in sun4i_spi_transfer_one(). When a timeout
occurs, does the error path properly stop the hardware transfer?

In sun4i_spi_transfer_one():

    time_left = wait_for_completion_timeout(&sspi->done,
                                            msecs_to_jiffies(tx_time));
    ...
    if (!time_left) {
        dev_warn(&host->dev, ...);
        ret = -ETIMEDOUT;
        sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, 0);
        synchronize_irq(sspi->irq);
    }

It appears this path disables interrupts and returns but fails to clear the
SUN4I_CTL_XCH bit or issue a soft-reset to the controller. Could this leave
the hardware active and corrupt the state machine for subsequent transfers,
especially since the next transfer might read back and write XCH=1 during
setup?


[Severity: High]
This is also a pre-existing issue, but could the lack of a hardware reset
during a timeout trigger an out-of-bounds write in sun4i_spi_drain_fifo()?

If a timeout leaves XCH=1, the next transfer might start prematurely and
load data based on the previous burst count into the RX FIFO. When
sun4i_spi_drain_fifo() runs, it appears to blindly trust the hardware FIFO
count without verifying it against the size of the receive buffer:

    static inline void sun4i_spi_drain_fifo(struct sun4i_spi *sspi, int len)
    {
        ...
        reg = sun4i_spi_read(sspi, SUN4I_FIFO_STA_REG);
        reg &= SUN4I_FIFO_STA_RF_CNT_MASK;
        cnt = reg >> SUN4I_FIFO_STA_RF_CNT_BITS;

        if (len > cnt)
            len = cnt;

        while (len--) {
            byte = readb(sspi->base_addr + SUN4I_RXDATA_REG);
            if (sspi->rx_buf)
                *sspi->rx_buf++ = byte;
        }
    }

Since sun4i_spi_drain_fifo() is called with len = SUN4I_FIFO_DEPTH, if the
hardware provides more bytes than tfr->len, will this loop unconditionally
write past the end of sspi->rx_buf?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914-spi-sun4i-fixes-v3-0-9fe919d5cecf@pengutronix.de?part=6