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

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

[Bartosz Golaszewski]

The BAM interrupt is requested with a devres helper and so on error it's
freed after probe() returns. We disable the clock before freeing or
masking it so it may still fire and we may end up reading BAM registers
with clock disabled.

Stop using devres for interrupts as we free it in remove() manually
anyway. Add an appropriate label and free the interrupt before disabling
the clock in error path and in remove().

Cc: stable@vger.kernel.org
Fixes: e7c0fe2a5c84 ("dmaengine: add Qualcomm BAM dma driver")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260427-qcom-qce-cmd-descr-v16-0-945fd1cafbbc%40oss.qualcomm.com?part=2
Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
This used to be part of the larger BAM DMA pipe locking series and never
got picked up despite months on the list. I'm resending it separately.
---
Changes in v25:
- Don't touch remove(), it's not wrong in its current version
- Link to v24: https://patch.msgid.link/20260723-qcom-qce-cmd-descr-v24-0-4f87bb4d9938@oss.qualcomm.com
---
 drivers/dma/qcom/bam_dma.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index 05a3b1f9e0c23dc5f861488fae03494867970523..a626746b5c93496e5c5e885b35a856d38c284448 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_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;
 
@@ -1366,7 +1366,7 @@ static int bam_dma_probe(struct platform_device *pdev)
 	ret = dma_async_device_register(&bdev->common);
 	if (ret) {
 		dev_err(bdev->dev, "failed to register dma async device\n");
-		goto err_bam_channel_exit;
+		goto err_free_irq;
 	}
 
 	ret = of_dma_controller_register(pdev->dev.of_node, bam_dma_xlate,
@@ -1385,6 +1385,8 @@ static int bam_dma_probe(struct platform_device *pdev)
 
 err_unregister_dma:
 	dma_async_device_unregister(&bdev->common);
+err_free_irq:
+	free_irq(bdev->irq, bdev);
 err_bam_channel_exit:
 	for (i = 0; i < bdev->num_channels; i++)
 		tasklet_kill(&bdev->channels[i].vc.task);
@@ -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);
 
 	for (i = 0; i < bdev->num_channels; i++) {
 		bam_dma_terminate_all(&bdev->channels[i].vc.chan);

---
base-commit: 9f24d789f03b22941b905ded43cb5ff8eea9ce62
change-id: 20261002-bam-dma-free-irq-cac4b268c465

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

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

[Frank Li]

On Fri, Oct 02, 2026 at 10:51:47AM +0200, Bartosz Golaszewski wrote:
> The BAM interrupt is requested with a devres helper and so on error it's
> freed after probe() returns. We disable the clock before freeing or
> masking it so it may still fire and we may end up reading BAM registers
> with clock disabled.

It is less possible to happen. I see other methods to fix similar issues

https://lore.kernel.org/all/20260608001128.80090-1-dennylin0707@gmail.com/
https://lore.kernel.org/dmaengine/20260927-dma40-fixes-v7-6-89f595e8851d@kernel.org/

>
> Stop using devres for interrupts as we free it in remove() manually
> anyway. Add an appropriate label and free the interrupt before disabling
> the clock in error path and in remove().

Just want to avoid bounce in future, change back to devm version.

>
> Cc: stable@vger.kernel.org
> Fixes: e7c0fe2a5c84 ("dmaengine: add Qualcomm BAM dma driver")
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://sashiko.dev/#/patchset/20260427-qcom-qce-cmd-descr-v16-0-945fd1cafbbc%40oss.qualcomm.com?part=2
> Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> ---
> This used to be part of the larger BAM DMA pipe locking series and never
> got picked up despite months on the list. I'm resending it separately.
> ---
> Changes in v25:
> - Don't touch remove(), it's not wrong in its current version
> - Link to v24: https://patch.msgid.link/20260723-qcom-qce-cmd-descr-v24-0-4f87bb4d9938@oss.qualcomm.com
> ---
>  drivers/dma/qcom/bam_dma.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index 05a3b1f9e0c23dc5f861488fae03494867970523..a626746b5c93496e5c5e885b35a856d38c284448 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_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;
>
> @@ -1366,7 +1366,7 @@ static int bam_dma_probe(struct platform_device *pdev)
>  	ret = dma_async_device_register(&bdev->common);

Can you update it use dmaenginem_sync_device_register() to update current
base?

>  	if (ret) {
>  		dev_err(bdev->dev, "failed to register dma async device\n");
> -		goto err_bam_channel_exit;
> +		goto err_free_irq;
>  	}
>
>  	ret = of_dma_controller_register(pdev->dev.of_node, bam_dma_xlate,

devm_of_dma_controller_register()

> @@ -1385,6 +1385,8 @@ static int bam_dma_probe(struct platform_device *pdev)
>
>  err_unregister_dma:
>  	dma_async_device_unregister(&bdev->common);
> +err_free_irq:
> +	free_irq(bdev->irq, bdev);
>  err_bam_channel_exit:
>  	for (i = 0; i < bdev->num_channels; i++)
>  		tasklet_kill(&bdev->channels[i].vc.task);
> @@ -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));

Actually there are problem

pm_runtime_force_suspend(&pdev->dev);  it will call suspend, which disable
clk,

...
writel_relaxed(0, bam_addr(bdev, 0,  BAM_IRQ_SRCS_MSK_EE));
access register

clk_disable_unprepare(bdev->bamclk);

Maybe cause refcount overflow because previous pm_runtime_force_suspend().

Actually DMA driver seldom remove. It has another issue if still have
consumer acquire channel because miss dev link between consumer and
provider.

Anyways, if you resolve above runtime pm problem, irq problem may not
existing.

