Re: [PATCH v17 02/14] 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 2 potential issue(s) to consider:
- [High] The patch fails to resolve the unclocked register access bug in `bam_dma_remove()`, resulting in a deterministic crash on device removal.
- [High] Race condition in `bam_dma_probe()` can trigger an IRQ storm resulting in a permanently disabled interrupt.
--

commit 39d4da1e9be1c385de4964ad3c6f55d2aea52136
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

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

This commit replaces the devres-managed IRQ allocation with manual IRQ
requesting and freeing. It ensures the interrupt is freed before
disabling the clock in the probe error path to prevent unclocked
register accesses if the IRQ fires.

Fixes: e7c0fe2a5c84 ("dmaengine: add Qualcomm BAM dma driver")
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 19116295f8325..cea44833201d6 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
> @@ -1302,8 +1302,7 @@ 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_HIGH, "bam_dma", bdev);
> +	ret = request_irq(bdev->irq, bam_dma_irq, IRQF_TRIGGER_HIGH, "bam_dma", bdev);
>  	if (ret)
>  		goto err_bam_channel_exit;

This isn't a new regression, but since the goal is to safely manage the BAM
interrupt, could requesting the IRQ here before PM runtime is enabled lead
to an IRQ storm?

If an interrupt asserts after request_irq() but before pm_runtime_enable()
is called later in the probe sequence, bam_dma_irq() will execute and call
pm_runtime_get_sync().

Because PM runtime is disabled, this returns -EACCES. The handler then
returns IRQ_NONE without clearing the hardware interrupt status.

Since the interrupt is level-triggered (IRQF_TRIGGER_HIGH), the hardware
will continuously re-assert it, causing an IRQ storm until the kernel's
spurious IRQ detector permanently disables the interrupt line.

Should PM runtime be enabled and marked active before requesting the IRQ?

[ ... ]

> @@ -1379,7 +1380,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);

The commit message notes that disabling the clock before freeing or masking
the interrupt can lead to unclocked BAM register accesses. Does this same
regression still exist here in bam_dma_remove()?

At the beginning of bam_dma_remove(), pm_runtime_force_suspend() is called:

static void bam_dma_remove(struct platform_device *pdev)
{
	struct bam_device *bdev = platform_get_drvdata(pdev);
	u32 i;

	pm_runtime_force_suspend(&pdev->dev);
        ...

This invokes bam_dma_runtime_suspend() which explicitly disables the BAM
clock.

Immediately afterward, the driver executes the writel_relaxed() shown above
to mask interrupts. Writing to BAM hardware registers with the clock gated
will deterministically trigger a synchronous external abort (bus crash) on
Qualcomm platforms. Furthermore, free_irq() is called after the clock is
disabled, leaving the same race condition open.

To safely tear down the device, would it be better to wake the device via
pm_runtime_get_sync(), mask and free the IRQ, terminate DMA, and only then
disable PM runtime and clocks?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519-qcom-qce-cmd-descr-v17-0-53a595414b79@oss.qualcomm.com?part=2