Re: [PATCH v2 06/11] spi: dw: Introduce enhanced single/dual/quad/octal spi

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] The newly introduced dw_spi_exec_enh_mem_op() lacks actual SPI data transfer execution logic, returning success without performing I/O and leaving hardware enabled.
- [High] Incorrect transfer type (trans_t) mapping for multi-bit commands without an address phase (e.g., 4-0-4) silently misconfigures the wire protocol.
--

--- Patch [6]: [PATCH v2 06/11] spi: dw: Introduce enhanced single/dual/quad/octal spi ---
commit 02b326945674bcd1a149d5a5a3e3cd78b2ccabda
Author: Sudip Mukherjee <sudip.mukherjee@sifive.com>

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

If the spi transfer is using enhanced single/dual/quad/octal spi mode,
then we need to update the SPI_CTRLR0 register. The SPI_CTRLR0 register
will be updated in dw_spi_update_config() via the values in
dw_spi_enh_cfg.

> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> index d654cb748646a..dbe8779ced865 100644
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c

[ ... ]

> @@ -828,6 +828,90 @@ static int dw_spi_exec_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
>  	return ret;
>  }
>  
> +static void dw_spi_init_enh_mem_buf(struct dw_spi *dws, 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 = {0};
> +	struct dw_spi_cfg cfg = {0};

[ ... ]

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

[Severity: High]
Does this code handle multi-bit commands without an address phase correctly?

For example, on a 4-0-4 operation where op->addr.nbytes == 0 and 
op->addr.buswidth == 0, op->data.buswidth (4) won't match op->addr.buswidth (0).
This logic would fall through to the else branch and set trans_t to
DW_SPI_ENH_CTRLR0_TRANS_TYPE_TT0, which misconfigures the wire protocol to
send the multi-bit command as a 1-bit Standard SPI command.

> +
> +	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;
> +
> +	if (op->dummy.buswidth)
> +		enh_cfg.wait_c = op->dummy.nbytes * BITS_PER_BYTE / op->dummy.buswidth;
> +
> +	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]
Is it intentional that this function omits the actual data transfer and chip
teardown?

It appears to return success (0) immediately after enabling the chip, without
calling any I/O routines like dw_spi_write_then_read() or dw_spi_stop_mem_op().
For read operations, this would leave the caller's receive buffer uninitialized
and the SPI chip enabled indefinitely.

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