Re: [PATCH v5 02/10] dmaengine: dw-edma: Add xfer_sz field to struct dw_edma_chunk

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] DMA API callbacks (`dw_edma_device_resume`, `dw_edma_device_pause`, `dw_edma_device_terminate_all`) execute without acquiring the virtual channel lock (`chan->vc.lock`), causing data races, lockdep assertions, and potential list corruption.
--

--- Patch [2]: [PATCH v5 02/10] dmaengine: dw-edma: Add xfer_sz field to struct dw_edma_chunk ---
commit b54484e8a9f1314d4de2bba9458c74895f047059
Author: Frank Li <Frank.Li@nxp.com>

dmaengine: dw-edma: Add xfer_sz field to struct dw_edma_chunk

This commit adds a new xfer_sz field to explicitly indicate the total
transfer size of a chunk, replacing the misleading use of ll_region.sz.

Tested-by: Koichiro Den <den@valinux.co.jp>
Signed-off-by: Frank Li <Frank.Li@nxp.com>

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 1fec1b52e3d47..53469c8c8b82e 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
> @@ -192,7 +192,7 @@ static int dw_edma_start_transfer(struct dw_edma_chan *chan)
>  		return 0;
>  
>  	dw_edma_core_start(dw, child, !desc->xfer_sz);
> -	desc->xfer_sz += child->ll_region.sz;
> +	desc->xfer_sz += child->xfer_sz;

[Severity: High]
This is a pre-existing issue, but does dw_edma_start_transfer() have a lock
safety problem when called from dw_edma_device_resume()?

Looking at dw_edma_device_resume(), it modifies chan->status and calls
dw_edma_start_transfer() locklessly:

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() manipulates the virt-dma lists and eventually
calls vchan_next_desc(&chan->vc) which has a lockdep_assert_held(&vc->lock),
this would trigger a lockdep splat.

Additionally, is there a risk of list corruption or a double free here if
this races with dw_edma_done_interrupt() or dw_edma_device_issue_pending()?
If device_resume races with dw_edma_done_interrupt, both might acquire the
same chunk and call kfree(child).

There appear to be similar missing locks in dw_edma_device_pause() and
dw_edma_device_terminate_all() when modifying shared internal state
variables like chan->status and chan->request locklessly.

>  	dw_edma_free_burst(child);
>  	list_del(&child->list);
>  	kfree(child);

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