Re: [PATCH 20/24] spi: Add ZTE ZX279128S SPI flash controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] spi: zx279128s: Endianness corruption in FIFO byte stream access
- [Medium] spi: zx279128s: hardware register overwrite while controller is busy
--

commit 4810c7a0849d088839c80d7f592b0dcb4167dff5
Author: Navid Ghahremani <ghahramani.navid@gmail.com>

spi: Add ZTE ZX279128S SPI flash controller

This commit adds support for the SPI flash controller of the ZTE
zx279128s SoC, implementing the spi-mem interface for bounded SPI-memory
operations.

> diff --git a/drivers/spi/spi-zx279128s-spifc.c b/drivers/spi/spi-zx279128s-spifc.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/spi/spi-zx279128s-spifc.c

[ ... ]

> +static int zx_spifc_read_fifo(struct zx_spifc *spifc, u8 *buf,
> +			      unsigned int len)
> +{
> +	unsigned int i;
> +	u32 word;
> +	int ret;
> +
> +	for (i = 0; i < len; i += 4) {
> +		ret = zx_spifc_wait_fifo(spifc, SPIFC_FIFO_RX_AVAIL);
> +		if (ret)
> +			return ret;
> +		word = readl(spifc->base + SPIFC_FIFO_DATA);
> +		memcpy(buf + i, &word, min(4U, len - i));

[Severity: High]
Does this code cause byte-order corruption on big-endian architectures?

The readl() accessor implicitly performs CPU-to-little-endian byteswapping.
When used with memcpy() to handle a byte stream, this byteswapping reverses
the byte order within each 32-bit word on big-endian systems.

Could stream accessors like readsl() or ioread32_rep() be used here
instead to preserve memory byte order without endianness conversions?

> +	}
> +
> +	return 0;
> +}

[ ... ]

> +static int zx_spifc_write_fifo(struct zx_spifc *spifc, const u8 *buf,
> +			       unsigned int len)
> +{
> +	unsigned int i = 0;
> +	bool started = false;
> +	u32 word;
> +	int ret;
> +
> +	while (i < len) {
> +		if (!(readl(spifc->base + SPIFC_FIFO_STATUS) &
> +		      SPIFC_FIFO_TX_FREE)) {
> +			if (!started) {
> +				zx_spifc_start(spifc);
> +				started = true;
> +			}
> +			ret = zx_spifc_wait_fifo(spifc, SPIFC_FIFO_TX_FREE);
> +			if (ret)
> +				return ret;
> +		}
> +		word = 0;
> +		memcpy(&word, buf + i, min(4U, len - i));
> +		writel(word, spifc->base + SPIFC_FIFO_DATA);

[Severity: High]
Similarly, does this writel() call reverse the byte order of the outgoing
stream on big-endian systems?

Could writesl() or iowrite32_rep() be used to avoid unintended
byteswapping during flash writes?

> +		i += 4;
> +	}
> +
> +	if (!started)
> +		zx_spifc_start(spifc);
> +
> +	return 0;
> +}

[ ... ]

> +static int zx_spifc_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
> +{
> +	struct zx_spifc *spifc = spi_controller_get_devdata(mem->spi->controller);
> +	void __iomem *base = spifc->base;
> +	u32 fmt0 = 0, fmt1 = 0, val;
> +	int ret;
> +
> +	ret = zx_spifc_set_speed(spifc, op->max_freq ?: mem->spi->max_speed_hz);
> +	if (ret)
> +		return ret;
> +
> +	if (op->addr.nbytes) {
> +		fmt0 |= SPIFC_FMT0_ADDR;
> +		fmt1 |= FIELD_PREP(SPIFC_FMT1_ADDR_BYTES, op->addr.nbytes - 1);
> +	}
> +	if (op->dummy.nbytes) {
> +		fmt0 |= SPIFC_FMT0_DUMMY;
> +		fmt1 |= FIELD_PREP(SPIFC_FMT1_DUMMY_BYTES, op->dummy.nbytes);
> +	}
> +	if (op->data.nbytes) {
> +		fmt0 |= op->data.dir == SPI_MEM_DATA_IN ? SPIFC_FMT0_DATA_IN :
> +							  SPIFC_FMT0_DATA_OUT;
> +		fmt1 |= FIELD_PREP(SPIFC_FMT1_DATA_LINES,
> +				   zx_spifc_lines(op->data.buswidth));
> +	}
> +
> +	zx_spifc_ack(spifc);
> +	writel(op->cmd.opcode, base + SPIFC_OPCODE);
> +	writel(fmt0, base + SPIFC_FMT0);

[Severity: Medium]
Does this code overwrite active controller registers while the hardware is
still busy?

If a previous SPI operation times out, the hardware's busy flag might
still be active. When zx_spifc_ack() is called, it observes the busy state
and skips the acknowledge:

    static void zx_spifc_ack(struct zx_spifc *spifc)
    {
        u32 val = readl(spifc->base + SPIFC_ACK);
        if (!(val & SPIFC_ACK_BUSY))
            writel(val | SPIFC_ACK_DONE, spifc->base + SPIFC_ACK);
    }

However, execution immediately continues and unconditionally writes new
configuration to SPIFC_OPCODE and SPIFC_FMT0. Modifying active controller
registers while the hardware state machine is processing a previous
transaction could lead to undefined behavior or bus lockups.

Should the driver ensure the hardware is idle or properly reset before
starting a new operation here?

> +	writel(fmt1, base + SPIFC_FMT1);
> +	writel(op->data.nbytes ? op->data.nbytes - 1 : 0, base + SPIFC_LEN);
> +	writel(op->addr.nbytes ? op->addr.val : 0, base + SPIFC_ADDR);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261010130024.177503-1-ghahramani.navid@gmail.com?part=20