usb: dwc3: qcom: fix wakeup implementation

Generic PHYs must be powered-off before they can be tore down.

Similarly, suspending legacy PHYs after having powered them off makes no sense.

Fix the dwc3_core_exit() (e.g. called during suspend) and open-coded dwc3_probe() error-path sequences that got this wrong.

Note that this makes dwc3_core_exit() match the dwc3_core_init() error path with respect to powering off the PHYs.

Fixes 03c1fd622f72 ("usb: dwc3: core: add phy cleanup for probe error handling")

Fixes c499ff71ff2a ("usb: dwc3: core: re-factor init and exit paths")

Cc [email protected] # 4.8

Reviewed-by Andrew Halaney <[email protected]>

Reviewed-by Matthias Kaehlcke <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/dwc3/core.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

drivers/usb/dwc3/core.c +10 -9

index c5c238ab3083..16d1f328775f 100644
@@ -833,15 +833,16 @@ static void dwc3_core_exit(struct dwc3 *dwc)
 {
 	dwc3_event_buffers_cleanup(dwc);
 
+	usb_phy_set_suspend(dwc->usb2_phy, 1);
+	usb_phy_set_suspend(dwc->usb3_phy, 1);
+	phy_power_off(dwc->usb2_generic_phy);
+	phy_power_off(dwc->usb3_generic_phy);
+
 	usb_phy_shutdown(dwc->usb2_phy);
 	usb_phy_shutdown(dwc->usb3_phy);
 	phy_exit(dwc->usb2_generic_phy);
 	phy_exit(dwc->usb3_generic_phy);
 
-	usb_phy_set_suspend(dwc->usb2_phy, 1);
-	usb_phy_set_suspend(dwc->usb3_phy, 1);
-	phy_power_off(dwc->usb2_generic_phy);
-	phy_power_off(dwc->usb3_generic_phy);
 	dwc3_clk_disable(dwc);
 	reset_control_assert(dwc->reset);
 }

@@ -1879,16 +1880,16 @@ static int dwc3_probe(struct platform_device *pdev)
 	dwc3_debugfs_exit(dwc);
 	dwc3_event_buffers_cleanup(dwc);
 
-	usb_phy_shutdown(dwc->usb2_phy);
-	usb_phy_shutdown(dwc->usb3_phy);
-	phy_exit(dwc->usb2_generic_phy);
-	phy_exit(dwc->usb3_generic_phy);
-
 	usb_phy_set_suspend(dwc->usb2_phy, 1);
 	usb_phy_set_suspend(dwc->usb3_phy, 1);
 	phy_power_off(dwc->usb2_generic_phy);
 	phy_power_off(dwc->usb3_generic_phy);
 
+	usb_phy_shutdown(dwc->usb2_phy);
+	usb_phy_shutdown(dwc->usb3_phy);
+	phy_exit(dwc->usb2_generic_phy);
+	phy_exit(dwc->usb3_generic_phy);
+
 	dwc3_ulpi_exit(dwc);
 
 err4:

Signature · 1 line

2.35.1

This reverts commit d9be8d5c5b032e5383ff5c404ff4155e9c705429.

Generic power-domain flags must be set before the power-domain is initialised and must specifically not be modified by drivers for devices that happen to be in the domain.

To make sure that USB power-domains are left enabled during system suspend when a device in the domain is in the wakeup path, the GENPD_FLAG_ACTIVE_WAKEUP flag should instead be set for the domain unconditionally when it is registered.

Note that this also avoids keeping power-domains on during suspend when wakeup has not been enabled (e.g. through sysfs).

For the runtime PM case, making sure that the PHYs are not suspended and that they are in the same domain as the controller prevents the domain from being suspended. If there are cases where this is not possible or desirable, the genpd implementation may need to be extended.

Fixes d9be8d5c5b03 ("usb: dwc3: qcom: Keep power domain on to retain controller status")

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/dwc3/dwc3-qcom.c | 28 +++++++---------------------
 1 file changed, 7 insertions(+), 21 deletions(-)

drivers/usb/dwc3/dwc3-qcom.c +7 -21

index c5e482f53e9d..be2e3dd36440 100644
@@ -17,7 +17,6 @@
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/phy/phy.h>
-#include <linux/pm_domain.h>
 #include <linux/usb/of.h>
 #include <linux/reset.h>
 #include <linux/iopoll.h>

@@ -757,13 +756,12 @@ dwc3_qcom_create_urs_usb_platdev(struct device *dev)
 
 static int dwc3_qcom_probe(struct platform_device *pdev)
 {
-	struct device_node *np = pdev->dev.of_node;
-	struct device *dev = &pdev->dev;
-	struct dwc3_qcom *qcom;
-	struct resource	*res, *parent_res = NULL;
-	int ret, i;
-	bool ignore_pipe_clk;
-	struct generic_pm_domain *genpd;
+	struct device_node	*np = pdev->dev.of_node;
+	struct device		*dev = &pdev->dev;
+	struct dwc3_qcom	*qcom;
+	struct resource		*res, *parent_res = NULL;
+	int			ret, i;
+	bool			ignore_pipe_clk;
 
 	qcom = devm_kzalloc(&pdev->dev, sizeof(*qcom), GFP_KERNEL);
 	if (!qcom)

@@ -772,8 +770,6 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, qcom);
 	qcom->dev = &pdev->dev;
 
