Re: [PATCH v7 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 3 potential issue(s) to consider:

Pre-existing issues:
- [High] The dmaengine API functions `dw_edma_device_resume`, `dw_edma_device_pause`, and `dw_edma_device_terminate_all` modify shared channel state and process descriptor lists without holding the `chan->vc.lock`.
- [High] The driver improperly handles `DMA_CYCLIC` transfers by treating them as finite transfers that complete after `cnt` periods, rather than running continuously. It calls `vchan_cookie_complete()` instead of `vchan_cyclic_callback()`, causing the transfer to terminate and the descriptor to be freed.
- [High] The `dw_edma_device_terminate_all()` function fails to remove and free pending descriptors from the virt-dma queues. It only sets a state flag (`EDMA_REQ_STOP`) for the active transfer without flushing the `desc_submitted` or `desc_issued` lists.
--

--- Patch [2]: [PATCH v7 02/10] dmaengine: dw-edma: Add xfer_sz field to struct dw_edma_chunk ---
commit 1deb608bf55aafc03752bddfec980c583d0db45e
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 struct dw_edma_chunk to explicitly
track the total transfer size. It replaces the misleading use of
ll_region.sz, which represents the memory size of the EDMA link list rather
than the data payload. The core start and setup functions are updated to
use this new field to correctly calculate transfer sizes.

Tested-by: Koichiro Den <den@valinux.co.jp>
Tested-By: Devendra Verma <devendra.verma@amd.com>
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 since dw_edma_start_transfer() is being
modified here, is it safe to call this function without holding
chan->vc.lock?

When a client driver calls dmaengine_resume(), it invokes
dw_edma_device_resume(), which directly calls dw_edma_start_transfer()
without acquiring the lock:

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

Inside dw_edma_start_transfer(), vchan_next_desc() is called, which has a
strict lockdep_assert_held(&vc->lock) requirement. Since
dw_edma_device_pause() and dw_edma_device_terminate_all() also modify
shared channel state without the lock, could this lead to list corruption
or use-after-free when competing with the hardirq handler
(dw_edma_done_interrupt) that does hold the lock?

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

[ ... ]

[Severity: High]
This is a pre-existing issue, but does dw_edma_device_terminate_all()
leave stale descriptors in the virt-dma queues?

When termination happens, the driver sets a state flag but skips flushing
the lists:

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_device_terminate_all() {
        ...
        } else if (chan->status == EDMA_ST_PAUSE) {
                chan->request = EDMA_REQ_STOP;
        }
        ...
}

Without calling vchan_get_all_descriptors() and vchan_dma_desc_free_list(),
if a client calls dmaengine_terminate_all() while descriptors are pending
and then reuses the channel, will those old descriptors be erroneously
executed and cause data corruption?

[Severity: High]
This is a pre-existing issue, but does the DMA_CYCLIC implementation
correctly support continuous hardware looping?

In dw_edma_device_prep_dma_cyclic(), the driver blindly accepts the cyclic
request and sets a finite count:

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_device_prep_dma_cyclic() {
        ...
        xfer.xfer.cyclic.cnt = count;
        ...
}

Later, the interrupt handler completes the descriptor instead of
triggering periodic callbacks:

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_done_interrupt() {
        ...
                        list_del(&vd->node);
                        vchan_cookie_complete(vd);
        ...
}

Should the interrupt handler call vchan_cyclic_callback(vd) so that
continuous audio or capture buffers don't incorrectly halt after the
initial count iterations?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713-edma_ll-v7-0-6fb7498c901e@nxp.com?part=2