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

[Frank Li <Frank.li@oss.nxp.com>]

On Mon, Jul 13, 2026 at 05:22:02PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
>
> New issues:
> - [High] Replacing bounded chunk descriptors with a single contiguous GFP_NOWAIT allocation breaks large DMA transfers due to memory fragmentation.

It should be never happen, not such long list happen.

> - [High] Resuming a paused DMA channel causes an out-of-bounds array read if the active descriptor was fully completed at the time it was paused.

Pause problem, need more work

https://lore.kernel.org/dmaengine/20260710080903.2392888-5-den@valinux.co.jp/

> - [Medium] Uninitialized trailing DMA bursts are submitted to hardware if SG iteration breaks early.

Never happen, pre-alloc enough list.

> - [Medium] Stale DMA residue is reported during a channel pause because `desc->done_burst` lags by one batch.

about pause /resume existing code have problem. need fix later.

Frank

>
> Pre-existing issues:
> - [High] Missing `chan->vc.lock` acquisition in `dw_edma_device_resume` and `dw_edma_device_pause` leads to data races and virt-dma list corruption.
> --
>
> --- Patch [10]: [PATCH v7 10/10] dmaengine: dw-edma: Remove struct dw_edma_chunk ---
> commit ac4e11776f7ec8bd87ba4484bd079463058cc76d
> Author: Frank Li <Frank.Li@nxp.com>
>
> dmaengine: dw-edma: Remove struct dw_edma_chunk
>
> This patch flattens the DMA descriptor layout by moving the burst array
> directly into struct dw_edma_desc and removing the linked-list chunk layer.
> It uses start_burst and done_burst to track current hardware submissions.
>
> > diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> > index c028011cc61ca..30eeb7bffad80 100644
> > --- a/drivers/dma/dw-edma/dw-edma-core.c
> > +++ b/drivers/dma/dw-edma/dw-edma-core.c
>
> [ ... ]
>
> > @@ -40,82 +40,54 @@ u64 dw_edma_get_pci_address(struct dw_edma_chan *chan, phys_addr_t cpu_addr)
> > -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]
> Does allocating this unbounded descriptor array at once with GFP_NOWAIT
> introduce a risk of memory fragmentation failures?
>
> Large DMA transfers previously used bounded chunk allocations. With this
> change, could a large scatter-gather or interleaved transfer spuriously fail
> with -ENOMEM since GFP_NOWAIT cannot sleep or perform direct reclaim?
>
> >  	if (unlikely(!desc))
> >  		return NULL;
>
> [ ... ]
>
> > @@ -107,18 +79,27 @@ static void dw_edma_core_start(struct dw_edma_desc *desc, bool first)
> > -	for (i = 0; i < chunk->nburst; i++)
> > -		dw_edma_core_ll_data(chan, &chunk->burst[i], i, chunk->cb,
> > -				     i == chunk->nburst - 1);
> > +	for (i = 0; i + desc->start_burst < desc->nburst; i++) {
> > +		u32 idx = i + desc->start_burst;
> >
> > -	dw_edma_core_ll_link(chan, i, chunk->cb, chan->ll_region.paddr);
> > +		if (i == chan->ll_max)
> > +			break;
> > +
> > +		dw_edma_core_ll_data(chan, &desc->burst[idx],
> > +				     i, desc->cb,
> > +				     idx == desc->nburst - 1 || i == chan->ll_max - 1);
> > +	}
> > +
> > +	desc->done_burst = desc->start_burst;
> > +	desc->start_burst += i;
>
> [Severity: Medium]
> Will this cause stale residue to be reported during a channel pause?
>
> Because desc->done_burst is updated to match start_burst only when the next
> batch starts, it appears to lag behind the actual completion if the channel
> is paused immediately after an interrupt. This lagging index is then used by
> dw_edma_device_tx_status() for residue calculation.
>
> >
> >  	if (first)
> >  		dw_edma_core_ch_enable(chan);
>
> [ ... ]
>
> > @@ -355,13 +321,11 @@ static struct dma_async_tx_descriptor *
> >  		fsz = xfer->xfer.il->frame_size;
> >  	}
> >
> > +	desc = dw_edma_alloc_desc(chan, cnt);
> > +	if (unlikely(!desc))
> > +		return NULL;
> > +
> >  	for (i = 0; i < cnt; i++) {
> >  		if (xfer->type == EDMA_XFER_SCATTER_GATHER && !sg)
> >  			break;
> >
> > -		if (!(i % bursts_max)) {
> > -			u32 n = min(cnt - i, bursts_max);
> > -
> > -			chunk = dw_edma_alloc_chunk(desc, n);
> > -			if (unlikely(!chunk))
> > -				goto err_alloc;
> > -		}
> > -
> > -		burst = chunk->burst + (i % bursts_max);
> > +		burst = desc->burst + i;
>
> [Severity: Medium]
> If this loop breaks early due to !sg, is desc->nburst left un-updated?
>
> If desc->nburst retains the originally overestimated count,
> dw_edma_core_start() will loop up to the original nburst. Does this risk
> submitting uninitialized (zeroed) DMA bursts to hardware, potentially
> leading to IOMMU faults or hardware hangs?
>
> >
> >  		if (xfer->type == EDMA_XFER_CYCLIC)
> >  			burst->sz = xfer->xfer.cyclic.len;
>
> [ ... ]
>
> > @@ -625,7 +574,7 @@ static void dw_edma_done_interrupt(struct dw_edma_chan *chan)
> >  		case EDMA_REQ_PAUSE:
> >  			chan->request = EDMA_REQ_NONE;
> >  			chan->status = EDMA_ST_PAUSE;
> >  			break;
>
> [Severity: High]
> If a descriptor completes exactly when the channel is paused, could this
> lead to an out-of-bounds array read upon resuming?
>
> If the PAUSE handler breaks out here without checking whether
> desc->start_burst >= desc->nburst, a fully completed descriptor remains on
> the list. When dw_edma_device_resume() is later called,
> dw_edma_start_transfer() would unconditionally fetch it and pass it to
> dw_edma_core_start(), which will access desc->burst[desc->start_burst].
> Since start_burst == nburst in this scenario, would this read past the end
> of the flexible array?
>
> [Severity: High]
> This is a pre-existing issue, but is it safe that dw_edma_device_pause()
> and dw_edma_device_resume() modify chan->request and traverse virt-dma
> descriptors without acquiring chan->vc.lock?
>
> Concurrent calls to dmaengine_pause() or dmaengine_resume() alongside the
> DMA completion interrupt could lead to data races and virt-dma descriptor
> list corruption.
>
> >
> >  		default:
> >  			break;
> >  		}
> >  	}
> >  	spin_unlock_irqrestore(&chan->vc.lock, flags);
> >  }
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260713-edma_ll-v7-0-6fb7498c901e@nxp.com?part=10