Re: [PATCH v3 07/10] dmaengine: dw-edma: Add non_ll_start() callback

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Lockless channel state modification and list manipulation in dmaengine callbacks.
--

commit 962243519d7014f0b711fe65a08901734b5c7e22
Author: Frank Li <Frank.Li@nxp.com>

dmaengine: dw-edma: Add non_ll_start() callback

This commit adds a non_ll_start() callback and moves the common
non-linked-list channel handling into the EDMA core so it can be shared
by both the EDMA and HDMA.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.h b/drivers/dma/dw-edma/dw-edma-core.h
> index bab4d49c92feb..e18d6e827c2c9 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.h
> +++ b/drivers/dma/dw-edma/dw-edma-core.h

[ ... ]

> @@ -201,7 +202,16 @@ dw_edma_core_handle_int(struct dw_edma_irq *dw_irq, enum dw_edma_dir dir,
>  static inline
>  void dw_edma_core_start(struct dw_edma *dw, struct dw_edma_chunk *chunk, bool first)
>  {
> -	dw->core->start(chunk, first);
> +	if (chunk->chan->non_ll) {
> +		struct dw_edma_burst *child;
> +
> +		child = list_first_entry_or_null(&chunk->burst->list,
> +						 struct dw_edma_burst, list);
> +		if (child)
> +			dw->core->non_ll_start(chunk->chan, child);
> +	} else {
> +		dw->core->start(chunk, first);
> +	}
>  }

[Severity: High]
This is a pre-existing issue, but does dw_edma_device_resume() modify
channel state and invoke dw_edma_start_transfer() locklessly?

Looking at drivers/dma/dw-edma/dw-edma-core.c, if a client driver invokes
dmaengine_resume() (which calls dw_edma_device_resume()) concurrently with
the channel's interrupt handler dw_edma_done_interrupt() or dmaengine_submit(),
dw_edma_device_resume() modifies chan->status and calls
dw_edma_start_transfer() without acquiring chan->vc.lock:

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

Meanwhile, dw_edma_done_interrupt() and dw_edma_device_issue_pending()
execute dw_edma_start_transfer() with chan->vc.lock held.

If dw_edma_device_resume() and dw_edma_done_interrupt() run concurrently,
both can invoke dw_edma_start_transfer() on the same channel, read the same
descriptor via vchan_next_desc(&chan->vc), and perform state modifications on
it:

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_start_transfer() {
    ...
    vd = vchan_next_desc(&chan->vc);
    ...
    list_del(&child->list);
    kfree(child);
    ...
}

Could this sequence lead to a double-free and list corruption?

Other functions like dw_edma_device_pause() and dw_edma_device_terminate_all()
appear to similarly manipulate channel state locklessly:

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_device_pause() {
    ...
    chan->request = EDMA_REQ_PAUSE;
    ...
}

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