* Re: [PATCH] dmaengine: sprd: fix clock imbalance when enabling ashb_clk fails
2026-08-13 10:53 [PATCH] dmaengine: sprd: fix clock imbalance when enabling ashb_clk fails Chaithanya Lagisetty
@ 2026-08-13 11:06 ` sashiko-bot
2026-08-13 16:39 ` Frank Li
1 sibling, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-08-13 11:06 UTC (permalink / raw)
To: Chaithanya Lagisetty; +Cc: Frank.Li, dmaengine, vkoul
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
Pre-existing issues:
- [High] The driver ignores `-EPROBE_DEFER` for the optional `ashb_clk`, which prevents proper probe deferral.
- [High] Unchecked `pm_runtime_get_sync()` failures leak the PM usage counter and cause unbalanced clock teardowns.
- [High] The interrupt handler returns without clearing the hardware interrupt if a channel has no active descriptor, triggering an infinite IRQ storm.
- [High] The interrupt handler is registered before critical resources are initialized, leading to a NULL pointer dereference.
- [High] Channels are manually removed from the device's channel list before unregistering the DMA device, leaking channel resources.
--
--- Patch [1]: [PATCH] dmaengine: sprd: fix clock imbalance when enabling ashb_clk fails ---
commit db2ba297caa82c1c816130ae2b34acb419871f3c
Author: Chaithanya Lagisetty <nagachaithanya9911@gmail.com>
dmaengine: sprd: fix clock imbalance when enabling ashb_clk fails
sprd_dma_enable() enables sdev->clk and then enables the optional
ashb_clk. If clk_prepare_enable() for ashb_clk fails, the function
returns the error without disabling sdev->clk, which was already
enabled. Both callers (probe and resume) treat a non-zero return as
nothing was enabled and bail out, so sdev->clk is leaked.
> diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> index 087fea3af2e41..3ea5c3ba1a8c8 100644
> --- a/drivers/dma/sprd-dma.c
> +++ b/drivers/dma/sprd-dma.c
> @@ -270,9 +270,16 @@ static int sprd_dma_enable(struct sprd_dma_dev *sdev)
> * The ashb_clk is optional and only for AGCP DMA controller, so we
> * need add one condition to check if the ashb_clk need enable.
> */
> - if (!IS_ERR(sdev->ashb_clk))
> + if (!IS_ERR(sdev->ashb_clk)) {
> ret = clk_prepare_enable(sdev->ashb_clk);
> + if (ret)
> + goto disable_clk;
> + }
>
> + return 0;
> +
> +disable_clk:
> + clk_disable_unprepare(sdev->clk);
> return ret;
> }
While reviewing this patch, I noticed some pre-existing regressions in the same
file.
drivers/dma/sprd-dma.c:sprd_dma_probe() {
[ ... ]
sdev->ashb_clk = devm_clk_get(&pdev->dev, "ashb_eb");
if (IS_ERR(sdev->ashb_clk))
dev_warn(&pdev->dev, "no optional ashb eb clock\n");
[ ... ]
[Severity: High]
This is a pre-existing issue, but does this code ignore -EPROBE_DEFER?
If the clock provider is probed after this driver, treating the clock as
permanently absent could prevent probe deferral and lead to missing hardware
clocks.
drivers/dma/sprd-dma.c:sprd_dma_alloc_chan_resources() {
return pm_runtime_get_sync(chan->device->dev);
}
drivers/dma/sprd-dma.c:sprd_dma_probe() {
[ ... ]
ret = pm_runtime_get_sync(&pdev->dev);
if (ret < 0)
goto err_rpm;
[ ... ]
err_register:
pm_runtime_put_noidle(&pdev->dev);
pm_runtime_disable(&pdev->dev);
err_rpm:
sprd_dma_disable(sdev);
return ret;
}
drivers/dma/sprd-dma.c:sprd_dma_remove() {
struct sprd_dma_dev *sdev = platform_get_drvdata(pdev);
struct sprd_dma_chn *c, *cn;
pm_runtime_get_sync(&pdev->dev);
[Severity: High]
This is a pre-existing issue, but do these pm_runtime_get_sync() calls
properly handle failure?
In sprd_dma_alloc_chan_resources(), the error is returned directly,
abandoning the incremented usage counter.
In sprd_dma_probe(), the err_rpm path skips pm_runtime_put_noidle() and
pm_runtime_disable().
In sprd_dma_remove(), failing to check the return value can lead to
unbalanced clock teardowns if the device is already suspended.
drivers/dma/sprd-dma.c:dma_irq_handle() {
[ ... ]
while (irq_status) {
i = __ffs(irq_status);
irq_status &= (irq_status - 1);
schan = &sdev->channels[i];
spin_lock(&schan->vc.lock);
sdesc = schan->cur_desc;
if (!sdesc) {
spin_unlock(&schan->vc.lock);
return IRQ_HANDLED;
}
[Severity: High]
This isn't a bug introduced by this patch, but does this early return cause an
interrupt storm?
If the handler exits here without calling sprd_dma_clear_int() to acknowledge
the hardware interrupt, the interrupt controller might immediately refire it
upon exit.
drivers/dma/sprd-dma.c:sprd_dma_probe() {
[ ... ]
sdev->irq = platform_get_irq(pdev, 0);
if (sdev->irq > 0) {
ret = devm_request_irq(&pdev->dev, sdev->irq, dma_irq_handle,
0, "sprd_dma", (void *)sdev);
if (ret < 0) {
dev_err(&pdev->dev, "request dma irq failed\n");
return ret;
}
} else {
dev_warn(&pdev->dev, "no interrupts for the dma controller\n");
}
sdev->glb_base = devm_platform_ioremap_resource(pdev, 0);
[Severity: High]
This is a pre-existing issue, but could this lead to a NULL pointer
dereference?
The interrupt is unmasked by devm_request_irq() before
devm_platform_ioremap_resource() initializes sdev->glb_base.
If an interrupt fires early, the handler will blindly dereference the
uninitialized base address in dma_irq_handle():
u32 irq_status = readl(sdev->glb_base + SPRD_DMA_GLB_INT_MSK_STS);
drivers/dma/sprd-dma.c:sprd_dma_remove() {
[ ... ]
list_for_each_entry_safe(c, cn, &sdev->dma_dev.channels,
vc.chan.device_node) {
list_del(&c->vc.chan.device_node);
tasklet_kill(&c->vc.task);
}
of_dma_controller_free(pdev->dev.of_node);
dma_async_device_unregister(&sdev->dma_dev);
[Severity: High]
This isn't a bug introduced by this patch, but does this manual list_del()
leak channel resources?
Removing the channels from the list causes dma_async_device_unregister() to
see an empty list and skip freeing its resources.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260813105354.2577040-1-nagachaithanya9911@gmail.com?part=1
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] dmaengine: sprd: fix clock imbalance when enabling ashb_clk fails
2026-08-13 10:53 [PATCH] dmaengine: sprd: fix clock imbalance when enabling ashb_clk fails Chaithanya Lagisetty
2026-08-13 11:06 ` sashiko-bot
@ 2026-08-13 16:39 ` Frank Li
1 sibling, 0 replies; 3+ messages in thread
From: Frank Li @ 2026-08-13 16:39 UTC (permalink / raw)
To: Chaithanya Lagisetty
Cc: Vinod Koul, Orson Zhai, Baolin Wang, Chunyan Zhang, Frank Li,
dmaengine, linux-kernel
On Thu, Aug 13, 2026 at 10:53:54AM +0000, Chaithanya Lagisetty wrote:
> [You don't often get email from nagachaithanya9911@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> sprd_dma_enable() enables sdev->clk and then enables the optional
> ashb_clk. If clk_prepare_enable() for ashb_clk fails, the function
> returns the error without disabling sdev->clk, which was already
> enabled. Both callers (probe and resume) treat a non-zero return as
> "nothing was enabled" and bail out, so sdev->clk is leaked.
>
> Disable sdev->clk on the ashb_clk enable failure path.
>
> Fixes: 9b3b8171f7f4 ("dmaengine: sprd: Add Spreadtrum DMA driver")
> Signed-off-by: Chaithanya Lagisetty <nagachaithanya9911@gmail.com>
> ---
> drivers/dma/sprd-dma.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> index 087fea3af2e4..3ea5c3ba1a8c 100644
> --- a/drivers/dma/sprd-dma.c
> +++ b/drivers/dma/sprd-dma.c
> @@ -270,9 +270,16 @@ static int sprd_dma_enable(struct sprd_dma_dev *sdev)
> * The ashb_clk is optional and only for AGCP DMA controller, so we
> * need add one condition to check if the ashb_clk need enable.
> */
> - if (!IS_ERR(sdev->ashb_clk))
> + if (!IS_ERR(sdev->ashb_clk)) {
> ret = clk_prepare_enable(sdev->ashb_clk);
> + if (ret)
> + goto disable_clk;
> + }
>
> + return 0;
> +
> +disable_clk:
> + clk_disable_unprepare(sdev->clk);
Convert to clk bulk API.
Frank
> return ret;
> }
>
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 3+ messages in thread