Re: [PATCH v4 11/24] dmaengine: dw-edma: Serialize LL event capture with channel kicks

On Wed, Jul 29, 2026 at 02:51:08PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Ringing the doorbell can race with a hardware interrupt, clearing and permanently losing a pending DMA completion event.
> --
> 
> --- Patch [11]: [PATCH v4 11/24] dmaengine: dw-edma: Serialize LL event capture with channel kicks ---
> commit 56231794b3b46dd52a7c7f61f0def40f1fed9890
> Author: Koichiro Den <[email protected]>
> 
> dmaengine: dw-edma: Serialize LL event capture with channel kicks
>     
> This commit introduces an event lock to serialize interrupt status reads,
> clears, snapshot recording, and LL doorbell writes. It aims to prevent a
> deferred handler from applying an event from a previous hardware run when a
> doorbell starts a new run in between.
> 
> > diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> > --- a/drivers/dma/dw-edma/dw-edma-core.c
> > +++ b/drivers/dma/dw-edma/dw-edma-core.c
> 
> [ ... ]
> 
> > @@ -269,6 +317,17 @@ static void dw_edma_core_ll_sync(struct dw_edma_chan *chan)
> >  static void dw_edma_core_ch_doorbell(struct dw_edma_chan *chan)
> >  {
> >  	dw_edma_core_ll_sync(chan);
> > +
> > +	guard(spinlock_irqsave)(dw_edma_event_lock(chan));
> > +
> > +	/*
> > +	 * A recorded event belongs to the current ring and must be consumed
> > +	 * before starting another hardware run.
> > +	 */
> > +	if (chan->ll_irq.event != DW_EDMA_LL_EVENT_NONE)
> > +		return;
> > +
> > +	dw_edma_ll_event_discard_locked(chan);
> 
> [Severity: High]
> Can this race with a hardware interrupt and permanently lose a completion event?
> 
> If dw_edma_core_ch_doorbell() is called (for example, when appending
> descriptors to a running channel) exactly when the hardware finishes a burst
> and asserts an interrupt, but before the hardirq handler can run,
> chan->ll_irq.event will still be DW_EDMA_LL_EVENT_NONE.
> 
> The code will bypass the early return and unconditionally call
> dw_edma_ll_event_discard_locked(), which clears the hardware interrupt
> status.
> 
> Does this result in the interrupt being completely lost? If so, the software
> state will permanently trail the hardware, which would cause the DMA channel
> to stall indefinitely.

Thanks for catching this.

That exact race cannot occur at this point (= patch 11), because dynamic append
is not yet enabled.

In the final tree, however, the race window seems real. The pending DONE status
can be cleared before the hard IRQ records it. It does not leave the channel
**permanently** stuck, though, because patch 15 adds a delayed stopped-tail
recheck which samples the channel state and LLP again.

A hardware STOP transition, including DONE status and interrupt assertion, can
land between the status check and the status clear performed by software. And
the check and clear cannot be made atomic. So the cleanest fix I see is to
permit the initial kick after an LL reset, then at most one kick after each STOP
boundary that software has confirmed as STOP. A quick test showed no obvious
regression. I'm planning to rework it that way for the next revision.

Best regards,
Koichiro

> 
> >  	dw_edma_core_do_ch_doorbell(chan);
> >  }
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=11