Re: [RFC PATCH 4/4] spi: cadence-xspi: add ACMD support for SPI NOR

[sashiko-bot@kernel.org]

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 <fei.xie@horizon.auto>

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.

> 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
[ ... ]
> +static int cdns_xspi_nor_init(struct cdns_xspi_dev *cdns_xspi,
> +			      struct spi_nor *nor)
> +{
[ ... ]
> +	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?

[ ... ]
> +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?

[ ... ]
> +	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?

> +	}
> +
> +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.

> +
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
[ ... ]
> @@ -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.

> +		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