Re: [PATCH v6 2/5] dmaengine: dw-edma: Reclaim LL entries from hardware progress

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] dmaengine: dw-edma: callbacks scheduled on STOP in dw_edma_ll_advance()
--

commit cb11666c731e633531785427e3b24bbfbd73eb2b
Author: Koichiro Den <den@valinux.co.jp>

dmaengine: dw-edma: Reclaim LL entries from hardware progress

This commit updates the linked-list completion handling to use DMA_LLP to
advance ll_done and reclaim completed ring entries in issue order. It also
defers STOP and PAUSE completion until the channel stops.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index b08bb31657c9e..74613c95ae112 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c

[ ... ]

> @@ -283,6 +345,45 @@ static void dw_edma_core_ch_maybe_doorbell(struct dw_edma_chan *chan)
>  	dw_edma_core_ch_doorbell(chan);
>  }
>  
> +/*
> + * Must be called with vc.lock held.
> + *
> + * HDMA does not update LLP continuously. Only use it to track running
> + * transfers when handling an interrupt.
> + */
> +static void dw_edma_ll_advance(struct dw_edma_chan *chan, bool event)
> +{
> +	bool stopped = dw_edma_ll_stopped(chan);
> +
> +	if (stopped || event)
> +		dw_edma_ll_consume(chan, dw_edma_core_ll_cur_idx(chan),
> +				   stopped);
> +
> +	if (chan->request != EDMA_REQ_NONE) {
> +		/* Wait for the channel to stop before completing STOP or PAUSE. */
> +		if (!stopped)
> +			return;
> +
> +		if (chan->request == EDMA_REQ_STOP) {
> +			dw_edma_finish_termination(chan);

[Severity: High]
Does calling dw_edma_ll_consume() in drivers/dma/dw-edma/dw-edma-core.c
before checking chan->request == EDMA_REQ_STOP cause completion callbacks to
be scheduled on a channel that is being terminated?

When dw_edma_device_terminate_all() is called while an LL channel is actively
transferring, it defers termination by setting chan->request = EDMA_REQ_STOP.
When the interrupt fires:

dw_edma_irq_work()
  dw_edma_ll_interrupt()
    dw_edma_ll_advance(chan, true)
      dw_edma_ll_consume()
        list_del(&vd->node);
        vchan_cookie_complete(vd);

In dw_edma_ll_consume(), any descriptor in chan->vc.desc_issued whose bursts
finished before the channel stopped is removed from chan->vc.desc_issued,
placed on chan->vc.desc_completed, and has chan->vc.task scheduled via
vchan_cookie_complete() to run its completion callback.

When dw_edma_ll_advance() then checks chan->request == EDMA_REQ_STOP and
calls dw_edma_finish_termination(), dw_edma_terminate_all_descs() only cleans
up descriptors still on chan->vc.desc_issued and chan->vc.desc_submitted,
leaving the descriptors already moved to chan->vc.desc_completed to invoke
their callbacks.

By comparison, the non-LL handler dw_edma_done_interrupt_locked() checks
EDMA_REQ_STOP and calls dw_edma_finish_termination() without calling
vchan_cookie_complete(), matching commit 99109a51efd2 ("dmaengine: dw-edma:
Terminate all descriptors without callbacks").

Should dw_edma_ll_advance() check for EDMA_REQ_STOP before calling
dw_edma_ll_consume() so that terminated descriptors do not schedule
completion callbacks?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030150742.2586776-1-den@valinux.co.jp?part=2