Re: [PATCH v5 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 2 potential issue(s) to consider:
- [High] The patch breaks DMA transfers in non-linked-list (`non_ll`) mode for multi-burst transfers by ignoring `bursts_max` and erroneously grouping multiple bursts into a single chunk.
- [High] Premature scatter-gather loop termination leaves trailing zero-initialized bursts that are blindly submitted to the DMA hardware.
--

--- Patch [9]: [PATCH v5 09/10] dmaengine: dw-edma: Use burst array instead of linked list ---
commit e4c02e28044af675e21dc05442589d1431ab18b6
Author: Frank Li <Frank.Li@nxp.com>

dmaengine: dw-edma: Use burst array instead of linked list

The commit replaces the linked list used for DMA descriptor bursts with a
pre-allocated burst array inside each chunk. This simplifies the descriptor
layout and eliminates multiple kzalloc() calls.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index f52d9fd18e573..01bee22fe3b3e 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
> @@ -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;
> -	struct dw_edma_burst *child;
>  	u32 i = 0;
> -	int j;
>  
>  	if (chan->non_ll) {
> -		child = list_first_entry_or_null(&chunk->burst->list,
> -						 struct dw_edma_burst, list);
> -		if (child)
> -			chan->dw->core->non_ll_start(chunk->chan, child);
> +		if (chunk->nburst == 1)
> +			chan->dw->core->non_ll_start(chunk->chan, &chunk->burst[0]);

[Severity: High]
If a client submits a transfer with cnt > 1 on a non-LL channel, the allocation
logic below sets nburst > 1.

When nburst > 1, this condition fails and the DMA hardware is never started.

Does this silently drop non-linked-list multi-burst transfers?

>  		return;
>  	}
>  
> -	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]
In dw_edma_device_transfer(), if the scatter-gather chain finishes early, the
loop breaks.

However, chunk->nburst is not updated to reflect the actual populated count.

This causes dw_edma_core_start() to iterate over and submit the trailing
zero-initialized descriptors (sz 0, sar 0, dar 0).

Could this loop submit uninitialized descriptors to the hardware?

[ ... ]

> @@ -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 % chan->ll_max)) {
> +			u32 n = min(cnt - i, chan->ll_max);
> +
> +			chunk = dw_edma_alloc_chunk(desc, n);

[Severity: High]
For non-LL mode transfers, using chan->ll_max instead of bursts_max causes
chunks to be allocated with a larger nburst than the expected bursts_max = 1.

This leads to the hardware start routine silently dropping the transfer.

Is it intentional to use chan->ll_max here instead of bursts_max?

>  			if (unlikely(!chunk))
>  				goto err_alloc;
>  		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-edma_ll-v5-0-e199053d4300@nxp.com?part=9