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

[sashiko-bot@kernel.org]

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