Re: [PATCH v3 4/7] spi: cadence-xspi: Clear "SDMA ready" IRQ flag after polling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] spi: cadence-xspi: Endianness data corruption in SDMA FIFO accessors
--

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

spi: cadence-xspi: Clear "SDMA ready" IRQ flag after polling

When polling for the "SDMA ready" event, once the corresponding bit is
set in the CDNS_XSPI_SDMA_TRIGGER we should reset it to zero. Otherwise,
any subsequent call to cdns_xspi_is_sdma_ready() will just return early.

Fixes: 931e389ded0f ("spi: cadence: Add Marvell xfer operation support")
Cc: stable@vger.kernel.org

> diff --git a/drivers/spi/spi-cadence-xspi.c b/drivers/spi/spi-cadence-xspi.c
> index 39c868a5b171c..095bf640b379a 100644
> --- a/drivers/spi/spi-cadence-xspi.c
> +++ b/drivers/spi/spi-cadence-xspi.c
> @@ -1068,12 +1068,20 @@ static bool cdns_xspi_is_sdma_ready(struct cdns_xspi_dev *cdns_xspi, bool sleep)
>  {
>  	u32 ctrl_stat;
>  
> -	return !readl_relaxed_poll_timeout
> -		(cdns_xspi->iobase + CDNS_XSPI_INTR_STATUS_REG,
> -		ctrl_stat,
> -		(ctrl_stat & CDNS_XSPI_SDMA_TRIGGER),
> -		sleep ? MRVL_XSPI_POLL_DELAY_US : 0,
> -		sleep ? MRVL_XSPI_POLL_TIMEOUT_US : 0);
> +	if (readl_relaxed_poll_timeout
> +	    (cdns_xspi->iobase + CDNS_XSPI_INTR_STATUS_REG,
> +	     ctrl_stat,
> +	     (ctrl_stat & CDNS_XSPI_SDMA_TRIGGER),
> +	     sleep ? MRVL_XSPI_POLL_DELAY_US : 0,
> +	     sleep ? MRVL_XSPI_POLL_TIMEOUT_US : 0)) {
> +		return false;
> +	}
> +
> +	/* Clear the flag */
> +	writel(CDNS_XSPI_SDMA_TRIGGER,
> +	       cdns_xspi->iobase + CDNS_XSPI_INTR_STATUS_REG);
> +
> +	return true;
>  }

[Severity: High]
Does this code corrupt data on big-endian architectures?

In m_ioreadq(), register accessors are used instead of stream accessors to
read bulk stream data from the SDMA FIFO:

drivers/spi/spi-cadence-xspi.c:m_ioreadq() {
    ...
		do {
			u64 b = readq(addr);
			*buffer++ = b;
		} while (--full_ops);
    ...
}

Because addr is not incremented, this reads from a FIFO. Register accessors
like readq() perform byte-swapping, which reverses the byte stream within
each 8-byte chunk on big-endian systems.

Similarly, for partial transfers in m_ioreadq():

drivers/spi/spi-cadence-xspi.c:m_ioreadq() {
    ...
	while (len) {
		u64 tmp_buf;

		tmp_buf = readq(addr);
		memcpy(buf, &tmp_buf, min(len, 8));
    ...
}

Combining a byte-swapping readq() with memcpy() extracts incorrect bytes on
big-endian systems because the reversed 64-bit word places the wrong stream
bytes in the lowest memory addresses.

The same behavior appears in m_iowriteq() when writing to the SDMA FIFO:

drivers/spi/spi-cadence-xspi.c:m_iowriteq() {
    ...
		do {
			writeq(*buffer++, addr);
		} while (--full_ops);
    ...
}

Should these be converted to stream accessors to preserve byte order on
big-endian systems?

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

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