[PATCH] dmaengine: mediatek: uart: make channel termination atomic

[PATCH] dmaengine: mediatek: uart: make channel termination atomic

[James Hilliard]

The 8250 transmit flush path calls dmaengine_terminate_async() with the
UART port spinlock held and interrupts disabled. UART APDMA termination
uses sleeping register polls and synchronize_irq(), which can also wait for
a threaded handler or for an interrupt pending on the calling CPU.

Serialize the stop sequence with the interrupt handler using the virtual
channel lock and use atomic register polling. A handler that was waiting
for that lock must check interrupt status before accessing a descriptor:
termination may already have masked and acknowledged its interrupt. This
allows termination to retire descriptors without waiting for the handler.

If the channel does not stop, return the timeout and retain its descriptors
instead of freeing state still owned by the transfer. Continue attempting
STOP after a flush timeout, since stopping the channel is sufficient for
termination even when the flush could not finish.

Fixes: 9135408c3ace ("dmaengine: mediatek: Add MediaTek UART APDMA support")
Cc: stable@vger.kernel.org
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 drivers/dma/mediatek/mtk-uart-apdma.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/dma/mediatek/mtk-uart-apdma.c b/drivers/dma/mediatek/mtk-uart-apdma.c
index f74e9a328588..73d4b701ac85 100644
--- a/drivers/dma/mediatek/mtk-uart-apdma.c
+++ b/drivers/dma/mediatek/mtk-uart-apdma.c
@@ -256,6 +256,12 @@ static irqreturn_t mtk_uart_apdma_irq_handler(int irq, void *dev_id)
 	unsigned long flags;
 
 	spin_lock_irqsave(&c->vc.lock, flags);
+	/* Termination may have acknowledged an interrupt before we got the lock. */
+	if (!mtk_uart_apdma_read(c, VFF_INT_FLAG)) {
+		spin_unlock_irqrestore(&c->vc.lock, flags);
+		return IRQ_NONE;
+	}
+
 	if (c->dir == DMA_DEV_TO_MEM)
 		mtk_uart_apdma_rx_handler(c);
 	else if (c->dir == DMA_MEM_TO_DEV)
@@ -400,10 +406,12 @@ static int mtk_uart_apdma_terminate_all(struct dma_chan *chan)
 	LIST_HEAD(head);
 	int ret;
 
+	spin_lock_irqsave(&c->vc.lock, flags);
+
 	mtk_uart_apdma_write(c, VFF_FLUSH, VFF_FLUSH_B);
 
-	ret = readx_poll_timeout(readl, c->base + VFF_FLUSH,
-			  status, status != VFF_FLUSH_B, 10, 100);
+	ret = readx_poll_timeout_atomic(readl, c->base + VFF_FLUSH,
+					status, status != VFF_FLUSH_B, 10, 100);
 	if (ret)
 		dev_err(c->vc.chan.device->dev, "flush: fail, status=0x%x\n",
 			mtk_uart_apdma_read(c, VFF_DEBUG_STATUS));
@@ -415,11 +423,14 @@ static int mtk_uart_apdma_terminate_all(struct dma_chan *chan)
 	 * 3. set stop as 0
 	 */
 	mtk_uart_apdma_write(c, VFF_STOP, VFF_STOP_B);
-	ret = readx_poll_timeout(readl, c->base + VFF_EN,
-			  status, !status, 10, 100);
-	if (ret)
+	ret = readx_poll_timeout_atomic(readl, c->base + VFF_EN,
+					status, !status, 10, 100);
+	if (ret) {
 		dev_err(c->vc.chan.device->dev, "stop: fail, status=0x%x\n",
 			mtk_uart_apdma_read(c, VFF_DEBUG_STATUS));
+		spin_unlock_irqrestore(&c->vc.lock, flags);
+		return ret;
+	}
 
 	mtk_uart_apdma_write(c, VFF_STOP, VFF_STOP_CLR_B);
 	mtk_uart_apdma_write(c, VFF_INT_EN, VFF_INT_EN_CLR_B);
