Re: [PATCH 1/7] dmaengine: ep93xx: drain tasklet before freeing channel resources

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] dmaengine: ep93xx: free_list spliced before tasklet_kill() leaks descriptors
--

commit 2aab3bd053da4cc14229fbf115862f410d9ef2f3
Author: Jiale Yao <yaojiale02@163.com>

dmaengine: ep93xx: drain tasklet before freeing channel resources

This commit frees the channel IRQ and kills the channel tasklet in
ep93xx_dma_free_chan_resources() before freeing descriptors and disabling the
clock.

> diff --git a/drivers/dma/ep93xx_dma.c b/drivers/dma/ep93xx_dma.c
> index 311e55a97ba9f..6134e9ca699dc 100644
> --- a/drivers/dma/ep93xx_dma.c
> +++ b/drivers/dma/ep93xx_dma.c
> @@ -1015,11 +1015,13 @@ static void ep93xx_dma_free_chan_resources(struct dma_chan *chan)
>  	list_splice_init(&edmac->free_list, &list);
>  	spin_unlock_irqrestore(&edmac->lock, flags);
>  
> +	free_irq(edmac->irq, edmac);
> +	tasklet_kill(&edmac->tasklet);

[Severity: High]
Should free_irq() and tasklet_kill() be called before checking edmac->active
and splicing edmac->free_list in ep93xx_dma_free_chan_resources()?

In ep93xx_dma_tasklet(), completed descriptors are moved from edmac->active
to a stack-local list under edmac->lock, and only returned to
edmac->free_list after dropping the lock:

drivers/dma/ep93xx_dma.c:ep93xx_dma_tasklet() {
    ...
	if (desc) {
		if (desc->complete) {
			/* mark descriptor complete for non cyclic case only */
			if (!test_bit(EP93XX_DMA_IS_CYCLIC, &edmac->flags))
				dma_cookie_complete(&desc->txd);
			list_splice_init(&edmac->active, &list);
		}
		dmaengine_desc_get_callback(&desc->txd, &cb);
	}
	spin_unlock_irq(&edmac->lock);

	/* Pick up the next descriptor from the queue */
	ep93xx_dma_advance_work(edmac);

	/* Now we can release all the chained descriptors */
	list_for_each_entry_safe(desc, d, &list, node) {
		dma_descriptor_unmap(&desc->txd);\
		ep93xx_dma_desc_put(edmac, desc);
	}
    ...
}

If ep93xx_dma_tasklet() is running concurrently and has already spliced
edmac->active into its local list and dropped edmac->lock,
ep93xx_dma_free_chan_resources() will splice edmac->free_list into its own
local list before waiting in tasklet_kill(&edmac->tasklet). While
tasklet_kill() waits for the tasklet to finish, ep93xx_dma_desc_put() will
place those in-flight struct ep93xx_dma_desc descriptors back onto
edmac->free_list, where they will not be freed by the subsequent kfree()
loop.

Additionally, if a tasklet scheduled by ep93xx_dma_interrupt() on
INTERRUPT_DONE has not yet entered its locked section when
ep93xx_dma_free_chan_resources() starts, edmac->active will still be
non-empty and hit BUG_ON(!list_empty(&edmac->active)) at the top of
ep93xx_dma_free_chan_resources().

Would draining the IRQ and tasklet before the BUG_ON() checks and
list_splice_init(&edmac->free_list, &list) avoid leaking descriptors and
hitting the BUG_ON()?

> +
>  	list_for_each_entry_safe(desc, d, &list, node)
>  		kfree(desc);
>  
>  	clk_disable_unprepare(edmac->clk);
> -	free_irq(edmac->irq, edmac);
>  }
>  
>  /**

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018040724.534641-1-yaojiale02@163.com?part=1