Re: [PATCH RESEND] dmaengine: qcom: bam_dma: Defer IRQ trigger type to device tree

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Unclocked MMIO accesses cause system crash (synchronous external abort).
- [High] Error path devres race conditions cause Use-After-Free and system crashes.
- [High] Missing `pm_runtime_put_noidle()` leaks device usage counter on error paths.
- [High] Requesting IRQ before enabling PM runtime can cause an interrupt storm and CPU hang.
--

--- Patch [1]: [PATCH RESEND] dmaengine: qcom: bam_dma: Defer IRQ trigger type to device tree ---
commit ddb45d4f1215300c79f379d22aecb598266fe011
Author: Vishnu Santhosh <vishnu.santhosh@oss.qualcomm.com>

dmaengine: qcom: bam_dma: Defer IRQ trigger type to device tree

The driver hardcodes IRQF_TRIGGER_HIGH when registering the BAM
interrupt, which overrides the trigger type specified in the device
tree. This patch uses IRQF_TRIGGER_NONE instead, which allows the kernel
to use the trigger type already configured by platform_get_irq().

> 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 is a pre-existing issue, but does requesting the IRQ here before enabling
PM runtime risk an interrupt storm and CPU hang?

Since devm_request_irq() is called before pm_runtime_enable(), if an
interrupt fires immediately, bam_dma_irq() handles it. However, because PM
runtime is disabled, pm_runtime_get_sync() returns -EACCES.

The handler then returns IRQ_NONE without clearing the BAM interrupt status:

drivers/dma/qcom/bam_dma.c:bam_dma_irq() {
...
	ret = pm_runtime_get_sync(bdev->dev);
	if (ret < 0)
		return IRQ_NONE;
...
}

For level-triggered interrupts, wouldn't this leave the line asserted and
cause an interrupt storm?

[Severity: High]
This isn't a bug introduced by this patch, but does relying on devres to free
the IRQ create a race condition on error paths?

If a failure occurs after this devm_request_irq() call, the error path
manual teardown leaves a window where the IRQ is still active:

drivers/dma/qcom/bam_dma.c:bam_dma_probe() {
...
err_tasklet_kill:
	tasklet_kill(&bdev->task);
err_disable_clk:
	clk_disable_unprepare(bdev->bamclk);

	return ret;
}

If an interrupt fires during this window before devres frees the IRQ,
bam_dma_irq() runs. It could either access unclocked MMIO or call
tasklet_schedule().

Does this mean the scheduled tasklet remains pending while devres frees bdev,
leading to a use-after-free when the tasklet executes?

[Severity: High]
This is a pre-existing issue, but do unclocked MMIO accesses cause system
crashes (synchronous external aborts)?

In bam_dma_irq(), process_channel_irqs() is called unconditionally before
pm_runtime_get_sync():

drivers/dma/qcom/bam_dma.c:bam_dma_irq() {
...
	srcs |= process_channel_irqs(bdev);

	ret = pm_runtime_get_sync(bdev->dev);
...
}

If a spurious interrupt fires while the device is runtime-suspended, it reads
registers without a clock.

Similarly, in bam_dma_remove():

drivers/dma/qcom/bam_dma.c:bam_dma_remove() {
...
	pm_runtime_force_suspend(&pdev->dev);
...
	writel_relaxed(0, bam_addr(bdev, 0,  BAM_IRQ_SRCS_MSK_EE));
...
}

Does writing to BAM_IRQ_SRCS_MSK_EE immediately after forcing the device
into suspend (which disables its clock) deterministically crash the system?

[Severity: High]
This isn't a bug introduced by this patch, but does the error path in PM
runtime handling leak the device usage counter?

If pm_runtime_get_sync() fails, it increments the usage counter but the
driver returns directly without dropping the reference:

drivers/dma/qcom/bam_dma.c:bam_pause() {
...
	ret = pm_runtime_get_sync(bdev->dev);
	if (ret < 0)
		return ret;
...
}

This missing pm_runtime_put_noidle() on failure appears across multiple
functions, including bam_start_dma(), bam_free_chan(), bam_pause(),
bam_resume(), and bam_dma_irq().

Can this cause the device to permanently fail to enter runtime suspend?

>  	if (ret)
>  		goto err_bam_channel_exit;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717-qcom-bam-dma-irq-trigger-v1-1-6405eac86f00@oss.qualcomm.com?part=1