[PATCH] usb: dwc2: fix a devres leak in hw_enable upon suspend resume

[PATCH] usb: dwc2: fix a devres leak in hw_enable upon suspend resume

[Fabrice Gasnier]

Each time the platform goes to low power, PM suspend / resume routines
call: __dwc2_lowlevel_hw_enable -> devm_add_action_or_reset().
This adds a new devres each time.
This may also happen at runtime, as dwc2_lowlevel_hw_enable() can be
called from udc_start().

This can be seen with tracing:
- echo 1 > /sys/kernel/debug/tracing/events/dev/devres_log/enable
- go to low power
- cat /sys/kernel/debug/tracing/trace

A new "ADD" entry is found upon each low power cycle:
... devres_log: 49000000.usb-otg ADD 82a13bba devm_action_release (8 bytes)
... devres_log: 49000000.usb-otg ADD 49889daf devm_action_release (8 bytes)
...

A second issue is addressed here:
- regulator_bulk_enable() is called upon each PM cycle (suspend/resume).
- regulator_bulk_disable() never gets called.

So the reference count for these regulators constantly increase, by one
upon each low power cycle, due to missing regulator_bulk_disable() call
in __dwc2_lowlevel_hw_disable().

The original fix that introduced the devm_add_action_or_reset() call,
fixed an issue during probe, that happens due to other errors in
dwc2_driver_probe() -> dwc2_core_reset(). Then the probe fails without
disabling regulators, when dr_mode == USB_DR_MODE_PERIPHERAL.

Rather fix the error path: disable all the low level hardware in the
error path, by using the "hsotg->ll_hw_enabled" flag. Checking dr_mode
has been introduced to avoid a dual call to dwc2_lowlevel_hw_disable().
"ll_hw_enabled" should achieve the same (and is used currently in the
remove() routine).

Fixes: 54c196060510 ("usb: dwc2: Always disable regulators on driver teardown")
Fixes: 33a06f1300a7 ("usb: dwc2: Fix error path in gadget registration")
Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
Change-Id: If038db394ae43cbbb40279b2eca4cba021644a6d
Reviewed-on: https://gerrit.st.com/c/mpu/oe/st/linux-stm32/+/291408
ACI: CITOOLS <MDG-smet-aci-reviews@list.st.com>
ACI: CIBUILD <MDG-smet-aci-builds@list.st.com>
---
 drivers/usb/dwc2/platform.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index 262c13b6362a..d1589ba7d322 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -91,13 +91,6 @@ static int dwc2_get_dr_mode(struct dwc2_hsotg *hsotg)
 	return 0;
 }
 
-static void __dwc2_disable_regulators(void *data)
-{
-	struct dwc2_hsotg *hsotg = data;
-
-	regulator_bulk_disable(ARRAY_SIZE(hsotg->supplies), hsotg->supplies);
-}
-
 static int __dwc2_lowlevel_hw_enable(struct dwc2_hsotg *hsotg)
 {
 	struct platform_device *pdev = to_platform_device(hsotg->dev);
@@ -108,11 +101,6 @@ static int __dwc2_lowlevel_hw_enable(struct dwc2_hsotg *hsotg)
 	if (ret)
 		return ret;
 
-	ret = devm_add_action_or_reset(&pdev->dev,
-				       __dwc2_disable_regulators, hsotg);
-	if (ret)
-		return ret;
-
 	if (hsotg->clk) {
 		ret = clk_prepare_enable(hsotg->clk);
 		if (ret)
@@ -168,7 +156,7 @@ static int __dwc2_lowlevel_hw_disable(struct dwc2_hsotg *hsotg)
 	if (hsotg->clk)
 		clk_disable_unprepare(hsotg->clk);
 
-	return 0;
+	return regulator_bulk_disable(ARRAY_SIZE(hsotg->supplies), hsotg->supplies);
 }
 
 /**
@@ -607,7 +595,7 @@ static int dwc2_driver_probe(struct platform_device *dev)
 	if (hsotg->params.activate_stm_id_vb_detection)
 		regulator_disable(hsotg->usb33d);
 error:
-	if (hsotg->dr_mode != USB_DR_MODE_PERIPHERAL)
+	if (hsotg->ll_hw_enabled)
 		dwc2_lowlevel_hw_disable(hsotg);
 	return retval;
 }
-- 
2.25.1

Re: [PATCH] usb: dwc2: fix a devres leak in hw_enable upon suspend resume

[Fabrice Gasnier]

On 3/15/23 17:32, Fabrice Gasnier wrote:
> Each time the platform goes to low power, PM suspend / resume routines
> call: __dwc2_lowlevel_hw_enable -> devm_add_action_or_reset().
> This adds a new devres each time.
> This may also happen at runtime, as dwc2_lowlevel_hw_enable() can be
> called from udc_start().
> 
> This can be seen with tracing:
> - echo 1 > /sys/kernel/debug/tracing/events/dev/devres_log/enable
> - go to low power
> - cat /sys/kernel/debug/tracing/trace
> 
> A new "ADD" entry is found upon each low power cycle:
> ... devres_log: 49000000.usb-otg ADD 82a13bba devm_action_release (8 bytes)
> ... devres_log: 49000000.usb-otg ADD 49889daf devm_action_release (8 bytes)
> ...
> 
> A second issue is addressed here:
> - regulator_bulk_enable() is called upon each PM cycle (suspend/resume).
> - regulator_bulk_disable() never gets called.
> 
> So the reference count for these regulators constantly increase, by one
> upon each low power cycle, due to missing regulator_bulk_disable() call
> in __dwc2_lowlevel_hw_disable().
> 
> The original fix that introduced the devm_add_action_or_reset() call,
> fixed an issue during probe, that happens due to other errors in
> dwc2_driver_probe() -> dwc2_core_reset(). Then the probe fails without
> disabling regulators, when dr_mode == USB_DR_MODE_PERIPHERAL.
> 
> Rather fix the error path: disable all the low level hardware in the
> error path, by using the "hsotg->ll_hw_enabled" flag. Checking dr_mode
> has been introduced to avoid a dual call to dwc2_lowlevel_hw_disable().
> "ll_hw_enabled" should achieve the same (and is used currently in the
> remove() routine).
> 
> Fixes: 54c196060510 ("usb: dwc2: Always disable regulators on driver teardown")
> Fixes: 33a06f1300a7 ("usb: dwc2: Fix error path in gadget registration")
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---

Hi all,

Please ignore this patch, I just sent a V2 to replace it.

Best Regards,
Fabrice