[PATCH 0/2] dmaengine: qcom: bam_dma: Fixes for unbind

[PATCH 0/2] dmaengine: qcom: bam_dma: Fixes for unbind

[Mostafa Saleh]

On Lenovo Mini-x Gen 10 (X1P42100 based) I am using VFIO-platform
to assing the QCE/BAM to VMs.

During unbind:
echo 1dc4000.dma-controller > /sys/bus/platform/drivers/bam-dma-engine/unbind
echo 1dfa000.crypto > /sys/bus/platform/drivers/qcrypto/unbind

I see a system reset quite often, looking more into this,
that seems to be related to runtime PM, with those fixes I don't
see that anymore.

This patches apply on linux-next.

Mostafa Saleh (2):
  dmaengine: qcom: bam_dma: Take a PM ref at remove()
  dmaengine: qcom: bam_dma: Do not touch remote BAM in remove()

 drivers/dma/qcom/bam_dma.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

-- 
2.56.0.212.g129ede20d9-goog

[PATCH 1/2] dmaengine: qcom: bam_dma: Take a PM ref at remove()

[Mostafa Saleh]

bam_dma_remove() calls pm_runtime_dont_use_autosuspend() and
pm_runtime_force_suspend(), which on a BAM with its own clock disables
bam_clk, and only then writes BAM_IRQ_SRCS_MSK_EE and terminates the
channels, touching registers of a block whose clock has just been
removed, causing the board to reset.

Sashiko(Locally) also reports, it then calls clk_disable_unprepare()
on a clock that runtime suspend already disabled, which can trigger
"already disabled" warning in clk_core_disable().

Hold a runtime PM reference across the register accesses and the
channel teardown. Once the hardware is quiet, disable runtime PM with
the device still active and drop the clock exactly once, balancing the
clk_prepare_enable() in probe.

Fixes: 7d2545599f5b ("dmaengine: qcom-bam-dma: Add pm_runtime support")
Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
 drivers/dma/qcom/bam_dma.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index f67a93a37d48..bd583f78e0c1 100644
--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -1403,12 +1403,10 @@ static void bam_dma_remove(struct platform_device *pdev)
 	struct bam_device *bdev = platform_get_drvdata(pdev);
 	u32 i;
 
-	pm_runtime_dont_use_autosuspend(&pdev->dev);
-	pm_runtime_force_suspend(&pdev->dev);
-
 	of_dma_controller_free(pdev->dev.of_node);
 	dma_async_device_unregister(&bdev->common);
 
+	pm_runtime_get_sync(&pdev->dev);
 	/* mask all interrupts for this execution environment */
 	writel_relaxed(0, bam_addr(bdev, 0,  BAM_IRQ_SRCS_MSK_EE));
 
@@ -1428,6 +1426,9 @@ static void bam_dma_remove(struct platform_device *pdev)
 
 	tasklet_kill(&bdev->task);
 
+	pm_runtime_dont_use_autosuspend(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+	pm_runtime_put_noidle(&pdev->dev);
 	clk_disable_unprepare(bdev->bamclk);
 }
 
-- 
2.56.0.212.g129ede20d9-goog

Re: [PATCH 1/2] dmaengine: qcom: bam_dma: Take a PM ref at remove()

[Frank Li]