-	genpd = pd_to_genpd(qcom->dev->pm_domain);
-
 	if (has_acpi_companion(dev)) {
 		qcom->acpi_pdata = acpi_device_get_match_data(dev);
 		if (!qcom->acpi_pdata) {

@@ -881,17 +877,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	if (ret)
 		goto interconnect_exit;
 
-	if (device_can_wakeup(&qcom->dwc3->dev)) {
-		/*
-		 * Setting GENPD_FLAG_ALWAYS_ON flag takes care of keeping
-		 * genpd on in both runtime suspend and system suspend cases.
-		 */
-		genpd->flags |= GENPD_FLAG_ALWAYS_ON;
-		device_init_wakeup(&pdev->dev, true);
-	} else {
-		genpd->flags |= GENPD_FLAG_RPM_ALWAYS_ON;
-	}
-
+	device_init_wakeup(&pdev->dev, 1);
 	qcom->is_suspended = false;
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);

Signature · 1 line

2.35.1

A recent change added a dependency to the USB host stack and broke gadget-only builds of the driver.

Fixes 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend")

Reported-by Randy Dunlap <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---

Changes in v2
- new patch

 drivers/usb/dwc3/dwc3-qcom.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/usb/dwc3/dwc3-qcom.c +4 -1

index be2e3dd36440..e9364141661b 100644
@@ -310,8 +310,11 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
 	 * currently supports only 1 port per controller. So
 	 * this is sufficient.
 	 */
+#ifdef CONFIG_USB
 	udev = usb_hub_find_child(hcd->self.root_hub, 1);
-
+#else
+	udev = NULL;
+#endif
 	if (!udev)
 		return USB_SPEED_UNKNOWN;
 

A recent change added a dependency to the USB host stack and broke gadget-only builds of the driver.

Fixes 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend")

Reported-by Randy Dunlap <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

--`-

Changes in v2
- new patch

 drivers/usb/dwc3/dwc3-qcom.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/usb/dwc3/dwc3-qcom.c +0 -0

index be2e3dd36440..e9364141661b 100644

drivers/usb/dwc3/dwc3-qcom.c +0 -0

 	if (!udev)
 		return USB_SPEED_UNKNOWN;
 

Signature · 1 line

2.35.1

Quoted 24 lines · Johan Hovold

A recent change added a dependency to the USB host stack and broke gadget-only builds of the driver.

Fixes 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend")

Reported-by Randy Dunlap <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

--`-

Changes in v2
- new patch

 drivers/usb/dwc3/dwc3-qcom.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/usb/dwc3/dwc3-qcom.c +1 -0

index be2e3dd36440..e9364141661b 100644
@@ -310,8 +310,11 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
 	 * currently supports only 1 port per controller. So
 	 * this is sufficient.
 	 */
+#ifdef CONFIG_USB
 	udev = usb_hub_find_child(hcd->self.root_hub, 1);

drivers/usb/dwc3/dwc3-qcom.c static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom) · line 317

Quoted 4 lines

drivers/usb/dwc3/dwc3-qcom.c +3 -1

-
+#else
+	udev = NULL;
+#endif

Perhaps the check should be moved to the caller instead? This function still references "usb_hcd" struct and I don't think that's intended for gadget only mode.

Quoted 5 lines

drivers/usb/dwc3/dwc3-qcom.c +0 -0

 	if (!udev)
 		return USB_SPEED_UNKNOWN;
 

Signature · 1 line

2.35.1

Quoted 25 lines · Manivannan Sadhasivam

Quoted 24 lines · Johan Hovold

A recent change added a dependency to the USB host stack and broke gadget-only builds of the driver.

Fixes 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend")

Reported-by Randy Dunlap <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

--`-

Changes in v2
- new patch

 drivers/usb/dwc3/dwc3-qcom.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/usb/dwc3/dwc3-qcom.c +1 -0

index be2e3dd36440..e9364141661b 100644
@@ -310,8 +310,11 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
 	 * currently supports only 1 port per controller. So
 	 * this is sufficient.
 	 */
+#ifdef CONFIG_USB
 	udev = usb_hub_find_child(hcd->self.root_hub, 1);

Quoted 8 lines

drivers/usb/dwc3/dwc3-qcom.c static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom) · line 317

Quoted 4 lines

drivers/usb/dwc3/dwc3-qcom.c +3 -1

-
+#else
+	udev = NULL;
+#endif

Perhaps the check should be moved to the caller instead? This function still references "usb_hcd" struct and I don't think that's intended for gadget only mode.

That wouldn't help with the build failure, which is what this patch is addressing.

Quoted 5 lines

Quoted 5 lines

drivers/usb/dwc3/dwc3-qcom.c +0 -0

 	if (!udev)
 		return USB_SPEED_UNKNOWN;
 

Signature · 1 line

2.35.1

Johan

Quoted 35 lines · Johan Hovold

Quoted 25 lines · Manivannan Sadhasivam

Quoted 24 lines · Johan Hovold

A recent change added a dependency to the USB host stack and broke gadget-only builds of the driver.

Fixes 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend")

Reported-by Randy Dunlap <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

--`-

Changes in v2
- new patch

 drivers/usb/dwc3/dwc3-qcom.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/usb/dwc3/dwc3-qcom.c +1 -0

index be2e3dd36440..e9364141661b 100644
@@ -310,8 +310,11 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
 	 * currently supports only 1 port per controller. So
 	 * this is sufficient.
 	 */
+#ifdef CONFIG_USB
 	udev = usb_hub_find_child(hcd->self.root_hub, 1);

Quoted 8 lines

drivers/usb/dwc3/dwc3-qcom.c static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom) · line 317

Quoted 4 lines

drivers/usb/dwc3/dwc3-qcom.c +3 -1

-
+#else
+	udev = NULL;
+#endif

Perhaps the check should be moved to the caller instead? This function still references "usb_hcd" struct and I don't think that's intended for gadget only mode.

Quoted 3 lines

That wouldn't help with the build failure, which is what this patch is addressing.

I should've put it clearly. You should guard the entire function and not just usb_hub_find_child(). This way it becomes clear that this whole function depends on the USB host functionality. Like,

#ifdef CONFIG_USB
static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom) {
...
}
#elif
static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom) {
return USB_SPEED_UNKNOWN
}
#endif

A recent change added a dependency to the USB host stack and broke gadget-only builds of the driver.

Fixes 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend")

Reported-by Randy Dunlap <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---

Changes in v2
- new patch

 drivers/usb/dwc3/dwc3-qcom.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/usb/dwc3/dwc3-qcom.c +0 -0

index be2e3dd36440..e9364141661b 100644

Quoted 14 lines · Johan Hovold

A recent change added a dependency to the USB host stack and broke gadget-only builds of the driver.

Fixes 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend")

Reported-by Randy Dunlap <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---

Changes in v2
- new patch

 drivers/usb/dwc3/dwc3-qcom.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/usb/dwc3/dwc3-qcom.c static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom) · line 314

Quoted 10 lines

drivers/usb/dwc3/dwc3-qcom.c +1 -0

index be2e3dd36440..e9364141661b 100644
@@ -310,8 +310,11 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
 	 * currently supports only 1 port per controller. So
 	 * this is sufficient.
 	 */
+#ifdef CONFIG_USB
 	udev = usb_hub_find_child(hcd->self.root_hub, 1);

If a gadget driver needs this for some reason, then the #ifdef should be put in a .h file, not in a .c file.

But step back a minute and ask why a host-config-only function is being called when a device is in gadget-only mode? This feels like a design/logic issue in this file, NOT something to paper over with a #ifdef in a .c file

This implies that if this device is NOT in a host configuration, then the suspend path of it is not configured properly at all, as why would it be checking or caring about this at all if this is in gadget-only mode?

