Re: [PATCH v18 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 5 potential issue(s) to consider:

New issues:
- [High] Calling free_irq() before unregistering the DMA controller creates a window where active DMA clients will hang forever.

Pre-existing issues:
- [Critical] Hardware registers are accessed after the device clock is disabled, causing a synchronous external abort.
- [Critical] The IRQ handler accesses hardware registers before verifying the device is active and its clocks are enabled.
- [High] The DMA device state is freed via devres while clients may still hold references to channels, leading to a Use-After-Free.
- [High] Requesting the IRQ before enabling runtime PM can cause an unhandled interrupt storm.
--

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

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

This commit stops using devres for the BAM interrupt and manually frees
it in the error path and in remove() before disabling the clock.
This prevents the interrupt from firing while the clock is 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
> --- 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);

[Severity: High]
This is a pre-existing issue, but does requesting the IRQ with
IRQF_TRIGGER_HIGH before pm_runtime_enable() is called risk an unhandled
interrupt storm?

If the interrupt line is already asserted at probe time, the handler will run
immediately. Because runtime PM is not yet enabled, pm_runtime_get_sync()
will fail in the handler, potentially causing it to return IRQ_NONE without
clearing the hardware interrupt.

[Severity: Critical]
This is also a pre-existing issue, but does the IRQ handler read hardware
registers (like BAM_IRQ_SRCS_EE) before using Runtime PM APIs to guarantee
the device is awake and its clocks are enabled?

If a spurious interrupt fires while the device is in runtime autosuspend,
could an unclocked hardware read trigger a synchronous external abort and
crash the system?

[ ... ]
> @@ -1371,6 +1372,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 freeing the IRQ here before calling dma_async_device_unregister() create
a window where active DMA clients will hang forever?

The interrupt handler is unregistered while the DMA controller is still fully
active and channels haven't been terminated. If any active transactions
complete or new ones are issued by a client during this window, wouldn't they
never receive an interrupt completion?

>  	pm_runtime_force_suspend(&pdev->dev);
>  
>  	of_dma_controller_free(pdev->dev.of_node);
>  	dma_async_device_unregister(&bdev->common);

[Severity: High]
This isn't a bug introduced by this patch, but does allocating the bam_device
state with devm_kzalloc() lead to a use-after-free when this function
returns?

While dma_async_device_unregister() prevents new channel allocations, clients
may still hold references to existing channels. Because devres frees the
memory immediately upon remove() returning, couldn't clients using DMA APIs
or scheduled tasklets access freed memory?

>  	/* mask all interrupts for this execution environment */
>  	writel_relaxed(0, bam_addr(bdev, 0,  BAM_IRQ_SRCS_MSK_EE));

[Severity: Critical]
This is a pre-existing issue, but does calling writel_relaxed() here access
hardware registers after pm_runtime_force_suspend() has already disabled the
bamclk?

Accessing peripheral registers when the module clock is disabled causes a
synchronous external abort on these platforms. Shouldn't this register
access occur before the device is suspended?

> -	devm_free_irq(bdev->dev, bdev->irq, bdev);
> -
>  	for (i = 0; i < bdev->num_channels; i++) {
>  		bam_dma_terminate_all(&bdev->channels[i].vc.chan);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522-qcom-qce-cmd-descr-v18-0-99103926bafc@oss.qualcomm.com?part=2