Re: [PATCH v24 02/14] dmaengine: qcom: bam_dma: free interrupt before the clock in error path

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

New issues:
- [High] Moving `free_irq()` to the beginning of `bam_dma_remove()` creates a teardown race condition that hangs concurrent DMA clients.

Pre-existing issues:
- [High] Hardware registers are accessed unconditionally after the clock is disabled, which will crash the system.
- [High] The IRQ handler reads hardware registers before verifying the device's power state, causing crashes on spurious interrupts.
- [High] Enabling the IRQ before Runtime PM creates a race condition that leads to an unhandled interrupt storm.
- [High] Unprotected concurrent read-modify-write on the global `BAM_IRQ_SRCS_MSK_EE` register can permanently mask DMA interrupts.
--

--- Patch [2]: [PATCH v24 02/14] dmaengine: qcom: bam_dma: free interrupt before the clock in error path ---
commit 5c7877cf778976fcc2d921fe8fab12e7fb1eecc1
Author: Bartosz Golaszewski <[email protected]>
Subject: dmaengine: qcom: bam_dma: free interrupt before the clock in error path

The BAM interrupt was requested with a devres helper, meaning on error it
was freed after probe returned and the clock was already disabled,
potentially causing crashes if the interrupt fired. This patch stops using
devres for the interrupt, instead manually freeing it before the clock is
disabled in both the error path and remove().

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 1bb26af0405f3..fc155e0d1870c 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
> @@ -1332,8 +1332,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);

[Severity: High]
This is a pre-existing issue, but does requesting the IRQ here before
Runtime PM is enabled create a race condition that could lead to an
unhandled interrupt storm?

If an error interrupt fires in the window before pm_runtime_enable() is
called, bam_dma_irq() will run, call pm_runtime_get_sync() (which returns
-EACCES), and return IRQ_NONE without acknowledging the global BAM interrupt.
Since the IRQ is level-triggered, won't it fire continuously?

[ ... ]
> @@ -1401,6 +1402,8 @@ static void bam_dma_remove(struct platform_device *pdev)
>  	struct bam_device *bdev = platform_get_drvdata(pdev);
>  	u32 i;
>  
> +	free_irq(bdev->irq, bdev);
> +

[Severity: High]
Does moving free_irq() to the beginning of bam_dma_remove() create a teardown
race condition that could hang concurrent DMA clients?

If a client submits a DMA request during device removal, in the window
after free_irq() runs but before dma_async_device_unregister() removes
the device from the DMA engine, won't the transaction start but its
interrupt can never be handled?

>  	pm_runtime_force_suspend(&pdev->dev);
>  
>  	of_dma_controller_free(pdev->dev.of_node);
> @@ -1409,8 +1412,6 @@ 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));

[Severity: High]
This is a pre-existing issue, but can this unconditional register access crash
the system if the clock was already disabled?

Since pm_runtime_force_suspend() forces the device to a suspended state
and disables the clock, doesn't writel_relaxed() access the memory-mapped
register BAM_IRQ_SRCS_MSK_EE without an active clock?


[Severity: High]
This isn't a bug introduced by this patch, but does the IRQ handler read
hardware registers before verifying the device's power state?

Looking at bam_dma_irq():

	srcs |= process_channel_irqs(bdev);

	/* kick off tasklet to start next dma transfer */
	if (srcs & P_IRQ)
		tasklet_schedule(&bdev->task);

	ret = pm_runtime_get_sync(bdev->dev);

If a spurious interrupt fires while the device is auto-suspended (clock
disabled), bam_dma_irq() calls process_channel_irqs() which issues an
unconditional readl_relaxed(). Could this cause a kernel panic since the
device power state is only checked later via pm_runtime_get_sync()?

[Severity: High]
This is a pre-existing issue, but can the unprotected read-modify-write on
BAM_IRQ_SRCS_MSK_EE permanently mask DMA interrupts?

Looking at bam_free_chan():

	/* mask irq for pipe/channel */
	val = readl_relaxed(bam_addr(bdev, 0, BAM_IRQ_SRCS_MSK_EE));
	val &= ~BIT(bchan->id);
	writel_relaxed(val, bam_addr(bdev, 0, BAM_IRQ_SRCS_MSK_EE));

If two different channels are initialized or freed concurrently by different
clients, could the read-modify-write operations race and cause one update to
be overwritten, since they only use per-channel locks or no locks?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2