Something else is wrong here, let's fix the root problem please. Maybe this driver should just never be built in gadget-only mode, as it is never intended to support that option?

Quoted 15 lines · Greg Kroah-Hartman

Quoted 14 lines · Johan Hovold

A recent change added a dependency to the USB host stack and broke gadget-only builds of the driver.

Fixes 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend")

Reported-by Randy Dunlap <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---

Changes in v2
- new patch

 drivers/usb/dwc3/dwc3-qcom.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Quoted 13 lines

drivers/usb/dwc3/dwc3-qcom.c static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom) · line 314

Quoted 10 lines

drivers/usb/dwc3/dwc3-qcom.c +1 -0

index be2e3dd36440..e9364141661b 100644
@@ -310,8 +310,11 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
 	 * currently supports only 1 port per controller. So
 	 * this is sufficient.
 	 */
+#ifdef CONFIG_USB
 	udev = usb_hub_find_child(hcd->self.root_hub, 1);

If a gadget driver needs this for some reason, then the #ifdef should be put in a .h file, not in a .c file.

Yeah, if we're keeping this long-term then yes, and possibly also otherwise.

Quoted 4 lines

But step back a minute and ask why a host-config-only function is being called when a device is in gadget-only mode? This feels like a design/logic issue in this file, NOT something to paper over with a #ifdef in a .c file

We're not as I'm fixing that bug in later in the series. I should probably have put this one after that fix, but figured fixing the build was more important than a harder-to-hit NULL-deref due to non-host mode not being considered when the offending series was merged.

Quoted 4 lines

This implies that if this device is NOT in a host configuration, then the suspend path of it is not configured properly at all, as why would it be checking or caring about this at all if this is in gadget-only mode?

Right, so see path 6/9 which addresses this by only calling this hack when in host mode:

	https://lore.kernel.org/all/[email protected]/

Quoted 3 lines

Something else is wrong here, let's fix the root problem please. Maybe this driver should just never be built in gadget-only mode, as it is never intended to support that option?

The problem is commit 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend"), which I considered simply reverting but as that breaks suspend completely on some boards I decided to try and fix it up while we work on a proper long-term solution (i.e. for how the dwc/xhci layers should be communicating to implement this).

Remember that it took two years and 21 revisions to get to the state we're at now after you merged the wakeup series in June.

The Qualcomm dwc3 runtime-PM implementation checks the xhci platform-device pointer in the wakeup-interrupt handler to determine whether the controller is in host mode and if so triggers a resume.

After a role switch in OTG mode the xhci platform-device would have been freed and the next wakeup from runtime suspend would access the freed memory.

Note that role switching is executed from a freezable workqueue, which guarantees that the pointer is stable during suspend.

Also note that runtime PM has been broken since commit 2664deb09306 ("usb: dwc3: qcom: Honor wakeup enabled/disabled state"), which incidentally also prevents this issue from being triggered.

Fixes a4333c3a6ba9 ("usb: dwc3: Add Qualcomm DWC3 glue driver")

Cc [email protected] # 4.18

Signed-off-by Johan Hovold <[email protected]>

The Qualcomm dwc3 runtime-PM implementation checks the xhci platform-device pointer in the wakeup-interrupt handler to determine whether the controller is in host mode and if so triggers a resume.

After a role switch in OTG mode the xhci platform-device would have been freed and the next wakeup from runtime suspend would access the freed memory.

Note that role switching is executed from a freezable workqueue, which guarantees that the pointer is stable during suspend.

Also note that runtime PM has been broken since commit 2664deb09306 ("usb: dwc3: qcom: Honor wakeup enabled/disabled state"), which incidentally also prevents this issue from being triggered.

Fixes a4333c3a6ba9 ("usb: dwc3: Add Qualcomm DWC3 glue driver")

Cc [email protected] # 4.18

Signed-off-by Johan Hovold <[email protected]>

---

Changes in v2
- new patch

 drivers/usb/dwc3/dwc3-qcom.c | 14 +++++++++++++-
 drivers/usb/dwc3/host.c      |  1 +
 2 files changed, 14 insertions(+), 1 deletion(-)

drivers/usb/dwc3/dwc3-qcom.c +13 -1

index e9364141661b..6884026b9fad 100644
@@ -298,6 +298,14 @@ static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom)
 	icc_put(qcom->icc_path_apps);
 }
 
+/* Only usable in contexts where the role can not change. */
+static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom)
+{
+	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
+
+	return dwc->xhci;
+}
+
 static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
 {
 	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);

@@ -460,7 +468,11 @@ static irqreturn_t qcom_dwc3_resume_irq(int irq, void *data)
 	if (qcom->pm_suspended)
 		return IRQ_HANDLED;
 
-	if (dwc->xhci)
+	/*
+	 * This is safe as role switching is done from a freezable workqueue
+	 * and the wakeup interrupts are disabled as part of resume.
+	 */
+	if (dwc3_qcom_is_host(qcom))
 		pm_runtime_resume(&dwc->xhci->dev);
 
 	return IRQ_HANDLED;

drivers/usb/dwc3/host.c +1 -0

index f56c30cf151e..f6f13e7f1ba1 100644
@@ -135,4 +135,5 @@ int dwc3_host_init(struct dwc3 *dwc)
 void dwc3_host_exit(struct dwc3 *dwc)
 {
 	platform_device_unregister(dwc->xhci);
+	dwc->xhci = NULL;
 }

Signature · 1 line

2.35.1

Quoted 15 lines · Johan Hovold

The Qualcomm dwc3 runtime-PM implementation checks the xhci platform-device pointer in the wakeup-interrupt handler to determine whether the controller is in host mode and if so triggers a resume.

After a role switch in OTG mode the xhci platform-device would have been freed and the next wakeup from runtime suspend would access the freed memory.

Note that role switching is executed from a freezable workqueue, which guarantees that the pointer is stable during suspend.

Also note that runtime PM has been broken since commit 2664deb09306 ("usb: dwc3: qcom: Honor wakeup enabled/disabled state"), which incidentally also prevents this issue from being triggered.

Quoted 3 lines

Fixes a4333c3a6ba9 ("usb: dwc3: Add Qualcomm DWC3 glue driver")

Cc [email protected] # 4.18

Signed-off-by Johan Hovold <[email protected]>

It'd be good to mention the introduction of dwc3_qcom_is_host() function. Initially I thought it is used in a single place, but going through the rest of the patches reveals that it is used later on.

Quoted 23 lines

