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

[sashiko-bot@kernel.org]

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.
- [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.
- [Medium] Uninitialized trailing DMA bursts are submitted to hardware if SG iteration breaks early.
- [Medium] Stale DMA residue is reported during a channel pause because `desc->done_burst` lags by one batch.

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