Re: [PATCH v3 04/24] dmaengine: dw-edma: Make DMA link list work as a circular buffer

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

Pre-existing issues:
- [High] DMA_CYCLIC transfers do not loop continuously in hardware and execute only once, failing to implement the cyclic DMA API contract.
--

--- Patch [4]: [PATCH v3 04/24] dmaengine: dw-edma: Make DMA link list work as a circular buffer ---
commit 23381b9c15312609c44ea3df9e57409eb0f1026d
Author: Frank Li <[email protected]>

dmaengine: dw-edma: Make DMA link list work as a circular buffer

This commit modifies the driver to treat the DMA linked list as a circular
buffer rather than rebuilding it completely for every transfer. It
introduces helpers to manage ring space, resets the ring state upon
termination, and appends entries with alternating cycle bits.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 6a25a050b89c4..d7a8a43b71d64 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
[ ... ]
> @@ -67,30 +73,75 @@ static void vchan_free_desc(struct virt_dma_desc *vdesc)
[ ... ]
>  static void dw_edma_core_ll_start(struct dw_edma_desc *desc)
>  {
>  	struct dw_edma_chan *chan = desc->chan;
>  	size_t i;
> -	bool first = !desc->start_burst;
> +	u32 free;
> +
> +	free = dw_edma_core_get_free_num(chan);
> +	for (i = desc->start_burst; i < desc->nburst && free; i++, free--) {

[Severity: High]
This is a pre-existing issue, but since we are modifying the linked list to
act as a circular buffer, how does the driver handle DMA_CYCLIC transfers?

Looking at dw_edma_core_ll_start(), it programs the hardware ring linearly
for the requested bursts. Because the execution stops deterministically after
processing the appended bursts due to a cycle bit mismatch, won't the DMA
engine halt when it reaches the unwritten ring slots instead of looping
continuously?

Clients using dmaengine_prep_dma_cyclic() for continuous audio or circular
buffering might encounter unexpected halts after executing the requested
number of periods instead of continuous execution.

[ ... ]
> @@ -645,6 +708,8 @@ static void dw_edma_done_interrupt(struct dw_edma_chan *chan)
>  							    DMA_TRANS_NOERROR);
>  				list_del(&vd->node);
>  				vchan_cookie_complete(vd);
> +				if (!chan->non_ll)
> +					chan->ll_done = chan->ll_head;
>  			}

[Severity: High]
This is also a pre-existing issue, but does this interrupt completion path
break the cyclic DMA API contract?

When a client submits a cyclic transfer, the interrupt handler appears to
unconditionally complete and remove the descriptor once all bursts are
submitted. 

Calling vchan_cookie_complete() and list_del() here terminates the cyclic
transfer completely, freeing the descriptor and preventing any continuous
hardware looping.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4