Re: [PATCH v1 08/11] spi: dw: adjust size of mem_op

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] A guaranteed divide-by-zero exception occurs in `dw_spi_exec_enh_mem_op()` when executing SPI operations that lack a dummy phase.
- [High] Clamping memory operation size to 65536 overflows the 16-bit NDF hardware register for Transmit-Only operations in enhanced mode.
- [High] Enhanced SPI mem ops disable the controller before the hardware finishes transmitting, causing trailing data loss.
- [High] Incorrect dummy cycle bounds check in Enhanced SPI mode allows `wait_c` overflow, leading to zero dummy cycles.
- [High] Incorrect FIFO write order for addresses larger than 32 bits in Enhanced SPI mode causes endianness reversal.
- [High] A hardcoded 5µs delay (`udelay(5)`) after asserting Chip Select leaves the RX FIFO susceptible to unhandled overflow on high-speed SPI buses.
--

--- Patch [8]: [PATCH v1 08/11] spi: dw: adjust size of mem_op ---
commit 1cc8ff7449ec8b3ec7ea070e7ac5dce9adb45f0c
Author: Sudip Mukherjee <sudip.mukherjee@sifive.com>

spi: dw: adjust size of mem_op
    
This patch adjusts the size of the data that can be sent or received
in enhanced mode, utilizing the new bounds to set the NDF register
appropriately.

> 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
> @@ -576,6 +576,13 @@ static int dw_spi_target_abort(struct spi_controller *ctlr)
>  	return 0;
>  }
>  
> +static int dw_spi_adjust_enh_mem_op_size(struct spi_mem *mem, struct spi_mem_op *op)
> +{
> +	op->data.nbytes = clamp_val(op->data.nbytes, 0, DW_SPI_NDF_MASK + 1);

[Severity: High]
Does this clamp operation cause an overflow when written to the hardware?

If op->data.nbytes is exactly 65536 (DW_SPI_NDF_MASK + 1), writing this
value to the DW_SPI_CTRLR1 register, which is a 16-bit register, will set bits
15:0 to 0. This silently truncates maximum-length 65536-byte block transfers
down to 0 frames, which could cause data corruption on large writes.

> +
> +	return 0;
> +}
> +

[ ... ]

> @@ -1017,13 +1024,14 @@ static void dw_spi_init_mem_ops(struct dw_spi *dws)
>  {
>  	if (!dws->mem_ops.exec_op && !(dws->caps & DW_SPI_CAP_CS_OVERRIDE) &&
>  	    !dws->set_cs) {
> -		dws->mem_ops.adjust_op_size = dw_spi_adjust_mem_op_size;
>  		if (dws->caps & DW_SPI_CAP_EMODE) {
>  			dws->mem_ops.exec_op = dw_spi_exec_enh_mem_op;

[Severity: Critical]
Does this execution path trigger a divide-by-zero exception when operations
lack a dummy phase?

In dw_spi_exec_enh_mem_op(), the dummy cycles are calculated like this:

drivers/spi/spi-dw-core.c:dw_spi_exec_enh_mem_op() {
    ...
    enh_cfg.wait_c = (op->dummy.nbytes * (BITS_PER_BYTE / op->dummy.buswidth));
    ...
}

When an operation has no dummy cycles, the SPI core leaves op->dummy.buswidth
initialized to 0. Due to C operator precedence, the division happens before the
multiplication, which triggers a divide-by-zero exception unconditionally on
basic commands like Read ID.


[Severity: High]
Can this sequence cause trailing data loss by disabling the controller
prematurely?

When the IRQ handler signals transfer completion:

drivers/spi/spi-dw-core.c:dw_spi_enh_handler() {
    ...
    spi_finalize_current_transfer(dws->ctlr);
    ...
}

The main thread wakes up and immediately stops the operation:

drivers/spi/spi-dw-core.c:dw_spi_exec_enh_mem_op() {
    ...
    dw_spi_stop_mem_op(dws, mem->spi);
    ...
}

This disables the controller (dw_spi_enable_chip(dws, 0)) without waiting
for the BUSY bit to clear. Doing so clears internal FIFOs and abruptly aborts
ongoing transmissions on the wire.


[Severity: High]
Does the bounds check for dummy cycles allow an overflow that results in
zero dummy cycles being executed?

In dw_spi_supports_enh_mem_op(), the check allows up to 32 dummy cycles:

drivers/spi/spi-dw-core.c:dw_spi_supports_enh_mem_op() {
    ...
    if (op->dummy.nbytes != 0 && op->dummy.nbytes / op->dummy.buswidth > 4)
    ...
}

However, the WAIT_CYCLES field in SPI_CTRLR0 is 5 bits wide. Passing
wait_c = 32 to FIELD_PREP() in dw_spi_update_config() with a 5-bit
mask silently truncates the value to 0:

drivers/spi/spi-dw-core.c:dw_spi_update_config() {
    ...
    cr0 |= FIELD_PREP(DW_SPI_ENH_CTRLR0_WAIT_CYCLE_MASK, enh_cfg->wait_c);
    ...
}

This would cause the hardware to execute 0 dummy cycles instead of 32,
destroying data synchronization on subsequent reads.


[Severity: High]
Does the FIFO write order for addresses larger than 32 bits cause an
endianness reversal?

In dw_spi_enh_write_cmd_addr(), the lower 32 bits are written before the
upper 32 bits:

drivers/spi/spi-dw-core.c:dw_spi_enh_write_cmd_addr() {
    ...
    dw_write_io_reg(dws, DW_SPI_DR, lower_32_bits(op->addr.val));
    if (op->addr.nbytes > 4) {
        /* address more than 32bit */
        dw_write_io_reg(dws, DW_SPI_DR, upper_32_bits(op->addr.val));
    }
    ...
}

Since the controller transmits FIFO words in the order they are written, the
lower 32 bits are sent over the wire first. The SPI protocol mandates sending
the highest byte first, which could result in accessing incorrect addresses on
the flash device.


[Severity: High]
Can the hardcoded 5 microsecond delay leave the RX FIFO susceptible to
overflow on high-speed SPI buses?

In dw_spi_exec_enh_mem_op(), the CPU sleeps while the hardware is active:

drivers/spi/spi-dw-core.c:dw_spi_exec_enh_mem_op() {
    ...
    dw_spi_enh_write_cmd_addr(dws, op, mem);

    udelay(5);

    dw_spi_enh_irq_setup(dws);
    ...
}

Asserting the chip select inherently starts the clocking of SPI data. On a fast
SPI bus, the hardware will receive many bytes during this 5 microsecond delay.
Because RX FIFO depths are limited, the FIFO could overflow while the CPU
sleeps before the interrupts are unmasked, failing the transfer.

>  			dws->mem_ops.supports_op = dw_spi_supports_enh_mem_op;
> +			dws->mem_ops.adjust_op_size = dw_spi_adjust_enh_mem_op_size;
>  		} else {

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