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

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>
---
Changes in v2: commit message updated to remove useless tags
---
 drivers/usb/dwc2/platform.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

Hi Fabrice,

>On 3/16/2023 12:41 PM, Fabrice Gasnier <fabrice.gasnier@foss.st.com> wrote:
>From: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
>Sent: Thursday, March 16, 2023 12:41 PM
>To: Minas Harutyunyan <hminas@synopsys.com>; gregkh@linuxfoundation.org;
>maz@kernel.org; m.szyprowski@samsung.com
>Cc: linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org; linux-stm32@st-
>md-mailman.stormreply.com; amelie.delaunay@foss.st.com;
>alexandre.torgue@foss.st.com; fabrice.gasnier@foss.st.com
>Subject: [PATCH v2] usb: dwc2: fix a devres leak in hw_enable upon suspend
>resume
>
>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>

Reviewed-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
Tested-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
Acked-by: Minas Harutyunyan <hminas@synopsys.com>

>---
>Changes in v2: commit message updated to remove useless tags
>---
> 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