drivers/usb/dwc3/dwc3-qcom.c +13 -1

+/* Only usable in contexts where the role can not change. */
+static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom)
+{
+	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
+
+	return dwc->xhci;
+}
+
 static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
 {
 	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);

@@ -460,7 +468,11 @@ static irqreturn_t qcom_dwc3_resume_irq(int irq, void *data)
 	if (qcom->pm_suspended)
 		return IRQ_HANDLED;
 
-	if (dwc->xhci)
+	/*
+	 * This is safe as role switching is done from a freezable workqueue
+	 * and the wakeup interrupts are disabled as part of resume.
+	 */
+	if (dwc3_qcom_is_host(qcom))
 		pm_runtime_resume(&dwc->xhci->dev);
 

Quoted 13 lines

(quoted diff) +0 -0

 	return IRQ_HANDLED;

drivers/usb/dwc3/host.c +1 -0

index f56c30cf151e..f6f13e7f1ba1 100644
@@ -135,4 +135,5 @@ int dwc3_host_init(struct dwc3 *dwc)
 void dwc3_host_exit(struct dwc3 *dwc)
 {
 	platform_device_unregister(dwc->xhci);
+	dwc->xhci = NULL;
 }

Signature · 1 line

2.35.1

Quoted 20 lines · Manivannan Sadhasivam

Quoted 18 lines · Johan Hovold

The Qualcomm dwc3 runtime-PM implementation checks the xhci platform-device pointer in the wakeup-interrupt handler to determine whether the controller is in host mode and if so triggers a resume.

After a role switch in OTG mode the xhci platform-device would have been freed and the next wakeup from runtime suspend would access the freed memory.

Note that role switching is executed from a freezable workqueue, which guarantees that the pointer is stable during suspend.

Also note that runtime PM has been broken since commit 2664deb09306 ("usb: dwc3: qcom: Honor wakeup enabled/disabled state"), which incidentally also prevents this issue from being triggered.

Fixes a4333c3a6ba9 ("usb: dwc3: Add Qualcomm DWC3 glue driver")

Cc [email protected] # 4.18

Signed-off-by Johan Hovold <[email protected]>

Quoted 3 lines

It'd be good to mention the introduction of dwc3_qcom_is_host() function. Initially I thought it is used in a single place, but going through the rest of the patches reveals that it is used later on.

I think the helper is warranted on its own as it serves as documentation of the underlying assumptions that this code relies on.

Quoted 23 lines

Quoted 23 lines

drivers/usb/dwc3/dwc3-qcom.c +13 -1

+/* Only usable in contexts where the role can not change. */
+static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom)
+{
+	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
+
+	return dwc->xhci;
+}
+
 static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
 {
 	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);

@@ -460,7 +468,11 @@ static irqreturn_t qcom_dwc3_resume_irq(int irq, void *data)
 	if (qcom->pm_suspended)
 		return IRQ_HANDLED;
 
-	if (dwc->xhci)
+	/*
+	 * This is safe as role switching is done from a freezable workqueue
+	 * and the wakeup interrupts are disabled as part of resume.
+	 */
+	if (dwc3_qcom_is_host(qcom))
 		pm_runtime_resume(&dwc->xhci->dev);
 

Quoted 13 lines

Quoted 13 lines

(quoted diff) +0 -0

 	return IRQ_HANDLED;

drivers/usb/dwc3/host.c +1 -0

index f56c30cf151e..f6f13e7f1ba1 100644
@@ -135,4 +135,5 @@ int dwc3_host_init(struct dwc3 *dwc)
 void dwc3_host_exit(struct dwc3 *dwc)
 {
 	platform_device_unregister(dwc->xhci);
+	dwc->xhci = NULL;
 }

Signature · 1 line

2.35.1

Johan

A device must enable wakeups during runtime suspend regardless of whether it is capable and allowed to wake the system up from system suspend.

Fixes 2664deb09306 ("usb: dwc3: qcom: Honor wakeup enabled/disabled state")

Signed-off-by Johan Hovold <[email protected]>

A device must enable wakeups during runtime suspend regardless of whether it is capable and allowed to wake the system up from system suspend.

Fixes 2664deb09306 ("usb: dwc3: qcom: Honor wakeup enabled/disabled state")

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/dwc3/dwc3-qcom.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

drivers/usb/dwc3/dwc3-qcom.c +11 -8

index 6884026b9fad..05b4666fde14 100644
@@ -397,7 +397,7 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
 	dwc3_qcom_enable_wakeup_irq(qcom->ss_phy_irq, 0);
 }
 
