> -----Original Message----- > From: Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> > Sent: Saturday, November 11, 2023 5:09 AM > To: Mehta, Piyush <piyush.mehta@xxxxxxx> > Cc: gregkh@xxxxxxxxxxxxxxxxxxx; Simek, Michal <michal.simek@xxxxxxx>; > Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx>; linux-usb@xxxxxxxxxxxxxxx; > linux-kernel@xxxxxxxxxxxxxxx; git (AMD-Xilinx) <git@xxxxxxx> > Subject: Re: [PATCH] usb: dwc3: xilinx: improve error handling for PM APIs > > On Wed, Nov 08, 2023, Piyush Mehta wrote: > > Improve error handling for PM APIs in the dwc3_xlnx_probe function by > > introducing devm_pm_runtime_enable and error label. Removed > > unnecessary API pm_runtime_disable call in dwc3_xlnx_remove. > > > > Signed-off-by: Piyush Mehta <piyush.mehta@xxxxxxx> > > --- > > drivers/usb/dwc3/dwc3-xilinx.c | 10 +++++++--- > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/usb/dwc3/dwc3-xilinx.c > > b/drivers/usb/dwc3/dwc3-xilinx.c index 5b7e92f476de..9cf26e9a1c3d > > 100644 > > --- a/drivers/usb/dwc3/dwc3-xilinx.c > > +++ b/drivers/usb/dwc3/dwc3-xilinx.c > > @@ -294,10 +294,15 @@ static int dwc3_xlnx_probe(struct > > platform_device *pdev) > > > > pm_runtime_set_active(dev); > > pm_runtime_enable(dev); > > + ret = devm_pm_runtime_enable(dev); > > You just did pm_runtime_enable() right above, why > devm_pm_runtime_enable() again? This will fix in next version. > > > + if (ret < 0) > > + goto err_pm_set_suspended; > > + > > pm_suspend_ignore_children(dev, false); > > - pm_runtime_get_sync(dev); > > + return pm_runtime_resume_and_get(dev); > > > > - return 0; > > +err_pm_set_suspended: > > + pm_runtime_set_suspended(dev); > > This doesn't look right. why set status suspended here? Status is set to suspended in the exit path to undo the state set by pm_runtime_set_active(). The initial runtime PM status of all devices is 'suspended'. There is a mention of in Documentation/power/runtime_pm.rst For this reason, once pm_runtime_set_active() has been called for the device, pm_runtime_enable() should be called for it too as soon as reasonably possible or its run-time PM status should be changed back to 'suspended' with the help of pm_runtime_set_suspended(). > > BR, > Thinh > > > > > err_clk_put: > > clk_bulk_disable_unprepare(priv_data->num_clocks, priv_data- > >clks); > > @@ -315,7 +320,6 @@ static void dwc3_xlnx_remove(struct > platform_device *pdev) > > clk_bulk_disable_unprepare(priv_data->num_clocks, priv_data- > >clks); > > priv_data->num_clocks = 0; > > > > - pm_runtime_disable(dev); > > pm_runtime_put_noidle(dev); > > pm_runtime_set_suspended(dev); > > } > > -- > > 2.25.1 > >
