Re: [PATCH v27] dmaengine: qcom: bam_dma: free interrupt before the clock in error path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] dmaengine: qcom: bam_dma: hardcoded IRQF_TRIGGER_HIGH overrides DT
- [High] dmaengine: qcom: unclocked register access and clock double-disable in remove()
- [High] qcom/bam_dma: interrupt storm if IRQ fires before pm_runtime_enable()
--

commit 066c83fa78866b6ea0bc75cfc8548e9a56be5f96
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

dmaengine: qcom: bam_dma: free interrupt before the clock in error path

This commit replaces devm_request_irq() with request_irq() to manually free
the interrupt before disabling the clock in the error path. It also aims to
apply the same ordering to the remove path to prevent reading BAM registers
with the clock disabled.

Closes: https://sashiko.dev/#/patchset/20260427-qcom-qce-cmd-descr-v16-0-945fd1cafbbc%40oss.qualcomm.com?part=2

> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index 94cb957d4b187..a626746b5c934 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
> @@ -1332,8 +1332,8 @@ static int bam_dma_probe(struct platform_device *pdev)
>  	for (i = 0; i < bdev->num_channels; i++)
>  		bam_channel_init(bdev, &bdev->channels[i], i);
>  
> -	ret = devm_request_irq(bdev->dev, bdev->irq, bam_dma_irq,
> -			IRQF_TRIGGER_NONE, "bam_dma", bdev);
> +	ret = request_irq(bdev->irq, bam_dma_irq, IRQF_TRIGGER_HIGH,
> +			  "bam_dma", bdev);

[Severity: High]
Does hardcoding IRQF_TRIGGER_HIGH override the platform-specific Device Tree
configuration?

The original implementation used IRQF_TRIGGER_NONE, which properly delegated
the trigger configuration to the firmware or Device Tree. If this interrupt
is wired as active-low or edge-triggered on some platforms, could hardcoding
this to HIGH misconfigure the interrupt controller and lead to missed
interrupts or failure to request the IRQ?

[Severity: High]
If a pending or spurious interrupt fires immediately after request_irq(), could
it cause an interrupt storm?

bam_dma_irq() calls pm_runtime_get_sync(bdev->dev). However, pm_runtime_enable()
is not called until later in probe:

drivers/dma/qcom/bam_dma.c:bam_dma_probe() {
    ...
    ret = request_irq(bdev->irq, bam_dma_irq, IRQF_TRIGGER_HIGH, ...);
    ...
    pm_runtime_enable(&pdev->dev);
    ...
}

If the interrupt fires before pm_runtime_enable() is called, pm_runtime_get_sync()
will fail, and the handler will return IRQ_NONE without clearing the hardware
interrupt registers. Since this is a level-triggered interrupt, won't the GIC
immediately re-enter the handler upon exit, causing a tight infinite interrupt
storm until the spurious interrupt detector permanently disables the IRQ line?

[ ... ]

> @@ -1410,7 +1412,7 @@ static void bam_dma_remove(struct platform_device *pdev)
>  	/* mask all interrupts for this execution environment */
>  	writel_relaxed(0, bam_addr(bdev, 0,  BAM_IRQ_SRCS_MSK_EE));
>  
> -	devm_free_irq(bdev->dev, bdev->irq, bdev);
> +	free_irq(bdev->irq, bdev);

[Severity: High]
The commit message states this patch will "free the interrupt before disabling
the clock in error path and in remove()".

However, in bam_dma_remove(), pm_runtime_force_suspend() is called before freeing
the interrupt and before accessing the hardware:

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));
    free_irq(bdev->irq, bdev);
    ...
    clk_disable_unprepare(bdev->bamclk);
}

Since pm_runtime_force_suspend() turns off the BAM clock, does this
writel_relaxed() access the hardware with the clock disabled, potentially
triggering a fatal bus fault or SError? Additionally, won't the unconditional
clk_disable_unprepare() at the end of remove() cause a clock imbalance WARN
trace?

Could the commit message and code be out of sync here since the reordering in
remove() appears to have been reverted?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-bam-dma-free-irq-v27-1-660e70223e3c@oss.qualcomm.com?part=1