| Message ID | 20230325165217.31069-6-manivannan.sadhasivam@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | usb: dwc3: qcom: Allow runtime PM | expand |
On Sat, Mar 25, 2023 at 10:22:17PM +0530, Manivannan Sadhasivam wrote: > dwc3-qcom driver is capable of doing runtime PM on its own, but currently > it requires userspace intervention to enable it. But there is no harm in > letting the driver to enable runtime PM on its own. So let's get rid of the > "pm_runtime_forbid()" and make sure that the dependency is maintained with > child devices using "pm_suspend_ignore_children(dev, false)". Well, the potential harm is that these paths have hardly been tested so enabling it by default is a risk (e.g. as you noticed when trying to enable this by default). And especially if we don't address the layering violations first. > Also during remove(), the device needs to be waken up first if it was > runtime suspended. Finally, pm_runtime_allow() can be removed. > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > drivers/usb/dwc3/dwc3-qcom.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > index f1059dfcb0e8..5f26bb66274f 100644 > --- a/drivers/usb/dwc3/dwc3-qcom.c > +++ b/drivers/usb/dwc3/dwc3-qcom.c > @@ -920,7 +920,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > qcom->is_suspended = false; > pm_runtime_set_active(dev); > pm_runtime_enable(dev); > - pm_runtime_forbid(dev); > + pm_suspend_ignore_children(dev, false); There's no need to explicitly disable ignore-children as that is the default. > return 0; > > @@ -948,6 +948,8 @@ static int dwc3_qcom_remove(struct platform_device *pdev) > struct device *dev = &pdev->dev; > int i; > > + pm_runtime_get_sync(dev); This call needs to be balanced. But this is a fix for a bug in the current implementation that should go in a separate patch. > + > device_remove_software_node(&qcom->dwc3->dev); > of_platform_depopulate(dev); > > @@ -960,7 +962,6 @@ static int dwc3_qcom_remove(struct platform_device *pdev) > dwc3_qcom_interconnect_exit(qcom); > reset_control_assert(qcom->resets); > > - pm_runtime_allow(dev); > pm_runtime_disable(dev); > > return 0; Johan
On Tue, Mar 28, 2023 at 11:46:36AM +0200, Johan Hovold wrote: > On Sat, Mar 25, 2023 at 10:22:17PM +0530, Manivannan Sadhasivam wrote: > > dwc3-qcom driver is capable of doing runtime PM on its own, but currently > > it requires userspace intervention to enable it. But there is no harm in > > letting the driver to enable runtime PM on its own. So let's get rid of the > > "pm_runtime_forbid()" and make sure that the dependency is maintained with > > child devices using "pm_suspend_ignore_children(dev, false)". > > Well, the potential harm is that these paths have hardly been tested so > enabling it by default is a risk (e.g. as you noticed when trying to > enable this by default). And especially if we don't address the layering > violations first. > I certainly tested this on a couple of boards with host and gadget mode and noticed no issue (except one issue noticed by Steev on a docking station with display but that should be related to orientation switch). Even if we allow runtime PM on this driver, still userspace needs to enable it for dwc3 and xhci drivers. So this essentially reduces one step in that process if someone tries to enable runtime PM for usb intentionally. So I don't forsee a potential harm here. > > Also during remove(), the device needs to be waken up first if it was > > runtime suspended. Finally, pm_runtime_allow() can be removed. > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > --- > > drivers/usb/dwc3/dwc3-qcom.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > > index f1059dfcb0e8..5f26bb66274f 100644 > > --- a/drivers/usb/dwc3/dwc3-qcom.c > > +++ b/drivers/usb/dwc3/dwc3-qcom.c > > @@ -920,7 +920,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > > qcom->is_suspended = false; > > pm_runtime_set_active(dev); > > pm_runtime_enable(dev); > > - pm_runtime_forbid(dev); > > + pm_suspend_ignore_children(dev, false); > > There's no need to explicitly disable ignore-children as that is the > default. > Other dwc drivers were doing it, so I thought someone (maintainer) wanted to explicitly disable ignore_children. But if that's not the case, I can remove it. > > return 0; > > > > @@ -948,6 +948,8 @@ static int dwc3_qcom_remove(struct platform_device *pdev) > > struct device *dev = &pdev->dev; > > int i; > > > > + pm_runtime_get_sync(dev); > > This call needs to be balanced. But this is a fix for a bug in the > current implementation that should go in a separate patch. > Ok. For balancing I could add pm_runtime_put_noidle() before pm_runtime_disable. - Mani > > + > > device_remove_software_node(&qcom->dwc3->dev); > > of_platform_depopulate(dev); > > > > @@ -960,7 +962,6 @@ static int dwc3_qcom_remove(struct platform_device *pdev) > > dwc3_qcom_interconnect_exit(qcom); > > reset_control_assert(qcom->resets); > > > > - pm_runtime_allow(dev); > > pm_runtime_disable(dev); > > > > return 0; > > Johan
On Tue, Mar 28, 2023 at 03:35:01PM +0530, Manivannan Sadhasivam wrote: > On Tue, Mar 28, 2023 at 11:46:36AM +0200, Johan Hovold wrote: > > On Sat, Mar 25, 2023 at 10:22:17PM +0530, Manivannan Sadhasivam wrote: > > > dwc3-qcom driver is capable of doing runtime PM on its own, but currently > > > it requires userspace intervention to enable it. But there is no harm in > > > letting the driver to enable runtime PM on its own. So let's get rid of the > > > "pm_runtime_forbid()" and make sure that the dependency is maintained with > > > child devices using "pm_suspend_ignore_children(dev, false)". > > > > Well, the potential harm is that these paths have hardly been tested so > > enabling it by default is a risk (e.g. as you noticed when trying to > > enable this by default). And especially if we don't address the layering > > violations first. > > > > I certainly tested this on a couple of boards with host and gadget mode and > noticed no issue (except one issue noticed by Steev on a docking station with > display but that should be related to orientation switch). > > Even if we allow runtime PM on this driver, still userspace needs to enable it > for dwc3 and xhci drivers. So this essentially reduces one step in that process > if someone tries to enable runtime PM for usb intentionally. So I don't forsee a > potential harm here. Well this whole driver is a mess so I don't have any problem imagining ways in which things can break. ;) > > > Also during remove(), the device needs to be waken up first if it was > > > runtime suspended. Finally, pm_runtime_allow() can be removed. > > > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > --- > > > drivers/usb/dwc3/dwc3-qcom.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > > > index f1059dfcb0e8..5f26bb66274f 100644 > > > --- a/drivers/usb/dwc3/dwc3-qcom.c > > > +++ b/drivers/usb/dwc3/dwc3-qcom.c > > > @@ -920,7 +920,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > > > qcom->is_suspended = false; > > > pm_runtime_set_active(dev); > > > pm_runtime_enable(dev); > > > - pm_runtime_forbid(dev); > > > + pm_suspend_ignore_children(dev, false); > > > > There's no need to explicitly disable ignore-children as that is the > > default. > > > > Other dwc drivers were doing it, so I thought someone (maintainer) wanted to > explicitly disable ignore_children. But if that's not the case, I can remove it. Yeah, please remove it. I doubt these runtime pm implementations have gotten much review. Note how several dwc3 glue drivers just do an unconditional get in probe(), which means that these paths are probably never exercised at all and effectively amounts to that pm_runtime_forbid() you are removing here. Probably there to tick off "runtime pm" on some internal project manager's list of "features". > > > return 0; > > > > > > @@ -948,6 +948,8 @@ static int dwc3_qcom_remove(struct platform_device *pdev) > > > struct device *dev = &pdev->dev; > > > int i; > > > > > > + pm_runtime_get_sync(dev); > > > > This call needs to be balanced. But this is a fix for a bug in the > > current implementation that should go in a separate patch. > > > > Ok. For balancing I could add pm_runtime_put_noidle() before pm_runtime_disable. You should do it after disabling runtime pm. > > > + > > > device_remove_software_node(&qcom->dwc3->dev); > > > of_platform_depopulate(dev); > > > > > > @@ -960,7 +962,6 @@ static int dwc3_qcom_remove(struct platform_device *pdev) > > > dwc3_qcom_interconnect_exit(qcom); > > > reset_control_assert(qcom->resets); > > > > > > - pm_runtime_allow(dev); > > > pm_runtime_disable(dev); > > > > > > return 0; Johan
On Tue, Mar 28, 2023 at 02:18:16PM +0200, Johan Hovold wrote: > On Tue, Mar 28, 2023 at 03:35:01PM +0530, Manivannan Sadhasivam wrote: > > On Tue, Mar 28, 2023 at 11:46:36AM +0200, Johan Hovold wrote: > > > On Sat, Mar 25, 2023 at 10:22:17PM +0530, Manivannan Sadhasivam wrote: > > > > dwc3-qcom driver is capable of doing runtime PM on its own, but currently > > > > it requires userspace intervention to enable it. But there is no harm in > > > > letting the driver to enable runtime PM on its own. So let's get rid of the > > > > "pm_runtime_forbid()" and make sure that the dependency is maintained with > > > > child devices using "pm_suspend_ignore_children(dev, false)". > > > > > > Well, the potential harm is that these paths have hardly been tested so > > > enabling it by default is a risk (e.g. as you noticed when trying to > > > enable this by default). And especially if we don't address the layering > > > violations first. > > > > > > > I certainly tested this on a couple of boards with host and gadget mode and > > noticed no issue (except one issue noticed by Steev on a docking station with > > display but that should be related to orientation switch). > > > > Even if we allow runtime PM on this driver, still userspace needs to enable it > > for dwc3 and xhci drivers. So this essentially reduces one step in that process > > if someone tries to enable runtime PM for usb intentionally. So I don't forsee a > > potential harm here. > > Well this whole driver is a mess so I don't have any problem imagining > ways in which things can break. ;) > > > > > Also during remove(), the device needs to be waken up first if it was > > > > runtime suspended. Finally, pm_runtime_allow() can be removed. > > > > > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > --- > > > > drivers/usb/dwc3/dwc3-qcom.c | 5 +++-- > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > > > > index f1059dfcb0e8..5f26bb66274f 100644 > > > > --- a/drivers/usb/dwc3/dwc3-qcom.c > > > > +++ b/drivers/usb/dwc3/dwc3-qcom.c > > > > @@ -920,7 +920,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > > > > qcom->is_suspended = false; > > > > pm_runtime_set_active(dev); > > > > pm_runtime_enable(dev); > > > > - pm_runtime_forbid(dev); > > > > + pm_suspend_ignore_children(dev, false); > > > > > > There's no need to explicitly disable ignore-children as that is the > > > default. > > > > > > > Other dwc drivers were doing it, so I thought someone (maintainer) wanted to > > explicitly disable ignore_children. But if that's not the case, I can remove it. > > Yeah, please remove it. I doubt these runtime pm implementations have > gotten much review. > > Note how several dwc3 glue drivers just do an unconditional get in > probe(), which means that these paths are probably never exercised at > all and effectively amounts to that pm_runtime_forbid() you are removing > here. > > Probably there to tick off "runtime pm" on some internal project > manager's list of "features". > Agree. > > > > return 0; > > > > > > > > @@ -948,6 +948,8 @@ static int dwc3_qcom_remove(struct platform_device *pdev) > > > > struct device *dev = &pdev->dev; > > > > int i; > > > > > > > > + pm_runtime_get_sync(dev); > > > > > > This call needs to be balanced. But this is a fix for a bug in the > > > current implementation that should go in a separate patch. > > > > > > > Ok. For balancing I could add pm_runtime_put_noidle() before pm_runtime_disable. > > You should do it after disabling runtime pm. > May I know why? Thanks, Mani > > > > + > > > > device_remove_software_node(&qcom->dwc3->dev); > > > > of_platform_depopulate(dev); > > > > > > > > @@ -960,7 +962,6 @@ static int dwc3_qcom_remove(struct platform_device *pdev) > > > > dwc3_qcom_interconnect_exit(qcom); > > > > reset_control_assert(qcom->resets); > > > > > > > > - pm_runtime_allow(dev); > > > > pm_runtime_disable(dev); > > > > > > > > return 0; > > Johan
On Tue, Mar 28, 2023 at 06:27:05PM +0530, Manivannan Sadhasivam wrote: > On Tue, Mar 28, 2023 at 02:18:16PM +0200, Johan Hovold wrote: > > > > > @@ -948,6 +948,8 @@ static int dwc3_qcom_remove(struct platform_device *pdev) > > > > > struct device *dev = &pdev->dev; > > > > > int i; > > > > > > > > > > + pm_runtime_get_sync(dev); > > > > > > > > This call needs to be balanced. But this is a fix for a bug in the > > > > current implementation that should go in a separate patch. > > > > > > Ok. For balancing I could add pm_runtime_put_noidle() before pm_runtime_disable. > > > > You should do it after disabling runtime pm. > > May I know why? The usage counter should be balanced after disabling runtime PM so that there is no window where you could get a racing suspend request. > > > > > + > > > > > device_remove_software_node(&qcom->dwc3->dev); > > > > > of_platform_depopulate(dev); > > > > > > > > > > @@ -960,7 +962,6 @@ static int dwc3_qcom_remove(struct platform_device *pdev) > > > > > dwc3_qcom_interconnect_exit(qcom); > > > > > reset_control_assert(qcom->resets); > > > > > > > > > > - pm_runtime_allow(dev); > > > > > pm_runtime_disable(dev); > > > > > > > > > > return 0; Johan
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c index f1059dfcb0e8..5f26bb66274f 100644 --- a/drivers/usb/dwc3/dwc3-qcom.c +++ b/drivers/usb/dwc3/dwc3-qcom.c @@ -920,7 +920,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev) qcom->is_suspended = false; pm_runtime_set_active(dev); pm_runtime_enable(dev); - pm_runtime_forbid(dev); + pm_suspend_ignore_children(dev, false); return 0; @@ -948,6 +948,8 @@ static int dwc3_qcom_remove(struct platform_device *pdev) struct device *dev = &pdev->dev; int i; + pm_runtime_get_sync(dev); + device_remove_software_node(&qcom->dwc3->dev); of_platform_depopulate(dev); @@ -960,7 +962,6 @@ static int dwc3_qcom_remove(struct platform_device *pdev) dwc3_qcom_interconnect_exit(qcom); reset_control_assert(qcom->resets); - pm_runtime_allow(dev); pm_runtime_disable(dev); return 0;
dwc3-qcom driver is capable of doing runtime PM on its own, but currently it requires userspace intervention to enable it. But there is no harm in letting the driver to enable runtime PM on its own. So let's get rid of the "pm_runtime_forbid()" and make sure that the dependency is maintained with child devices using "pm_suspend_ignore_children(dev, false)". Also during remove(), the device needs to be waken up first if it was runtime suspended. Finally, pm_runtime_allow() can be removed. Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> --- drivers/usb/dwc3/dwc3-qcom.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)