Re: [PATCH v7 09/10] dmaengine: dw-edma: Use burst array instead of linked list

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

On Mon, Jul 13, 2026 at 05:25:21PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
>
> New issues:
> - [High] Early break in scatter-gather loop leaves `chunk->nburst` incorrectly sized, causing uninitialized descriptors to be submitted to the hardware.
> - [Medium] A zero value for `bursts_max` due to small linked-list region sizes causes a fatal divide-by-zero exception.
> - [Medium] Replacing multiple small slab allocations with a single large flexible array allocation in atomic context risks memory allocation failures.
>
> Pre-existing issues:
> - [High] `dw_edma_device_resume` calls `dw_edma_start_transfer` without holding the channel lock, causing list corruption.
> --
>
> --- Patch [9]: [PATCH v7 09/10] dmaengine: dw-edma: Use burst array instead of linked list ---
> commit 7aeaeaa1123dc1b005fda28afac5f4dff692b62b
> Author: Frank Li <Frank.Li@nxp.com>
>
> dmaengine: dw-edma: Use burst array instead of linked list
>
> Creating a DMA descriptor requires at least three kzalloc() calls because
> each burst is allocated as a linked-list node. Since the number of bursts
> is already known when the descriptor is created, a linked list is not
> necessary. Allocate a burst array when creating each chunk to simplify the
> code and eliminate one kzalloc() call.
>
> Tested-by: Koichiro Den <den@valinux.co.jp>
> Tested-By: Devendra Verma <devendra.verma@amd.com>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
>
> > diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> > index f52d9fd18e573..c028011cc61ca 100644
> > --- a/drivers/dma/dw-edma/dw-edma-core.c
> > +++ b/drivers/dma/dw-edma/dw-edma-core.c
> [ ... ]
> > -static struct dw_edma_chunk *dw_edma_alloc_chunk(struct dw_edma_desc *desc)
> > +static struct dw_edma_chunk *dw_edma_alloc_chunk(struct dw_edma_desc *desc, u32 nburst)
> >  {
> >  	struct dw_edma_chan *chan = desc->chan;
> >  	struct dw_edma_chunk *chunk;
> >
> > -	chunk = kzalloc_obj(*chunk, GFP_NOWAIT);
> > +	chunk = kzalloc_flex(*chunk, burst, nburst, GFP_NOWAIT);
>
> [Severity: Medium]
> Does this single large flexible array allocation risk failing under memory
> pressure?
>
> If the hardware configuration results in a large linked-list region,
> bursts_max can exceed 2700. This turns what used to be many small 24-byte
> allocations into a high-order contiguous allocation in an atomic context
> with GFP_NOWAIT, which is highly likely to fail if memory is fragmented.

This is most likely never happen. It is should be wrong if such bigger
linked list by once trasfer.

>
> [ ... ]
> > @@ -166,23 +103,17 @@ static void vchan_free_desc(struct virt_dma_desc *vdesc)
> >  static void dw_edma_core_start(struct dw_edma_chunk *chunk, bool first)
> >  {
> >  	struct dw_edma_chan *chan = chunk->chan;
> [ ... ]
> > -	j = chunk->bursts_alloc;
> > -	list_for_each_entry(child, &chunk->burst->list, list) {
> > -		j--;
> > -		dw_edma_core_ll_data(chan, child, i++, chunk->cb, !j);
> > -	}
> > +	for (i = 0; i < chunk->nburst; i++)
> > +		dw_edma_core_ll_data(chan, &chunk->burst[i], i, chunk->cb,
> > +				     i == chunk->nburst - 1);
>
> [Severity: High]
> Can this loop submit uninitialized trailing descriptors to the hardware if
> the scatter-gather loop breaks early?
>
> In dw_edma_device_transfer(), if the SG list is exhausted before reaching
> cnt (which can happen if the original length was passed instead of the
> mapped length), the loop breaks out early.

This never happen because pre-alloc enough space it.

>
> Since chunk->nburst is pre-allocated and not reduced to the actual number
> of prepared elements, this dw_edma_core_start() loop will process the
> remaining zero-initialized elements.
>
> Because the completion interrupt flag is only set when
> i == chunk->nburst - 1, the valid elements won't trigger an interrupt,
> and the zero-initialized elements might cause a hardware hang.
>
> [ ... ]
> > @@ -206,14 +137,13 @@ static int dw_edma_start_transfer(struct dw_edma_chan *chan)
> >  	if (!desc)
> >  		return 0;
> >
> > -	child = list_first_entry_or_null(&desc->chunk->list,
> > +	child = list_first_entry_or_null(&desc->chunk_list,
> >  					 struct dw_edma_chunk, list);
>
> [Severity: High]
> This is a pre-existing issue, but does dw_edma_device_resume() call
> dw_edma_start_transfer() without holding the channel lock?
>
> When the dmaengine core invokes device_resume() without holding the
> virtual channel lock, dw_edma_device_resume() modifies the channel state
> and calls dw_edma_start_transfer().
>
> Because dw_edma_start_transfer() calls vchan_next_desc() and reads the
> chunk lists locklessly, this can race with concurrent interrupts
> (dw_edma_done_interrupt) or software calls (dw_edma_device_issue_pending)
> that legitimately take chan->vc.lock to modify these same lists.
>
> Can this lead to list corruption or a use-after-free?
>
> [ ... ]
> > @@ -530,15 +456,15 @@ dw_edma_device_transfer(struct dw_edma_transfer *xfer,
> >  		if (xfer->type == EDMA_XFER_SCATTER_GATHER && !sg)
> >  			break;
> >
> > -		if (chunk->bursts_alloc == bursts_max) {
> > -			chunk = dw_edma_alloc_chunk(desc);
> > +		if (!(i % bursts_max)) {
> > +			u32 n = min(cnt - i, bursts_max);
> > +
> > +			chunk = dw_edma_alloc_chunk(desc, n);
>
> [Severity: Medium]
> Does this modulo operation risk a divide-by-zero exception if bursts_max
> is zero?
>
> If the hardware or device tree configures a linked-list region size
> between 24 and 47 bytes, ll_max will evaluate to 0.

It is wrong dt config. dt region should be n*4k space. and next patch
remove bursts_max.

Frank

>
> This leaves bursts_max at 0, which would cause the kernel to panic when
> evaluating this modulo operation.
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260713-edma_ll-v7-0-6fb7498c901e@nxp.com?part=9