Re: [PATCH v6 10/10] dmaengine: dw-edma: Remove struct dw_edma_chunk

[sashiko-bot@kernel.org]

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

New issues:
- [High] Refactoring from bounded chunk arrays to a monolithic descriptor array introduces unbounded high-order atomic memory allocations that are prone to failure under fragmentation.

Pre-existing issues:
- [High] Pre-existing locking violation: `dw_edma_device_resume` invokes `dw_edma_start_transfer` without acquiring the virtual channel lock.
--

--- Patch [10]: [PATCH v6 10/10] dmaengine: dw-edma: Remove struct dw_edma_chunk ---
commit 04c138d15d0c47167bf151873232daba3b9881ad
Author: Frank Li <Frank.Li@nxp.com>
Subject: dmaengine: dw-edma: Remove struct dw_edma_chunk

This commit simplifies the DMA descriptor layout by removing the
dw_edma_chunk linked-list layer. The burst array is now allocated
directly within dw_edma_desc to reduce the number of allocations
required per descriptor.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 202862a828b4d..30eeb7bffad80 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c

[ ... ]

> -static struct dw_edma_desc *dw_edma_alloc_desc(struct dw_edma_chan *chan)
> +static struct dw_edma_desc *
> +dw_edma_alloc_desc(struct dw_edma_chan *chan, size_t nburst)
>  {
>  	struct dw_edma_desc *desc;
>  
> -	desc = kzalloc_obj(*desc, GFP_NOWAIT);
> +	desc = kzalloc_flex(*desc, burst, nburst, GFP_NOWAIT);

[Severity: High]
Will this monolithic allocation approach cause large DMA transfers to fail?

In dw_edma_device_transfer(), the total burst count is passed directly here
as nburst without chunking. For large scatter-gather transfers, nburst could
be very large.

Since GFP_NOWAIT cannot sleep or reclaim memory, won't these large atomic
allocations fail reliably under memory fragmentation?

>  	if (unlikely(!desc))
>  		return NULL;
>  
>  	desc->chan = chan;
> -
> -	INIT_LIST_HEAD(&desc->chunk_list);
> +	desc->nburst = nburst;
> +	desc->cb = true;

[ ... ]

>  static int dw_edma_start_transfer(struct dw_edma_chan *chan)
>  {
> -	struct dw_edma_chunk *child;
>  	struct dw_edma_desc *desc;
>  	struct virt_dma_desc *vd;
>  
>  	vd = vchan_next_desc(&chan->vc);

[Severity: High]
This is a pre-existing issue, but is it safe for dw_edma_device_resume()
to call dw_edma_start_transfer() without holding the virtual channel lock?

dw_edma_start_transfer() calls vchan_next_desc() here, which actively unlinks
entries from the list.

If dw_edma_device_resume() bypasses spin_lock_irqsave(&chan->vc.lock) and a
concurrent hardware interrupt occurs, could this corrupt the virtual channel
lists?

>  	if (!vd)
>  		return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-edma_ll-v6-0-1471d278b73a@nxp.com?part=10