Re: [PATCH v4 1/6] dmaengine: zynqmp_dma: Use PM guard for probe-time runtime PM reference
From: Golla, Nagendra
Date: Mon Nov 02 2026 - 00:46:55 EST
Hi Frank,
On 10/22/2026 9:22 PM, Frank Li wrote:
On Thu, Oct 22, 2026 at 10:40:53AM +0530, Golla, Nagendra wrote:
Hi Frank,
On 10/10/2026 8:39 AM, Frank Li wrote:
On Fri, Oct 09, 2026 at 09:30:10AM +0530, Golla Nagendra wrote:Thanks for the Review> If use devm_pm_runtime_enable(), needn't this one.
Replace pm_runtime_resume_and_get() with the PM_RUNTIME_ACQUIRE_AUTOSUSPEND
guard to manage the probe-time runtime PM reference lifecycle. If the guard
acquire fails, fail probe immediately so a broken device is never
registered.
When runtime PM is not enabled for the device, call
zynqmp_dma_runtime_resume() directly after pm_runtime_enable() to bring the
clocks up for probe.
On probe failure after a successful guard acquire, drop the reference with
zynqmp_dma_probe_pm_put(), inhibit the guard destructor with
retain_and_null_ptr(), and gate the clocks through
zynqmp_dma_probe_fail(). When runtime PM is enabled,
zynqmp_dma_probe_pm_put() uses pm_runtime_put_sync_suspend(). When runtime
PM is not enabled for the device, balance the guard usage counter with
pm_runtime_put_noidle() and gate the clocks with
zynqmp_dma_runtime_suspend(). Runtime PM does not suspend a device that
is already disabled, so pair every direct zynqmp_dma_runtime_suspend()
call with pm_runtime_set_suspended() to keep the runtime PM status in
sync with the clocks.
On probe success, release the probe-time reference with
pm_runtime_put_sync_autosuspend() when runtime PM is enabled for the
device. When runtime PM is not enabled for the device, balance the guard
usage counter with pm_runtime_put_noidle() only and leave the clocks
enabled so that later channel allocation can use the hardware. Inhibit the
guard destructor with retain_and_null_ptr(pm).
Signed-off-by: Golla Nagendra <nagendra.golla@xxxxxxx>
---
drivers/dma/xilinx/zynqmp_dma.c | 68 ++++++++++++++++++++++++++++++-----------
1 file changed, 50 insertions(+), 18 deletions(-)
diff --git a/drivers/dma/xilinx/zynqmp_dma.c b/drivers/dma/xilinx/zynqmp_dma.c
index 3b7dfa53f972..a48106e404e7 100644
--- a/drivers/dma/xilinx/zynqmp_dma.c
+++ b/drivers/dma/xilinx/zynqmp_dma.c
@@ -18,6 +18,7 @@
#include <linux/slab.h>
#include <linux/clk.h>
#include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/cleanup.h>
#include <linux/pm_runtime.h>
#include "../dmaengine.h"
@@ -1077,6 +1078,29 @@ static const struct dev_pm_ops zynqmp_dma_dev_pm_ops = {
zynqmp_dma_runtime_resume, NULL)
};
+static void zynqmp_dma_probe_pm_put(struct device *dev)
+{
+ if (pm_runtime_enabled(dev))
+ pm_runtime_put_sync_suspend(dev);
+ else
+ pm_runtime_put_noidle(dev);
+}
+
+static int zynqmp_dma_probe_fail(struct zynqmp_dma_device *zdev, int ret)
+{
+ bool rpm_disabled = !pm_runtime_enabled(zdev->dev);
+
+ zynqmp_dma_chan_remove(zdev->chan);
+ zynqmp_dma_probe_pm_put(zdev->dev);
+ pm_runtime_disable(zdev->dev);
+ if (rpm_disabled || !pm_runtime_status_suspended(zdev->dev)) {
+ zynqmp_dma_runtime_suspend(zdev->dev);
+ pm_runtime_set_suspended(zdev->dev);
+ }
+
+ return ret;
+}
+
/**
* zynqmp_dma_probe - Driver probe function
* @pdev: Pointer to the platform_device structure
@@ -1087,7 +1111,7 @@ static int zynqmp_dma_probe(struct platform_device *pdev)
{
struct zynqmp_dma_device *zdev;
struct dma_device *p;
- int ret;
+ int ret, pm_err;
zdev = devm_kzalloc(&pdev->dev, sizeof(*zdev), GFP_KERNEL);
if (!zdev)
@@ -1128,21 +1152,30 @@ static int zynqmp_dma_probe(struct platform_device *pdev)
pm_runtime_set_autosuspend_delay(zdev->dev, ZDMA_PM_TIMEOUT);
pm_runtime_use_autosuspend(zdev->dev);
pm_runtime_enable(zdev->dev);
- ret = pm_runtime_resume_and_get(zdev->dev);
- if (ret < 0) {
- dev_err(&pdev->dev, "device wakeup failed.\n");
- pm_runtime_disable(zdev->dev);
- }
if (!pm_runtime_enabled(zdev->dev)) {
ret = zynqmp_dma_runtime_resume(zdev->dev);
- if (ret)
+ if (ret) {
+ pm_runtime_disable(zdev->dev);
return ret;
+ }
+ }
+ PM_RUNTIME_ACQUIRE_AUTOSUSPEND(zdev->dev, pm);
+ pm_err = PM_RUNTIME_ACQUIRE_ERR(&pm);
+ if (pm_err) {
+ retain_and_null_ptr(pm);
Not sure why need call retain_and_null_ptr() here, generally only need
call retain_and_null_ptr() at success pass if you use want to keep
runtime_pm active. at fail case, needn't do that.
+ pm_runtime_disable(zdev->dev);
We intentionally keep the nondevmpm_runtime_enable()pm_runtime_disable()
pair here and do not switch to devm_pm_runtime_enable().
devm_pm_runtime_enable() registers a devm action that calls
pm_runtime_disable() automatically after .remove returns. Since remove()
already calls pm_runtime_disable() explicitly itself, switching to the devm
variant would mean pm_runtime_disable() gets called a second time for the
same device
— driving disable_depth into underflow, along with any other
invalid state that comes from disabling an already-disabled runtime PM.
If we remove the explicit pm_runtime_disable() from remove() to avoid that
double-disable call, it would affect the fix introduced in commit
9c58a42c1612 ("dmaengine: zynqmp_dma: fix race between runtime PM and device
removal"),
https://lore.kernel.org/all/20260630064844.705173-2-nagendra.golla@xxxxxxx/
which added that explicit disable specifically to close a race between
runtime PM and device removal.
"[Severity: High]
This isn't a bug introduced by this patch, but does zynqmp_dma_remove() need
to halt the DMA hardware before disabling clocks?
The function disables device clocks via zynqmp_dma_runtime_suspend() but never
writes to the hardware to halt active transfers or waits for idle.
Could stopping clocks while an AXI master is mid-transaction violate the bus
protocol and cause an interconnect hang?"
Is this one?
Apologies for the delayed response.
No, the commit I mentioned does not refer to this issue. This patch resolve the race with respect to runtime PM in the remove function . The old code first checks whether the device is runtime active, and based on that it decides whether to manually suspend the device. But during the small gap between that check and the manual suspend call, runtime PM is still enabled, so a runtime PM transition can occur in parallel. In that window, the PM core may call the driver runtime suspend callback itself. Then the remove path also calls the same suspend callback based on the earlier active-state check, so the device may be suspended twice.
Frank
this is why pm_runtime_disable() is moved before the suspend decision. because it closes the race window by blocking further runtime PM transitions and stabilizing the device state.
this was the potential condition reported by the internal AI review
The issue you pointed out regarding an ongoing DMA transfer during removal is separate and needs further investigation. The commit I mentioned does not address stopping an active transfer or waiting for the hardware to become idle before disabling the clocks.
Thanks,
Nagendra
Dropping it would reopen that same race.
Thanks,
Nagendra
+ if (!pm_runtime_status_suspended(zdev->dev)) {
+ zynqmp_dma_runtime_suspend(zdev->dev);
+ pm_runtime_set_suspended(zdev->dev);
+ }
+ return dev_err_probe(&pdev->dev, pm_err, "device wakeup failed\n");
}
ret = zynqmp_dma_chan_probe(zdev, pdev);
if (ret) {
dev_err_probe(&pdev->dev, ret, "Probing channel failed\n");
- goto free_chan_resources;
+ retain_and_null_ptr(pm);
+ return zynqmp_dma_probe_fail(zdev, ret);
}
p->dst_addr_widths = BIT(zdev->chan->bus_width / 8);
@@ -1151,7 +1184,8 @@ static int zynqmp_dma_probe(struct platform_device *pdev)
ret = dma_async_device_register(&zdev->common);
if (ret) {
dev_err(zdev->dev, "failed to register the dma device\n");
- goto free_chan_resources;
+ retain_and_null_ptr(pm);
+ return zynqmp_dma_probe_fail(zdev, ret);
}
ret = of_dma_controller_register(pdev->dev.of_node,
@@ -1159,19 +1193,17 @@ static int zynqmp_dma_probe(struct platform_device *pdev)
if (ret) {
dev_err_probe(&pdev->dev, ret, "Unable to register DMA to DT\n");
dma_async_device_unregister(&zdev->common);
- goto free_chan_resources;
+ retain_and_null_ptr(pm);
+ return zynqmp_dma_probe_fail(zdev, ret);
}
- pm_runtime_put_sync_autosuspend(zdev->dev);
+ if (pm_runtime_enabled(zdev->dev))
+ pm_runtime_put_sync_autosuspend(zdev->dev);
+ else
+ pm_runtime_put_noidle(zdev->dev);
+ retain_and_null_ptr(pm);
You mix use auto cleanup and pm_runtime_put_sync_autosuspen() and
pm_runtime_put_noidle() to make code mess up.
Frank
return 0;
-
-free_chan_resources:
- zynqmp_dma_chan_remove(zdev->chan);
- if (!pm_runtime_enabled(zdev->dev))
- zynqmp_dma_runtime_suspend(zdev->dev);
- pm_runtime_disable(zdev->dev);
- return ret;
}
/**
--
2.43.7