Re: [PATCH v3 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:

New issues:
- [High] Early loop termination during scatter-gather mapping leaves trailing bursts uninitialized, leading to invalid DMA hardware configuration and silent hangs.

Pre-existing issues:
- [High] Missing spinlock in dw_edma_device_resume() leads to list corruption, data races, and lockdep warnings.
--

commit a2364f066e8539a3a2c9d2afab52db8336748992
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. Allocate a burst array when
creating each chunk to simplify the code and eliminate one kzalloc() call.

> 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]);
>  		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]
Does this loop iterate over uninitialized bursts if the scatter-gather
loop in dw_edma_device_transfer() terminates early?

In dw_edma_device_transfer(), if the loop breaks early because !sg,
chunk->nburst remains at its originally allocated count rather than the
actual number of initialized elements.

Since the irq flag is only set on the very last element
(i == chunk->nburst - 1), and that element might be uninitialized, could
this program the DMA hardware with zero-sized transfers and cause a
silent hang?

>  
>  	dw_edma_core_ll_link(chan, i, chunk->cb, chan->ll_region.paddr);
>  
[ ... ]
> @@ -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);
>  			if (unlikely(!chunk))
>  				goto err_alloc;
>  		}

[Severity: High]
This is a pre-existing issue, but looking at dw_edma_device_resume(),
it appears to modify state and access lists without taking the channel's
virtual spinlock:

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_device_resume() {
    ...
	} else {
		chan->status = EDMA_ST_BUSY;
		dw_edma_start_transfer(chan);
	}
    ...
}

Since dw_edma_start_transfer() eventually calls vchan_next_desc() which
expects lockdep_assert_held(&vc->lock), and hardware interrupts could fire
simultaneously calling dw_edma_done_interrupt(), can this lead to list
corruption and data races?

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