-static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
+static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
 {
 	u32 val;
 	int i, ret;

@@ -416,7 +416,7 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
 	if (ret)
 		dev_warn(qcom->dev, "failed to disable interconnect: %d\n", ret);
 
-	if (device_may_wakeup(qcom->dev)) {
+	if (wakeup) {
 		qcom->usb2_speed = dwc3_qcom_read_usb2_speed(qcom);
 		dwc3_qcom_enable_interrupts(qcom);
 	}

@@ -426,7 +426,7 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
 	return 0;
 }
 
-static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
+static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
 {
 	int ret;
 	int i;

@@ -434,7 +434,7 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
 	if (!qcom->is_suspended)
 		return 0;
 
-	if (device_may_wakeup(qcom->dev))
+	if (wakeup)
 		dwc3_qcom_disable_interrupts(qcom);
 
 	for (i = 0; i < qcom->num_clocks; i++) {

@@ -945,9 +945,11 @@ static int dwc3_qcom_remove(struct platform_device *pdev)
 static int __maybe_unused dwc3_qcom_pm_suspend(struct device *dev)
 {
 	struct dwc3_qcom *qcom = dev_get_drvdata(dev);
+	bool wakeup = device_may_wakeup(dev);
 	int ret = 0;
 
-	ret = dwc3_qcom_suspend(qcom);
+
+	ret = dwc3_qcom_suspend(qcom, wakeup);
 	if (!ret)
 		qcom->pm_suspended = true;
 

@@ -957,9 +959,10 @@ static int __maybe_unused dwc3_qcom_pm_suspend(struct device *dev)
 static int __maybe_unused dwc3_qcom_pm_resume(struct device *dev)
 {
 	struct dwc3_qcom *qcom = dev_get_drvdata(dev);
+	bool wakeup = device_may_wakeup(dev);
 	int ret;
 
-	ret = dwc3_qcom_resume(qcom);
+	ret = dwc3_qcom_resume(qcom, wakeup);
 	if (!ret)
 		qcom->pm_suspended = false;
 

@@ -970,14 +973,14 @@ static int __maybe_unused dwc3_qcom_runtime_suspend(struct device *dev)
 {
 	struct dwc3_qcom *qcom = dev_get_drvdata(dev);
 
-	return dwc3_qcom_suspend(qcom);
+	return dwc3_qcom_suspend(qcom, true);
 }
 
 static int __maybe_unused dwc3_qcom_runtime_resume(struct device *dev)
 {
 	struct dwc3_qcom *qcom = dev_get_drvdata(dev);
 
-	return dwc3_qcom_resume(qcom);
+	return dwc3_qcom_resume(qcom, true);
 }
 
 static const struct dev_pm_ops dwc3_qcom_dev_pm_ops = {

Signature · 1 line

2.35.1

Hi Johan,

I love your patch! Perhaps something to improve:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on linus/master next-20220804] [cannot apply to robh/for-next v5.19]
[If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Johan-Hovold/usb-dwc3-qcom-fix-wakeup-implementation/20220804-231122
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: arc-randconfig-r002-20220804 (https://download.01.org/0day-ci/archive/20220805/[email protected]/config)
compiler: arc-elf-gcc (GCC) 12.1.0

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/f3778ca026b16474e49c5e0188a0eb91d73eef2f
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Johan-Hovold/usb-dwc3-qcom-fix-wakeup-implementation/20220804-231122 git checkout f3778ca026b16474e49c5e0188a0eb91d73eef2f
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash drivers/usb/dwc3/

If you fix the issue, kindly add following tag where applicable

Reported-by kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

drivers/usb/dwc3/dwc3-qcom.c: In function 'dwc3_qcom_read_usb2_speed':

Quoted 1 line

Quoted 1 line

drivers/usb/dwc3/dwc3-qcom.c:313:25: warning: variable 'hcd' set but not used [-Wunused-but-set-variable]

     313 |         struct usb_hcd *hcd;
         |                         ^~~> 

Quoted 31 lines · kernel test robot

Hi Johan,

I love your patch! Perhaps something to improve:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on linus/master next-20220804] [cannot apply to robh/for-next v5.19]
[If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Johan-Hovold/usb-dwc3-qcom-fix-wakeup-implementation/20220804-231122
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: arc-randconfig-r002-20220804 (https://download.01.org/0day-ci/archive/20220805/202208050544.ijUhoUyB-lkp(a)intel.com/config)
compiler: arc-elf-gcc (GCC) 12.1.0

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/f3778ca026b16474e49c5e0188a0eb91d73eef2f
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Johan-Hovold/usb-dwc3-qcom-fix-wakeup-implementation/20220804-231122 git checkout f3778ca026b16474e49c5e0188a0eb91d73eef2f
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash drivers/usb/dwc3/

If you fix the issue, kindly add following tag where applicable

Reported-by kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

Quoted 4 lines

drivers/usb/dwc3/dwc3-qcom.c: In function 'dwc3_qcom_read_usb2_speed':

Quoted 1 line

Quoted 1 line

drivers/usb/dwc3/dwc3-qcom.c:313:25: warning: variable 'hcd' set but not used [-Wunused-but-set-variable]

     313 |         struct usb_hcd *hcd;
         |                         ^~~> 

I'm not seeing this one with gcc-10.3.0, but I'll slap a __maybe_unused in there to keep your robot's W=1 builds quiet.

Add a wakeup-source property to the binding to describe whether the wakeup interrupts can wake the system from suspend.

Acked-by Rob Herring <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---

Changes in v2
- disallow 'wakeup-source' in child node (Krzysztof)

 Documentation/devicetree/bindings/usb/qcom,dwc3.yaml | 5 +++++
 1 file changed, 5 insertions(+)

Documentation/devicetree/bindings/usb/qcom,dwc3.yaml +5 -0

index fea3e7092ace..d5959bdea63e 100644
@@ -108,12 +108,17 @@ properties:
       HS/FS/LS modes are supported.
     type: boolean
 
+  wakeup-source: true
+
 # Required child node:
 
 patternProperties:
   "^usb@[0-9a-f]+$":
     $ref: snps,dwc3.yaml#
 
+    properties:
+      wakeup-source: false
+
 required:
   - compatible
   - reg

Signature · 1 line

2.35.1

Quoted 5 lines · Johan Hovold

Add a wakeup-source property to the binding to describe whether the wakeup interrupts can wake the system from suspend.

Acked-by Rob Herring <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

So this is based on the fact that Qcom glue wrapper is supplying the wakeup interrupts. But isn't it possible that on other platform, the DWC IP can supply wakeup interrupts?

In the driver, the wakeup-source parsing has been moved to the Qcom glue driver. But this contradicts with the binding.

Quoted 10 lines · Manivannan Sadhasivam

Quoted 5 lines · Johan Hovold

Add a wakeup-source property to the binding to describe whether the wakeup interrupts can wake the system from suspend.

Acked-by Rob Herring <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

So this is based on the fact that Qcom glue wrapper is supplying the wakeup interrupts. But isn't it possible that on other platform, the DWC IP can supply wakeup interrupts?

Yeah, possibly, and that's why Rob suggested keeping the 'wakeup-source' property also in the core node.

Quoted 2 lines

In the driver, the wakeup-source parsing has been moved to the Qcom glue driver. But this contradicts with the binding.

That's irrelevant. The core driver does not implement wakeup support. It was just added as a hack for the Qualcomm driver, and you won't get wakeup-capability for other platforms by just parsing the property in the core driver.

When/if wakeup support for such a platform is added, then the core driver may need to look at the property again.

Johan

Quoted 23 lines · Johan Hovold

Quoted 10 lines · Manivannan Sadhasivam

Quoted 5 lines · Johan Hovold

Add a wakeup-source property to the binding to describe whether the wakeup interrupts can wake the system from suspend.

Acked-by Rob Herring <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

So this is based on the fact that Qcom glue wrapper is supplying the wakeup interrupts. But isn't it possible that on other platform, the DWC IP can supply wakeup interrupts?

Yeah, possibly, and that's why Rob suggested keeping the 'wakeup-source' property also in the core node.

Quoted 2 lines

In the driver, the wakeup-source parsing has been moved to the Qcom glue driver. But this contradicts with the binding.

That's irrelevant. The core driver does not implement wakeup support. It was just added as a hack for the Qualcomm driver, and you won't get wakeup-capability for other platforms by just parsing the property in the core driver.

Quoted 3 lines

When/if wakeup support for such a platform is added, then the core driver may need to look at the property again.

My point is, the platform drivers are free to add "wakeup-source" property in the DWC node. Then in that case, the DWC driver should handle the capability, isn't it?

I know it is broken currently, but moving the wakeup parsing code is not helping either.

And... I'm aware of the fact that the binding should describe the hardware and not the limitation of the driver. So perhaps we should document it in the driver as a TODO or something?

Quoted 24 lines · Manivannan Sadhasivam

Quoted 23 lines · Johan Hovold

Quoted 10 lines · Manivannan Sadhasivam

Quoted 5 lines · Johan Hovold

Add a wakeup-source property to the binding to describe whether the wakeup interrupts can wake the system from suspend.

Acked-by Rob Herring <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

So this is based on the fact that Qcom glue wrapper is supplying the wakeup interrupts. But isn't it possible that on other platform, the DWC IP can supply wakeup interrupts?

Yeah, possibly, and that's why Rob suggested keeping the 'wakeup-source' property also in the core node.

Quoted 2 lines

In the driver, the wakeup-source parsing has been moved to the Qcom glue driver. But this contradicts with the binding.

That's irrelevant. The core driver does not implement wakeup support. It was just added as a hack for the Qualcomm driver, and you won't get wakeup-capability for other platforms by just parsing the property in the core driver.

Quoted 7 lines

Quoted 3 lines

When/if wakeup support for such a platform is added, then the core driver may need to look at the property again.

My point is, the platform drivers are free to add "wakeup-source" property in the DWC node. Then in that case, the DWC driver should handle the capability, isn't it?

No, not really. They wouldn't violate the current binding, but it would arguably still be wrong to do so unless that platform actually supports wakeup without involvement from a glue layer.

Perhaps we should reconsider reverting the binding update adding this property to the core node and only add it selectively for the platforms for which is actually applies (if they even exist).

Quoted 2 lines

I know it is broken currently, but moving the wakeup parsing code is not helping either.

It's not even broken. It has never even been implemented.

Just because someone added a hack that should probably never have been merged in the first place, doesn't mean we should somehow pretend that we support it.

Quoted 3 lines

And... I'm aware of the fact that the binding should describe the hardware and not the limitation of the driver. So perhaps we should document it in the driver as a TODO or something?

I'd rather just revert the binding update to avoid having discussions like this. We don't even know if it's possible to support on any platform yet (and remember that none of this has even been in an rc release yet).

Johan

It is the Qualcomm glue wakeup interrupts that may be able to wake the system from suspend and this can now be described in the devicetree.

Move the wakeup-source property handling over from the core driver and instead propagate the capability setting to the core device during probe.

This is needed as there is currently no way for the core driver to query the wakeup setting of the glue device, but it is the core driver that manages the PHY power state during suspend.

Also don't leave the PHYs enabled when system wakeup has been disabled through sysfs.

Fixes 649f5c842ba3 ("usb: dwc3: core: Host wake up support from system suspend")

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/dwc3/core.c      | 5 ++---
 drivers/usb/dwc3/dwc3-qcom.c | 6 +++++-
 2 files changed, 7 insertions(+), 4 deletions(-)

drivers/usb/dwc3/core.c +0 -1

index 16d1f328775f..8c8e32651473 100644
@@ -1822,7 +1822,6 @@ static int dwc3_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, dwc);
 	dwc3_cache_hwparams(dwc);
-	device_init_wakeup(&pdev->dev, of_property_read_bool(dev->of_node, "wakeup-source"));
 
 	spin_lock_init(&dwc->lock);
 	mutex_init(&dwc->mutex);

Quoted 34 lines · Johan Hovold

It is the Qualcomm glue wakeup interrupts that may be able to wake the system from suspend and this can now be described in the devicetree.

Move the wakeup-source property handling over from the core driver and instead propagate the capability setting to the core device during probe.

This is needed as there is currently no way for the core driver to query the wakeup setting of the glue device, but it is the core driver that manages the PHY power state during suspend.

Also don't leave the PHYs enabled when system wakeup has been disabled through sysfs.

Fixes 649f5c842ba3 ("usb: dwc3: core: Host wake up support from system suspend")

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/dwc3/core.c      | 5 ++---
 drivers/usb/dwc3/dwc3-qcom.c | 6 +++++-
 2 files changed, 7 insertions(+), 4 deletions(-)

drivers/usb/dwc3/core.c +0 -1

index 16d1f328775f..8c8e32651473 100644
@@ -1822,7 +1822,6 @@ static int dwc3_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, dwc);
 	dwc3_cache_hwparams(dwc);
-	device_init_wakeup(&pdev->dev, of_property_read_bool(dev->of_node, "wakeup-source"));
 
 	spin_lock_init(&dwc->lock);
 	mutex_init(&dwc->mutex);

drivers/usb/dwc3/core.c static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) · line 1986

Quoted 5 lines

drivers/usb/dwc3/core.c +1 -1

@@ -1984,7 +1983,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
 		dwc3_core_exit(dwc);
 		break;
 	case DWC3_GCTL_PRTCAP_HOST:
-		if (!PMSG_IS_AUTO(msg) && !device_can_wakeup(dwc->dev)) {
+		if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) {

Let me explain the rationale for why device_can_wakeup() was used here:

On QCOM SC7180 based Chromebooks we observe that the onboard USB hub consumes ~80 mW during system suspend when the PHYs are disabled, as opposed to ~17 mW when the PHYs remain enabled. This is a significant delta when the device is on a battery power.

The initial idea was to leave the PHYs always enabled (in a low power mode), but then I dug up commit c4a5153e87fd ("usb: dwc3: core: Power-off core/PHYs on system_suspend in host mode"), which provides a rationale for the PHYs being powered off:

Commit 689bf72c6e0d ("usb: dwc3: Don't reinitialize core during host bus-suspend/resume") updated suspend/resume routines to not power_off and reinit PHYs/core for host mode.
It broke platforms that rely on DWC3 core to power_off PHYs to enter low power state on system suspend.

Unfortunately we don't know which platforms are impacted by this. The idea behind using device_can_wakeup() was to use it as a proxy for platforms that are *not* impacted. If a platform supports USB wakeup supposedly the SoC can enter its low power mode during system suspend with the PHYs enabled.

By now I'm not 100% sure if the above assumption is correct. I recently saw allegations that the power consumption of a given QC SoC with USB wakeup support drops significantly when wakeup is disabled (i.e. when the PHYs are off), but haven't confirmed this yet.

Quoted 65 lines · Matthias Kaehlcke

Quoted 34 lines · Johan Hovold

It is the Qualcomm glue wakeup interrupts that may be able to wake the system from suspend and this can now be described in the devicetree.

Move the wakeup-source property handling over from the core driver and instead propagate the capability setting to the core device during probe.

This is needed as there is currently no way for the core driver to query the wakeup setting of the glue device, but it is the core driver that manages the PHY power state during suspend.

Also don't leave the PHYs enabled when system wakeup has been disabled through sysfs.

Fixes 649f5c842ba3 ("usb: dwc3: core: Host wake up support from system suspend")

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/dwc3/core.c      | 5 ++---
 drivers/usb/dwc3/dwc3-qcom.c | 6 +++++-
 2 files changed, 7 insertions(+), 4 deletions(-)

drivers/usb/dwc3/core.c +0 -1

index 16d1f328775f..8c8e32651473 100644
@@ -1822,7 +1822,6 @@ static int dwc3_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, dwc);
 	dwc3_cache_hwparams(dwc);
-	device_init_wakeup(&pdev->dev, of_property_read_bool(dev->of_node, "wakeup-source"));
 
 	spin_lock_init(&dwc->lock);
 	mutex_init(&dwc->mutex);

drivers/usb/dwc3/core.c static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) · line 1986

Quoted 5 lines

drivers/usb/dwc3/core.c +1 -1

@@ -1984,7 +1983,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
 		dwc3_core_exit(dwc);
 		break;
 	case DWC3_GCTL_PRTCAP_HOST:
-		if (!PMSG_IS_AUTO(msg) && !device_can_wakeup(dwc->dev)) {
+		if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) {

Let me explain the rationale for why device_can_wakeup() was used here:

On QCOM SC7180 based Chromebooks we observe that the onboard USB hub consumes ~80 mW during system suspend when the PHYs are disabled, as opposed to ~17 mW when the PHYs remain enabled. This is a significant delta when the device is on a battery power.

The initial idea was to leave the PHYs always enabled (in a low power mode), but then I dug up commit c4a5153e87fd ("usb: dwc3: core: Power-off core/PHYs on system_suspend in host mode"), which provides a rationale for the PHYs being powered off:

Commit 689bf72c6e0d ("usb: dwc3: Don't reinitialize core during host bus-suspend/resume") updated suspend/resume routines to not power_off and reinit PHYs/core for host mode.
It broke platforms that rely on DWC3 core to power_off PHYs to enter low power state on system suspend.

Unfortunately we don't know which platforms are impacted by this. The idea behind using device_can_wakeup() was to use it as a proxy for platforms that are *not* impacted. If a platform supports USB wakeup supposedly the SoC can enter its low power mode during system suspend with the PHYs enabled.

Quoted 4 lines

By now I'm not 100% sure if the above assumption is correct. I recently saw allegations that the power consumption of a given QC SoC with USB wakeup support drops significantly when wakeup is disabled (i.e. when the PHYs are off), but haven't confirmed this yet.

So far power measurements don't support the claim that SoC power consumption is substantially lower with USB wakeup disabled/the PHYs off. I asked the person who made that claim to provide more details/data (the discussion is in an internal forum).

Quoted 71 lines · Matthias Kaehlcke

Quoted 69 lines · Matthias Kaehlcke

Quoted 34 lines · Johan Hovold

It is the Qualcomm glue wakeup interrupts that may be able to wake the system from suspend and this can now be described in the devicetree.

Move the wakeup-source property handling over from the core driver and instead propagate the capability setting to the core device during probe.

This is needed as there is currently no way for the core driver to query the wakeup setting of the glue device, but it is the core driver that manages the PHY power state during suspend.

Also don't leave the PHYs enabled when system wakeup has been disabled through sysfs.

Fixes 649f5c842ba3 ("usb: dwc3: core: Host wake up support from system suspend")

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/dwc3/core.c      | 5 ++---
 drivers/usb/dwc3/dwc3-qcom.c | 6 +++++-
 2 files changed, 7 insertions(+), 4 deletions(-)

drivers/usb/dwc3/core.c +0 -1

index 16d1f328775f..8c8e32651473 100644
@@ -1822,7 +1822,6 @@ static int dwc3_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, dwc);
 	dwc3_cache_hwparams(dwc);
-	device_init_wakeup(&pdev->dev, of_property_read_bool(dev->of_node, "wakeup-source"));
 
 	spin_lock_init(&dwc->lock);
 	mutex_init(&dwc->mutex);

drivers/usb/dwc3/core.c static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) · line 1986

Quoted 5 lines

drivers/usb/dwc3/core.c +1 -1

@@ -1984,7 +1983,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
 		dwc3_core_exit(dwc);
 		break;
 	case DWC3_GCTL_PRTCAP_HOST:
-		if (!PMSG_IS_AUTO(msg) && !device_can_wakeup(dwc->dev)) {
+		if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) {

Let me explain the rationale for why device_can_wakeup() was used here:

On QCOM SC7180 based Chromebooks we observe that the onboard USB hub consumes ~80 mW during system suspend when the PHYs are disabled, as opposed to ~17 mW when the PHYs remain enabled. This is a significant delta when the device is on a battery power.

The initial idea was to leave the PHYs always enabled (in a low power mode), but then I dug up commit c4a5153e87fd ("usb: dwc3: core: Power-off core/PHYs on system_suspend in host mode"), which provides a rationale for the PHYs being powered off:

Commit 689bf72c6e0d ("usb: dwc3: Don't reinitialize core during host bus-suspend/resume") updated suspend/resume routines to not power_off and reinit PHYs/core for host mode.
It broke platforms that rely on DWC3 core to power_off PHYs to enter low power state on system suspend.

Unfortunately we don't know which platforms are impacted by this. The idea behind using device_can_wakeup() was to use it as a proxy for platforms that are *not* impacted. If a platform supports USB wakeup supposedly the SoC can enter its low power mode during system suspend with the PHYs enabled.

By now I'm not 100% sure if the above assumption is correct. I recently saw allegations that the power consumption of a given QC SoC with USB wakeup support drops significantly when wakeup is disabled (i.e. when the PHYs are off), but haven't confirmed this yet.

Quoted 4 lines

So far power measurements don't support the claim that SoC power consumption is substantially lower with USB wakeup disabled/the PHYs off. I asked the person who made that claim to provide more details/data (the discussion is in an internal forum).

Thanks for the background on this. So clearly it has nothing to with supporting wakeup as the commit summary claimed, and this should probably never have been made to depend on wakeup capability either.

I'll revisit this after the merge window, but perhaps we should just rip this out completely and use a more descriptive property to configure the PHY suspend state. But depending on the results from your internal measurements, perhaps not even that is needed.

It is the Qualcomm glue wakeup interrupts that may be able to wake the system from suspend and this can now be described in the devicetree.

Move the wakeup-source property handling over from the core driver and instead propagate the capability setting to the core device during probe.

This is needed as there is currently no way for the core driver to query the wakeup setting of the glue device, but it is the core driver that manages the PHY power state during suspend.

Also don't leave the PHYs enabled when system wakeup has been disabled through sysfs.

Fixes 649f5c842ba3 ("usb: dwc3: core: Host wake up support from system suspend")

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/dwc3/core.c      | 5 ++---
 drivers/usb/dwc3/dwc3-qcom.c | 6 +++++-
 2 files changed, 7 insertions(+), 4 deletions(-)

drivers/usb/dwc3/core.c +2 -3

index 16d1f328775f..8c8e32651473 100644
@@ -1822,7 +1822,6 @@ static int dwc3_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, dwc);
 	dwc3_cache_hwparams(dwc);
-	device_init_wakeup(&pdev->dev, of_property_read_bool(dev->of_node, "wakeup-source"));
 
 	spin_lock_init(&dwc->lock);
 	mutex_init(&dwc->mutex);

@@ -1984,7 +1983,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
 		dwc3_core_exit(dwc);
 		break;
 	case DWC3_GCTL_PRTCAP_HOST:
-		if (!PMSG_IS_AUTO(msg) && !device_can_wakeup(dwc->dev)) {
+		if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) {
 			dwc3_core_exit(dwc);
 			break;
 		}

@@ -2045,7 +2044,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
 		spin_unlock_irqrestore(&dwc->lock, flags);
 		break;
 	case DWC3_GCTL_PRTCAP_HOST:
-		if (!PMSG_IS_AUTO(msg) && !device_can_wakeup(dwc->dev)) {
+		if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) {
 			ret = dwc3_core_init_for_resume(dwc);
 			if (ret)
 				return ret;

drivers/usb/dwc3/dwc3-qcom.c +5 -1

index 6ae0b7fc4e2c..b05f67d206d2 100644
@@ -786,6 +786,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	struct resource		*res, *parent_res = NULL;
 	int			ret, i;
 	bool			ignore_pipe_clk;
+	bool			wakeup_source;
 
 	qcom = devm_kzalloc(&pdev->dev, sizeof(*qcom), GFP_KERNEL);
 	if (!qcom)

@@ -901,7 +902,10 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	if (ret)
 		goto interconnect_exit;
 
-	device_init_wakeup(&pdev->dev, 1);
+	wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source");
+	device_init_wakeup(&pdev->dev, wakeup_source);
+	device_init_wakeup(&qcom->dwc3->dev, wakeup_source);
+
 	qcom->is_suspended = false;
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);

Signature · 1 line

2.35.1

Quoted 6 lines · Johan Hovold

It is the Qualcomm glue wakeup interrupts that may be able to wake the system from suspend and this can now be described in the devicetree.

Move the wakeup-source property handling over from the core driver and instead propagate the capability setting to the core device during probe.

The "wakeup-source" property is still defined in the DWC binding, so other platform glue drivers are free to assume that wakeup capability is handled by the DWC driver.

Quoted 7 lines

This is needed as there is currently no way for the core driver to query the wakeup setting of the glue device, but it is the core driver that manages the PHY power state during suspend.

Also don't leave the PHYs enabled when system wakeup has been disabled through sysfs.

Can you please elaborate on how this is handled in the patch?

Quoted 53 lines

Fixes 649f5c842ba3 ("usb: dwc3: core: Host wake up support from system suspend")

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/dwc3/core.c      | 5 ++---
 drivers/usb/dwc3/dwc3-qcom.c | 6 +++++-
 2 files changed, 7 insertions(+), 4 deletions(-)

drivers/usb/dwc3/core.c +2 -3

index 16d1f328775f..8c8e32651473 100644
@@ -1822,7 +1822,6 @@ static int dwc3_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, dwc);
 	dwc3_cache_hwparams(dwc);
-	device_init_wakeup(&pdev->dev, of_property_read_bool(dev->of_node, "wakeup-source"));
 
 	spin_lock_init(&dwc->lock);
 	mutex_init(&dwc->mutex);

@@ -1984,7 +1983,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
 		dwc3_core_exit(dwc);
 		break;
 	case DWC3_GCTL_PRTCAP_HOST:
-		if (!PMSG_IS_AUTO(msg) && !device_can_wakeup(dwc->dev)) {
+		if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) {
 			dwc3_core_exit(dwc);
 			break;
 		}

@@ -2045,7 +2044,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
 		spin_unlock_irqrestore(&dwc->lock, flags);
 		break;
 	case DWC3_GCTL_PRTCAP_HOST:
-		if (!PMSG_IS_AUTO(msg) && !device_can_wakeup(dwc->dev)) {
+		if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) {
 			ret = dwc3_core_init_for_resume(dwc);
 			if (ret)
 				return ret;

drivers/usb/dwc3/dwc3-qcom.c +1 -0

index 6ae0b7fc4e2c..b05f67d206d2 100644
@@ -786,6 +786,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	struct resource		*res, *parent_res = NULL;
 	int			ret, i;
 	bool			ignore_pipe_clk;
+	bool			wakeup_source;
 
 	qcom = devm_kzalloc(&pdev->dev, sizeof(*qcom), GFP_KERNEL);
 	if (!qcom)

@@ -901,7 +902,10 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	if (ret)
 		goto interconnect_exit;
 

Quoted 10 lines

drivers/usb/dwc3/dwc3-qcom.c +4 -1

-	device_init_wakeup(&pdev->dev, 1);
+	wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source");
+	device_init_wakeup(&pdev->dev, wakeup_source);
+	device_init_wakeup(&qcom->dwc3->dev, wakeup_source);
+
 	qcom->is_suspended = false;
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);

Signature · 1 line

2.35.1

Clean up the suspend callbacks by separating the error and success paths to improve readability.

Also drop a related redundant initialisation.

Signed-off-by Johan Hovold <[email protected]>

Changes in v2
- add review and ack tags
- fix a gadget-only build breakage (new patch)
- fix a use-after-free on wakeup from runtime suspend (new patch)
- disable wakeup completely instead of falling back to the "disconnected" host configuration when not acting as host
- disallow 'wakeup-source' in child node in the binding