@@ -429,9 +440,6 @@ static int mtk_uart_apdma_terminate_all(struct dma_chan *chan)
 	else if (c->dir == DMA_MEM_TO_DEV)
 		mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_TX_INT_CLR_B);
 
-	synchronize_irq(c->irq);
-
-	spin_lock_irqsave(&c->vc.lock, flags);
 	vchan_get_all_descriptors(&c->vc, &head);
 	spin_unlock_irqrestore(&c->vc.lock, flags);
 

---
base-commit: 93f51579e7df248780214094418f205253383cc5
change-id: 20261022-submit-mtk-uart-dma-terminate-atomic-v1-19693366ae4c

Best regards,
--  
James Hilliard <james.hilliard1@gmail.com>

Re: [PATCH] dmaengine: mediatek: uart: make channel termination atomic

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] dmaengine: mediatek: dangling c->desc in mtk_uart_apdma_terminate_all()
--

commit add32020b23bda4849b015bc2e7b816090491c89
Author: James Hilliard <james.hilliard1@gmail.com>

dmaengine: mediatek: uart: make channel termination atomic

This commit updates mtk_uart_apdma_terminate_all() to serialize hardware
channel termination under c->vc.lock using atomic register polling instead of
sleeping polls and synchronize_irq(). It also checks VFF_INT_FLAG in
mtk_uart_apdma_irq_handler() under c->vc.lock in case termination already
acknowledged the interrupt, and aborts descriptor cleanup if stopping the
channel times out.

> diff --git a/drivers/dma/mediatek/mtk-uart-apdma.c b/drivers/dma/mediatek/mtk-uart-apdma.c
> index f74e9a3285887..73d4b701ac858 100644
> --- a/drivers/dma/mediatek/mtk-uart-apdma.c
> +++ b/drivers/dma/mediatek/mtk-uart-apdma.c
> @@ -256,6 +256,12 @@ static irqreturn_t mtk_uart_apdma_irq_handler(int irq, void *dev_id)
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&c->vc.lock, flags);
> +	/* Termination may have acknowledged an interrupt before we got the lock. */
> +	if (!mtk_uart_apdma_read(c, VFF_INT_FLAG)) {
> +		spin_unlock_irqrestore(&c->vc.lock, flags);
> +		return IRQ_NONE;
> +	}
> +
>  	if (c->dir == DMA_DEV_TO_MEM)
>  		mtk_uart_apdma_rx_handler(c);
>  	else if (c->dir == DMA_MEM_TO_DEV)
> @@ -400,10 +406,12 @@ static int mtk_uart_apdma_terminate_all(struct dma_chan *chan)
>  	LIST_HEAD(head);
>  	int ret;
>  
> +	spin_lock_irqsave(&c->vc.lock, flags);
> +
>  	mtk_uart_apdma_write(c, VFF_FLUSH, VFF_FLUSH_B);
>  
> -	ret = readx_poll_timeout(readl, c->base + VFF_FLUSH,
> -			  status, status != VFF_FLUSH_B, 10, 100);
> +	ret = readx_poll_timeout_atomic(readl, c->base + VFF_FLUSH,
> +					status, status != VFF_FLUSH_B, 10, 100);
>  	if (ret)
>  		dev_err(c->vc.chan.device->dev, "flush: fail, status=0x%x\n",
>  			mtk_uart_apdma_read(c, VFF_DEBUG_STATUS));
> @@ -415,11 +423,14 @@ static int mtk_uart_apdma_terminate_all(struct dma_chan *chan)
>  	 * 3. set stop as 0
>  	 */
>  	mtk_uart_apdma_write(c, VFF_STOP, VFF_STOP_B);
> -	ret = readx_poll_timeout(readl, c->base + VFF_EN,
> -			  status, !status, 10, 100);
> -	if (ret)
> +	ret = readx_poll_timeout_atomic(readl, c->base + VFF_EN,
> +					status, !status, 10, 100);
> +	if (ret) {
>  		dev_err(c->vc.chan.device->dev, "stop: fail, status=0x%x\n",
>  			mtk_uart_apdma_read(c, VFF_DEBUG_STATUS));
> +		spin_unlock_irqrestore(&c->vc.lock, flags);
> +		return ret;
> +	}
>  
>  	mtk_uart_apdma_write(c, VFF_STOP, VFF_STOP_CLR_B);
>  	mtk_uart_apdma_write(c, VFF_INT_EN, VFF_INT_EN_CLR_B);
> @@ -429,9 +440,6 @@ static int mtk_uart_apdma_terminate_all(struct dma_chan *chan)
>  	else if (c->dir == DMA_MEM_TO_DEV)
>  		mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_TX_INT_CLR_B);
>  
> -	synchronize_irq(c->irq);
> -
> -	spin_lock_irqsave(&c->vc.lock, flags);
>  	vchan_get_all_descriptors(&c->vc, &head);
>  	spin_unlock_irqrestore(&c->vc.lock, flags);

