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

[sashiko-bot@kernel.org]

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