Re: [PATCH v1 09/11] spi: dw: detect enhanced spi mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] detect_enh_mode() leaks detected multi-lane modes into ctlr->mode_bits on clock stretch test failure.
- [High] Advertising multi-bit SPI modes globally without supporting them in dw_spi_transfer_one() causes standard multi-lane transfers to corrupt data.
- [High] The register writability test falsely detects OCTAL mode support on QUAD-only hardware.
- [High] dw_spi_hw_init() blindly overwrites ctlr->mode_bits, wiping out dynamically established capabilities like SPI_LOOP after a suspend/resume cycle.
--

--- Patch [9]: [PATCH v1 09/11] spi: dw: detect enhanced spi mode ---
commit 0f885f7a3380b812c7026e7502b6a3b2baf64278
Author: Sudip Mukherjee <sudip.mukherjee@sifive.com>

spi: dw: detect enhanced spi mode

All the SSI controllers supporting enhanced spi modes might not support
all the three dual or quad or octal modes. Detect the modes that are
supported and finally enable the DW_SPI_CAP_EMODE capability which will
start using all the enhanced spi functions that has been added.

> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> index 526b54385cd3e..ef1abb540f069 100644
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c
[ ... ]
> @@ -1083,6 +1083,64 @@ static void dw_spi_cleanup(struct spi_device *spi)
[ ... ]
> +static u16 detect_enh_mode(struct dw_spi *dws)
> +{
[ ... ]
> +	/* test octal mode */
> +	tmp_val = DW_SPI_CTRLR0_SPI_FRF_OCT_SPI << frf_shift;
> +	dw_writel(dws, DW_SPI_CTRLR0, tmp_val);
> +	if ((tmp_val & dw_readl(dws, DW_SPI_CTRLR0)) == tmp_val)
> +		mode |= SPI_TX_OCTAL | SPI_RX_OCTAL;

[Severity: High]
Can this writability test falsely detect Octal mode support on Quad-only
hardware?

If the SPI_FRF field is 2 bits wide, Quad mode would require both physical
flip-flops to exist. When the driver tests for Octal mode by writing a value
that sets both bits, the readback will confirm both bits are set. This would
hold the value and trick the driver into assuming the IP supports internal
Octal logic when it may only be synthesized for Quad.

> +
> +	if (!mode)
> +		goto disable_enh;
> +
> +	/* test clock stretching */
> +	dw_writel(dws, DW_SPI_SPI_CTRLR0, DW_SPI_ENH_CTRLR0_CLK_STRETCH_EN);
> +	if ((DW_SPI_ENH_CTRLR0_CLK_STRETCH_EN & dw_readl(dws, DW_SPI_SPI_CTRLR0)) !=
> +	    DW_SPI_ENH_CTRLR0_CLK_STRETCH_EN)
> +		/*
> +		 * If clock stretching is not enabled then do not use
> +		 * enhanced mode.
> +		 */
> +		goto disable_enh;

[Severity: High]
Does this error path leak the detected multi-lane modes?

When the clock stretching test fails, the code jumps to the disable_enh label
without clearing the local mode variable. The non-zero mode bitmask is then
returned and unconditionally bitwise-ORed into ctlr->mode_bits by the caller.

Could this falsely advertise multi-lane capabilities to the SPI subsystem
when the controller does not actually support enhanced mode?

> +
> +	dws->caps |= DW_SPI_CAP_EMODE;
> +
> +disable_enh:
> +	dw_writel(dws, DW_SPI_CTRLR0, tmp_ctrlr0);
> +	dw_writel(dws, DW_SPI_SPI_CTRLR0, tmp_spi_ctrlr0);
> +	dw_spi_enable_chip(dws, 1);
> +
> +	return mode;
> +}
[ ... ]
> @@ -1162,6 +1220,9 @@ static void dw_spi_hw_init(struct device *dev, struct dw_spi *dws)
[ ... ]
>  		dws->caps |= DW_SPI_CAP_DFS32;
>  	}
>  
> +	dws->ctlr->mode_bits = SPI_CPOL | SPI_CPHA;

[Severity: High]
Does this direct assignment overwrite dynamically established capabilities?

During initial probe, dw_spi_add_controller() sets capabilities like:
    ctlr->mode_bits |= SPI_LOOP;

When the system resumes, dw_spi_resume_controller() calls dw_spi_hw_init():

drivers/spi/spi-dw-core.c:dw_spi_resume_controller() {
    ...
    dw_spi_hw_init(&dws->ctlr->dev, dws);
    ...
}

Will this blind assignment wipe out flags like SPI_LOOP after a
suspend/resume cycle?

> +	dws->ctlr->mode_bits |= detect_enh_mode(dws);

[Severity: High]
Does advertising these multi-bit SPI modes globally cause standard multi-lane
transfers to corrupt data?

By adding these multi-lane modes to the controller's mode_bits, they are
exposed to standard (non-mem_ops) SPI transfers. If a user-space application
or kernel driver initiates a standard spi_transfer with tx_nbits > 1, the SPI
core will accept it. 

However, looking at dw_spi_transfer_one(), the standard transfer handler
appears to ignore multi-lane requests and executes everything in 1-bit
standard mode:

drivers/spi/spi-dw-core.c:dw_spi_transfer_one() {
    ...
    .spi_frf = DW_SPI_CTRLR0_SPI_FRF_STD_SPI,
    ...
}

Will this result in data corruption and protocol mismatch on the SPI bus?

> +
>  	/* enable HW fixup for explicit CS deselect for Amazon's alpine chip */
>  	if (dws->caps & DW_SPI_CAP_CS_OVERRIDE)
>  		dw_writel(dws, DW_SPI_CS_OVERRIDE, 0xF);

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