Re: [PATCH v2 03/11] spi: dw: add check for support of enhanced spi

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Flawed buswidth validation erroneously rejects valid multi-bit operations (when phases are absent) and incorrectly accepts physically unsupported operations.
--

--- Patch [3]: [PATCH v2 03/11] spi: dw: add check for support of enhanced spi ---
commit 0eaff4ea0f38f3c8901810547641176fba69f40c
Author: Sudip Mukherjee <[email protected]>

spi: dw: add check for support of enhanced spi

This commit adds support for validating single/dual/quad/octal SPI memory
operations 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
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c
[ ... ]
> +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]
Will this erroneously reject valid multi-bit operations that do not have a
data phase (such as 1-4-0 or 4-4-0)?

If a multi-bit operation like an Erase command has no data phase,
op->data.nbytes is 0 and op->data.buswidth evaluates to 0. Since the
address buswidth (e.g., 4) does not equal the data buswidth (0), this check
evaluates to true and incorrectly returns false.

Should there be a check for op->data.nbytes != 0 before comparing the bus
widths?

> +
> +	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]
Does this logic allow hardware-unsupported mixed-buswidth permutations?

For an invalid operation like 4-1-4, op->cmd.buswidth (4) does not equal
op->addr.buswidth (1), but it does equal op->data.buswidth (4). This makes
the final condition (op->cmd.buswidth != op->data.buswidth) false, which
short-circuits the if statement and allows the operation.

Since the DW SPI controller's TRANS_TYPE cannot natively support such
permutations, should this logic be adjusted to prevent accepting operations
that will result in corrupted transfers on the SPI bus?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3