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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] The loop in `dw_edma_core_start` uses an incorrect upper bound for multi-chunk DMA transfers, causing out-of-bounds heap slab reads, invalid DMA programming, and missing completion interrupts.
- [Medium] Incorrect DMA residue calculation in `dw_edma_device_tx_status` under-reports remaining bytes due to an off-by-one array index.
- [Low] Dead fields remain in `struct dw_edma_desc` after the removal of the chunk layer.
--

commit bff8cdb94c387e71cb6eb50f2573846d0ceb45ec
Author: Frank Li <Frank.Li@nxp.com>

dmaengine: dw-edma: Remove struct dw_edma_chunk

This commit removes the intermediate linked-list layer (struct dw_edma_chunk)
from the DMA descriptor layout. It moves the burst array directly into
struct dw_edma_desc and uses start_burst and done_burst to track progress.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
[ ... ]
> -static void dw_edma_core_start(struct dw_edma_chunk *chunk, bool first)
> +static void dw_edma_core_start(struct dw_edma_desc *desc, bool first)
>  {
> -	struct dw_edma_chan *chan = chunk->chan;
> +	struct dw_edma_chan *chan = desc->chan;
>  	u32 i = 0;
>  
>  	if (chan->non_ll) {
> -		if (chunk->nburst == 1)
> -			chan->dw->core->non_ll_start(chunk->chan, &chunk->burst[0]);
> +		chan->dw->core->non_ll_start(chan, &desc->burst[desc->start_burst]);
> +		desc->done_burst = desc->start_burst;
> +		desc->start_burst += 1;
>  		return;
>  	}
>  
> -	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->nburst; i++) {
> +		if (i == chan->ll_max - 1)
> +			break;
> +
> +		dw_edma_core_ll_data(chan, &desc->burst[i + desc->start_burst],
> +				     i, desc->cb,
> +				     i == desc->nburst - 1 || i == chan->ll_max - 2);
> +	}

[Severity: Critical]
Is the loop bound desc->nburst correct when resuming a partially completed
transfer?

For a DMA transfer where the total bursts exceed the hardware chunk limit
(chan->ll_max - 1), desc->start_burst advances after the first chunk. On
subsequent chunks, the loop limit doesn't account for the bursts already
processed. This allows the index calculation i + desc->start_burst to
exceed the size of the allocated desc->burst array, resulting in an
out-of-bounds slab read.

Additionally, does the terminal IRQ condition i == desc->nburst - 1 ever
evaluate to true on the final chunk? Since i is a relative chunk index and
desc->nburst is the absolute total, this could prevent the final completion
interrupt from firing.

> +
> +	desc->done_burst = desc->start_burst;
> +	desc->start_burst += i;
>  
> -	dw_edma_core_ll_link(chan, i, chunk->cb, chan->ll_region.paddr);
> +	dw_edma_core_ll_link(chan, i, desc->cb, chan->ll_region.paddr);
>  
>  	if (first)
>  		dw_edma_core_ch_enable(chan);
[ ... ]
> @@ -337,8 +299,10 @@ dw_edma_device_tx_status(struct dma_chan *dchan, dma_cookie_t cookie,
>  	vd = vchan_find_desc(&chan->vc, cookie);
>  	if (vd) {
>  		desc = vd2dw_edma_desc(vd);
> -		if (desc)
> -			residue = desc->alloc_sz - desc->xfer_sz;
> +
> +		residue = desc->alloc_sz;
> +		if (desc && desc->done_burst)
> +			residue -= desc->burst[desc->done_burst].xfer_sz;

[Severity: Medium]
Could this residue calculation under-report the remaining bytes?

When querying the status of a partially complete multi-chunk transfer
(desc->done_burst > 0), this subtracts the cumulative transfer size at
desc->burst[desc->done_burst].xfer_sz. This array index actually holds the
transfer size including the first burst of the currently executing chunk.

Should this use desc->done_burst - 1 instead, to subtract only the bytes
from fully completed chunks?

>  	}
>  	spin_unlock_irqrestore(&chan->vc.lock, flags);
>  
[ ... ]
> diff --git a/drivers/dma/dw-edma/dw-edma-core.h b/drivers/dma/dw-edma/dw-edma-core.h
> --- a/drivers/dma/dw-edma/dw-edma-core.h
> +++ b/drivers/dma/dw-edma/dw-edma-core.h
> @@ -66,6 +59,12 @@ struct dw_edma_desc {
>  
>  	u32				alloc_sz;
>  	u32				xfer_sz;
> +
> +	u32				done_burst;
> +	u32				start_burst;
> +	u8				cb;
> +	u32				nburst;
> +	struct dw_edma_burst            burst[] __counted_by(nburst);
>  };

[Severity: Low]
Since the chunk linked-list layer was removed in this commit, are the
chunk_list and chunks_alloc fields still necessary in struct dw_edma_desc?

It appears xfer_sz might also be unused now that the previous transfer size
tracking was removed from dw_edma_start_transfer().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702-edma_ll-v3-0-877aa463740c@nxp.com?part=10