Re: [PATCH v3 08/10] dmaengine: dw-edma: Use common dw_edma_core_start() for both eDMA and HDMA

[sashiko-bot@kernel.org]

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

New issues:
- [Low] The implementations of the `.ll_link` callback silently ignore the `addr` parameter.

Pre-existing issues:
- [High] dw_edma_device_resume calls dw_edma_start_transfer without holding the virtual channel lock, leading to a data race.
--

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

dmaengine: dw-edma: Use common dw_edma_core_start() for both eDMA and HDMA

The commit removes the .start() callback functions for eDMA and HDMA
and consolidates them by using a common dw_edma_core_start() function
for both engine types.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 2652ad8e7a8f6..f52d9fd18e573 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
> @@ -163,9 +163,37 @@ static void vchan_free_desc(struct virt_dma_desc *vdesc)
>  	dw_edma_free_desc(vd2dw_edma_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);
> +		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);
> +	}
> +
> +	dw_edma_core_ll_link(chan, i, chunk->cb, chan->ll_region.paddr);

[Severity: Low]
Does the underlying implementation of .ll_link actually use the address
parameter passed here?

Looking at dw_edma_v0_core_ll_link() in dw-edma-v0-core.c, it appears to
ignore the addr parameter entirely and hardcodes chan->ll_region.paddr:

static void
dw_edma_v0_core_ll_link(struct dw_edma_chan *chan, u32 idx, bool cb, u64 addr)
{
	...
	dw_edma_v0_write_ll_link(chan, idx, control, chan->ll_region.paddr);
}

The same seems to be true for dw_hdma_v0_core_ll_link(). While it currently
works because chan->ll_region.paddr is explicitly passed, would this cause
silent failures for any future callers attempting to pass a different address?

> +
> +	if (first)
> +		dw_edma_core_ch_enable(chan);
> +
> +	dw_edma_core_ch_doorbell(chan);
> +}
> +
>  static int dw_edma_start_transfer(struct dw_edma_chan *chan)
>  {

[Severity: High]
This is a pre-existing issue, but does dw_edma_start_transfer() race with
other contexts when called from dw_edma_device_resume()?

In dw_edma_device_resume(), dw_edma_start_transfer() is called without holding
the virtual channel lock:

static int dw_edma_device_resume(struct dma_chan *dchan)
{
	...
	} else {
		chan->status = EDMA_ST_BUSY;
		dw_edma_start_transfer(chan);
	}

	return err;
}

However, dw_edma_start_transfer() subsequently accesses the virtual channel
list through vchan_next_desc(), which expects chan->vc.lock to be held. Can
this lead to list corruption or data races if another thread concurrently
calls dw_edma_device_issue_pending() or if an interrupt is being processed?

> -	struct dw_edma *dw = chan->dw;
>  	struct dw_edma_chunk *child;
>  	struct dw_edma_desc *desc;
>  	struct virt_dma_desc *vd;

[ ... ]

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