* [PATCH] dmaengine: loongson1-apb-dma: fix residue calculation for queued descriptors
@ 2026-07-20 14:25 Mahad Ibrahim
2026-07-21 7:32 ` Keguang Zhang
0 siblings, 1 reply; 3+ messages in thread
From: Mahad Ibrahim @ 2026-07-20 14:25 UTC (permalink / raw)
To: Keguang Zhang, Vinod Koul
Cc: Frank Li, linux-mips, dmaengine, linux-kernel, 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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] dmaengine: loongson1-apb-dma: fix residue calculation for queued descriptors
2026-07-20 14:25 [PATCH] dmaengine: loongson1-apb-dma: fix residue calculation for queued descriptors Mahad Ibrahim
@ 2026-07-21 7:32 ` Keguang Zhang
2026-07-21 16:14 ` Mahad Ibrahim
0 siblings, 1 reply; 3+ messages in thread
From: Keguang Zhang @ 2026-07-21 7:32 UTC (permalink / raw)
To: Mahad Ibrahim; +Cc: Vinod Koul, Frank Li, linux-mips, dmaengine, linux-kernel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] dmaengine: loongson1-apb-dma: fix residue calculation for queued descriptors
2026-07-21 7:32 ` Keguang Zhang
@ 2026-07-21 16:14 ` Mahad Ibrahim
0 siblings, 0 replies; 3+ messages in thread
From: Mahad Ibrahim @ 2026-07-21 16:14 UTC (permalink / raw)
To: Keguang Zhang; +Cc: Vinod Koul, Frank Li, linux-mips, dmaengine, linux-kernel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-07-21 16:15 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-20 14:25 [PATCH] dmaengine: loongson1-apb-dma: fix residue calculation for queued descriptors Mahad Ibrahim
2026-07-21 7:32 ` Keguang Zhang
2026-07-21 16:14 ` Mahad Ibrahim
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox