* Re: [PATCH] dmaengine: dw-axi-dmac: Fix cfgr_clk leak in resume error path
2026-06-24 7:43 [PATCH] dmaengine: dw-axi-dmac: Fix cfgr_clk leak in resume error path Myeonghun Pak
@ 2026-06-24 7:54 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-06-24 7:54 UTC (permalink / raw)
To: Myeonghun Pak; +Cc: Frank.Li, vkoul, dmaengine
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
Pre-existing issues:
- [High] Missing `synchronize_irq()` in runtime suspend callback with shared IRQ.
- [High] Shared IRQ handler accesses hardware registers without checking runtime PM state.
- [High] Missing `pm_runtime_set_active()` in probe leaves the device in an out-of-sync `RPM_SUSPENDED` state, causing a permanent clock leak.
- [High] Shared IRQ is requested before the device clocks are enabled, leading to potential crashes.
- [High] Hardware registers are accessed before runtime PM resume, and asynchronous PM get is used during channel allocation.
--
commit cf365d7df99aa900a547c9c8fd173c3cf21aa48a
Author: Myeonghun Pak <mhun512@gmail.com>
dmaengine: dw-axi-dmac: Fix cfgr_clk leak in resume error path
This commit fixes a clock leak in axi_dma_resume(). If core_clk fails to
enable, the function now properly disables cfgr_clk before returning an
error.
> 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 4d53f077e9d25..6356a52056f38 100644
> --- a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> +++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> @@ -1333,12 +1333,17 @@ static int axi_dma_resume(struct axi_dma_chip *chip)
>
> ret = clk_prepare_enable(chip->core_clk);
> if (ret < 0)
> - return ret;
> + goto err_disable_cfgr_clk;
>
> axi_dma_enable(chip);
> axi_dma_irq_enable(chip);
>
> return 0;
> +
> +err_disable_cfgr_clk:
> + clk_disable_unprepare(chip->cfgr_clk);
> +
> + return ret;
> }
[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential problem
with how axi_dma_resume() is called during driver probe:
drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:dw_probe() {
...
pm_runtime_enable(chip->dev);
pm_runtime_get_noresume(chip->dev);
ret = axi_dma_resume(chip);
if (ret < 0)
goto err_pm_disable;
axi_dma_hw_init(chip);
pm_runtime_put(chip->dev);
...
}
Does this sequence leave the device in an out-of-sync suspended state?
Since pm_runtime_set_active() is not called before pm_runtime_enable(), the
runtime PM state remains at its default suspended state. When
pm_runtime_put() is called, it might complete without invoking the runtime
suspend callback, which could cause a permanent clock leak.
[Severity: High]
This is a pre-existing issue, but there appears to be a race condition when
requesting the shared IRQ early in probe:
drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:dw_probe() {
...
ret = axi_req_irqs(pdev, chip);
if (ret)
return ret;
INIT_LIST_HEAD(&dw->dma.channels);
for (i = 0; i < hdata->nr_channels; i++) {
...
pm_runtime_get_noresume(chip->dev);
ret = axi_dma_resume(chip);
...
}
If another device shares the same IRQ line and has a pending interrupt, could
dw_axi_dma_interrupt() execute immediately after axi_req_irqs() but before
axi_dma_resume() enables the hardware clocks?
If so, the IRQ handler might access unclocked hardware and cause a crash.
[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential issue in
the IRQ handler itself:
drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:dw_axi_dma_interrupt() {
struct axi_dma_chip *chip = dev_id;
...
/* Disable DMAC interrupts. We'll enable them after processing channels */
axi_dma_irq_disable(chip);
...
}
If the device is suspended and its clocks are disabled, and a shared IRQ
fires from another device, does this unconditionally access the unclocked
register in axi_dma_irq_disable() without checking the runtime PM state?
Would it be better to use pm_runtime_get_if_active() here before accessing
the hardware registers?
[Severity: High]
This is a pre-existing issue, but I see a missing synchronize_irq() in the
suspend path:
drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:axi_dma_suspend() {
axi_dma_irq_disable(chip);
axi_dma_disable(chip);
clk_disable_unprepare(chip->core_clk);
clk_disable_unprepare(chip->cfgr_clk);
return 0;
}
Since the driver uses a shared IRQ, could the hardware clocks be disabled
here while a shared IRQ handler is executing concurrently on another CPU?
The subsystem guidelines require calling synchronize_irq() for drivers
using shared IRQs before powering down hardware.
[Severity: High]
This isn't a bug introduced by this patch, but hardware registers might be
accessed before runtime PM resume during channel allocation:
drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:dma_chan_alloc_chan_resources() {
/* ASSERT: channel is idle */
if (axi_chan_is_hw_enable(chan)) {
dev_err(chan2dev(chan), "%s is non-idle!\n",
axi_chan_name(chan));
return -EBUSY;
}
...
pm_runtime_get(chan->chip->dev);
...
}
Does axi_chan_is_hw_enable() perform direct hardware reads while the device
might be suspended?
Also, since pm_runtime_get() is asynchronous, will subsequent prep functions
called by the client access hardware while the device is still suspended?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624074307.68365-1-mhun512@gmail.com?part=1
^ permalink raw reply [flat|nested] 2+ messages in thread