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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Replacing chunked allocations with a single, unbounded array allocation causes a severe risk of `GFP_NOWAIT` failures for large transfers.
- [High] The device TX status calculation incorrectly subtracts the size of the uncompleted burst instead of the last completed burst, resulting in an under-reported residue.
- [High] Double subtraction of the link descriptor limit in `dw_edma_core_start()` incorrectly limits chunk sizes and causes a hardware hang for small link regions.

Pre-existing issues:
- [High] The functions `dw_edma_device_resume`, `dw_edma_device_pause`, and `dw_edma_device_terminate_all` modify channel states and access descriptor lists without holding `chan->vc.lock`.
--

--- Patch [10]: [PATCH v4 10/10] dmaengine: dw-edma: Remove struct dw_edma_chunk ---
commit 4d352ddf553a3f2f63c8953fcbcf205cb6c98a87
Author: Frank Li <Frank.Li@nxp.com>

dmaengine: dw-edma: Remove struct dw_edma_chunk

This commit removes the dw_edma_chunk layer to simplify the descriptor
layout, embedding the burst array directly into dw_edma_desc. It aims
to reduce the number of kzalloc calls when creating a DMA descriptor.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 01bee22fe3b3e..474debb53c470 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, u32 nburst)
>  {
>  	struct dw_edma_desc *desc;
>  
> -	desc = kzalloc_obj(*desc, GFP_NOWAIT);
> +	desc = kzalloc_flex(*desc, burst, nburst, GFP_NOWAIT);

[Severity: High]
Does this unbounded allocation risk failing under memory fragmentation?

The previous logic used bounded chunk sizes, but nburst is now passed directly
from the scatter-gather list length or interleaved frames in
dw_edma_device_transfer(), which could be thousands of entries. Allocating a
large contiguous array in an atomic context with GFP_NOWAIT seems highly prone
to failure for large transfers.

>  	if (unlikely(!desc))
>  		return NULL;
[ ... ]
> -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)
>  {
[ ... ]
> +	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 - 1)
> +			break;

[Severity: High]
Does this double-subtract the link descriptor limit?

chan->ll_max is initialized elsewhere as (sz / EDMA_LL_SZ) - 1 to account for
the link descriptor. If chan->ll_max is 1, breaking at i == 0 submits a batch
with 0 data elements. Could this immediately complete and fire an interrupt,
potentially triggering an infinite loop of 0-length transfers?

> +
> +		dw_edma_core_ll_data(chan, &desc->burst[idx],
> +				     i, desc->cb,
> +				     idx == desc->nburst - 1 || i == chan->ll_max - 2);
> +	}
[ ... ]
> @@ -337,8 +301,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: High]
Should this calculation use desc->done_burst - 1 instead of desc->done_burst?

desc->done_burst holds the index of the first burst currently executing in
hardware. Subtracting desc->burst[desc->done_burst].xfer_sz includes the size
of an unfinished burst, which under-reports the residue. This differs from
dw_hdma_set_callback_result() which uses desc->done_burst - 1.

>  	}
>  	spin_unlock_irqrestore(&chan->vc.lock, flags);
[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but looking at
dw_edma_device_resume() and dw_edma_device_pause() in
drivers/dma/dw-edma/dw-edma-core.c:

dw_edma_device_resume()
    chan->status = EDMA_ST_BUSY;
    dw_edma_start_transfer(chan);

dw_edma_device_pause()
    chan->request = EDMA_REQ_PAUSE;

Can modifying the channel state and calling dw_edma_start_transfer() (which
accesses vc->desc_issued) locklessly cause data races?

Other functions like dw_edma_device_issue_pending() and
dw_edma_done_interrupt() protect these structures with
spin_lock_irqsave(&chan->vc.lock, flags).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-edma_ll-v4-0-cc128f0afb61@nxp.com?part=10