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/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:
>>>> 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.
> 
> "[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.
> 
> Frank
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.

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
>>>>
>>