Frank

>
> -	devm_free_irq(bdev->dev, bdev->irq, bdev);
> +	free_irq(bdev->irq, bdev);
>
>  	for (i = 0; i < bdev->num_channels; i++) {
>  		bam_dma_terminate_all(&bdev->channels[i].vc.chan);
>
> ---
> base-commit: 9f24d789f03b22941b905ded43cb5ff8eea9ce62
> change-id: 20261002-bam-dma-free-irq-cac4b268c465
>
> Best regards,
> --
> Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
>

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

[Vinod Koul]

On 02-10-26, 12:49, Frank Li wrote:
> On Fri, Oct 02, 2026 at 10:51:47AM +0200, Bartosz Golaszewski wrote:
> > The BAM interrupt is requested with a devres helper and so on error it's
> > freed after probe() returns. We disable the clock before freeing or
> > masking it so it may still fire and we may end up reading BAM registers
> > with clock disabled.
> 
> It is less possible to happen. I see other methods to fix similar issues

Actually this is the right change. devres for irq was a bad idea IMO. We
need to ensure device is quiesced on free and ensure no tasklets can be
triggered. devm acts later and at such time we might get supurious/valid
irq triggering tasklet while we are unwinding...

> 
> https://lore.kernel.org/all/20260608001128.80090-1-dennylin0707@gmail.com/
> https://lore.kernel.org/dmaengine/20260927-dma40-fixes-v7-6-89f595e8851d@kernel.org/
> 
> >
> > Stop using devres for interrupts as we free it in remove() manually
> > anyway. Add an appropriate label and free the interrupt before disabling
> > the clock in error path and in remove().
> 
> Just want to avoid bounce in future, change back to devm version.
> 
> >
> > Cc: stable@vger.kernel.org
> > Fixes: e7c0fe2a5c84 ("dmaengine: add Qualcomm BAM dma driver")
> > Reported-by: Sashiko <sashiko-bot@kernel.org>
> > Closes: https://sashiko.dev/#/patchset/20260427-qcom-qce-cmd-descr-v16-0-945fd1cafbbc%40oss.qualcomm.com?part=2
> > Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> > ---
> > This used to be part of the larger BAM DMA pipe locking series and never
> > got picked up despite months on the list. I'm resending it separately.
> > ---
> > Changes in v25:
> > - Don't touch remove(), it's not wrong in its current version
> > - Link to v24: https://patch.msgid.link/20260723-qcom-qce-cmd-descr-v24-0-4f87bb4d9938@oss.qualcomm.com
> > ---
> >  drivers/dma/qcom/bam_dma.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> > index 05a3b1f9e0c23dc5f861488fae03494867970523..a626746b5c93496e5c5e885b35a856d38c284448 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_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;
> >
> > @@ -1366,7 +1366,7 @@ static int bam_dma_probe(struct platform_device *pdev)
> >  	ret = dma_async_device_register(&bdev->common);
> 
> Can you update it use dmaenginem_sync_device_register() to update current
> base?
> 
> >  	if (ret) {
> >  		dev_err(bdev->dev, "failed to register dma async device\n");
> > -		goto err_bam_channel_exit;
> > +		goto err_free_irq;
> >  	}
> >
> >  	ret = of_dma_controller_register(pdev->dev.of_node, bam_dma_xlate,
> 
> devm_of_dma_controller_register()
> 
> > @@ -1385,6 +1385,8 @@ static int bam_dma_probe(struct platform_device *pdev)
> >
> >  err_unregister_dma:
> >  	dma_async_device_unregister(&bdev->common);
> > +err_free_irq:
> > +	free_irq(bdev->irq, bdev);
> >  err_bam_channel_exit:
> >  	for (i = 0; i < bdev->num_channels; i++)
> >  		tasklet_kill(&bdev->channels[i].vc.task);
> > @@ -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));
> 
> Actually there are problem
> 
> pm_runtime_force_suspend(&pdev->dev);  it will call suspend, which disable
> clk,
> 
> ...
> writel_relaxed(0, bam_addr(bdev, 0,  BAM_IRQ_SRCS_MSK_EE));
> access register
> 
> clk_disable_unprepare(bdev->bamclk);
> 
> Maybe cause refcount overflow because previous pm_runtime_force_suspend().
> 
> Actually DMA driver seldom remove. It has another issue if still have
> consumer acquire channel because miss dev link between consumer and
> provider.
> 
> Anyways, if you resolve above runtime pm problem, irq problem may not
> existing.
> 
> Frank
> 
> >
> > -	devm_free_irq(bdev->dev, bdev->irq, bdev);
> > +	free_irq(bdev->irq, bdev);
> >
> >  	for (i = 0; i < bdev->num_channels; i++) {
> >  		bam_dma_terminate_all(&bdev->channels[i].vc.chan);
> >
> > ---
> > base-commit: 9f24d789f03b22941b905ded43cb5ff8eea9ce62
> > change-id: 20261002-bam-dma-free-irq-cac4b268c465
> >
> > Best regards,
> > --
> > Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> >

-- 
~Vinod

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

[Vinod Koul]

On 02-10-26, 10:51, Bartosz Golaszewski wrote:
> The BAM interrupt is requested with a devres helper and so on error it's
> freed after probe() returns. We disable the clock before freeing or
> masking it so it may still fire and we may end up reading BAM registers
> with clock disabled.
> 
> Stop using devres for interrupts as we free it in remove() manually
> anyway. Add an appropriate label and free the interrupt before disabling
> the clock in error path and in remove().

This doesnt apply on the current code, can you pleease rebase on the
next

-- 
~Vinod