Re: [PATCH v4 1/6] dmaengine: zynqmp_dma: Use PM guard for probe-time runtime PM reference

["Golla, Nagendra" <Nagendra.Golla@amd.com>]

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:
>> 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@amd.com>
>> ---
>>   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);
> 
Thanks for the Review> If use devm_pm_runtime_enable(), needn't this one.

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@amd.com/
which added that explicit disable specifically to close a race between 
runtime PM and device removal.
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
>>