Re: [PATCH v2 61/64] dmaengine: dw: defer callbacks via channel BH

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Concurrent list modification between dwc_scan_descriptors() and dwc_terminate_all() leads to an infinite loop and list corruption.
--

--- Patch [61]: [PATCH v2 61/64] dmaengine: dw: defer callbacks via channel BH ---
commit d2107b5998f3b825a629b786e29b948ab851a743
Author: Allen Pais <[email protected]>
Subject: dmaengine: dw: defer callbacks via channel BH

Queue completed descriptors on the channel and invoke their callbacks
from per-channel BH work instead of the controller tasklet. Add a
synchronize hook so termination can wait for deferred callbacks before
their descriptor storage is released.

> diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
> index dd75f97a33b3d..e780116a944bc 100644
> --- a/drivers/dma/dw/core.c
> +++ b/drivers/dma/dw/core.c
> @@ -236,25 +236,24 @@ dwc_descriptor_complete(struct dw_dma_chan *dwc, struct dw_desc *desc,
>  	struct dma_async_tx_descriptor	*txd = &desc->txd;
>  	struct dw_desc			*child;
>  	unsigned long			flags;
>  
>  	dev_vdbg(chan2dev(&dwc->chan), "descriptor %u complete\n", txd->cookie);
>  
>  	spin_lock_irqsave(&dwc->lock, flags);
>  	dma_cookie_complete(txd);
> -	if (callback_required)
> -		dmaengine_desc_get_callback(txd, &cb);
> -	else
> -		memset(&cb, 0, sizeof(cb));
> +	list_del_init(&desc->desc_node);
>  
>  	/* async_tx_ack */
>  	list_for_each_entry(child, &desc->tx_list, desc_node)
>  		async_tx_ack(&child->txd);
>  	async_tx_ack(&desc->txd);
> -	dwc_desc_put(dwc, desc);
> +	if (callback_required) {
> +		list_add_tail(&desc->desc_node, &dwc->completed_list);

[Severity: High]
Can this concurrent list modification lead to an infinite loop during channel
termination?

If dwc_tx_status() races with dwc_terminate_all(), dwc_scan_descriptors()
temporarily drops dwc->lock while iterating over dwc->active_list.

If dwc_terminate_all() runs concurrently, it acquires the lock, splices
active_list to a local list, drops the lock, and begins locklessly iterating
the local list:

drivers/dma/dw/core.c:dwc_terminate_all() {
    ...
    list_splice_init(&dwc->queue, &list);
    list_splice_init(&dwc->active_list, &list);

    spin_unlock_irqrestore(&dwc->lock, flags);

    /* Flush all pending and queued descriptors */
    list_for_each_entry_safe(desc, _desc, &list, desc_node)
        dwc_descriptor_complete(dwc, desc, false);
}

When dwc_scan_descriptors() resumes, it calls dwc_descriptor_complete()
which now modifies desc->desc_node by moving it to dwc->completed_list.

Because dwc_terminate_all()'s lockless list_for_each_entry_safe() traversal
may have already prefetched a descriptor that is subsequently moved, its
iteration could jump into dwc->completed_list.

This would prevent the loop termination condition from ever matching the
stack-allocated list head in dwc_terminate_all(), causing an infinite loop.
Could this sequence happen in practice?

> +		dma_chan_schedule_bh(&dwc->chan);
> +	} else {
> +		dwc_desc_put(dwc, desc);
> +	}
>  	spin_unlock_irqrestore(&dwc->lock, flags);
> -
> -	dmaengine_desc_callback_invoke(&cb, NULL);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=61