Re: [RFC PATCH 4/4] spi: cadence-xspi: add ACMD support for SPI NOR
flat view
From: sashiko-bot@kernel.org
Date: 2026-09-21 10:19:47
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] spi: cadence-xspi: ACMD timeout is too short for SPI NOR erase operations - [High] spi: cadence-xspi: Type confusion when casting SPI client's drvdata to struct spi_nor - [High] spi: cadence-xspi: Global state pollution breaks multi-device support - [Medium] spi: cadence-xspi: SPI NOR ACMD support is dead code - [Medium] spi: cadence-xspi: Data race on acmd_info.initialized causes 0-byte I/O failures -- commit 9403976e689b7174f09268bad857344a4209db95 Author: fei.xie [off-list ref] spi: cadence-xspi: add ACMD support for SPI NOR Program the automatic command sequences from the SPI NOR operation templates. Uses the PIO command path with MDMA data transfers for reads, page programs and erases. Unsupported command layouts continue to use STIG mode.
quoted hunk ↗ jump to hunk
diff --git a/drivers/spi/spi-cadence-xspi.c b/drivers/spi/spi-cadence-xspi.c --- a/drivers/spi/spi-cadence-xspi.c +++ b/drivers/spi/spi-cadence-xspi.c
[ ... ]
quoted hunk ↗ jump to hunk
+static int cdns_xspi_nor_init(struct cdns_xspi_dev *cdns_xspi, + struct spi_nor *nor) +{
[ ... ]
quoted hunk ↗ jump to hunk
+ if (!spi_nor_protocol_is_dtr(nor->reg_proto) && + !cdns_xspi->acmd_info.nor_status_initialized) { + ret = cdns_xspi_nor_status_seq_init(cdns_xspi, &status_op); + if (ret) + return ret; + } + + cdns_xspi->acmd_info.initialized = true; + + return 0; +}
[Severity: High] If multiple SPI NOR flashes are connected on different chip selects, will subsequent devices skip initialization and incorrectly reuse the first flash's hardware parameters? [ ... ]
quoted hunk ↗ jump to hunk
+static int cdns_xspi_send_nor_pio_command(struct cdns_xspi_dev *cdns_xspi, + struct spi_mem *mem, + const struct spi_mem_op *op) +{ + struct spi_nor *nor = spi_mem_get_drvdata(mem); + u8 opcode = cdns_xspi_get_opcode(op); + int ret; + + if (!nor) + goto use_stig;
[ ... ]
+ if (!nor->mtd.size || !nor->mtd.writebufsize) + goto use_stig;
[Severity: High] If a device other than a SPI NOR flash (like spidev or at25) is bound to the controller, could this cast cause an out-of-bounds memory access or read garbage data when dereferencing nor->mtd.size? [ ... ]
quoted hunk ↗ jump to hunk
+ if (opcode == nor->erase_opcode && op->data.dir == SPI_MEM_NO_DATA) { + if (nor->mtd.numeraseregions || + !cdns_xspi_nor_helper_sequences_ready(cdns_xspi, nor)) + goto use_stig; + + ret = cdns_xspi_nor_erase_seq_init(cdns_xspi, nor, op); + if (ret == -EOPNOTSUPP) + goto use_stig; + if (ret) + return ret; + + return cdns_xspi_pio_erase(cdns_xspi, op->addr.val);
[Severity: High] Does this sector erase operation trigger the hardcoded CDNS_XSPI_ACMD_TIMEOUT_MS limit? Since CDNS_XSPI_ACMD_TIMEOUT_MS is 1000 (1 second), and standard block/sector erases commonly take 2-3 seconds, will this cause the operation to fail with -ETIMEDOUT and potentially wedge the controller?
quoted hunk ↗ jump to hunk
+ } + +use_stig: + return cdns_xspi_send_stig_command(cdns_xspi, op, + op->data.dir != SPI_MEM_NO_DATA); +} + +static int cdns_xspi_send_pio_command(struct cdns_xspi_dev *cdns_xspi, + struct spi_mem *mem, + const struct spi_mem_op *op) +{ + switch (cdns_xspi->flash_type) { + case CDNS_XSPI_FLASH_TYPE_NAND: + return cdns_xspi_send_nand_pio_command(cdns_xspi, mem, op); + + case CDNS_XSPI_FLASH_TYPE_NOR: + return cdns_xspi_send_nor_pio_command(cdns_xspi, mem, op);
[Severity: Medium] Is this NOR ACMD path actually reachable during normal operation? Looking at cdns_driver_data, the use_acmd flag defaults to false for NOR. This causes cdns_xspi_probe() to fall back to CDNS_XSPI_WORK_MODE_STIG mode, completely bypassing the ACMD logic introduced by this patch.
quoted hunk ↗ jump to hunk
+ + default: + return -EOPNOTSUPP; + } +}
[ ... ]
quoted hunk ↗ jump to hunk
@@ -1484,8 +1900,18 @@ static int cdns_xspi_adjust_mem_op_size(struct spi_mem *mem, struct spi_mem_op * { struct cdns_xspi_dev *cdns_xspi = spi_controller_get_devdata(mem->spi->controller); + size_t max_len = cdns_xspi->sdmasize; + + if (cdns_xspi->work_mode == CDNS_XSPI_WORK_MODE_ACMD) { + if (cdns_xspi->acmd_info.initialized) + max_len = min_t(size_t, max_len, + cdns_xspi->dma_buf_len);
[Severity: Medium] Could this locklessly read initialized == true while observing a stale dma_buf_len == 0 due to a lack of memory barriers? If Thread A writes initialized = true in cdns_xspi_nor_init() after configuring dma_buf_len, and Thread B concurrently evaluates this check for a different device outside the controller lock, the operation size could be incorrectly clamped to 0 bytes.
quoted hunk ↗ jump to hunk
+ else if (cdns_xspi->flash_type == CDNS_XSPI_FLASH_TYPE_NOR) + max_len = min_t(size_t, max_len, + CDNS_XSPI_NOR_DMA_BUF_SIZE); + } - op->data.nbytes = clamp_val(op->data.nbytes, 0, cdns_xspi->sdmasize); + op->data.nbytes = min_t(size_t, op->data.nbytes, max_len); return 0; }
-- Sashiko AI review · https://sashiko.dev/#/patchset/20260921093701.1341766-1-fei.xie@horizon.auto?part=4