Re: [PATCH v3 1/4] dmaengine: dw: Fix XFER bit set, but channel not idle error

[James Hilliard]

On Fri, Oct 23, 2026 at 3:36 PM Frank Li <Frank.li@xxxxxxxxxxx> wrote:
>
> On Fri, Oct 23, 2026 at 10:27:22AM -0600, James Hilliard wrote:
> > From: Serge Semin <fancer.lancer@xxxxxxxxx>
> >
> > If a client driver gets to use the DW DMAC engine device tougher
> > than usual, with occasional DMA-transfers termination and restart, then
> > the next error can be randomly spotted in the system log:
> >
> > > dma dma0chan0: BUG: XFER bit set, but channel not idle!
> >
> > For instance that happens in case of the 8250 UART port driver handling
> > the looped back high-speed traffic (in my case > 1.5Mbaud) by means of the
> > DMA-engine interface.
> >
> > The error happens due to the two-staged nature of the DW DMAC IRQs
> > handling procedure and due to the critical section break in the meantime.
> > In particular in case if the DMA-transfer is terminated and restarted:
> > 1. after the IRQ-handler submitted the tasklet but before the tasklet
> >    started handling the DMA-descriptors in dwc_scan_descriptors();
> > 2. after the XFER completion flag was detected in the
> >    dwc_scan_descriptors() method, but before the dwc_complete_all() method
> >    is called
> > the error denoted above is printed due to the overlap of the last transfer
> > completion and the new transfer execution stages.
> >
> > Clear the channel interrupt status when stopping it, and keep the XFER
> > check, descriptor detachment and queued-transfer restart in one locked
> > section. An empty active list needs no completion processing.
> >
> > When the locked XFER recheck finds no event, only update residue. A stale
> > tasklet snapshot may belong to a terminated transfer, and a zero residue
> > only means the source has been read, not that destination writes have
> > finished. Requiring RAW.XFER also prevents the fallback scan from freeing
> > a descriptor still linked on active_list.
> >
> > Fixes: 69cea5a00d31 ("dmaengine/dw_dmac: Replace spin_lock* with irqsave variants and enable submission from callback")
> > Fixes: 3bfb1d20b547 ("dmaengine: Driver for the Synopsys DesignWare DMA controller")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Serge Semin <fancer.lancer@xxxxxxxxx>
> > [james: require a locked XFER event and handle an empty active list]
> > Signed-off-by: James Hilliard <james.hilliard1@xxxxxxxxx>
> > ---
> >  drivers/dma/dw/core.c | 70 ++++++++++++++++++++-------------------------------
> >  1 file changed, 27 insertions(+), 43 deletions(-)
> >
> > diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
> > index dd75f97a33b3..07a5f5105787 100644
> > --- a/drivers/dma/dw/core.c
> > +++ b/drivers/dma/dw/core.c
> > @@ -141,6 +141,12 @@ static inline void dwc_chan_disable(struct dw_dma *dw, struct dw_dma_chan *dwc)
> >       channel_clear_bit(dw, CH_EN, dwc->mask);
> >       while (dma_readl(dw, CH_EN) & dwc->mask)
> >               cpu_relax();
> > +
> > +     dma_writel(dw, CLEAR.XFER, dwc->mask);
> > +     dma_writel(dw, CLEAR.BLOCK, dwc->mask);
> > +     dma_writel(dw, CLEAR.SRC_TRAN, dwc->mask);
> > +     dma_writel(dw, CLEAR.DST_TRAN, dwc->mask);
> > +     dma_writel(dw, CLEAR.ERROR, dwc->mask);
> >  }
> >
> >  /*----------------------------------------------------------------------*/
> > @@ -257,34 +263,6 @@ dwc_descriptor_complete(struct dw_dma_chan *dwc, struct dw_desc *desc,
> >       dmaengine_desc_callback_invoke(&cb, NULL);
> >  }
> >
> > -static void dwc_complete_all(struct dw_dma *dw, struct dw_dma_chan *dwc)
> > -{
> > -     struct dw_desc *desc, *_desc;
> > -     LIST_HEAD(list);
> > -     unsigned long flags;
> > -
> > -     spin_lock_irqsave(&dwc->lock, flags);
> > -     if (dma_readl(dw, CH_EN) & dwc->mask) {
> > -             dev_err(chan2dev(&dwc->chan),
> > -                     "BUG: XFER bit set, but channel not idle!\n");
> > -
> > -             /* Try to continue after resetting the channel... */
> > -             dwc_chan_disable(dw, dwc);
> > -     }
> > -
> > -     /*
> > -      * Submit queued descriptors ASAP, i.e. before we go through
> > -      * the completed ones.
> > -      */
> > -     list_splice_init(&dwc->active_list, &list);
> > -     dwc_dostart_first_queued(dwc);
> > -
> > -     spin_unlock_irqrestore(&dwc->lock, flags);
> > -
> > -     list_for_each_entry_safe(desc, _desc, &list, desc_node)
> > -             dwc_descriptor_complete(dwc, desc, true);
> > -}
> > -
> >  /* Returns how many bytes were already received from source */
> >  static inline u32 dwc_get_sent(struct dw_dma_chan *dwc)
> >  {
> > @@ -302,6 +280,7 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
> >       struct dw_desc *child;
> >       u32 status_xfer;
> >       unsigned long flags;
> > +     LIST_HEAD(list);
> >
> >       spin_lock_irqsave(&dwc->lock, flags);
> >       llp = channel_readl(dwc, LLP);
> > @@ -310,6 +289,10 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
> >       if (status_xfer & dwc->mask) {
> >               /* Everything we've submitted is done */
> >               dma_writel(dw, CLEAR.XFER, dwc->mask);
> > +             if (list_empty(&dwc->active_list)) {
> > +                     spin_unlock_irqrestore(&dwc->lock, flags);
> > +                     return;
> > +             }
> >
> >               if (test_bit(DW_DMA_IS_SOFT_LLP, &dwc->flags)) {
> >                       struct list_head *head, *active = dwc->tx_node_active;
> > @@ -341,9 +324,23 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
> >                       clear_bit(DW_DMA_IS_SOFT_LLP, &dwc->flags);
> >               }
> >
> > +             if (dma_readl(dw, CH_EN) & dwc->mask) {
> > +                     dev_err(chan2dev(&dwc->chan),
> > +                             "BUG: XFER bit set, but channel not idle!\n");
> > +
> > +                     /* Try to continue after resetting the channel... */
> > +                     dwc_chan_disable(dw, dwc);
> > +             }
> > +
> > +             /* Detach the completed transfer before starting queued work. */
> > +             list_splice_init(&dwc->active_list, &list);
> > +             dwc_dostart_first_queued(dwc);
> >               spin_unlock_irqrestore(&dwc->lock, flags);
> >
> > -             dwc_complete_all(dw, dwc);
> > +             list_for_each_entry_safe(desc, _desc, &list, desc_node) {
> > +                     list_del_init(&desc->desc_node);
> > +                     dwc_descriptor_complete(dwc, desc, true);
> > +             }
>
> what's purpose move dwc_descriptor_complete() implement into here?

To keep the XFER check and descriptor detachment under the same lock.
Previously, the scanner dropped the lock before dwc_complete_all()
reacquired it, allowing termination/restart to replace the active transfer
in between. The old completion event could then complete the new transfer.
Callbacks still run after unlocking.

>
> Frank
>
> >               return;
> >       }
> >
> > @@ -389,22 +386,9 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
> >                       desc->residue -= child->len;
> >               }
> >
> > -             /*
> > -              * No descriptors so far seem to be in progress, i.e.
> > -              * this one must be done.
> > -              */
> > -             spin_unlock_irqrestore(&dwc->lock, flags);
> > -             dwc_descriptor_complete(dwc, desc, true);
> > -             spin_lock_irqsave(&dwc->lock, flags);
> >       }
> >
> > -     dev_err(chan2dev(&dwc->chan),
> > -             "BUG: All descriptors done, but channel not idle!\n");
> > -
> > -     /* Try to continue after resetting the channel... */
> > -     dwc_chan_disable(dw, dwc);
> > -
> > -     dwc_dostart_first_queued(dwc);
> > +     /* Residue describes source reads; only RAW.XFER completes a transfer. */
> >       spin_unlock_irqrestore(&dwc->lock, flags);
> >  }
> >
> >
> > --
> > 2.53.0
> >