Re: [PATCH] dmaengine: dw-axi-dmac: fix cfgr_clk leak when core_clk enable fails

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

Pre-existing issues:
- [High] Probe error paths in `dw_probe()` fail to properly unwind PM runtime usage counts, suspend hardware, disable clocks, and kill tasklets.
- [High] `dw_remove()` manually manages clocks without considering PM runtime state, ignores return values, and permanently leaks clocks.
- [High] Premature registration of a shared IRQ handler exposes uninitialized spinlocks and disabled clocks to asynchronous execution.
- [High] `axi_req_irqs()` silently fails to handle negative error codes from `platform_irq_count()`, turning probe deferral into a permanent functional failure.
- [High] `axi_req_irqs()` causes an out-of-bounds heap write if the Device Tree provides more interrupts than `DMAC_MAX_CHANNELS`.
- [High] Manual removal of channels from the DMA device list in `dw_remove()` permanently leaks sysfs entries and percpu memory.
--

commit e8b14e92b204535fe4c9b4b46beb8535c9f3137d
Author: Li Youhong <liyouhong@kylinos.cn>

dmaengine: dw-axi-dmac: fix cfgr_clk leak when core_clk enable fails

This commit modifies axi_dma_resume() to fix a clock leak. If enabling core_clk
fails, it now disables cfgr_clk on the error path before returning.

> diff --git a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> index eebed2474210d..6ccb3fcfa3972 100644
> --- a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> +++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> @@ -1335,8 +1335,10 @@ static int axi_dma_resume(struct axi_dma_chip *chip)
>  		return ret;
>  
>  	ret = clk_prepare_enable(chip->core_clk);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		clk_disable_unprepare(chip->cfgr_clk);
>  		return ret;
> +	}
>  
>  	axi_dma_enable(chip);
>  	axi_dma_irq_enable(chip);

[Severity: High]
This isn't a bug introduced by this patch, but looking at dw_probe(), 
there are multiple error paths that appear to leak resources when failing:

	pm_runtime_get_noresume(chip->dev);
	ret = axi_dma_resume(chip);
	if (ret < 0)
		goto err_pm_disable;

When axi_dma_resume() fails, the goto bypasses pm_runtime_put_noidle(),
which permanently leaks the PM runtime usage counter.

Additionally, if dmaenginem_async_device_register() fails:

	pm_runtime_put(chip->dev);

	ret = dmaenginem_async_device_register(&dw->dma);
	if (ret)
		goto err_pm_disable;
...
err_pm_disable:
	pm_runtime_disable(chip->dev);

Since pm_runtime_put() schedules an async suspend, calling
pm_runtime_disable() immediately after cancels the pending suspend. This
leaves the hardware active and clocks permanently enabled. axi_dma_suspend()
and tasklet_kill() are also not called here. If an incoming interrupt fires
before the devm-managed IRQ is freed, could it schedule a tasklet that
executes after the channel memory has been freed by devres?

[Severity: High]
This is a pre-existing issue, but does dw_remove() properly manage
clocks and PM state?

static void dw_remove(struct platform_device *pdev)
{
...
	/* Enable clk before accessing to registers */
	clk_prepare_enable(chip->cfgr_clk);
	clk_prepare_enable(chip->core_clk);
...
	pm_runtime_disable(chip->dev);
	axi_dma_suspend(chip);
...

The return values for clk_prepare_enable() are ignored, which could cause a
synchronous bus error on subsequent register accesses if the clocks fail to
enable. Furthermore, if the device was already active with clocks enabled,
this blind increment raises the count to 2. The subsequent call to
axi_dma_suspend() decrements it back to 1, permanently leaking the clocks.

[Severity: High]
This is a pre-existing issue, but could the shared IRQ requested in dw_probe()
fire too early?

	ret = axi_req_irqs(pdev, chip);
	if (ret)
		return ret;

	INIT_LIST_HEAD(&dw->dma.channels);

axi_req_irqs() requests the IRQ with IRQF_SHARED before vchan_init()
initializes the channel spinlocks and before axi_dma_resume() enables the
device clocks. If another device sharing the same IRQ line triggers an
interrupt during this window, dw_axi_dma_interrupt() will immediately execute.
Will this cause it to read DMA registers without a clock and acquire
uninitialized spinlocks?

[Severity: High]
This is a pre-existing issue, but does axi_req_irqs() properly handle probe
deferral and negative error codes?

static int axi_req_irqs(struct platform_device *pdev, struct axi_dma_chip *chip)
{
	int irq_count = platform_irq_count(pdev);
	int ret;

	for (int i = 0; i < irq_count; i++) {

If platform_irq_count() returns a negative error code like -EPROBE_DEFER,
irq_count is negative. The loop condition i < irq_count evaluates to false
immediately, bypassing the loop. This silently hides the error and returns 0,
resulting in a successful probe without any interrupts registered.

[Severity: High]
This is a pre-existing issue, but does axi_req_irqs() prevent an out-of-bounds
write when processing the interrupts?

	for (int i = 0; i < irq_count; i++) {
		chip->irq[i] = platform_get_irq(pdev, i);

chip->irq is a statically sized array of DMAC_MAX_CHANNELS elements. If the
device tree provides more interrupts than DMAC_MAX_CHANNELS, won't this loop
overwrite memory past the end of the array?

[Severity: High]
This is a pre-existing issue, but does dw_remove() cause memory and sysfs
entry leaks by prematurely removing channels?

	list_for_each_entry_safe(chan, _chan, &dw->dma.channels,
				 vc.chan.device_node) {
		list_del(&chan->vc.chan.device_node);
		tasklet_kill(&chan->vc.task);
	}

By manually calling list_del() and emptying the list, the devres unregistration
callback (dmaenginem_async_device_unregister) will iterate over an empty list.
Does this cause it to skip calling device_unregister() and free_percpu() for
the channels?

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