Re: [PATCH v1 05/11] spi: dw: Introduce enhanced single/dual/quad/octal spi

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Divide-by-zero in wait cycle calculation causes kernel panic on commands without dummy cycles.
- [Critical] Enhanced memory operations completely fail to perform actual data transfers, causing uninitialized memory reads and silently dropped writes.
- [High] `dw_spi_exec_enh_mem_op` ignores the operation-specific maximum frequency, violating the SPI memory API contract.
- [High] Calculated wait cycles can exceed the 5-bit hardware register limit and are silently truncated to 0, corrupting read timing.
- [High] Unclamped data length for enhanced mode write operations leads to 16-bit hardware register truncation.
--

--- Patch [5]: [PATCH v1 05/11] spi: dw: Introduce enhanced single/dual/quad/octal spi ---
commit 0f1cbd98fc45570180f08cb46b29e531be3e743b
Author: Sudip Mukherjee <sudip.mukherjee@sifive.com>

spi: dw: Introduce enhanced single/dual/quad/octal spi

This commit introduces support for enhanced single/dual/quad/octal SPI modes
in the DesignWare SPI controller driver. It adds a new function to execute
enhanced memory operations and updates the SPI_CTRLR0 register configuration
when these modes are utilized.

> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> index 010a665b17b76..58159e8269f53 100644
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c
> @@ -815,6 +815,89 @@ static int dw_spi_exec_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)

[ ... ]

> +static int dw_spi_exec_enh_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
> +{
> +	struct spi_controller *ctlr = mem->spi->controller;
> +	struct dw_spi *dws = spi_controller_get_devdata(ctlr);
> +	struct dw_spi_enh_cfg enh_cfg;
> +	struct dw_spi_cfg cfg;
> +
> +	switch (op->data.buswidth) {

[ ... ]

> +	dw_spi_init_enh_mem_buf(dws, op);
> +
> +	cfg.dfs = 8;
> +	cfg.freq = clamp(mem->spi->max_speed_hz, 0U, dws->max_mem_freq);

[Severity: High]
Does this code ignore the operation-specific maximum frequency?

The spi_mem_op struct specifies op->max_freq which dictates the maximum
safe clock speed for this specific command. 

Using mem->spi->max_speed_hz instead might cause operations with stricter
frequency limits to execute at the higher overall bus speed, potentially
leading to hardware timing violations.

> +	cfg.ndf = op->data.nbytes;
> +	if (op->data.dir == SPI_MEM_DATA_IN)
> +		cfg.tmode = DW_SPI_CTRLR0_TMOD_RO;
> +	else
> +		cfg.tmode = DW_SPI_CTRLR0_TMOD_TO;

[Severity: High]
Can enhanced transmit-only mode operations suffer from size truncation?

Enhanced Transmit Only mode uses DW_SPI_CTRLR1 to count frames to transmit,
and this is a 16-bit register. 

While dw_spi_adjust_mem_op_size() clamps SPI_MEM_DATA_IN, it does not
clamp SPI_MEM_DATA_OUT operations. If an outbound transfer is large,
such as exactly 65536 bytes, the unclamped length will be truncated when
written to the 16-bit DW_SPI_CTRLR1 register.

> +
> +	if (op->data.buswidth == op->addr.buswidth &&
> +	    op->data.buswidth == op->cmd.buswidth)
> +		enh_cfg.trans_t = DW_SPI_ENH_CTRLR0_TRANS_TYPE_TT2;

[ ... ]

> +	enh_cfg.addr_l = op->addr.nbytes << 1;
> +	if (op->cmd.nbytes == 2)
> +		enh_cfg.inst_l = DW_SPI_ENH_CTRLR0_INST_L_INST_L16;
> +	else if (op->cmd.nbytes == 1)
> +		enh_cfg.inst_l = DW_SPI_ENH_CTRLR0_INST_L_INST_L8;
> +	else
> +		enh_cfg.inst_l = DW_SPI_ENH_CTRLR0_INST_L_INST_L0;
> +
> +	enh_cfg.wait_c = (op->dummy.nbytes * (BITS_PER_BYTE / op->dummy.buswidth));

[Severity: Critical]
Can this cause a divide-by-zero crash if the operation has no dummy phase?

For operations like READ ID or standard read/write without dummies, both
op->dummy.nbytes and op->dummy.buswidth are initialized to 0. 

Since there is no check before the division, this will cause a kernel panic
whenever such a command is sent in enhanced mode.

[Severity: High]
Additionally, could this calculation result in a value that exceeds the
hardware register limit?

If op->dummy.nbytes is 4 and op->dummy.buswidth is 1, enh_cfg.wait_c
evaluates to 32. 

Later, dw_spi_update_config() applies a 5-bit mask to this value:

cr0 |= FIELD_PREP(DW_SPI_ENH_CTRLR0_WAIT_CYCLE_MASK, enh_cfg->wait_c);

Since 32 requires 6 bits, FIELD_PREP will truncate it to 0, incorrectly
configuring the controller for 0 wait cycles instead of 32.

> +
> +	dw_spi_enable_chip(dws, 0);
> +
> +	dw_spi_update_config(dws, mem->spi, &cfg, &enh_cfg);
> +
> +	dw_spi_enable_chip(dws, 1);
> +
> +	return 0;

[Severity: Critical]
Does this function complete without actually performing the SPI data
transfers?

The function enables the SPI chip and then immediately returns 0. It
appears to omit the steps to write the command, address, or data to the
data register, and does not read from the RX FIFO. 

Will this cause read operations to return uninitialized buffer memory and
write operations to silently drop all data?

> +}
> +
>  /*
>   * Initialize the default memory operations if a glue layer hasn't specified
>   * custom ones. Direct mapping operations will be preserved anyway since DW SPI

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709055204.138168-1-changhuang.liang@starfivetech.com?part=5