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

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

[Mahad Ibrahim]

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@gmail.com>
---
 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);
+			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

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

[Keguang Zhang]

Hi Mahad,

Thanks for catching this use-after-iterator issue.

On Mon, Jul 20, 2026 at 10:25 PM Mahad Ibrahim
<mahad.ibrahim.dev@gmail.com> 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@gmail.com>
> ---
>  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

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@gmail.com> 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@gmail.com> 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@gmail.com>
> > ---
> >  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

[PATCH v2] dmaengine: loongson1-apb-dma: avoid using iterator variable after list_for_each_entry()

[Mahad Ibrahim]

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 at
the end of the loop leaves the lli pointer at the currently executing LLI.
However this also subsequently runs for a non-match lli, in which it
points at the head. This causes a type confusion bug which treats the
head, which is a ls1x_dma_desc, as a ls1x_dma_lli object. Additionally it
goes forwards and prints garbage via the dev_dbg.

Fix the type confusion bug by only allowing matched LLI descriptor chains
to print the current LLI and residue calculation, as failing to match
should be treated as an unexpected condition.

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

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@gmail.com>
---

v2:
- encapsulate residue calculation and dev_dbg inside the
  list_for_each_entry() macro. Treat non-matching LLI as an unexpected
  case.


 drivers/dma/loongson/loongson1-apb-dma.c | 27 ++++++++++++++----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/dma/loongson/loongson1-apb-dma.c b/drivers/dma/loongson/loongson1-apb-dma.c
index 89786cbd20ab..8658d5377795 100644
--- a/drivers/dma/loongson/loongson1-apb-dma.c
+++ b/drivers/dma/loongson/loongson1-apb-dma.c
@@ -446,22 +446,27 @@ 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)
-					break;
+			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",
-				&lli->phys);
+				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;
+				/* 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");
 		}
 	}
 
-	dma_set_residue(state, bytes);
-
 	return status;
 }
 
-- 
2.54.0