Re: [PATCH V3 1/6] dmaengine: zynqmp_dma: Fix PM usage count handling in probe error path
"Golla, Nagendra" <[email protected]>
| Newsgroups | org.kernel.vger.dmaengine,dev.linux.lists.sashiko-reviews,org.kernel.vger.linux-devicetree |
|---|---|
| Message-ID | <[email protected]> |
Hi, On 8/10/2026 3:58 PM, [email protected] wrote: > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: > - [Medium] The introduced pm_runtime_put_sync() fails to synchronously suspend the device because autosuspend is enabled, leaving clocks active. > -- > > --- Patch [1]: [PATCH V3 1/6] dmaengine: zynqmp_dma: Fix PM usage count handling in probe error path --- > commit 8bb5754b8f17c08d0144740ac308428274806a68 > Author: Golla Nagendra <[email protected]> > > dmaengine: zynqmp_dma: Fix PM usage count handling in probe error path > > This commit attempts to fix a reference leak on the probe error path. When > probe fails after successfully waking up the device, it adds a missing > runtime PM put call to drop the usage count and allow the device to suspend. > >> diff --git a/drivers/dma/xilinx/zynqmp_dma.c b/drivers/dma/xilinx/zynqmp_dma.c >> --- a/drivers/dma/xilinx/zynqmp_dma.c >> +++ b/drivers/dma/xilinx/zynqmp_dma.c > > [ ... ] > >> @@ -1152,6 +1152,8 @@ static int zynqmp_dma_probe(struct platform_device *pdev) >> err_disable_pm: >> if (!pm_runtime_enabled(zdev->dev)) >> zynqmp_dma_runtime_suspend(zdev->dev); >> + else >> + pm_runtime_put_sync(zdev->dev); >> pm_runtime_disable(zdev->dev); > > [Severity: Medium] > Does this actually suspend the device and disable the clocks? > > Since autosuspend is enabled for this device earlier in zynqmp_dma_probe(): > pm_runtime_use_autosuspend(zdev->dev); > > Calling pm_runtime_put_sync() will only schedule an asynchronous suspend > timer. The immediately following pm_runtime_disable() will cancel this > pending timer before the suspend callback ever runs, leaving the clocks > permanently enabled. > > Should this use pm_runtime_put_sync_suspend() instead to force a synchronous > suspend and bypass the autosuspend delay before disabling runtime PM? Thanks for catching this Agreed will fix this in the next version Thanks, Nagendra > >> return ret; >> } >