Re: [PATCH] dmaengine: loongson1-apb-dma: fix residue calculation for queued descriptors

[Mahad Ibrahim]

Hello Keguang,

Thank you for the quick response and feedback. I'll adopt the single-loop
structure.

Additionally, Sashiko pointed out another possible flaw, which occurs when
the hardware is idle. The hw[LS1X_DMADESC_NEXT] of the last terminator LLI
in a chain is 0 and the current next_phys the hardware returns with

    next_phys = chan->curr_lli->hw[LS1X_DMADESC_NEXT];

is also 0. Which would match, however I do not think it's the preferred
behavior, the residue would only be the length of the last LLI. If
you wish I can add it as an additional patch on top of the single-loop
structure.

Best regards,

Mahad Ibrahim

On Tue, Jul 21, 2026 at 12:32 PM Keguang Zhang <keguang.zhang@xxxxxxxxx> wrote:
>
> Hi Mahad,
>
> Thanks for catching this use-after-iterator issue.
>
> On Mon, Jul 20, 2026 at 10:25 PM Mahad Ibrahim
> <mahad.ibrahim.dev@xxxxxxxxx> wrote:
> >
> > ls1x_dma_tx_status() locates the descriptor actively being processed by
> > walking the LLI list and comparing the hardware reported next descriptor
> > pointer against each element's next-descriptor pointer.
> >
> > A list_for_each_entry macro is used in the comparison phase, which
> > internally expands to the container_of macro to find the current
> > ls1x_dma_lli's next-descriptor pointer to compare against. However a
> > problem occurs when the loop reaches the final lli list element and wraps
> > around to the head. The head is a different struct ls1x_dma_desc. It goes
> > down to the offset the node member would be at in ls1x_dma_lli inside of
> > ls1x_dma_desc.
> >
> > This leaves the lli pointer somewhere inside of ls1x_dma_desc at the end
> > of the loop. The subsequent phys debug value read therefore would print
> > garbage as a valid phys address.
> >
> > The primary bug occurs in the residue calculation list_for_each_entry_from
> > macro. The lli pointer still points to somewhere inside of ls1x_dma_desc,
> > when the list_for_each_entry_from performs the same wrong offset
> > calculation on ls1x_dma_desc to determine the stop condition of the loop,
> > it lands at the same offset that lli pointer is on, and the loop
> > immediately stops.
> >
> > This produces a 0 residue value, while it should be the entire length of
> > the lli list chain. The driver advertises DMA_RESIDUE_GRANULARITY_SEGMENT,
> > so a 0 residue would mean the transfer has completed.
> >
> > Found by the following Coccinelle check:
> >
> >   scripts/coccinelle/iterators/use_after_iter.cocci
> >
> >   drivers/dma/loongson/loongson1-apb-dma.c:461:6-9: ERROR: invalid
> >   reference to the index variable of the iterator on line 450
> >
> > Note the check still reports this line after the fix; the remaining use
> > is safe (the cursor is only dereferenced on the match path, or reset to
> > the first entry otherwise).
> >
> > I did not see a bug upstream detailing this error, nor do I have the
> > hardware to confirm this bug or error, all this is from a pure code
> > examination.
> >
> > As I do not possess the hardware, I cannot test the patch. Compile tested
> > only with mips64-linux-gnu-gcc.
> >
> > Signed-off-by: Mahad Ibrahim <mahad.ibrahim.dev@xxxxxxxxx>
> > ---
> >  drivers/dma/loongson/loongson1-apb-dma.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/dma/loongson/loongson1-apb-dma.c b/drivers/dma/loongson/loongson1-apb-dma.c
> > index 89786cbd20ab..8bb6e6e5719d 100644
> > --- a/drivers/dma/loongson/loongson1-apb-dma.c
> > +++ b/drivers/dma/loongson/loongson1-apb-dma.c
> > @@ -439,6 +439,7 @@ static enum dma_status ls1x_dma_tx_status(struct dma_chan *dchan,
> >                         struct ls1x_dma_desc *desc = to_ls1x_dma_desc(vd);
> >                         struct ls1x_dma_lli *lli;
> >                         dma_addr_t next_phys;
> > +                       bool found = false;
> >
> >                         /* get the current lli */
> >                         if (ls1x_dma_query(chan, &chan->curr_lli->phys))
> > @@ -447,11 +448,17 @@ static enum dma_status ls1x_dma_tx_status(struct dma_chan *dchan,
> >                         /* locate the current lli */
> >                         next_phys = chan->curr_lli->hw[LS1X_DMADESC_NEXT];
> >                         list_for_each_entry(lli, &desc->lli_list, node)
> > -                               if (lli->hw[LS1X_DMADESC_NEXT] == next_phys)
> > +                               if (lli->hw[LS1X_DMADESC_NEXT] == next_phys) {
> > +                                       found = true;
> >                                         break;
> > -
> > -                       dev_dbg(chan2dev(dchan), "current lli_phys=%pad",
> > -                               &lli->phys);
> > +                               }
> > +
> > +                       if (!found)
> > +                               lli = list_first_entry(&desc->lli_list,
> > +                                                      struct ls1x_dma_lli, node);
>
> However, I don't think falling back to list_first_entry() when no
> match is found is appropriate. Since next_phys should always match one
> of the LLIs in the descriptor chain, failing to find a match should be
> treated as an unexpected condition rather than silently falling back
> to the first LLI.
> I would suggest something like:
>
>        scoped_guard(spinlock_irqsave, &chan->vc.lock) {
>                vd = vchan_find_desc(&chan->vc, cookie);
>                if (vd) {
>                        struct ls1x_dma_desc *desc = to_ls1x_dma_desc(vd);
>                        struct ls1x_dma_lli *lli;
>                        dma_addr_t next_phys;
>
>                        /* get the current lli */
>                        if (ls1x_dma_query(chan, &chan->curr_lli->phys))
>                                return status;
>
>                        /* locate the current lli */
>                        next_phys = chan->curr_lli->hw[LS1X_DMADESC_NEXT];
>                        list_for_each_entry(lli, &desc->lli_list, node) {
>                                if (lli->hw[LS1X_DMADESC_NEXT] != next_phys)
>                                        continue;
>
>                                dev_dbg(chan2dev(dchan), "current
> lli_phys=%pad\n", &lli->phys);
>
>                                /* count the residues */
>                                list_for_each_entry_from(lli,
> &desc->lli_list, node)
>                                        bytes +=
> lli->hw[LS1X_DMADESC_LENGTH] * chan->bus_width;
>
>                                dma_set_residue(state, bytes);
>                                return status;
>                        }
>
>                        dev_warn(chan2dev(dchan), "unable to locate
> current LLI\n");
>                }
>        }
>
>        return status;
>
> > +                       else
> > +                               dev_dbg(chan2dev(dchan), "current lli_phys=%pad",
> > +                                       &lli->phys);
> >
> >                         /* count the residues */
> >                         list_for_each_entry_from(lli, &desc->lli_list, node)
> > --
> > 2.54.0
> >
>
>
> --
> Best regards,
>
> Keguang Zhang