On Tue, Oct 20, 2026 at 06:08:02PM +0000, Mostafa Saleh wrote:
> [You don't often get email from smostafa@google.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> bam_dma_remove() calls pm_runtime_dont_use_autosuspend() and
> pm_runtime_force_suspend(), which on a BAM with its own clock disables
> bam_clk, and only then writes BAM_IRQ_SRCS_MSK_EE and terminates the
> channels, touching registers of a block whose clock has just been
> removed, causing the board to reset.
>
> Sashiko(Locally) also reports, it then calls clk_disable_unprepare()
> on a clock that runtime suspend already disabled, which can trigger
> "already disabled" warning in clk_core_disable().
>
> Hold a runtime PM reference across the register accesses and the
> channel teardown. Once the hardware is quiet, disable runtime PM with
> the device still active and drop the clock exactly once, balancing the
> clk_prepare_enable() in probe.
>
> Fixes: 7d2545599f5b ("dmaengine: qcom-bam-dma: Add pm_runtime support")
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
>  drivers/dma/qcom/bam_dma.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index f67a93a37d48..bd583f78e0c1 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
> @@ -1403,12 +1403,10 @@ static void bam_dma_remove(struct platform_device *pdev)
>         struct bam_device *bdev = platform_get_drvdata(pdev);
>         u32 i;
>
> -       pm_runtime_dont_use_autosuspend(&pdev->dev);
> -       pm_runtime_force_suspend(&pdev->dev);
> -
>         of_dma_controller_free(pdev->dev.of_node);
>         dma_async_device_unregister(&bdev->common);
>
> +       pm_runtime_get_sync(&pdev->dev);
>         /* mask all interrupts for this execution environment */
>         writel_relaxed(0, bam_addr(bdev, 0,  BAM_IRQ_SRCS_MSK_EE));
>
> @@ -1428,6 +1426,9 @@ static void bam_dma_remove(struct platform_device *pdev)
>
>         tasklet_kill(&bdev->task);
>
> +       pm_runtime_dont_use_autosuspend(&pdev->dev);

After

commit d1706f9da9f40dffe8621cc0ee03b847f41f1d6d
Author: Joshua Crofts <joshua.crofts1@gmail.com>
Date:   Sat Sep 19 10:24:28 2026 +0200

    PM: runtime: call pm_runtime_dont_use_autosuspend() on reinit()

    Add a pm_runtime_dont_use_autosuspend() call to pm_runtime_reinit() to
    ensure autosuspend is disabled on driver teardown.

    Suggested-by: Frank Li <Frank.Li@nxp.com>
    Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
    Link: https://patch.msgid.link/20260919-move-dont-use-autosuspend-v1-1-f6e2d1315c23@gmail.com
    Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>


Needn't call pm_runtime_dont_use_autosuspend()


> +       pm_runtime_disable(&pdev->dev);

Can you use devm_pm_*() call to fix this problem.

Frank
> +       pm_runtime_put_noidle(&pdev->dev);
>         clk_disable_unprepare(bdev->bamclk);
>  }
>
> --
> 2.56.0.212.g129ede20d9-goog
>

Re: [PATCH 1/2] dmaengine: qcom: bam_dma: Take a PM ref at remove()

[Mostafa Saleh]

