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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch introduces an unconditional `dev_warn()` when a matching LLI is not found, leading to severe kernel log spam when querying queued or recently completed DMA descriptors.

Pre-existing issues:
- [High] The driver fails to start the next queued descriptor in the interrupt handler, breaking queued non-cyclic DMA transfers.
- [High] Missing validation of `src_addr_width` and `dst_addr_width` allows a divide-by-zero kernel panic.
--

--- Patch [1]: [PATCH v2] dmaengine: loongson1-apb-dma: avoid using iterator variable after list_for_each_entry() ---
commit 4909625fdc8e10245c908a5b3d250dd01aa7c238
Author: Mahad Ibrahim <mahad.ibrahim.dev@gmail.com>

    dmaengine: loongson1-apb-dma: avoid using iterator variable after list_for_each_entry()

    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.

> diff --git a/drivers/dma/loongson/loongson1-apb-dma.c b/drivers/dma/loongson/loongson1-apb-dma.c
> index 89786cbd20ab5..8658d53777957 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");

[Severity: High]
Does this new unconditional warning cause severe kernel log spam during
normal operation?

If a client submits a descriptor, calls dma_async_issue_pending(), and then
polls its status via dmaengine_tx_status(), the hardware might be executing
a different descriptor (or sitting idle).

In this case, ls1x_dma_query() fetches a physical address that does not match
any LLI in the queried descriptor's list, leading to this warning triggering
repeatedly for queued or recently completed descriptors.


[Severity: High]
This is a pre-existing issue, but does the driver fail to start the next
queued descriptor in the interrupt handler, breaking queued non-cyclic DMA
transfers?

Looking at the interrupt handler ls1x_dma_irq_handler():

	if (chan->is_cyclic) {
		vchan_cyclic_callback(vd);
	} else {
		list_del(&vd->node);
		vchan_cookie_complete(vd);
	}

It removes the completed descriptor and marks it as complete, but appears
to lack the logic to initiate the next pending transfer in the virt-dma queue
(e.g., using vchan_next_desc() and ls1x_dma_start()). Can this cause
subsequent queued descriptors to wait indefinitely?


[Severity: High]
This is a pre-existing issue, but does the missing validation of
user-supplied bus widths allow a divide-by-zero kernel panic?

In ls1x_dma_slave_config(), the user-supplied bus widths are saved without
validation:

	chan->src_addr = config->src_addr;
	chan->src_addr_width = config->src_addr_width;
	chan->dst_addr = config->dst_addr;
	chan->dst_addr_width = config->dst_addr_width;

Later, in ls1x_dma_prep_lli(), this value is assigned to chan->bus_width
and directly used as a divisor:

	lli->hw[LS1X_DMADESC_LENGTH] = buf_len / chan->bus_width;

If a client driver configures the DMA channel but leaves src_addr_width
or dst_addr_width undefined (0), could this lead to an immediate system crash?

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