Re: [PATCH] dmaengine: mediatek: hsdma: fix runtime PM leak on init failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Premature DMA controller registration exposes uninitialized hardware to consumers.
- [High] Hardware and clocks are disabled before the DMA engine is unregistered during teardown, causing unclocked accesses.
- [High] Unsafe `tasklet_kill()` ordering in `remove` allows tasklets to be rescheduled, leading to Use-After-Free.
- [High] Manual channel list deletion subverts the DMA subsystem's cleanup, leaking sysfs entries.
--

commit a02c13cb6b6d7dd4b66478026a93c3fbb55427ef
Author: Myeonghun Pak <mhun512@gmail.com>

dmaengine: mediatek: hsdma: fix runtime PM leak on init failure

mtk_hsdma_hw_init() enables runtime PM and gets a runtime PM reference
before enabling the HSDMA clock. It currently ignores failures from
pm_runtime_get_sync(); if runtime resume fails, the usage count remains
held. If clk_prepare_enable() then fails, runtime PM is left enabled with
the usage count held.

Use pm_runtime_resume_and_get() so resume failures do not leak the usage
count, and unwind runtime PM when clk_prepare_enable() fails.

> diff --git a/drivers/dma/mediatek/mtk-hsdma.c b/drivers/dma/mediatek/mtk-hsdma.c
> index a43412ff5eddc..31f05d5d04151 100644
> --- a/drivers/dma/mediatek/mtk-hsdma.c
> +++ b/drivers/dma/mediatek/mtk-hsdma.c

[ ... ]

> @@ -983,7 +992,9 @@ static int mtk_hsdma_probe(struct platform_device *pdev)
>  		goto err_unregister;
>  	}
>  
> -	mtk_hsdma_hw_init(hsdma);
> +	err = mtk_hsdma_hw_init(hsdma);
> +	if (err)
> +		goto err_free;

[Severity: High]
This isn't a bug introduced by this patch, but I noticed that premature DMA
controller registration exposes uninitialized hardware to consumers.

In drivers/dma/mediatek/mtk-hsdma.c:mtk_hsdma_probe(),
dma_async_device_register() and of_dma_controller_register() are invoked
before mtk_hsdma_hw_init(). A consumer device probing concurrently can
request a channel via the device tree.

The consumer will then trigger mtk_hsdma_alloc_chan_resources(), which
accesses MTK_HSDMA_GLO to configure the hardware. Because the clock is not
yet enabled by mtk_hsdma_hw_init(), this unclocked access crashes the
system.

Could we initialize the hardware before registering the DMA controller to
avoid this race condition?

>  
>  	err = devm_request_irq(&pdev->dev, hsdma->irq,
>  			       mtk_hsdma_irq, 0,
> @@ -991,7 +1002,7 @@ static int mtk_hsdma_probe(struct platform_device *pdev)
>  	if (err) {
>  		dev_err(&pdev->dev,
>  			"request_irq failed with err %d\n", err);
> -		goto err_free;
> +		goto err_deinit;
>  	}
>  
>  	platform_set_drvdata(pdev, hsdma);
> @@ -1000,8 +1011,9 @@ static int mtk_hsdma_probe(struct platform_device *pdev)
>  
>  	return 0;
>  
> -err_free:
> +err_deinit:
>  	mtk_hsdma_hw_deinit(hsdma);
> +err_free:
>  	of_dma_controller_free(pdev->dev.of_node);
>  err_unregister:
>  	dma_async_device_unregister(dd);

[Severity: High]
This is a pre-existing issue, but hardware and clocks are disabled before
the DMA engine is unregistered during teardown, which can cause unclocked
accesses.

In the probe error path (err_deinit) and in mtk_hsdma_remove(),
mtk_hsdma_hw_deinit() is called before dma_async_device_unregister().
mtk_hsdma_hw_deinit() disables the device clocks.

Since the DMA controller is still registered and active, consumers might
still attempt to use the channels or release them. Releasing a channel
calls mtk_hsdma_free_chan_resources(), which accesses MTK_HSDMA_GLO. With
the clock disabled, this results in an external abort.

Should dma_async_device_unregister() be called before disabling the
hardware?

While reviewing this driver, I also noticed a couple of pre-existing issues
in drivers/dma/mediatek/mtk-hsdma.c:mtk_hsdma_remove():

mtk_hsdma_remove() {
    ...
    list_del(&vc->vc.chan.device_node);
    tasklet_kill(&vc->vc.task);
    ...
    mtk_dma_write(hsdma, MTK_HSDMA_INT_ENABLE, 0);
    synchronize_irq(hsdma->irq);
    ...
    dma_async_device_unregister(&hsdma->ddev);
}

[Severity: High]
This is a pre-existing issue, but the unsafe tasklet_kill() ordering in
mtk_hsdma_remove() allows tasklets to be rescheduled, leading to a potential
use-after-free.

tasklet_kill() is called to stop the tasklets for all virtual channels
before disabling the DMA interrupt (MTK_HSDMA_INT_ENABLE) and synchronizing
IRQs.

If an interrupt fires after the tasklet is killed, the IRQ handler
(mtk_hsdma_irq) will execute, calling vchan_cookie_complete(), which
explicitly schedules the tasklet via tasklet_schedule().

After mtk_hsdma_remove() returns, the device memory is freed by devres,
but the tasklet remains queued and will execute on the freed memory.

Can we disable and synchronize the interrupts before killing the tasklets?


[Severity: High]
This is another pre-existing issue in mtk_hsdma_remove(), but manual channel
list deletion seems to subvert the DMA subsystem's cleanup, leaking sysfs
entries.

The driver manually loops over the channels and calls
list_del(&vc->vc.chan.device_node).

When it subsequently calls dma_async_device_unregister(), the DMA core
subsystem loops over &device->channels to unregister each channel's sysfs
devices (via __dma_async_device_channel_unregister()).

Because the list was prematurely emptied by the driver, the core sees an
empty list, skips the cleanup, and leaks the sysfs structures.

Is it safe to remove the manual list_del() calls so the DMA core can
properly clean up the channels?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624081701.19358-1-mhun512@gmail.com?part=1