Re: [PATCH v2 07/11] spi: dw: send cmd and addr to start the spi transfer

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing Data Phase and Completion Wait in Enhanced SPI Mode
- [Medium] Address Truncation on 16-bit I/O Width
- [Medium] Preemption / FIFO Overflow Vulnerability
--

--- Patch [7]: [PATCH v2 07/11] spi: dw: send cmd and addr to start the spi transfer ---
commit 0e79a8e7a3709454ed5a5603c033614bdc1b0b5a
Author: Sudip Mukherjee <[email protected]>

spi: dw: send cmd and addr to start the spi transfer
    
In enhanced spi mode, read or write will start by sending the cmd
and address (if present).

diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
index dbe8779ced865..3388f62dc695c 100644
--- a/drivers/spi/spi-dw-core.c
+++ b/drivers/spi/spi-dw-core.c
[ ... ]
> +static void dw_spi_enh_write_cmd_addr(struct dw_spi *dws, const struct spi_mem_op *op)
> +{
> +       /* Send cmd as 32 bit value */
> +       dw_write_io_reg(dws, DW_SPI_DR, op->cmd.opcode);
> +       if (op->addr.nbytes) {
> +               dw_write_io_reg(dws, DW_SPI_DR, lower_32_bits(op->addr.val));

[Severity: Medium]
Does passing a 32-bit chunk to dw_write_io_reg() truncate the address if the
controller is configured with a 16-bit I/O width?

Looking at dw_write_io_reg(), it uses writew_relaxed() when dws->reg_io_width
is 2. This would drop the upper 16 bits of the lower 32-bit value when
executing an enhanced SPI operation with an address phase.

> +               if (op->addr.nbytes > 4) {
> +                       /* address more than 32bit */
> +                       dw_write_io_reg(dws, DW_SPI_DR, upper_32_bits(op->addr.val));
> +               }
> +       }
> +}
> +
[ ... ]
>  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;
> @@ -909,6 +922,8 @@ static int dw_spi_exec_enh_mem_op(struct spi_mem *mem, const struct spi_mem_op *
>  
>         dw_spi_enable_chip(dws, 1);
>  
> +       dw_spi_enh_write_cmd_addr(dws, op);
> +
>         return 0;
>  }

[Severity: High]
Is the payload data phase, chip select assertion, and completion wait
intentionally omitted here?

By returning 0 immediately after dw_spi_enh_write_cmd_addr(), the data
phase (SPI_MEM_DATA_IN or SPI_MEM_DATA_OUT) seems to be skipped entirely,
which would leave read buffers uninitialized and fail to transfer write
payloads.

[Severity: Medium]
Does dw_spi_exec_enh_mem_op() need to disable local interrupts and preemption
during the IO transfer?

The standard dw_spi_exec_mem_op() wraps the transfer in local_irq_save() and
preempt_disable() to prevent the DesignWare APB SSI RX FIFO from overflowing
if the CPU is preempted or handles a long interrupt. If the CPU is preempted
here in enhanced mode under heavy system load, the incoming data might
similarly overflow the RX FIFO.

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