On Tue, Oct 20, 2026 at 01:47:30PM -0500, Frank Li wrote:
> On Tue, Oct 20, 2026 at 06:08:02PM +0000, Mostafa Saleh wrote:
> > [You don't often get email from smostafa@google.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> >
> > bam_dma_remove() calls pm_runtime_dont_use_autosuspend() and
> > pm_runtime_force_suspend(), which on a BAM with its own clock disables
> > bam_clk, and only then writes BAM_IRQ_SRCS_MSK_EE and terminates the
> > channels, touching registers of a block whose clock has just been
> > removed, causing the board to reset.
> >
> > Sashiko(Locally) also reports, it then calls clk_disable_unprepare()
> > on a clock that runtime suspend already disabled, which can trigger
> > "already disabled" warning in clk_core_disable().
> >
> > Hold a runtime PM reference across the register accesses and the
> > channel teardown. Once the hardware is quiet, disable runtime PM with
> > the device still active and drop the clock exactly once, balancing the
> > clk_prepare_enable() in probe.
> >
> > Fixes: 7d2545599f5b ("dmaengine: qcom-bam-dma: Add pm_runtime support")
> > Signed-off-by: Mostafa Saleh <smostafa@google.com>
> > ---
> >  drivers/dma/qcom/bam_dma.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> > index f67a93a37d48..bd583f78e0c1 100644
> > --- a/drivers/dma/qcom/bam_dma.c
> > +++ b/drivers/dma/qcom/bam_dma.c
> > @@ -1403,12 +1403,10 @@ static void bam_dma_remove(struct platform_device *pdev)
> >         struct bam_device *bdev = platform_get_drvdata(pdev);
> >         u32 i;
> >
> > -       pm_runtime_dont_use_autosuspend(&pdev->dev);
> > -       pm_runtime_force_suspend(&pdev->dev);
> > -
> >         of_dma_controller_free(pdev->dev.of_node);
> >         dma_async_device_unregister(&bdev->common);
> >
> > +       pm_runtime_get_sync(&pdev->dev);
> >         /* mask all interrupts for this execution environment */
> >         writel_relaxed(0, bam_addr(bdev, 0,  BAM_IRQ_SRCS_MSK_EE));
> >
> > @@ -1428,6 +1426,9 @@ static void bam_dma_remove(struct platform_device *pdev)
> >
> >         tasklet_kill(&bdev->task);
> >
> > +       pm_runtime_dont_use_autosuspend(&pdev->dev);
> 
> After
> 
> commit d1706f9da9f40dffe8621cc0ee03b847f41f1d6d
> Author: Joshua Crofts <joshua.crofts1@gmail.com>
> Date:   Sat Sep 19 10:24:28 2026 +0200
> 
>     PM: runtime: call pm_runtime_dont_use_autosuspend() on reinit()
> 
>     Add a pm_runtime_dont_use_autosuspend() call to pm_runtime_reinit() to
>     ensure autosuspend is disabled on driver teardown.
> 
>     Suggested-by: Frank Li <Frank.Li@nxp.com>
>     Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
>     Link: https://patch.msgid.link/20260919-move-dont-use-autosuspend-v1-1-f6e2d1315c23@gmail.com
>     Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> 
> Needn't call pm_runtime_dont_use_autosuspend()

I see, thanks for pointing that out, I will remove it in v2.

> 
> 
> > +       pm_runtime_disable(&pdev->dev);
> 
> Can you use devm_pm_*() call to fix this problem.

AFAICT, in that case we need to gate the clk_disable_unprepare()
in remove() as the suspend() also runs after remove(), so
clk_disable() would be called twice.

The problem seems to be bam_dma_runtime_resume/suspend() calling
clk_enable/disable() but also remove() calls disable(), so something
as:

--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -1379,7 +1379,7 @@ static int bam_dma_probe(struct platform_device *pdev)
        pm_runtime_use_autosuspend(&pdev->dev);
        pm_runtime_mark_last_busy(&pdev->dev);
        pm_runtime_set_active(&pdev->dev);
-       pm_runtime_enable(&pdev->dev);
+       devm_pm_runtime_enable(&pdev->dev);
 
        return 0;
 
@@ -1433,9 +1433,10 @@ static void bam_dma_remove(struct platform_device *pdev)
 
        tasklet_kill(&bdev->task);
 
-       pm_runtime_disable(&pdev->dev);
        pm_runtime_put_noidle(&pdev->dev);
+#ifndef CONFIG_PM
        clk_disable_unprepare(bdev->bamclk);
+#endif
 }
 
 static int __maybe_unused bam_dma_runtime_suspend(struct device *dev)


I can see example for both patterns in tree. However, 2) seems to
be more common:
1) Gating CONFIG_PM: drivers/mailbox/mtk-cmdq-mailbox.c
2) Get/disable/put_noidle then clk_disable(): drivers/mmc/host/sdhci-msm.c

Thanks,
Mostafa
> 
> Frank
> > +       pm_runtime_put_noidle(&pdev->dev);
> >         clk_disable_unprepare(bdev->bamclk);
> >  }
> >
> > --
> > 2.56.0.212.g129ede20d9-goog
> >

[PATCH 2/2] dmaengine: qcom: bam_dma: Do not touch remote BAM in remove()

[Mostafa Saleh]

Unbinding the crypto BAM through sysfs on X1P42100 resets the board.
bam_dma_remove() clears BAM_IRQ_SRCS_MSK_EE unconditionally. For a BAM
that is controlled or powered remotely the register may not be
accessible: the clock and power of the block belong to the remote side
or to the consumer of the BAM, which is why bam_init() already avoids
every register access for such a BAM when the DT provides num-channels
and qcom,num-ees.

