[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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:

Pre-existing issues:
- [High] Calling `issue_pending` while a transfer is active forcefully restarts the active transfer from the beginning.
- [High] The DMA IRQ handler fails to start the next pending descriptor when the current transfer completes.
- [High] Zero-length scatterlists or cyclic configurations with buf_len < period_len cause NULL pointer dereferences and memory corruption.
- [High] IRQ and devres leak on resource allocation failure.
- [High] Lockless read-modify-write race on the shared global `LS1X_DMA_CTRL` register corrupts commands and stops wrong channels.
- [High] Idle hardware causes false-positive LLI matches, reporting partial residue for pending transfers.
- [Medium] The driver stalls the CPU for up to 3ms inside a `spinlock_irqsave` critical section.
- [Medium] The driver tracks `bus_width` globally per channel rather than per descriptor, corrupting residue calculations.
--

--- Patch [1]: [PATCH] dmaengine: loongson1-apb-dma: fix residue calculation for queued descriptors ---
commit 62e04cef060bef1de9eb6ca4a2d6840c563a954f
Author: Mahad Ibrahim <mahad.ibrahim.dev@gmail.com>

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

This commit fixes a bug in ls1x_dma_tx_status() where the loop locating the
current descriptor could wrap around to the list head. The wrapped pointer
caused the subsequent list_for_each_entry_from() to stop immediately, resulting
in a residue of 0 being reported.

> diff --git a/drivers/dma/loongson/loongson1-apb-dma.c b/drivers/dma/loongson/loongson1-apb-dma.c
> index 89786cbd20ab5..8bb6e6e5719df 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,
[ ... ]
>  			/* 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);
> +				}

[Severity: High]
This is a pre-existing issue, but might idle hardware cause false-positive
LLI matches and bypass the newly added fallback logic?

The final LLI of all non-cyclic descriptors is initialized with NEXT == 0.
When the hardware is idle after a previous transfer, its next_phys register
remains 0.

If a client queries the status of a pending, unstarted descriptor, will the
driver incorrectly match next_phys = 0 to the descriptor's final LLI, setting
found = true, and report only the length of the last segment as the residue
instead of the full size?

> +
> +			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)

[Severity: Medium]
This isn't a bug introduced by this patch, but will tracking bus_width
globally per channel rather than per descriptor corrupt residue calculations?

In ls1x_dma_prep_lli(), the bus width for a specific transfer is cached
globally in chan->bus_width. Later, ls1x_dma_tx_status() calculates the
residue:

	bytes += lli->hw[LS1X_DMADESC_LENGTH] * chan->bus_width;

If a client queues multiple transfers with different configurations or
directions on the same channel, will querying the status of older transfers
use the width of the most recently prepared transfer and result in an
incorrect byte count?

[Severity: High]
This is a pre-existing issue, but does calling vchan_issue_pending() while a
transfer is active forcefully restart the active transfer from the beginning?

When vchan_issue_pending() is called, it moves submitted descriptors to the
issued list and returns true if the list is not empty.

In ls1x_dma_issue_pending(), it unconditionally fetches the first descriptor:

	if (vchan_issue_pending(&chan->vc)) {
		struct virt_dma_desc *vd = vchan_next_desc(&chan->vc);
		...
		ls1x_dma_start(chan, &lli->phys);
	}

Because vchan_next_desc() returns the currently running transfer (it is only
removed upon completion), this immediately calls ls1x_dma_start() and
instructs the hardware to begin a new transfer at the active descriptor's
start address.

Can this destroy the current hardware state machine and restart the ongoing
transfer, leading to data corruption?

[Severity: High]
This isn't a bug introduced by this patch, but is it possible that the DMA IRQ
handler fails to start the next pending descriptor when the current transfer
completes?

When a non-cyclic transfer completes, ls1x_dma_irq_handler() removes the
completed descriptor from the issued list and marks it complete:

	} else {
		list_del(&vd->node);
		vchan_cookie_complete(vd);
	}

It does not fetch the next queued descriptor or call ls1x_dma_start() to
initiate the next transfer.

If a client submits multiple descriptors, will the channel stall indefinitely
after finishing the first one?

[Severity: High]
This is a pre-existing issue, but can zero-length scatterlists or cyclic
configurations with buf_len < period_len cause NULL pointer dereferences and
memory corruption?

In ls1x_dma_prep_dma_cyclic(), if buf_len < period_len, sg_len becomes 0.
ls1x_dma_prep_lli() is then called with sg_len == 0, bypassing the iteration
loop, leaving lli and first pointers uninitialized.

If is_cyclic is true, the driver immediately dereferences the NULL first
pointer:

	if (is_cyclic) {
		lli->hw[LS1X_DMADESC_NEXT] = first->phys | LS1X_DMA_NEXT_VALID;
		chan->is_cyclic = is_cyclic;
	}

If is_cyclic is false, it returns success with an empty lli_list.
When ls1x_dma_issue_pending() processes it:

	lli = list_first_entry(&desc->lli_list, struct ls1x_dma_lli, node);
	ls1x_dma_start(chan, &lli->phys);

Can this force list_first_entry() to return a garbage offset and instruct
the hardware to execute from random memory addresses?

[Severity: High]
This isn't a bug introduced by this patch, but is there an IRQ and devres
leak on resource allocation failure?

ls1x_dma_alloc_chan_resources() dynamically registers an IRQ handler using
devm_request_irq().

If subsequent allocations like dma_pool_create() fail:

	if (!chan->lli_pool)
		return -ENOMEM;

It returns an error without calling devm_free_irq(). The DMA engine core
handles the failure but does not call the corresponding free_chan_resources()
callback on allocation failure.

Will the IRQ handler remain permanently registered, allowing devm allocations
to accumulate and potentially exhausting memory or registering duplicate
handlers upon repeated client requests?

[Severity: High]
This is a pre-existing issue, but can a lockless read-modify-write race on
the shared global LS1X_DMA_CTRL register corrupt commands and stop wrong
channels?

The LS1X_DMA_CTRL register is a single global register mapped to
chan->reg_base and shared across all DMA channels. ls1x_dma_stop() performs a
lockless read-modify-write:

	int val = readl(chan->reg_base + LS1X_DMA_CTRL);

	writel(val | LS1X_DMA_STOP, chan->reg_base + LS1X_DMA_CTRL);

Because it does not clear the previous channel ID field in val, does it send
the STOP command to whichever channel was last written to the register, even
if ls1x_dma_stop() was called for a different channel?

Will concurrent operations using per-channel locks in ls1x_dma_start() and
ls1x_dma_query() also silently race and corrupt the register?

[Severity: Medium]
This isn't a bug introduced by this patch, but could stalling the CPU for up
to 3ms inside a spinlock_irqsave() critical section be problematic?

ls1x_dma_tx_status() acquires a per-channel spinlock and disables interrupts.
It then calls ls1x_dma_query(), which loops polling the hardware:

	ret = readl_poll_timeout_atomic(chan->reg_base + LS1X_DMA_CTRL, val,
					!(val & LS1X_DMA_ASK_VALID), 0, 3000);

Can stalling a CPU for 3000 microseconds with interrupts masked degrade system
latency and trigger soft lockup detectors under worst-case hardware delays?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720142538.2766-1-mahad.ibrahim.dev@gmail.com?part=1