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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Resource Leak of DMA Channel on probe failure
- [Medium] Missing dma_map_resource for AHB physical address
- [Low] Build Failure: 64-bit modulo by 32-bit variable on 32-bit platforms

Pre-existing issues:
- [High] Data Truncation of physical memory-mapped address
--

--- Patch [2]: [PATCH v2 2/3] spi: nxp-xspi: Improve AHB read performance with DMA ---
commit e55bddd05b9c54ad3776775a32876cb677ace5d2
Author: Han Xu <han.xu@nxp.com>

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

Use eDMA for XSPI AHB read to improve the performance.

> diff --git a/drivers/spi/spi-nxp-xspi.c b/drivers/spi/spi-nxp-xspi.c
> index 037eac24e6fd2..0658c0557ff17 100644
> --- a/drivers/spi/spi-nxp-xspi.c
> +++ b/drivers/spi/spi-nxp-xspi.c
[ ... ]
> @@ -796,6 +807,82 @@ 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);
> +
> +	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,
> +					 op->data.nbytes,
> +					 DMA_PREP_INTERRUPT | DMA_CTRL_ACK);

[Severity: High]
This is a pre-existing issue, but since memmap_phy is declared as a u32 in
struct nxp_xspi, will physical addresses located above the 4GB boundary be
silently truncated to 32 bits? If so, this addition to op->addr.val might
cause the DMA controller to access the wrong physical memory.

[Severity: Medium]
If the eDMA controller operates behind an IOMMU that does not identity-map
this MMIO region, will this raw physical address trigger an IOMMU translation
fault?

Should xspi->memmap_phy + op->addr.val be mapped via dma_map_resource()
to obtain a dma_addr_t before being passed into dmaengine_prep_dma_memcpy()?

> +	if (!desc) {
> +		dev_err(xspi->dev, "failed to prepare AHB RX DMA descriptor\n");
> +		ret = -EIO;
> +		goto err_unmap;
> +	}
[ ... ]
> @@ -1029,6 +1127,7 @@ static int nxp_xspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>  static int nxp_xspi_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
>  {
[ ... ]
> +		/*
> +		 * For AHB DMA read, align the transfer to the DMA alignment
> +		 * boundaries. If the start address is unaligned, shorten this
> +		 * transfer so the next one starts on an aligned boundary.
> +		 * Otherwise, if the length is unaligned, round it down.
> +		 */
> +		if (xspi->dma_rx && !needs_ip_only(xspi) &&
> +		    (op->data.nbytes >= xspi->devtype_data->ahb_buf_size)) {
> +			if (op->addr.val % alignment)
> +				op->data.nbytes =
> +					ALIGN(op->addr.val, alignment) - op->addr.val;

[Severity: Low]
Because op->addr.val is a 64-bit integer (u64) and alignment is a 32-bit
integer, will this modulo operation cause an unresolved symbol error for
__umoddi3 or __aeabi_uldivmod during the final link stage on 32-bit
architectures?

> +			else if (op->data.nbytes % alignment)
> +				op->data.nbytes =
> +					ALIGN_DOWN(op->data.nbytes, alignment);
> +		}
>  	}
>  
>  	return 0;
[ ... ]
> @@ -1252,6 +1371,11 @@ static int nxp_xspi_probe(struct platform_device *pdev)
>  	if (irq < 0)
>  		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);
>  	ret = devm_pm_runtime_enable(dev);

[Severity: High]
Since nxp_xspi_cleanup() is registered via devm_add_action_or_reset() at the
end of nxp_xspi_probe():

    init_completion(&xspi->c);
    ret = devm_request_irq(dev, irq,
            nxp_xspi_irq_handler, 0, pdev->name, xspi);
    ...
    ret = devm_add_action_or_reset(dev, nxp_xspi_cleanup, xspi);

what happens if devm_pm_runtime_enable() (or any other initialization step
before the cleanup action is registered) fails? Will the DMA channel
allocated in nxp_xspi_dma_init() leak on these early error returns?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-xspi_dma_upstream-v2-0-9e19d80e7762@nxp.com?part=2