| Message ID | 20231108043441.2103940-1-piyush.mehta@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb: dwc3: xilinx: improve error handling for PM APIs | expand |
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@amd.com> > --- > 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? > + 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? 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 >
> -----Original Message----- > From: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > Sent: Saturday, November 11, 2023 5:09 AM > To: Mehta, Piyush <piyush.mehta@amd.com> > Cc: gregkh@linuxfoundation.org; Simek, Michal <michal.simek@amd.com>; > Thinh Nguyen <Thinh.Nguyen@synopsys.com>; linux-usb@vger.kernel.org; > linux-kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com> > 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@amd.com> > > --- > > 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 > >
Sorry for the delay response. On Tue, Nov 14, 2023, Pandey, Radhey Shyam wrote: > > -----Original Message----- > > From: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > > Sent: Saturday, November 11, 2023 5:09 AM > > To: Mehta, Piyush <piyush.mehta@amd.com> > > Cc: gregkh@linuxfoundation.org; Simek, Michal <michal.simek@amd.com>; > > Thinh Nguyen <Thinh.Nguyen@synopsys.com>; linux-usb@vger.kernel.org; > > linux-kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com> > > 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@amd.com> > > > --- > > > 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(). > If your intention of treating the status of devm_pm_runtime_enable() as the status of pm_runtime_enable(), then that's not correct. BR, Thinh
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); + 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); 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); }
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@amd.com> --- drivers/usb/dwc3/dwc3-xilinx.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)