Re: [PATCH v1 04/11] spi: dw: add check for support of enhanced spi

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The dummy cycle check uses integer division incorrectly, allowing wait cycle configurations that silently overflow the 5-bit hardware wait cycle register.
- [High] The buswidth validation logic incorrectly rejects valid NO_DATA operations (e.g., QPI Erase) while accepting unsupported mixed-buswidth operations (e.g., 4-1-4).
--

--- Patch [4]: [PATCH v1 04/11] spi: dw: add check for support of enhanced spi ---
commit fe456b0bd9838cdf376b52d9ec69265dbf1ef090
Author: Sudip Mukherjee <sudip.mukherjee@sifive.com>

spi: dw: add check for support of enhanced spi

This commit queries the spi controller for buswidths it supports before doing
a mem op. It adds dual/quad/octal mode checks if the controller has the
DW_SPI_CAP_EMODE capability.

> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> index 6befe47929b4c..010a665b17b76 100644
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c
> @@ -521,6 +521,32 @@ static int dw_spi_adjust_mem_op_size(struct spi_mem *mem, struct spi_mem_op *op)
>  	return 0;
>  }
>  
> +static bool dw_spi_supports_enh_mem_op(struct spi_mem *mem,
> +				       const struct spi_mem_op *op)
> +{
> +	if (op->addr.nbytes != 0 && op->addr.buswidth != 1 &&
> +	    op->addr.buswidth != op->data.buswidth)
> +		return false;

[Severity: High]
Does this logic incorrectly reject valid operations that have no data phase,
like Quad SPI Erase?

If an operation has no data phase (op->data.nbytes == 0), the SPI core
initializes op->data.buswidth to 0. For an erase command over QSPI
(e.g., 1-4-0), op->addr.buswidth != op->data.buswidth evaluates to 4 != 0,
which erroneously returns false.

> +
> +	if (op->addr.nbytes >= 8)
> +		return false;
> +
> +	if (op->cmd.buswidth != 1 && op->cmd.buswidth != op->addr.buswidth &&
> +	    op->cmd.buswidth != op->data.buswidth)
> +		return false;

[Severity: High]
Can this logical AND chain accidentally accept unsupported configurations?

For an operation like 4-1-4, op->cmd.buswidth != op->data.buswidth evaluates
as 4 != 4 (false), which short-circuits the AND chain and bypasses the
rejection.

> +
> +	if (op->dummy.nbytes && !op->dummy.buswidth)
> +		return false;
> +
> +	if (op->dummy.nbytes != 0 && op->data.dir == SPI_MEM_DATA_OUT)
> +		return false;
> +
> +	if (op->dummy.nbytes != 0 && op->dummy.nbytes / op->dummy.buswidth > 4)
> +		return false;

[Severity: High]
Does this integer division silently allow configurations that overflow the
5-bit hardware wait cycle register?

Due to truncation, a configuration with op->dummy.nbytes = 19 and
op->dummy.buswidth = 4 will evaluate as 4 > 4 (false) and be accepted.

This corresponds to 38 SPI clocks ((19 * 8) / 4). Since the hardware wait
cycle register (DW_SPI_ENH_CTRLR0_WAIT_CYCLE_MASK) has a maximum limit of
31 cycles, programming 38 will be bitwise-ANDed by FIELD_PREP() to 6 cycles.
This could result in sampling data out of sync and corrupting the read.

> +
> +	return spi_mem_default_supports_op(mem, op);
> +}

[ ... ]

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