Re: [PATCH v4 7/7] dmaengine: dw: Prevent tx-status calling DMA-desc callback

[Frank Li <Frank.li@oss.nxp.com>]

On Thu, Oct 29, 2026 at 04:29:41PM -0600, James Hilliard wrote:
> From: Serge Semin <fancer.lancer@gmail.com>
>
> The DMA transfer status getter can invoke a completion callback from
> dwc_scan_descriptors(). An 8250 RX timeout queries that status while
> holding the UART port lock, and its DMA callback takes the same lock:
>
>   serial8250_handle_irq()
>     uart_port_lock_irqsave(port)
>     serial8250_rx_dma_flush()
>       dmaengine_tx_status()
>         dwc_scan_descriptors()
>           dwc_descriptor_free()
>             dma_rx_complete()
>               uart_port_lock_irqsave(port)
>
> Queue completed descriptors on a channel completion list under the channel
> lock. Let the controller tasklet detach that list and invoke callbacks
> outside the lock. Termination discards notifications still on the shared
> list; the tasklet owns descriptors it has already detached, and the
> synchronization hook waits for that run.
>
> Keep handling raw completion events from tx_status(). Clients such as
> catpt poll transfers without DMA_PREP_INTERRUPT and can mask the controller
> interrupt externally. Polling must still advance software-emulated LLP
> blocks and complete cookies without waiting for tasklet execution. Hold
> the channel lock across event handling and status calculation, then
> schedule the tasklet for deferred notifications.
>
> On channel release, discard queued notifications after draining the
> tasklet, before clearing the controller's in-use state.
>
> Fixes: 3bfb1d20b547 ("dmaengine: Driver for the Synopsys DesignWare DMA controller")
> Cc: stable@vger.kernel.org
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> [james: defer notifications while preserving polled completion and cleanup]
> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
>
> ---
>
> Changelog RFC:
> - Instead of just dropping the dwc_scan_descriptors() method invocation
>   calculate the residue in the Tx-status getter.
> ---
>  drivers/dma/dw/core.c | 111 ++++++++++++++++++++++++++++++--------------------
>  drivers/dma/dw/regs.h |   1 +
>  2 files changed, 68 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
> index 66065d7b26ae..d80be93f0c49 100644
> --- a/drivers/dma/dw/core.c
> +++ b/drivers/dma/dw/core.c
> @@ -262,6 +262,22 @@ dwc_descriptor_free(struct dw_dma_chan *dwc, struct dw_desc *desc,
>  	dmaengine_desc_callback_invoke(&cb, NULL);
>  }
>
> +static void dwc_complete(struct dw_dma_chan *dwc)
> +{

dwc_free() ?

> +	struct dw_desc *desc, *_desc;
> +	unsigned long flags;
> +	LIST_HEAD(list);
> +
> +	spin_lock_irqsave(&dwc->lock, flags);
> +	list_splice_tail_init(&dwc->completed, &list);
> +	spin_unlock_irqrestore(&dwc->lock, flags);
> +
> +	list_for_each_entry_safe(desc, _desc, &list, desc_node) {
> +		list_del_init(&desc->desc_node);
> +		dwc_descriptor_free(dwc, desc, true);
> +	}
> +}
> +
>  /* Returns how many bytes were already received from source */
>  static inline u32 dwc_get_sent(struct dw_dma_chan *dwc)
>  {
> @@ -274,19 +290,17 @@ static inline u32 dwc_get_sent(struct dw_dma_chan *dwc)
>
>  static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
>  {
> -	struct dw_desc *desc, *_desc;
> +	struct dw_desc *desc;
>  	struct dw_desc *child;
> -	unsigned long flags;
> -	LIST_HEAD(list);
>
> -	spin_lock_irqsave(&dwc->lock, flags);
> +	lockdep_assert_held(&dwc->lock);
>
>  	if (!(dma_readl(dw, RAW.XFER) & dwc->mask))
> -		goto unlock;
> +		return;
>
>  	dma_writel(dw, CLEAR.XFER, dwc->mask);
>  	if (list_empty(&dwc->active_list))
> -		goto unlock;
> +		return;
>
>  	desc = dwc_first_active(dwc);
>  	if (test_bit(DW_DMA_IS_SOFT_LLP, &dwc->flags)) {
> @@ -304,7 +318,7 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
>
>  			/* Submit next block, including for polled transfers. */
>  			dwc_do_single_block(dwc, child);
> -			goto unlock;
> +			return;
>  		}
>
>  		clear_bit(DW_DMA_IS_SOFT_LLP, &dwc->flags);
> @@ -319,15 +333,8 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
>  	}
>
>  	dma_cookie_complete(&desc->txd);
> -	list_move_tail(&desc->desc_node, &list);
> +	list_move_tail(&desc->desc_node, &dwc->completed);
>  	dwc_dostart_first_queued(dwc);
> -unlock:
> -	spin_unlock_irqrestore(&dwc->lock, flags);
> -
> -	list_for_each_entry_safe(desc, _desc, &list, desc_node) {
> -		list_del_init(&desc->desc_node);
> -		dwc_descriptor_free(dwc, desc, true);
> -	}
>  }
>
>  static inline void dwc_dump_lli(struct dw_dma_chan *dwc, struct dw_desc *desc)
> @@ -344,18 +351,17 @@ static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc)
>  {
>  	struct dw_desc *bad_desc;
>  	struct dw_desc *child;
> -	unsigned long flags;
>
> -	spin_lock_irqsave(&dwc->lock, flags);
> +	lockdep_assert_held(&dwc->lock);
>
>  	/* Termination may have cleared the error since the tasklet sampled it. */
>  	if (!(dma_readl(dw, RAW.ERROR) & dwc->mask))
> -		goto unlock;
> +		return;
>
>  	dwc_chan_disable(dw, dwc);
>  	clear_bit(DW_DMA_IS_SOFT_LLP, &dwc->flags);
>  	if (list_empty(&dwc->active_list))
> -		goto unlock;
> +		return;
>
>  	/*
>  	 * The descriptor currently at the head of the active list is
> @@ -381,40 +387,41 @@ static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc)
>
>  	/* Pretend the descriptor completed successfully */
>  	dma_cookie_complete(&bad_desc->txd);
> -	spin_unlock_irqrestore(&dwc->lock, flags);
> +	list_add_tail(&bad_desc->desc_node, &dwc->completed);
> +}
>
> -	dwc_descriptor_free(dwc, bad_desc, true);
> -	return;
> +static void dwc_handle_channel(struct dw_dma *dw, struct dw_dma_chan *dwc)
> +{
> +	lockdep_assert_held(&dwc->lock);
>
> -unlock:
> -	spin_unlock_irqrestore(&dwc->lock, flags);
> +	if (dma_readl(dw, RAW.ERROR) & dwc->mask)
> +		dwc_handle_error(dw, dwc);
> +	else
> +		dwc_scan_descriptors(dw, dwc);
>  }
>
>  static void dw_dma_tasklet(struct tasklet_struct *t)
>  {
>  	struct dw_dma *dw = from_tasklet(dw, t, tasklet);
>  	struct dw_dma_chan *dwc;
> -	u32 status_xfer;
> -	u32 status_err;
> +	unsigned long flags;
>  	unsigned int i;
> +	u32 status;
>
>  	/* Channel release may have drained a polled completion before this run. */
>  	if (!dw->in_use)
>  		return;
>
> -	status_xfer = dma_readl(dw, RAW.XFER);
> -	status_err = dma_readl(dw, RAW.ERROR);
> -
> -	dev_vdbg(dw->dma.dev, "%s: status_err=%x\n", __func__, status_err);
> +	status = dma_readl(dw, RAW.XFER) | dma_readl(dw, RAW.ERROR);
>
>  	for (i = 0; i < dw->dma.chancnt; i++) {
>  		dwc = &dw->chan[i];
> -		if (test_bit(DW_DMA_IS_CYCLIC, &dwc->flags))
> -			dev_vdbg(dw->dma.dev, "Cyclic xfer is not implemented\n");
> -		else if (status_err & (1 << i))
> -			dwc_handle_error(dw, dwc);
> -		else if (status_xfer & (1 << i))
> -			dwc_scan_descriptors(dw, dwc);
> +		spin_lock_irqsave(&dwc->lock, flags);
> +		/* The snapshot selects work; recheck events under the channel lock. */
> +		if (status & dwc->mask)
> +			dwc_handle_channel(dw, dwc);
> +		spin_unlock_irqrestore(&dwc->lock, flags);
> +		dwc_complete(dwc);
>  	}
>
>  	/* Re-enable interrupts */
> @@ -890,6 +897,7 @@ static int dwc_terminate_all(struct dma_chan *chan)
>  	list_splice_init(&dwc->active_list, &list);
>  	list_for_each_entry(desc, &list, desc_node)
>  		dma_cookie_complete(&desc->txd);
> +	list_splice_init(&dwc->completed, &list);
>
>  	spin_unlock_irqrestore(&dwc->lock, flags);
>
> @@ -960,10 +968,9 @@ static u32 dwc_get_residue_and_status(struct dw_dma_chan *dwc, dma_cookie_t cook
>  				      enum dma_status *status)
>  {
>  	struct dw_desc *desc;
> -	unsigned long flags;
>  	u32 residue;
>
> -	spin_lock_irqsave(&dwc->lock, flags);
> +	lockdep_assert_held(&dwc->lock);
>
>  	desc = dwc_find_desc(dwc, cookie);
>  	if (desc) {
> @@ -983,7 +990,6 @@ static u32 dwc_get_residue_and_status(struct dw_dma_chan *dwc, dma_cookie_t cook
>  		residue = 0;
>  	}
>
> -	spin_unlock_irqrestore(&dwc->lock, flags);
>  	return residue;
>  }
>
> @@ -993,19 +999,26 @@ dwc_tx_status(struct dma_chan *chan,
>  	      struct dma_tx_state *txstate)
>  {
>  	struct dw_dma_chan	*dwc = to_dw_dma_chan(chan);
> +	struct dw_dma		*dw = to_dw_dma(chan->device);
>  	enum dma_status		ret;
> +	unsigned long		flags;
>
> +	spin_lock_irqsave(&dwc->lock, flags);
>  	ret = dma_cookie_status(chan, cookie, txstate);
>  	if (ret == DMA_COMPLETE)
> -		return ret;
> +		goto unlock;
>
> -	dwc_scan_descriptors(to_dw_dma(chan->device), dwc);
> +	/* Poll completion even when the client has masked the controller IRQ. */
> +	dwc_handle_channel(dw, dwc);
> +	if (!list_empty(&dwc->completed))
> +		tasklet_schedule(&dw->tasklet);
>
>  	ret = dma_cookie_status(chan, cookie, txstate);
> -	if (ret == DMA_COMPLETE)
> -		return ret;
> +	if (ret != DMA_COMPLETE)
> +		dma_set_residue(txstate, dwc_get_residue_and_status(dwc, cookie, &ret));
>
> -	dma_set_residue(txstate, dwc_get_residue_and_status(dwc, cookie, &ret));
> +unlock:
> +	spin_unlock_irqrestore(&dwc->lock, flags);
>  	return ret;
>  }
>
> @@ -1082,8 +1095,11 @@ static void dwc_free_chan_resources(struct dma_chan *chan)
>  {
>  	struct dw_dma_chan	*dwc = to_dw_dma_chan(chan);
>  	struct dw_dma		*dw = to_dw_dma(chan->device);
> +	struct dw_desc		*desc, *_desc;
>  	unsigned long		flags;
> +	LIST_HEAD(list);
>
> +	/* Polled completion can leave descriptors waiting for the tasklet. */
>  	tasklet_disable(&dw->tasklet);
>
>  	dev_dbg(chan2dev(chan), "%s: descs allocated=%u\n", __func__,
> @@ -1095,6 +1111,7 @@ static void dwc_free_chan_resources(struct dma_chan *chan)
>  	BUG_ON(dma_readl(to_dw_dma(chan->device), CH_EN) & dwc->mask);
>
>  	spin_lock_irqsave(&dwc->lock, flags);
> +	list_splice_init(&dwc->completed, &list);
>
>  	/* Clear custom channel configuration */
>  	memset(&dwc->dws, 0, sizeof(struct dw_dma_slave));
> @@ -1106,6 +1123,11 @@ static void dwc_free_chan_resources(struct dma_chan *chan)
>
>  	spin_unlock_irqrestore(&dwc->lock, flags);
>
> +	list_for_each_entry_safe(desc, _desc, &list, desc_node) {
> +		list_del_init(&desc->desc_node);
> +		dwc_descriptor_free(dwc, desc, false);
> +	}
> +

modify dwc_complete() and call it from here also ?

Frank

>  	/* Disable controller in case it was a last user */
>  	dw->in_use &= ~dwc->mask;
>  	if (!dw->in_use)
> @@ -1243,6 +1265,7 @@ int do_dma_probe(struct dw_dma_chip *chip)
>
>  		INIT_LIST_HEAD(&dwc->active_list);
>  		INIT_LIST_HEAD(&dwc->queue);
> +		INIT_LIST_HEAD(&dwc->completed);
>
>  		channel_clear_bit(dw, CH_EN, dwc->mask);
>
> diff --git a/drivers/dma/dw/regs.h b/drivers/dma/dw/regs.h
> index 5969d9cc8d7a..c41f18281efe 100644
> --- a/drivers/dma/dw/regs.h
> +++ b/drivers/dma/dw/regs.h
> @@ -279,6 +279,7 @@ struct dw_dma_chan {
>  	unsigned long		flags;
>  	struct list_head	active_list;
>  	struct list_head	queue;
> +	struct list_head	completed;
>
>  	unsigned int		descs_allocated;
>
>
> --
> 2.53.0
>