Re: [PATCH v3 07/24] dmaengine: dw-edma: Centralize LL doorbell decisions

On Mon, Jul 27, 2026 at 03:14:57PM -0400, Frank Li wrote:
> On Mon, Jul 27, 2026 at 05:23:25PM +0000, [email protected] wrote:
> > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > - [Medium] The DONE interrupt handler fails to clear EDMA_REQ_STOP and EDMA_REQ_PAUSE if the descriptor list is empty, contradicting the commit message's stated intent to prepare for LL progress reclamation.
> > --
> >
> > --- Patch [7]: [PATCH v3 07/24] dmaengine: dw-edma: Centralize LL doorbell decisions ---
> > commit ee61354242533ea66a01a91592b3314e404a95eb
> > Author: Koichiro Den <[email protected]>
> >
> > dmaengine: dw-edma: Centralize LL doorbell decisions
> >
> > LL fill currently rings the doorbell every time. This patch moves that decision
> > to dw_edma_core_ch_maybe_doorbell(), which rings only for a BUSY LL channel with
> > pending entries. This centralization prevents EDMA_REQ_STOP completion from
> > restarting terminated work.
> >
> > > diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> > > index b419ff2becf9e..c036510dabcb3 100644
> > > --- a/drivers/dma/dw-edma/dw-edma-core.c
> > > +++ b/drivers/dma/dw-edma/dw-edma-core.c
> >
> > [ ... ]
> >
> > > @@ -733,6 +743,8 @@ static void dw_edma_done_interrupt(struct dw_edma_chan *chan)
> > >  	default:
> > >  		break;
> > >  	}
> >
> > [Severity: Medium]
> > Will this lead to a permanent channel hang when LL progress reclamation is
> > introduced?
> >
> > Looking at dw_edma_done_interrupt() just above this change, the DONE interrupt
> > handler fails to clear EDMA_REQ_STOP and EDMA_REQ_PAUSE if the descriptor list
> > is empty:
> >
> > drivers/dma/dw-edma/dw-edma-core.c:dw_edma_done_interrupt()
> > 	case EDMA_REQ_STOP:
> > 		vd = vchan_next_desc(&chan->vc);
> > 		if (!vd)
> > 			break;
> >
> > 		dw_edma_finish_termination(chan);
> > 		break;
> >
> > The commit "Dispatch DONE interrupts by channel request" explicitly claims to
> > restructure the code so that STOP and PAUSE still have state to update even
> > when no descriptor remains.
> >
> > However, the implementation adds if (!vd) break; which completely skips the
> > state updates when no descriptor remains.
> >
> > If a future patch asynchronously removes the last descriptor before the DONE
> > interrupt handles an EDMA_REQ_STOP, won't the channel remain permanently
> > stuck in EDMA_REQ_STOP without completing termination, leading to an infinite
> > loop in dw_edma_wait_termination?
> 
> Is it true?

Sorry, I missed replying to this.

I don't think so.

The preceding patch 6 has a misleading commit message, which seems to have led
to this false positive. This is not a bisectability issue either. The later
patch adds both progress reclamation and vd-independent STOP/PAUSE handling. At
this point, patch 6 is just a non-functional change. I likely forgot to update
the message while rearranging the series for bisectability issue which Sashiko
had pointed out on the previous revision.

In short, I think the code is fine, but patch 6's commit message needs fixing.

Best regards,
Koichiro

> 
> Frank
> >
> > > +	dw_edma_core_ch_maybe_doorbell(chan);
> > > +
> > >  	spin_unlock_irqrestore(&chan->vc.lock, flags);
> > >  }
> >
> > --
> > Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7