[Severity: High]
When mtk_uart_apdma_terminate_all() runs while a transfer is active on the
channel (for example, via dmaengine_terminate_async() from
serial8250_tx_dma_flush() or serial8250_rx_dma_flush()), does c->desc remain a
dangling pointer to freed memory?

When a transfer is started in mtk_uart_apdma_issue_pending(), the active
descriptor remains on c->vc.desc_issued while c->desc points to it:

drivers/dma/mediatek/mtk-uart-apdma.c:mtk_uart_apdma_issue_pending() {
	...
	if (vchan_issue_pending(&c->vc) && !c->desc) {
		vd = vchan_next_desc(&c->vc);
		c->desc = to_mtk_uart_apdma_desc(&vd->tx);
	...
}

The only place that clears c->desc is mtk_uart_apdma_chan_complete_handler(),
called from mtk_uart_apdma_irq_handler():

drivers/dma/mediatek/mtk-uart-apdma.c:mtk_uart_apdma_chan_complete_handler() {
	struct mtk_uart_apdma_desc *d = c->desc;

	if (d) {
		list_del(&d->vd.node);
		vchan_cookie_complete(&d->vd);
		c->desc = NULL;
	}
}

In mtk_uart_apdma_terminate_all(), VFF_INT_EN and VFF_INT_FLAG are cleared
under c->vc.lock (so any pending mtk_uart_apdma_irq_handler() sees
!mtk_uart_apdma_read(c, VFF_INT_FLAG) and returns IRQ_NONE early), and
vchan_get_all_descriptors(&c->vc, &head) splices c->vc.desc_issued into head
before vchan_dma_desc_free_list(&c->vc, &head) frees the descriptor.

Because c->desc is not cleared during termination, subsequent calls to
mtk_uart_apdma_issue_pending() will see a non-NULL c->desc and refuse to start
any new transfers on the channel, and any later interrupt on the channel would
dereference the freed descriptor in mtk_uart_apdma_rx_handler() and
mtk_uart_apdma_chan_complete_handler().

Should mtk_uart_apdma_terminate_all() also set c->desc = NULL while holding
c->vc.lock?

>  
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-submit-mtk-uart-dma-terminate-atomic-v1-v1-1-aa498ba5da25@gmail.com?part=1

Re: [PATCH] dmaengine: mediatek: uart: make channel termination atomic

[James Hilliard]

On Thu, Oct 22, 2026 at 3:25 PM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] dmaengine: mediatek: dangling c->desc in mtk_uart_apdma_terminate_all()
> --
>
> commit add32020b23bda4849b015bc2e7b816090491c89
> Author: James Hilliard <james.hilliard1@gmail.com>
>
> dmaengine: mediatek: uart: make channel termination atomic
>
> This commit updates mtk_uart_apdma_terminate_all() to serialize hardware
> channel termination under c->vc.lock using atomic register polling instead of
> sleeping polls and synchronize_irq(). It also checks VFF_INT_FLAG in
> mtk_uart_apdma_irq_handler() under c->vc.lock in case termination already
> acknowledged the interrupt, and aborts descriptor cleanup if stopping the
> channel times out.
>
> > diff --git a/drivers/dma/mediatek/mtk-uart-apdma.c b/drivers/dma/mediatek/mtk-uart-apdma.c
> > index f74e9a3285887..73d4b701ac858 100644
> > --- a/drivers/dma/mediatek/mtk-uart-apdma.c
> > +++ b/drivers/dma/mediatek/mtk-uart-apdma.c
> > @@ -256,6 +256,12 @@ static irqreturn_t mtk_uart_apdma_irq_handler(int irq, void *dev_id)
> >       unsigned long flags;
> >
> >       spin_lock_irqsave(&c->vc.lock, flags);
> > +     /* Termination may have acknowledged an interrupt before we got the lock. */
> > +     if (!mtk_uart_apdma_read(c, VFF_INT_FLAG)) {
> > +             spin_unlock_irqrestore(&c->vc.lock, flags);
> > +             return IRQ_NONE;
> > +     }
> > +
> >       if (c->dir == DMA_DEV_TO_MEM)
> >               mtk_uart_apdma_rx_handler(c);
> >       else if (c->dir == DMA_MEM_TO_DEV)
> > @@ -400,10 +406,12 @@ static int mtk_uart_apdma_terminate_all(struct dma_chan *chan)
> >       LIST_HEAD(head);
> >       int ret;
> >
> > +     spin_lock_irqsave(&c->vc.lock, flags);
> > +
> >       mtk_uart_apdma_write(c, VFF_FLUSH, VFF_FLUSH_B);
> >
> > -     ret = readx_poll_timeout(readl, c->base + VFF_FLUSH,
> > -                       status, status != VFF_FLUSH_B, 10, 100);
> > +     ret = readx_poll_timeout_atomic(readl, c->base + VFF_FLUSH,
> > +                                     status, status != VFF_FLUSH_B, 10, 100);
> >       if (ret)
> >               dev_err(c->vc.chan.device->dev, "flush: fail, status=0x%x\n",
> >                       mtk_uart_apdma_read(c, VFF_DEBUG_STATUS));
> > @@ -415,11 +423,14 @@ static int mtk_uart_apdma_terminate_all(struct dma_chan *chan)
> >        * 3. set stop as 0
> >        */
> >       mtk_uart_apdma_write(c, VFF_STOP, VFF_STOP_B);
> > -     ret = readx_poll_timeout(readl, c->base + VFF_EN,
> > -                       status, !status, 10, 100);
> > -     if (ret)
> > +     ret = readx_poll_timeout_atomic(readl, c->base + VFF_EN,
> > +                                     status, !status, 10, 100);
> > +     if (ret) {
> >               dev_err(c->vc.chan.device->dev, "stop: fail, status=0x%x\n",
> >                       mtk_uart_apdma_read(c, VFF_DEBUG_STATUS));
> > +             spin_unlock_irqrestore(&c->vc.lock, flags);
> > +             return ret;
> > +     }
> >
> >       mtk_uart_apdma_write(c, VFF_STOP, VFF_STOP_CLR_B);
> >       mtk_uart_apdma_write(c, VFF_INT_EN, VFF_INT_EN_CLR_B);
> > @@ -429,9 +440,6 @@ static int mtk_uart_apdma_terminate_all(struct dma_chan *chan)
> >       else if (c->dir == DMA_MEM_TO_DEV)
> >               mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_TX_INT_CLR_B);
> >
> > -     synchronize_irq(c->irq);
> > -
> > -     spin_lock_irqsave(&c->vc.lock, flags);
> >       vchan_get_all_descriptors(&c->vc, &head);
> >       spin_unlock_irqrestore(&c->vc.lock, flags);
>
> [Severity: High]
> When mtk_uart_apdma_terminate_all() runs while a transfer is active on the
> channel (for example, via dmaengine_terminate_async() from
> serial8250_tx_dma_flush() or serial8250_rx_dma_flush()), does c->desc remain a
> dangling pointer to freed memory?
>
> When a transfer is started in mtk_uart_apdma_issue_pending(), the active
> descriptor remains on c->vc.desc_issued while c->desc points to it:
>
> drivers/dma/mediatek/mtk-uart-apdma.c:mtk_uart_apdma_issue_pending() {
>         ...
>         if (vchan_issue_pending(&c->vc) && !c->desc) {
>                 vd = vchan_next_desc(&c->vc);
>                 c->desc = to_mtk_uart_apdma_desc(&vd->tx);
>         ...
> }
>
> The only place that clears c->desc is mtk_uart_apdma_chan_complete_handler(),
> called from mtk_uart_apdma_irq_handler():
>
> drivers/dma/mediatek/mtk-uart-apdma.c:mtk_uart_apdma_chan_complete_handler() {
>         struct mtk_uart_apdma_desc *d = c->desc;
>
>         if (d) {
>                 list_del(&d->vd.node);
>                 vchan_cookie_complete(&d->vd);
>                 c->desc = NULL;
>         }
> }
>
> In mtk_uart_apdma_terminate_all(), VFF_INT_EN and VFF_INT_FLAG are cleared
> under c->vc.lock (so any pending mtk_uart_apdma_irq_handler() sees
> !mtk_uart_apdma_read(c, VFF_INT_FLAG) and returns IRQ_NONE early), and
> vchan_get_all_descriptors(&c->vc, &head) splices c->vc.desc_issued into head
> before vchan_dma_desc_free_list(&c->vc, &head) frees the descriptor.
>
> Because c->desc is not cleared during termination, subsequent calls to
> mtk_uart_apdma_issue_pending() will see a non-NULL c->desc and refuse to start
> any new transfers on the channel, and any later interrupt on the channel would
> dereference the freed descriptor in mtk_uart_apdma_rx_handler() and
> mtk_uart_apdma_chan_complete_handler().
>
> Should mtk_uart_apdma_terminate_all() also set c->desc = NULL while holding
> c->vc.lock?

This issue is fixed separately here:
https://lore.kernel.org/all/20261022-submit-mtk-uart-dma-retire-v1-v1-1-3f9b99a9d89a@gmail.com/

>
> >
> >
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20261022-submit-mtk-uart-dma-terminate-atomic-v1-v1-1-aa498ba5da25@gmail.com?part=1

Re: [PATCH] dmaengine: mediatek: uart: make channel termination atomic

[Frank Li]

On Thu, Oct 22, 2026 at 03:18:26PM -0600, James Hilliard wrote:
> The 8250 transmit flush path calls dmaengine_terminate_async() with the
> UART port spinlock held and interrupts disabled. UART APDMA termination
> uses sleeping register polls and synchronize_irq(), which can also wait for
> a threaded handler or for an interrupt pending on the calling CPU.
>
> Serialize the stop sequence with the interrupt handler using the virtual
> channel lock and use atomic register polling. A handler that was waiting
> for that lock must check interrupt status before accessing a descriptor:
> termination may already have masked and acknowledged its interrupt. This
> allows termination to retire descriptors without waiting for the handler.
>
> If the channel does not stop, return the timeout and retain its descriptors
> instead of freeing state still owned by the transfer. Continue attempting
> STOP after a flush timeout, since stopping the channel is sufficient for
> termination even when the flush could not finish.
>
> Fixes: 9135408c3ace ("dmaengine: mediatek: Add MediaTek UART APDMA support")
> Cc: stable@vger.kernel.org
> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/dma/mediatek/mtk-uart-apdma.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/dma/mediatek/mtk-uart-apdma.c b/drivers/dma/mediatek/mtk-uart-apdma.c
> index f74e9a328588..73d4b701ac85 100644
> --- a/drivers/dma/mediatek/mtk-uart-apdma.c
> +++ b/drivers/dma/mediatek/mtk-uart-apdma.c
> @@ -256,6 +256,12 @@ static irqreturn_t mtk_uart_apdma_irq_handler(int irq, void *dev_id)
>  	unsigned long flags;
>
>  	spin_lock_irqsave(&c->vc.lock, flags);
> +	/* Termination may have acknowledged an interrupt before we got the lock. */
> +	if (!mtk_uart_apdma_read(c, VFF_INT_FLAG)) {
> +		spin_unlock_irqrestore(&c->vc.lock, flags);
> +		return IRQ_NONE;
> +	}
> +
>  	if (c->dir == DMA_DEV_TO_MEM)
>  		mtk_uart_apdma_rx_handler(c);
>  	else if (c->dir == DMA_MEM_TO_DEV)
> @@ -400,10 +406,12 @@ static int mtk_uart_apdma_terminate_all(struct dma_chan *chan)
>  	LIST_HEAD(head);
>  	int ret;
>
> +	spin_lock_irqsave(&c->vc.lock, flags);
> +
>  	mtk_uart_apdma_write(c, VFF_FLUSH, VFF_FLUSH_B);
>
> -	ret = readx_poll_timeout(readl, c->base + VFF_FLUSH,
> -			  status, status != VFF_FLUSH_B, 10, 100);
> +	ret = readx_poll_timeout_atomic(readl, c->base + VFF_FLUSH,
> +					status, status != VFF_FLUSH_B, 10, 100);
>  	if (ret)
>  		dev_err(c->vc.chan.device->dev, "flush: fail, status=0x%x\n",
>  			mtk_uart_apdma_read(c, VFF_DEBUG_STATUS));
> @@ -415,11 +423,14 @@ static int mtk_uart_apdma_terminate_all(struct dma_chan *chan)
>  	 * 3. set stop as 0
>  	 *
>  	mtk_uart_apdma_write(c, VFF_STOP, VFF_STOP_B);
> -	ret = readx_poll_timeout(readl, c->base + VFF_EN,
> -			  status, !status, 10, 100);
> -	if (ret)
> +	ret = readx_poll_timeout_atomic(readl, c->base + VFF_EN,
> +					status, !status, 10, 100);
> +	if (ret) {
>  		dev_err(c->vc.chan.device->dev, "stop: fail, status=0x%x\n",
>  			mtk_uart_apdma_read(c, VFF_DEBUG_STATUS));
> +		spin_unlock_irqrestore(&c->vc.lock, flags);
> +		return ret;
> +	}
>
>  	mtk_uart_apdma_write(c, VFF_STOP, VFF_STOP_CLR_B);
>  	mtk_uart_apdma_write(c, VFF_INT_EN, VFF_INT_EN_CLR_B);
> @@ -429,9 +440,6 @@ static int mtk_uart_apdma_terminate_all(struct dma_chan *chan)
>  	else if (c->dir == DMA_MEM_TO_DEV)
>  		mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_TX_INT_CLR_B);
>
> -	synchronize_irq(c->irq);
> -
> -	spin_lock_irqsave(&c->vc.lock, flags);
>  	vchan_get_all_descriptors(&c->vc, &head);
>  	spin_unlock_irqrestore(&c->vc.lock, flags);
>
>
> ---
> base-commit: 93f51579e7df248780214094418f205253383cc5
> change-id: 20261022-submit-mtk-uart-dma-terminate-atomic-v1-19693366ae4c
>
> Best regards,
> --
> James Hilliard <james.hilliard1@gmail.com>
>