Re: [PATCH 2/3] spi: nxp-xspi: Improve AHB read performance with DMA

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Calling dma_map_single() directly on op->data.buf.in without verifying it is a linearly mapped virtual address causes kernel panics or silent memory corruption with vmalloc buffers.
- [High] Directly passing the memory-mapped physical address as a DMA address to the DMA engine bypasses the DMA API and causes IOMMU translation faults.
- [Medium] The DMA channel allocated for RX is permanently leaked if the probe function encounters an error before the cleanup action is registered.
--

commit 8ae000beff392fc0d98a1b38fb68f9efe125110e
Author: Han Xu <han.xu@nxp.com>

spi: nxp-xspi: Improve AHB read performance with DMA

This commit introduces eDMA support for XSPI AHB reads to improve
performance. It falls back to CPU reads if DMA is unavailable or for
small transfers.

> diff --git a/drivers/spi/spi-nxp-xspi.c b/drivers/spi/spi-nxp-xspi.c
> index 385302a6e62f2..fd9140da6755a 100644
> --- a/drivers/spi/spi-nxp-xspi.c
> +++ b/drivers/spi/spi-nxp-xspi.c
[ ... ]
> @@ -799,6 +805,78 @@ static int nxp_xspi_ahb_read(struct nxp_xspi *xspi, const struct spi_mem_op *op)
[ ... ]
> +static int nxp_xspi_ahb_dma_read(struct nxp_xspi *xspi,
> +				 const struct spi_mem_op *op)
> +{
> +	struct dma_async_tx_descriptor *desc;
> +	struct dma_chan *chan = xspi->dma_rx;
> +	unsigned long timeout;
> +	dma_addr_t dma_addr;
> +	int ret = 0;
> +
> +	dma_addr = dma_map_single(chan->device->dev,
> +				  op->data.buf.in, op->data.nbytes,
> +				  DMA_FROM_DEVICE);

[Severity: High]
How does this code handle vmalloc addresses?

SPI memory operations (e.g., via MTD/UBIFS) frequently pass large vmalloc'd
buffers in op->data.buf.in. Calling dma_map_single() on a vmalloc address
causes virt_to_phys() to produce an invalid physical address, leading to a
kernel panic or silent memory corruption.

Since dmaengine_prep_dma_memcpy() cannot accept an sg_table, should the
driver verify !is_vmalloc_addr(op->data.buf.in) and fall back to CPU read?

> +
> +	if (dma_mapping_error(chan->device->dev, dma_addr)) {
> +		dev_err(xspi->dev, "failed to map DMA buffer for AHB read\n");
> +		return -ENOMEM;
> +	}
> +
> +	desc = dmaengine_prep_dma_memcpy(chan, dma_addr,
> +					 xspi->memmap_phy + op->addr.val,

[Severity: High]
Is it safe to pass the memory-mapped physical address directly to the DMA
engine descriptor without mapping it first?

The driver computes the source address as xspi->memmap_phy + op->addr.val
and passes it directly as a dma_addr_t. Bypassing the DMA API (such as
dma_map_resource()) means the physical address is never mapped into the
DMA controller's IOVA space. This breaks on platforms where the eDMA
operates behind an IOMMU.

> +					 op->data.nbytes,
> +					 DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
[ ... ]
> @@ -1254,7 +1359,12 @@ static int nxp_xspi_probe(struct platform_device *pdev)
>  	/* Find the irq */
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0)
> -		return dev_err_probe(dev, irq,  "Failed to get irq source");
> +		return dev_err_probe(dev, irq, "Failed to get irq source");
> +
> +	/* DMA is optional, failure(other than -EPROBE_DEFER) falls back to CPU */
> +	ret = nxp_xspi_dma_init(dev, xspi);
> +	if (ret == -EPROBE_DEFER)
> +		return ret;
>  
>  	pm_runtime_set_autosuspend_delay(dev, XSPI_RPM_TIMEOUT_MS);
>  	pm_runtime_use_autosuspend(dev);

[Severity: Medium]
Could this leak the DMA channel if an error occurs later in the probe
sequence?

nxp_xspi_dma_init() allocates an unmanaged DMA channel via
dma_request_chan(), but the teardown logic using devm_add_action_or_reset()
isn't registered until much later in the probe sequence. If intermediate
steps fail (such as devm_pm_runtime_enable()), it appears the probe returns
early without releasing the DMA channel.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617215520.3327836-1-han.xu@oss.nxp.com?part=2