* Re: [PATCH] dmaengine: qcom: bam_dma: Defer IRQ trigger type to device tree
2026-06-11 6:17 [PATCH] dmaengine: qcom: bam_dma: Defer IRQ trigger type to device tree Vishnu Santhosh
@ 2026-06-11 6:28 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-06-11 6:28 UTC (permalink / raw)
To: Vishnu Santhosh; +Cc: vkoul, Frank.Li, dmaengine
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
New issues:
- [High] Allowing edge-triggered interrupts without a polling loop in the handler introduces a race condition that permanently deadlocks the DMA controller.
Pre-existing issues:
- [High] bam_dma_remove accesses hardware registers after forcing PM runtime suspend, leading to unclocked register accesses and system crash.
- [High] The probe error path cleans up tasklets and disables clocks before freeing the interrupt, leading to a use-after-free and unclocked access race condition.
- [High] The interrupt is requested before PM runtime is fully enabled, causing a permanent interrupt storm if an interrupt fires during probe.
- [High] Failure paths after `pm_runtime_get_sync()` return without decrementing the PM usage counter, causing an unbalanced usage count and leaking DMA resources.
- [High] The interrupt handler unconditionally returns IRQ_HANDLED even for spurious interrupts, defeating the kernel's spurious interrupt detection.
--
commit ee883954f4570a653a82ea860b70acc6baa4eada
Author: Vishnu Santhosh <vishnu.santhosh@oss.qualcomm.com>
Date: Thu Jun 11 06:17:11 2026 +0000
dmaengine: qcom: bam_dma: Defer IRQ trigger type to device tree
This commit changes the hardcoded IRQF_TRIGGER_HIGH flag to
IRQF_TRIGGER_NONE when registering the BAM interrupt. This delegates the
trigger type configuration to the device tree, resolving missed DMA
completions on platforms that use edge-triggered interrupts.
> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index 19116295f8325..6c3e2ca8a572f 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
> @@ -1303,7 +1303,7 @@ static int bam_dma_probe(struct platform_device *pdev)
> bam_channel_init(bdev, &bdev->channels[i], i);
>
> ret = devm_request_irq(bdev->dev, bdev->irq, bam_dma_irq,
> - IRQF_TRIGGER_HIGH, "bam_dma", bdev);
> + IRQF_TRIGGER_NONE, "bam_dma", bdev);
[Severity: High]
This delegates the trigger type to the device tree and allows edge-triggered
interrupts.
Does this regression introduce a potential deadlock if the hardware asserts
a new completion before the previous interrupt is cleared?
If bam_dma_irq() only reads the BAM status registers once per invocation,
and a new completion arrives after the status register is read, the
overall interrupt line might remain asserted.
Because there is no new edge, the interrupt controller would never fire
again.
Should we add a loop to drain all pending events until the status register
reads zero?
[Severity: High]
This is a pre-existing issue, but requesting the IRQ here before
pm_runtime_enable() and pm_runtime_set_active() completes might cause an
interrupt storm.
If an early interrupt fires while the device's PM runtime status is still
suspended:
drivers/dma/qcom/bam_dma.c:bam_dma_irq() {
...
ret = pm_runtime_get_sync(bdev->dev);
if (ret < 0)
return IRQ_NONE;
...
}
Will pm_runtime_get_sync() fail and return IRQ_NONE without clearing the
hardware interrupt, keeping the interrupt line asserted permanently?
> if (ret)
> goto err_bam_channel_exit;
[Severity: High]
This is a pre-existing issue, but does this error path create a
use-after-free and unclocked access race condition?
If a failure occurs later in probe, the error path manually kills tasklets
and disables the clock:
drivers/dma/qcom/bam_dma.c:bam_dma_probe() {
...
err_bam_channel_exit:
for (i = 0; i < bdev->num_channels; i++)
tasklet_kill(&bdev->channels[i].vc.task);
err_tasklet_kill:
tasklet_kill(&bdev->task);
err_disable_clk:
clk_disable_unprepare(bdev->bamclk);
return ret;
}
Since devm_request_irq() was used, the interrupt remains active until devres
cleanup.
If an interrupt fires in this window, will bam_dma_irq() access unclocked
BAM registers and schedule a killed tasklet?
[ ... ]
[Severity: High]
This is a pre-existing issue, but does bam_dma_remove() access hardware
registers after they have been unclocked?
During driver unbinding, pm_runtime_force_suspend() disables the BAM clock.
Immediately after, the code writes to the registers:
drivers/dma/qcom/bam_dma.c:bam_dma_remove() {
...
pm_runtime_force_suspend(&pdev->dev);
of_dma_controller_free(pdev->dev.of_node);
dma_async_device_unregister(&bdev->common);
/* mask all interrupts for this execution environment */
writel_relaxed(0, bam_addr(bdev, 0, BAM_IRQ_SRCS_MSK_EE));
...
}
Will accessing these unclocked peripheral registers generate an asynchronous
bus fault and crash the system?
[Severity: High]
This is a pre-existing issue, but does bam_free_chan() leak the PM usage
counter and DMA resources on failure?
When pm_runtime_get_sync() returns an error, it still increments the PM
usage counter.
The failure path returns immediately:
drivers/dma/qcom/bam_dma.c:bam_free_chan() {
...
ret = pm_runtime_get_sync(bdev->dev);
if (ret < 0)
return;
vchan_free_chan_resources(to_virt_chan(chan));
...
}
Does returning here without calling pm_runtime_put_noidle() permanently
prevent the device from suspending, and skip freeing the channel resources
leading to a memory leak?
[Severity: High]
This is a pre-existing issue, but does bam_dma_irq() incorrectly return
IRQ_HANDLED for spurious interrupts?
If no channel interrupts are found and the global BAM interrupt is not set,
the handler falls through:
drivers/dma/qcom/bam_dma.c:bam_dma_irq() {
...
pm_runtime_mark_last_busy(bdev->dev);
pm_runtime_put_autosuspend(bdev->dev);
return IRQ_HANDLED;
}
Does this unconditionally return IRQ_HANDLED even for spurious interrupts,
defeating the kernel's spurious interrupt detection and potentially causing
an infinite interrupt loop?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260611-qcom-bam-dma-irq-trigger-v1-1-21c216e00b2a@oss.qualcomm.com?part=1
^ permalink raw reply [flat|nested] 2+ messages in thread