Re: [PATCH v3 5/9] dmaengine: dw-edma: Serialize channel state checks

[Koichiro Den]

On Wed, Jul 15, 2026 at 01:58:13PM -0500, Frank Li wrote:
> On Thu, Jul 16, 2026 at 02:57:36AM +0900, Koichiro Den wrote:
> > pause() and resume() read and update channel state without holding vc.lock,
> > while the interrupt handlers update the same state under it. Take the same
> > lock around those state checks so that request, status, and configured stay
> > consistent.
> >
> > For example, pause() can observe EDMA_ST_BUSY right before the interrupt
> > handler completes the final descriptor and moves the channel to
> > EDMA_ST_IDLE, and then record EDMA_REQ_PAUSE on an already idle channel. No
> > further interrupt will acknowledge the request, and since issue_pending()
> > requires EDMA_REQ_NONE, the channel is wedged for good: terminate_all()
> > leaves the stale request behind, so even reconfiguring the channel does not
> > recover it.
> >
> > issue_pending() already runs under vc.lock, but it tests configured before
> > taking it. Move that test under the lock as well, so that the decision to
> > start work is made against the current value rather than one observed
> > before a concurrent terminate_all() deconfigured the channel.
> >
> > Fixes: e63d79d1ffcd ("dmaengine: Add Synopsys eDMA IP core driver")
> > Reviewed-by: Frank Li <Frank.Li@xxxxxxx>
> > Signed-off-by: Koichiro Den <den@xxxxxxxxxxxxx>
> > ---
> > Changes in v3:
> >   - No changes.
> >
> >  drivers/dma/dw-edma/dw-edma-core.c | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> > index b06b299661c0..8b0e2af734da 100644
> > --- a/drivers/dma/dw-edma/dw-edma-core.c
> > +++ b/drivers/dma/dw-edma/dw-edma-core.c
> > @@ -300,8 +300,10 @@ static int dw_edma_device_config(struct dma_chan *dchan,
> >  static int dw_edma_device_pause(struct dma_chan *dchan)
> >  {
> >  	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> > +	unsigned long flags;
> >  	int err = 0;
> >
> > +	spin_lock_irqsave(&chan->vc.lock, flags);
> 
> new code, can you use clean up guard()

Sure, will do so in v4.

Thanks for reviewing,
Koichiro

> 
> Frank
> 
> >  	if (!chan->configured)
> >  		err = -EPERM;
> >  	else if (chan->status != EDMA_ST_BUSY)
> > @@ -310,6 +312,7 @@ static int dw_edma_device_pause(struct dma_chan *dchan)
> >  		err = -EPERM;
> >  	else
> >  		chan->request = EDMA_REQ_PAUSE;
> > +	spin_unlock_irqrestore(&chan->vc.lock, flags);
> >
> >  	return err;
> >  }
> > @@ -317,8 +320,10 @@ static int dw_edma_device_pause(struct dma_chan *dchan)
> >  static int dw_edma_device_resume(struct dma_chan *dchan)
> >  {
> >  	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> > +	unsigned long flags;
> >  	int err = 0;
> >
> > +	spin_lock_irqsave(&chan->vc.lock, flags);
> >  	if (!chan->configured) {
> >  		err = -EPERM;
> >  	} else if (chan->status != EDMA_ST_PAUSE) {
> > @@ -330,6 +335,7 @@ static int dw_edma_device_resume(struct dma_chan *dchan)
> >  		if (!dw_edma_start_transfer(chan))
> >  			chan->status = EDMA_ST_IDLE;
> >  	}
> > +	spin_unlock_irqrestore(&chan->vc.lock, flags);
> >
> >  	return err;
> >  }
> > @@ -373,11 +379,9 @@ static void dw_edma_device_issue_pending(struct dma_chan *dchan)
> >  	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> >  	unsigned long flags;
> >
> > -	if (!chan->configured)
> > -		return;
> > -
> >  	spin_lock_irqsave(&chan->vc.lock, flags);
> > -	if (vchan_issue_pending(&chan->vc) && chan->request == EDMA_REQ_NONE &&
> > +	if (chan->configured && vchan_issue_pending(&chan->vc) &&
> > +	    chan->request == EDMA_REQ_NONE &&
> >  	    chan->status == EDMA_ST_IDLE) {
> >  		chan->status = EDMA_ST_BUSY;
> >  		dw_edma_start_transfer(chan);
> > --
> > 2.51.0
> >