* Re: [PATCH v2] dmaengine: mediatek: hsdma: fix runtime PM leak on init failure
2026-06-27 13:49 [PATCH v2] dmaengine: mediatek: hsdma: fix runtime PM leak on init failure Myeonghun Pak
@ 2026-06-27 14:01 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-06-27 14:01 UTC (permalink / raw)
To: Myeonghun Pak; +Cc: vkoul, dmaengine, Frank.Li
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Tasklets are killed before disabling hardware interrupts, creating a race condition that leads to use-after-free.
- [High] The DMA controller is registered and exposed to the system before its hardware is initialized, creating a window for unclocked hardware access.
- [High] Hardware is deactivated before the DMA device is unregistered, allowing active clients to access unclocked hardware.
- [High] Acquiring a spinlock without disabling IRQs in process context risks a deadlock with the hardware interrupt handler.
- [High] The driver disables the hardware clock without waiting for the DMA controller to become idle, potentially causing bus hangs.
- [Medium] Manual removal of channels from the core device list in `remove()` prevents the DMA engine core from unregistering them, causing sysfs and memory leaks.
--
commit 88d1e6220aea175827fde9c64724395b19cd385b
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, the usage count remains held.
Use devm_pm_runtime_enable() to manage runtime PM enablement, and use
pm_runtime_resume_and_get() so resume failures do not leak the usage count.
If clk_prepare_enable() fails after a successful runtime resume, drop the
runtime PM reference before returning.
The probe path also ignores the return value from mtk_hsdma_hw_init(), so a
failed hardware init can continue as a successful probe. Propagate
mtk_hsdma_hw_init() failures from probe, while keeping a separate unwind
label so mtk_hsdma_hw_deinit() is only called after hardware init succeeds.
> diff --git a/drivers/dma/mediatek/mtk-hsdma.c b/drivers/dma/mediatek/mtk-hsdma.c
> index a43412ff5eddc..27cb370106c77 100644
> --- a/drivers/dma/mediatek/mtk-hsdma.c
> +++ b/drivers/dma/mediatek/mtk-hsdma.c
[ ... ]
> @@ -868,7 +878,6 @@ static int mtk_hsdma_hw_deinit(struct mtk_hsdma_device *hsdma)
> mtk_dma_write(hsdma, MTK_HSDMA_GLO, 0);
>
> clk_disable_unprepare(hsdma->clk);
[Severity: High]
This is a pre-existing issue, but does disabling the hardware clock without
waiting for the DMA controller to become idle risk bus hangs?
In mtk_hsdma_hw_deinit(), the driver clears the MTK_HSDMA_GLO register and
immediately calls clk_disable_unprepare(). If the DMA engine is actively
flushing data to the bus, abruptly disabling the clock can freeze the system
or cause imprecise external aborts.
Should it poll MTK_HSDMA_GLO_BUSY using mtk_hsdma_busy_wait() prior to
disabling the clock?
>
> pm_runtime_put_sync(hsdma2dev(hsdma));
[ ... ]
> @@ -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);
[Severity: High]
This isn't a bug introduced by this patch, but does registering the DMA
controller before its hardware is initialized create a window for unclocked
hardware access?
In mtk_hsdma_probe(), dma_async_device_register() is called before
mtk_hsdma_hw_init() enables the hardware clocks and PM:
err = dma_async_device_register(dd);
if (err)
return err;
err = of_dma_controller_register(pdev->dev.of_node,
of_dma_xlate_by_chan_id, hsdma);
if (err) {
...
goto err_unregister;
}
err = mtk_hsdma_hw_init(hsdma);
This makes channels available to clients before initialization completes. If a
client requests a channel immediately (e.g., via deferred probe) and triggers
mtk_hsdma_alloc_chan_resources() before mtk_hsdma_hw_init() completes, it
will access unclocked hardware registers, potentially causing a system crash
via imprecise external abort.
> + if (err)
> + goto err_free;
>
> err = devm_request_irq(&pdev->dev, hsdma->irq,
[ ... ]
> @@ -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 does deactivating the hardware before
unregistering the DMA device allow active clients to access unclocked
hardware?
In both the probe error path and mtk_hsdma_remove(), mtk_hsdma_hw_deinit()
cuts the clock and drops PM before dma_async_device_unregister() runs.
If a client holds a channel when the DMA provider unbinds, calling
dma_release_channel() will trigger mtk_hsdma_free_chan_resources(), which
accesses unclocked registers because the hardware was prematurely deactivated.
This can cause a system crash via imprecise external abort.
[Severity: Critical]
This is a pre-existing issue, but are tasklets killed before disabling
hardware interrupts in mtk_hsdma_remove()?
Looking at mtk_hsdma_remove(), tasklet_kill() runs while MTK_HSDMA_INT_ENABLE
is still active:
tasklet_kill(&vc->vc.task);
...
/* Disable DMA interrupt */
mtk_dma_write(hsdma, MTK_HSDMA_INT_ENABLE, 0);
/* Waits for any pending IRQ handlers to complete */
synchronize_irq(hsdma->irq);
If an interrupt fires immediately after tasklet_kill(), mtk_hsdma_irq()
handles it and calls vchan_cookie_complete(), which reschedules the tasklet.
After remove() returns, the devres-managed memory for the channel is freed,
but the tasklet remains queued and will execute on freed memory, causing a
kernel panic.
[Severity: Medium]
This is a pre-existing issue, but does manual removal of channels from the
core device list in mtk_hsdma_remove() prevent the DMA engine core from
unregistering them?
In mtk_hsdma_remove():
list_del(&vc->vc.chan.device_node);
This empties the core's channel list. When dma_async_device_unregister()
subsequently iterates over device->channels, the list is empty, bypassing
__dma_async_device_channel_unregister() completely, which causes sysfs and
memory leaks.
[Severity: High]
This is a pre-existing issue, but does acquiring a spinlock without disabling
IRQs in process context risk a deadlock with the hardware interrupt handler?
In mtk_hsdma_free_active_desc():
spin_lock(&hvc->vc.lock);
if (!list_empty(&hvc->desc_hw_processing)) {
hvc->issue_synchronize = true;
sync_needed = true;
}
spin_unlock(&hvc->vc.lock);
This is called from process context (via dmaengine_terminate_all()) and
acquires hvc->vc.lock using plain spin_lock(). The hardware interrupt handler
mtk_hsdma_irq() also acquires this lock. If the interrupt fires while the lock
is held in process context, it will spin forever causing a hard deadlock.
Should this use spin_lock_irqsave() instead?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260627134915.8749-1-mhun512@gmail.com?part=1
^ permalink raw reply [flat|nested] 2+ messages in thread