The write is also unnecessary. On a remotely controlled BAM the driver
only ever sets per channel bits in the EE mask, in bam_chan_init_hw(),
and bam_free_chan() clears them again. Both run on behalf of a
consumer, which keeps the block clocked while they do.

Skip the write for both kinds of remotely managed BAM, using the same
condition as bam_init().

Fixes: 5172c9eb89d4 ("dmaengine: qcom: bam_dma: add controlled-remotely dt property")
Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
 drivers/dma/qcom/bam_dma.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index bd583f78e0c1..d818fa1dba57 100644
--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -1407,8 +1407,15 @@ static void bam_dma_remove(struct platform_device *pdev)
 	dma_async_device_unregister(&bdev->common);
 
 	pm_runtime_get_sync(&pdev->dev);
-	/* mask all interrupts for this execution environment */
-	writel_relaxed(0, bam_addr(bdev, 0,  BAM_IRQ_SRCS_MSK_EE));
+
+	/*
+	 * Mask all interrupts for this execution environment. A remotely
+	 * controlled or powered BAM is left alone: it may be unclocked, and
+	 * the only bits this driver sets in its mask are the per channel
+	 * ones that bam_free_chan() cleared.
+	 */
+	if (!bdev->controlled_remotely && !bdev->powered_remotely)
+		writel_relaxed(0, bam_addr(bdev, 0,  BAM_IRQ_SRCS_MSK_EE));
 
 	free_irq(bdev->irq, bdev);
 
-- 
2.56.0.212.g129ede20d9-goog

Re: [PATCH 2/2] dmaengine: qcom: bam_dma: Do not touch remote BAM in remove()

[Frank Li]

On Tue, Oct 20, 2026 at 06:08:03PM +0000, Mostafa Saleh wrote:
> [You don't often get email from smostafa@google.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> Unbinding the crypto BAM through sysfs on X1P42100 resets the board.
> bam_dma_remove() clears BAM_IRQ_SRCS_MSK_EE unconditionally. For a BAM
> that is controlled or powered remotely the register may not be
> accessible: the clock and power of the block belong to the remote side
> or to the consumer of the BAM, which is why bam_init() already avoids
> every register access for such a BAM when the DT provides num-channels
> and qcom,num-ees.
>
> The write is also unnecessary. On a remotely controlled BAM the driver
> only ever sets per channel bits in the EE mask, in bam_chan_init_hw(),
> and bam_free_chan() clears them again. Both run on behalf of a
> consumer, which keeps the block clocked while they do.
>
> Skip the write for both kinds of remotely managed BAM, using the same
> condition as bam_init().
>
> Fixes: 5172c9eb89d4 ("dmaengine: qcom: bam_dma: add controlled-remotely dt property")
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/dma/qcom/bam_dma.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index bd583f78e0c1..d818fa1dba57 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
> @@ -1407,8 +1407,15 @@ static void bam_dma_remove(struct platform_device *pdev)
>         dma_async_device_unregister(&bdev->common);
>
>         pm_runtime_get_sync(&pdev->dev);
> -       /* mask all interrupts for this execution environment */
> -       writel_relaxed(0, bam_addr(bdev, 0,  BAM_IRQ_SRCS_MSK_EE));
> +
> +       /*
> +        * Mask all interrupts for this execution environment. A remotely
> +        * controlled or powered BAM is left alone: it may be unclocked, and
> +        * the only bits this driver sets in its mask are the per channel
> +        * ones that bam_free_chan() cleared.
> +        */
> +       if (!bdev->controlled_remotely && !bdev->powered_remotely)
> +               writel_relaxed(0, bam_addr(bdev, 0,  BAM_IRQ_SRCS_MSK_EE));
>
>         free_irq(bdev->irq, bdev);
>
> --
> 2.56.0.212.g129ede20d9-goog
>