| Message ID | 20230316084127.126084-1-fabrice.gasnier@foss.st.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f747313249b74f323ddf841a9c8db14d989f296a |
| Headers | show |
| Series | [v2] usb: dwc2: fix a devres leak in hw_enable upon suspend resume | expand |
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
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; }
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(-)