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

[sashiko-bot@kernel.org]

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.

[ ... ]
> @@ -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.

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.

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