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

[sashiko-bot@kernel.org]

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