Re: [PATCH v5 05/10] dmaengine: dw-edma: Add helper dw_(edma|hdma)_v0_core_ch_enable()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Multiple dmaengine control functions (resume, pause, terminate_all) modify channel state flags and access descriptor lists without holding the required virt-dma lock (`chan->vc.lock`).
--

--- Patch [5]: [PATCH v5 05/10] dmaengine: dw-edma: Add helper dw_(edma|hdma)_v0_core_ch_enable() ---
commit def5321a113246514770ebf2362c6bd5549dc443
Author: Frank Li <Frank.Li@nxp.com>

dmaengine: dw-edma: Add helper dw_(edma|hdma)_v0_core_ch_enable()

Move the channel-enable logic into a new helper function,
dw_(edma|hdma)_v0_core_ch_enable(), in preparation for supporting dynamic
link entry additions. No functional changes.

> diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
> index c341aa5343417..8d38867cd9983 100644
> --- a/drivers/dma/dw-edma/dw-edma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c

[ ... ]

> @@ -366,74 +427,11 @@ static void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
>  {
>  	struct dw_edma_chan *chan = chunk->chan;
>  	struct dw_edma *dw = chan->dw;
> -	unsigned long flags;
> -	u32 tmp;
>  
>  	dw_edma_v0_core_write_chunk(chunk);
>  
> -	if (first) {
> -		/* Enable engine */
> -		SET_RW_32(dw, chan->dir, engine_en, BIT(0));

[Severity: High]
This isn't a bug introduced by this patch, but it appears multiple dmaengine
control functions (resume, pause, terminate_all) modify channel state flags
and access descriptor lists without holding the required virt-dma lock
(chan->vc.lock).

For example, dw_edma_start_transfer() (which ends up calling
dw_edma_v0_core_start()) is called locklessly from
drivers/dma/dw-edma/dw-edma-core.c:dw_edma_device_resume():

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

Could calling dw_edma_start_transfer() locklessly here lead to list
corruption, use-after-free, or a kernel oops?

This function evaluates vchan_next_desc(&chan->vc) to inspect vc->desc_issued
and modifies desc->chunk->list via list_del():

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_start_transfer() {
...
        vd = vchan_next_desc(&chan->vc);
        if (!vd)
                return 0;

        desc = vd2dw_edma_desc(vd);
...
        list_del(&child->list);
...
}

Concurrently, dw_edma_done_interrupt() or dw_edma_device_issue_pending()
on another CPU can acquire chan->vc.lock and execute list updates. Modifying
and iterating these lists without holding the protecting vc.lock during
concurrent modification structurally leads to list corruption.

A similar lockless state modification happens in dw_edma_device_pause():

        else
                chan->request = EDMA_REQ_PAUSE;

And also in dw_edma_device_terminate_all():

        else {
                chan->request = EDMA_REQ_STOP;
        }

Should these state updates and list accesses be protected by the virt-dma lock?

> -		if (dw->chip->mf == EDMA_MF_HDMA_COMPAT) {
> -			switch (chan->id) {
> -			case 0:

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-edma_ll-v5-0-e199053d4300@nxp.com?part=5