2022-08-04 15:12:18

by Johan Hovold

[permalink] [raw]
Subject: [PATCH v2 0/9] usb: dwc3: qcom: fix wakeup implementation

This series fixes some of the fallout after the recently merged series
that added wakeup support to the Qualcomm dwc3 driver:

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

The first patch fixes a long standing PHY power sequencing issue in dwc3
core.

The second patch reverts a power-domain hack that was added by the above
series. There are other ways to implement this which doesn't violate the
genpd interface, and if for some reason those are not sufficient, the
genpd implementation needs to be extended, not hacked around.

The third patch fixes a build breakage caused by the same series. (new
in v2)

The fourth patch fixes another long-standing bug which could lead to a
use-after-free when using runtime PM. (new in v2)

The next patch fixes another issue in the Qualcomm dwc3 implementation
that has been added a while back and which breaks runtime PM.

The sixt patch fixes a NULL-pointer dereference or use-after-free when
suspending controllers in peripheral or OTG mode due to a hack that was
added to suspend path. Unfortunately, it seems the hack needs to stay
for now if we want functioning suspend on some Qualcomm platforms.

The remaining patches moves the wakeup-source property over from the
core node to the glue node in the binding and instead propagates the
wakeup capability to the former during probe.

Note that this incidentally also avoids adding probe-deferral hacks to
the driver as was recently proposed to deal with another problem with
the current implementation:

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

With this series I have functioning USB system suspend and wakeup as
well as somewhat functioning runtime PM in host mode on sc8280xp. Note
that the PHYs apparently do not not to be left enabled for wakeup on
this platform.

Some issues remain such as that root-hub connect/disconnect events
cannot selectively be disabled.

And of course, the suspend speed hack needs to be replaced at some
point but that likely requires some more heavy lifting in the dwc3
implementation.

Johan

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


Johan Hovold (9):
usb: dwc3: fix PHY disable sequence
Revert "usb: dwc3: qcom: Keep power domain on to retain controller
status"
usb: dwc3: qcom: fix gadget-only builds
usb: dwc3: qcom: fix use-after-free on runtime-PM wakeup
usb: dwc3: qcom: fix runtime PM wakeup
usb: dwc3: qcom: fix peripheral and OTG suspend
dt-bindings: usb: qcom,dwc3: add wakeup-source property
usb: dwc3: qcom: fix wakeup implementation
usb: dwc3: qcom: clean up suspend callbacks

.../devicetree/bindings/usb/qcom,dwc3.yaml | 5 +
drivers/usb/dwc3/core.c | 24 ++---
drivers/usb/dwc3/dwc3-qcom.c | 96 +++++++++++--------
drivers/usb/dwc3/host.c | 1 +
4 files changed, 76 insertions(+), 50 deletions(-)

--
2.35.1



2022-08-04 15:12:38

by Johan Hovold

[permalink] [raw]
Subject: [PATCH v2 2/9] Revert "usb: dwc3: qcom: Keep power domain on to retain controller status"

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(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index c5e482f53e9d..be2e3dd36440 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -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);
--
2.35.1


2022-08-04 15:13:32

by Johan Hovold

[permalink] [raw]
Subject: [PATCH v2 4/9] usb: dwc3: qcom: fix use-after-free on runtime-PM wakeup

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(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index e9364141661b..6884026b9fad 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -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;
diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index f56c30cf151e..f6f13e7f1ba1 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -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;
}
--
2.35.1


2022-08-04 15:16:19

by Johan Hovold

[permalink] [raw]
Subject: [PATCH v2 8/9] usb: dwc3: qcom: fix wakeup implementation

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(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 16d1f328775f..8c8e32651473 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -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;
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 6ae0b7fc4e2c..b05f67d206d2 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -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);
--
2.35.1


2022-08-04 15:29:12

by Johan Hovold

[permalink] [raw]
Subject: [PATCH v2 1/9] usb: dwc3: fix PHY disable sequence

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(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index c5c238ab3083..16d1f328775f 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -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:
--
2.35.1


2022-08-04 15:36:33

by Johan Hovold

[permalink] [raw]
Subject: [PATCH v2 7/9] dt-bindings: usb: qcom,dwc3: add wakeup-source property

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(+)

diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
index fea3e7092ace..d5959bdea63e 100644
--- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
@@ -108,12 +108,17 @@ properties:
HS/FS/LS modes are supported.
type: boolean

+ wakeup-source: true
+
# Required child node:

patternProperties:
"^[email protected][0-9a-f]+$":
$ref: snps,dwc3.yaml#

+ properties:
+ wakeup-source: false
+
required:
- compatible
- reg
--
2.35.1


2022-08-04 15:38:21

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v2 0/9] usb: dwc3: qcom: fix wakeup implementation

On Thu, Aug 04, 2022 at 05:09:52PM +0200, Johan Hovold wrote:

> 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

Forgot to mention that I also dropped the revert of the dt-binding
commit adding 'wakeup-source' to the core node as Rob suggested.

Johan

2022-08-04 15:54:12

by Johan Hovold

[permalink] [raw]
Subject: [PATCH v2 9/9] usb: dwc3: qcom: clean up suspend callbacks

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]>
---
drivers/usb/dwc3/dwc3-qcom.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index b05f67d206d2..197583ff3f3d 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -959,14 +959,15 @@ 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;
-
+ int ret;

ret = dwc3_qcom_suspend(qcom, wakeup);
- if (!ret)
- qcom->pm_suspended = true;
+ if (ret)
+ return ret;

- return ret;
+ qcom->pm_suspended = true;
+
+ return 0;
}

static int __maybe_unused dwc3_qcom_pm_resume(struct device *dev)
@@ -976,10 +977,12 @@ static int __maybe_unused dwc3_qcom_pm_resume(struct device *dev)
int ret;

ret = dwc3_qcom_resume(qcom, wakeup);
- if (!ret)
- qcom->pm_suspended = false;
+ if (ret)
+ return ret;

- return ret;
+ qcom->pm_suspended = false;
+
+ return 0;
}

static int __maybe_unused dwc3_qcom_runtime_suspend(struct device *dev)
--
2.35.1


2022-08-04 15:57:41

by Johan Hovold

[permalink] [raw]
Subject: [PATCH v2 6/9] usb: dwc3: qcom: fix peripheral and OTG suspend

A recent commit implementing wakeup support in host mode instead broke
suspend for peripheral and OTG mode.

The hack that was added in the suspend path to determine the speed of
any device connected to the USB2 bus not only accesses internal driver
data for a child device, but also dereferences a NULL pointer or
accesses freed data when the controller is not acting as host.

There's no quick fix to the layering violation, but since reverting
would leave us with broken suspend in host mode with wakeup triggering
immediately, let's keep the hack for now.

Fix the immediate issues by only checking the host bus speed and
enabling wakeup interrupts when acting as host.

Fixes: 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend")
Signed-off-by: Johan Hovold <[email protected]>
---

Changes in v2
- disable wakeup completely instead of falling back to the
"disconnected" host configuration when not acting as host

drivers/usb/dwc3/dwc3-qcom.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 05b4666fde14..6ae0b7fc4e2c 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -309,8 +309,13 @@ static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom)
static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
{
struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
- struct usb_hcd *hcd = platform_get_drvdata(dwc->xhci);
struct usb_device *udev;
+ struct usb_hcd *hcd;
+
+ /*
+ * FIXME: Fix this layering violation.
+ */
+ hcd = platform_get_drvdata(dwc->xhci);

/*
* It is possible to query the speed of all children of
@@ -416,7 +421,11 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
if (ret)
dev_warn(qcom->dev, "failed to disable interconnect: %d\n", ret);

- if (wakeup) {
+ /*
+ * The role is stable during suspend as role switching is done from a
+ * freezable workqueue.
+ */
+ if (dwc3_qcom_is_host(qcom) && wakeup) {
qcom->usb2_speed = dwc3_qcom_read_usb2_speed(qcom);
dwc3_qcom_enable_interrupts(qcom);
}
@@ -434,7 +443,7 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
if (!qcom->is_suspended)
return 0;

- if (wakeup)
+ if (dwc3_qcom_is_host(qcom) && wakeup)
dwc3_qcom_disable_interrupts(qcom);

for (i = 0; i < qcom->num_clocks; i++) {
--
2.35.1


2022-08-04 15:58:13

by Johan Hovold

[permalink] [raw]
Subject: [PATCH v2 3/9] usb: dwc3: qcom: fix gadget-only builds

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(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index be2e3dd36440..e9364141661b 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -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;

--
2.35.1


2022-08-04 15:58:17

by Johan Hovold

[permalink] [raw]
Subject: [PATCH v2 5/9] usb: dwc3: qcom: fix runtime PM wakeup

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(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 6884026b9fad..05b4666fde14 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -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 = {
--
2.35.1


2022-08-04 15:59:29

by Matthias Kaehlcke

[permalink] [raw]
Subject: Re: [PATCH v2 4/9] usb: dwc3: qcom: fix use-after-free on runtime-PM wakeup

On Thu, Aug 04, 2022 at 05:09:56PM +0200, Johan Hovold wrote:
> 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]>

Reviewed-by: Matthias Kaehlcke <[email protected]>

2022-08-04 17:11:59

by Matthias Kaehlcke

[permalink] [raw]
Subject: Re: [PATCH v2 9/9] usb: dwc3: qcom: clean up suspend callbacks

On Thu, Aug 04, 2022 at 05:10:01PM +0200, Johan Hovold wrote:
> 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]>

Reviewed-by: Matthias Kaehlcke <[email protected]>

2022-08-04 17:17:41

by Matthias Kaehlcke

[permalink] [raw]
Subject: Re: [PATCH v2 8/9] usb: dwc3: qcom: fix wakeup implementation

On Thu, Aug 04, 2022 at 05:10:00PM +0200, Johan Hovold wrote:
> 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(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 16d1f328775f..8c8e32651473 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -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)) {

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.

2022-08-04 19:03:06

by Randy Dunlap

[permalink] [raw]
Subject: Re: [PATCH v2 3/9] usb: dwc3: qcom: fix gadget-only builds

Hi John,

On 8/4/22 08:09, Johan Hovold wrote:
> 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]>

LGTM. Thanks.

Reviewed-by: Randy Dunlap <[email protected]>

> ---
>
> Changes in v2
> - new patch
>
> drivers/usb/dwc3/dwc3-qcom.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index be2e3dd36440..e9364141661b 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -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;
>

--
~Randy

2022-08-04 20:38:41

by Matthias Kaehlcke

[permalink] [raw]
Subject: Re: [PATCH v2 5/9] usb: dwc3: qcom: fix runtime PM wakeup

On Thu, Aug 04, 2022 at 05:09:57PM +0200, Johan Hovold wrote:
> 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]>

Reviewed-by: Matthias Kaehlcke <[email protected]>
Tested-by: Matthias Kaehlcke <[email protected]>

2022-08-04 22:06:51

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH v2 6/9] usb: dwc3: qcom: fix peripheral and OTG suspend

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':
>> drivers/usb/dwc3/dwc3-qcom.c:313:25: warning: variable 'hcd' set but not used [-Wunused-but-set-variable]
313 | struct usb_hcd *hcd;
| ^~~


vim +/hcd +313 drivers/usb/dwc3/dwc3-qcom.c

308
309 static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
310 {
311 struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
312 struct usb_device *udev;
> 313 struct usb_hcd *hcd;
314
315 /*
316 * FIXME: Fix this layering violation.
317 */
318 hcd = platform_get_drvdata(dwc->xhci);
319
320 /*
321 * It is possible to query the speed of all children of
322 * USB2.0 root hub via usb_hub_for_each_child(). DWC3 code
323 * currently supports only 1 port per controller. So
324 * this is sufficient.
325 */
326 #ifdef CONFIG_USB
327 udev = usb_hub_find_child(hcd->self.root_hub, 1);
328 #else
329 udev = NULL;
330 #endif
331 if (!udev)
332 return USB_SPEED_UNKNOWN;
333
334 return udev->speed;
335 }
336

--
0-DAY CI Kernel Test Service
https://01.org/lkp

2022-08-05 07:07:44

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v2 6/9] usb: dwc3: qcom: fix peripheral and OTG suspend

On Fri, Aug 05, 2022 at 05:38:30AM +0800, kernel test robot wrote:
> 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':
> >> 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.

Johan

2022-08-05 07:19:44

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v2 6/9] usb: dwc3: qcom: fix peripheral and OTG suspend

On Fri, Aug 05, 2022 at 08:58:00AM +0200, Johan Hovold wrote:
> On Fri, Aug 05, 2022 at 05:38:30AM +0800, kernel test robot wrote:
> > 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':
> > >> 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.

Correction: of course I'm seeing it in the affected build configuration...

Johan

2022-08-05 17:11:17

by Matthias Kaehlcke

[permalink] [raw]
Subject: Re: [PATCH v2 8/9] usb: dwc3: qcom: fix wakeup implementation

On Thu, Aug 04, 2022 at 09:59:44AM -0700, Matthias Kaehlcke wrote:
> On Thu, Aug 04, 2022 at 05:10:00PM +0200, Johan Hovold wrote:
> > 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(-)
> >
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 16d1f328775f..8c8e32651473 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -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)) {
>
> 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.

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).

2022-08-06 13:59:31

by Manivannan Sadhasivam

[permalink] [raw]
Subject: Re: [PATCH v2 1/9] usb: dwc3: fix PHY disable sequence

On Thu, Aug 04, 2022 at 05:09:53PM +0200, Johan Hovold wrote:
> 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]>

Reviewed-by: Manivannan Sadhasivam <[email protected]>

Thanks,
Mani

> ---
> drivers/usb/dwc3/core.c | 19 ++++++++++---------
> 1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index c5c238ab3083..16d1f328775f 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -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:
> --
> 2.35.1
>

--
மணிவண்ணன் சதாசிவம்

2022-08-06 14:05:01

by Manivannan Sadhasivam

[permalink] [raw]
Subject: Re: [PATCH v2 2/9] Revert "usb: dwc3: qcom: Keep power domain on to retain controller status"

On Thu, Aug 04, 2022 at 05:09:54PM +0200, Johan Hovold wrote:
> 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]>

Reviewed-by: Manivannan Sadhasivam <[email protected]>

Thanks,
Mani

> ---
> drivers/usb/dwc3/dwc3-qcom.c | 28 +++++++---------------------
> 1 file changed, 7 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index c5e482f53e9d..be2e3dd36440 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -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);
> --
> 2.35.1
>

--
மணிவண்ணன் சதாசிவம்

2022-08-06 14:35:30

by Manivannan Sadhasivam

[permalink] [raw]
Subject: Re: [PATCH v2 3/9] usb: dwc3: qcom: fix gadget-only builds

On Thu, Aug 04, 2022 at 05:09:55PM +0200, Johan Hovold wrote:
> 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(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index be2e3dd36440..e9364141661b 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -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

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.

Thanks,
Mani

> if (!udev)
> return USB_SPEED_UNKNOWN;
>
> --
> 2.35.1
>

--
மணிவண்ணன் சதாசிவம்

2022-08-06 14:38:19

by Manivannan Sadhasivam

[permalink] [raw]
Subject: Re: [PATCH v2 5/9] usb: dwc3: qcom: fix runtime PM wakeup

On Thu, Aug 04, 2022 at 05:09:57PM +0200, Johan Hovold wrote:
> 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]>

Reviewed-by: Manivannan Sadhasivam <[email protected]>

Thanks,
Mani

> ---
> drivers/usb/dwc3/dwc3-qcom.c | 19 +++++++++++--------
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 6884026b9fad..05b4666fde14 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -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 = {
> --
> 2.35.1
>

--
மணிவண்ணன் சதாசிவம்

2022-08-06 14:40:36

by Manivannan Sadhasivam

[permalink] [raw]
Subject: Re: [PATCH v2 4/9] usb: dwc3: qcom: fix use-after-free on runtime-PM wakeup

On Thu, Aug 04, 2022 at 05:09:56PM +0200, Johan Hovold wrote:
> 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]>

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.

Reviewed-by: Manivannan Sadhasivam <[email protected]>

Thanks,
Mani

> ---
>
> 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(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index e9364141661b..6884026b9fad 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -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;
> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
> index f56c30cf151e..f6f13e7f1ba1 100644
> --- a/drivers/usb/dwc3/host.c
> +++ b/drivers/usb/dwc3/host.c
> @@ -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;
> }
> --
> 2.35.1
>

--
மணிவண்ணன் சதாசிவம்

2022-08-06 14:59:43

by Manivannan Sadhasivam

[permalink] [raw]
Subject: Re: [PATCH v2 8/9] usb: dwc3: qcom: fix wakeup implementation

On Thu, Aug 04, 2022 at 05:10:00PM +0200, Johan Hovold wrote:
> 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.

> 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?

Thanks,
Mani

> 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(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 16d1f328775f..8c8e32651473 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -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;
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 6ae0b7fc4e2c..b05f67d206d2 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -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);
> --
> 2.35.1
>

--
மணிவண்ணன் சதாசிவம்

2022-08-06 15:13:51

by Manivannan Sadhasivam

[permalink] [raw]
Subject: Re: [PATCH v2 7/9] dt-bindings: usb: qcom,dwc3: add wakeup-source property

On Thu, Aug 04, 2022 at 05:09:59PM +0200, Johan Hovold wrote:
> 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.

Thanks,
Mani

> ---
>
> Changes in v2
> - disallow 'wakeup-source' in child node (Krzysztof)
>
> Documentation/devicetree/bindings/usb/qcom,dwc3.yaml | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> index fea3e7092ace..d5959bdea63e 100644
> --- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> @@ -108,12 +108,17 @@ properties:
> HS/FS/LS modes are supported.
> type: boolean
>
> + wakeup-source: true
> +
> # Required child node:
>
> patternProperties:
> "^[email protected][0-9a-f]+$":
> $ref: snps,dwc3.yaml#
>
> + properties:
> + wakeup-source: false
> +
> required:
> - compatible
> - reg
> --
> 2.35.1
>

--
மணிவண்ணன் சதாசிவம்

2022-08-06 16:26:52

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v2 8/9] usb: dwc3: qcom: fix wakeup implementation

On Fri, Aug 05, 2022 at 09:58:35AM -0700, Matthias Kaehlcke wrote:
> On Thu, Aug 04, 2022 at 09:59:44AM -0700, Matthias Kaehlcke wrote:
> > On Thu, Aug 04, 2022 at 05:10:00PM +0200, Johan Hovold wrote:
> > > 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(-)
> > >
> > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > index 16d1f328775f..8c8e32651473 100644
> > > --- a/drivers/usb/dwc3/core.c
> > > +++ b/drivers/usb/dwc3/core.c
> > > @@ -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)) {
> >
> > 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.
>
> 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.

Johan

2022-08-06 16:35:16

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v2 8/9] usb: dwc3: qcom: fix wakeup implementation

On Sat, Aug 06, 2022 at 08:27:19PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Aug 04, 2022 at 05:10:00PM +0200, Johan Hovold wrote:
> > 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.

No, just because the binding says that the hardware supports something
doesn't mean it's implemented. And in this case it isn't.

There's no core support for wakeup and this is just used to determine
the PHY power state during suspend (see my reply to Matthias).

But this is also why I initially suggested reverting the binding change
until some platform actually turns out to need it.

> > 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?

Generally device_can_wakeup() should not be used to make policy
decisions (e.g. whether to power of the PHYs) as this should be
configurable through sysfs which is honoured by device_may_wakeup().

But I'll revisit this in a couple of weeks. We should probably just
revert the patch that made the PHY power state depend on
device_can_wakeup() as it apparently isn't needed for wakeup at all.

> > 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(-)
> >
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 16d1f328775f..8c8e32651473 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -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;
> > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > index 6ae0b7fc4e2c..b05f67d206d2 100644
> > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > @@ -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);
> > --
> > 2.35.1

Johan

2022-08-06 16:35:43

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v2 3/9] usb: dwc3: qcom: fix gadget-only builds

On Sat, Aug 06, 2022 at 07:45:36PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Aug 04, 2022 at 05:09:55PM +0200, Johan Hovold wrote:
> > 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(-)
> >
> > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > index be2e3dd36440..e9364141661b 100644
> > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > @@ -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
>
> 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.

> > if (!udev)
> > return USB_SPEED_UNKNOWN;
> >
> > --
> > 2.35.1

Johan

2022-08-06 16:53:40

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v2 4/9] usb: dwc3: qcom: fix use-after-free on runtime-PM wakeup

On Sat, Aug 06, 2022 at 08:03:11PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Aug 04, 2022 at 05:09:56PM +0200, Johan Hovold wrote:
> > 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]>
>
> 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.

> > +/* 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;
> > diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
> > index f56c30cf151e..f6f13e7f1ba1 100644
> > --- a/drivers/usb/dwc3/host.c
> > +++ b/drivers/usb/dwc3/host.c
> > @@ -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;
> > }
> > --
> > 2.35.1

Johan

2022-08-06 16:53:49

by Manivannan Sadhasivam

[permalink] [raw]
Subject: Re: [PATCH v2 3/9] usb: dwc3: qcom: fix gadget-only builds

On Sat, Aug 06, 2022 at 06:04:21PM +0200, Johan Hovold wrote:
> On Sat, Aug 06, 2022 at 07:45:36PM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Aug 04, 2022 at 05:09:55PM +0200, Johan Hovold wrote:
> > > 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(-)
> > >
> > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > > index be2e3dd36440..e9364141661b 100644
> > > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > > @@ -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
> >
> > 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.
>

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

Thanks,
Mani

> > > if (!udev)
> > > return USB_SPEED_UNKNOWN;
> > >
> > > --
> > > 2.35.1
>
> Johan

--
மணிவண்ணன் சதாசிவம்

2022-08-06 16:56:46

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v2 7/9] dt-bindings: usb: qcom,dwc3: add wakeup-source property

On Sat, Aug 06, 2022 at 08:38:48PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Aug 04, 2022 at 05:09:59PM +0200, Johan Hovold wrote:
> > 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.

> 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

2022-08-06 16:58:02

by Manivannan Sadhasivam

[permalink] [raw]
Subject: Re: [PATCH v2 7/9] dt-bindings: usb: qcom,dwc3: add wakeup-source property

On Sat, Aug 06, 2022 at 06:41:37PM +0200, Johan Hovold wrote:
> On Sat, Aug 06, 2022 at 08:38:48PM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Aug 04, 2022 at 05:09:59PM +0200, Johan Hovold wrote:
> > > 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.
>
> > 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.
>

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?

Thanks,
Mani

> Johan

--
மணிவண்ணன் சதாசிவம்

2022-08-06 17:51:43

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v2 3/9] usb: dwc3: qcom: fix gadget-only builds

On Sat, Aug 06, 2022 at 10:12:50PM +0530, Manivannan Sadhasivam wrote:
> On Sat, Aug 06, 2022 at 06:04:21PM +0200, Johan Hovold wrote:
> > On Sat, Aug 06, 2022 at 07:45:36PM +0530, Manivannan Sadhasivam wrote:
> > > On Thu, Aug 04, 2022 at 05:09:55PM +0200, Johan Hovold wrote:
> > > > 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(-)
> > > >
> > > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > > > index be2e3dd36440..e9364141661b 100644
> > > > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > > > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > > > @@ -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
> > >
> > > 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.
> >
>
> 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

Yeah, that's what Krishna suggested but I wanted to keep the ifdeffery
minimal when fixing the build failure (we generally try to avoid adding
stub functions to implementation files).

Non-host mode was clearly never considered when adding the function in
question as the code blows up otherwise regardless of whether CONFIG_USB
is enabled or not (and a later patch in the series addresses that).

But I'll revisit this too in a couple of weeks.

Thanks for reviewing.

Johan

2022-08-06 17:51:44

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v2 7/9] dt-bindings: usb: qcom,dwc3: add wakeup-source property

On Sat, Aug 06, 2022 at 10:22:38PM +0530, Manivannan Sadhasivam wrote:
> On Sat, Aug 06, 2022 at 06:41:37PM +0200, Johan Hovold wrote:
> > On Sat, Aug 06, 2022 at 08:38:48PM +0530, Manivannan Sadhasivam wrote:
> > > On Thu, Aug 04, 2022 at 05:09:59PM +0200, Johan Hovold wrote:
> > > > 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.
> >
> > > 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.
> >
>
> 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).

> 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.

> 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

2022-08-06 17:53:26

by Manivannan Sadhasivam

[permalink] [raw]
Subject: Re: [PATCH v2 4/9] usb: dwc3: qcom: fix use-after-free on runtime-PM wakeup

On Sat, Aug 06, 2022 at 06:08:51PM +0200, Johan Hovold wrote:
> On Sat, Aug 06, 2022 at 08:03:11PM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Aug 04, 2022 at 05:09:56PM +0200, Johan Hovold wrote:
> > > 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]>
> >
> > 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.
>

That's even better.

Thanks,
Mani

> > > +/* 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;
> > > diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
> > > index f56c30cf151e..f6f13e7f1ba1 100644
> > > --- a/drivers/usb/dwc3/host.c
> > > +++ b/drivers/usb/dwc3/host.c
> > > @@ -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;
> > > }
> > > --
> > > 2.35.1
>
> Johan

--
மணிவண்ணன் சதாசிவம்

2022-08-08 08:08:40

by Manivannan Sadhasivam

[permalink] [raw]
Subject: Re: [PATCH v2 7/9] dt-bindings: usb: qcom,dwc3: add wakeup-source property

On Sat, Aug 06, 2022 at 07:09:44PM +0200, Johan Hovold wrote:
> On Sat, Aug 06, 2022 at 10:22:38PM +0530, Manivannan Sadhasivam wrote:
> > On Sat, Aug 06, 2022 at 06:41:37PM +0200, Johan Hovold wrote:
> > > On Sat, Aug 06, 2022 at 08:38:48PM +0530, Manivannan Sadhasivam wrote:
> > > > On Thu, Aug 04, 2022 at 05:09:59PM +0200, Johan Hovold wrote:
> > > > > 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.
> > >
> > > > 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.
> > >
> >
> > 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).
>

That sounds right to me.

> > 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.
>
> > 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).
>

Okay.

Thanks,
Mani

> Johan

--
மணிவண்ணன் சதாசிவம்

2022-08-08 13:30:53

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH v2 3/9] usb: dwc3: qcom: fix gadget-only builds

On Thu, Aug 04, 2022 at 05:09:55PM +0200, Johan Hovold wrote:
> 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(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index be2e3dd36440..e9364141661b 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -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?

thanks,

greg k-h

2022-08-08 14:08:54

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v2 3/9] usb: dwc3: qcom: fix gadget-only builds

On Mon, Aug 08, 2022 at 03:05:36PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Aug 04, 2022 at 05:09:55PM +0200, Johan Hovold wrote:
> > 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(-)
> >
> > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > index be2e3dd36440..e9364141661b 100644
> > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > @@ -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.

> 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.

> 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]/

> 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.

Johan

2022-08-08 17:35:41

by Matthias Kaehlcke

[permalink] [raw]
Subject: Re: [PATCH v2 8/9] usb: dwc3: qcom: fix wakeup implementation

On Sat, Aug 06, 2022 at 06:22:37PM +0200, Johan Hovold wrote:
> On Fri, Aug 05, 2022 at 09:58:35AM -0700, Matthias Kaehlcke wrote:
> > On Thu, Aug 04, 2022 at 09:59:44AM -0700, Matthias Kaehlcke wrote:
> > > On Thu, Aug 04, 2022 at 05:10:00PM +0200, Johan Hovold wrote:
> > > > 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(-)
> > > >
> > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > > index 16d1f328775f..8c8e32651473 100644
> > > > --- a/drivers/usb/dwc3/core.c
> > > > +++ b/drivers/usb/dwc3/core.c
> > > > @@ -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)) {
> > >
> > > 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.
> >
> > 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.

To be clear, there are two different (supposed) impacts on suspend power:

1. with the PHYs powered off an onboard hub on SC7180/SC7280 boards draws
~80mW during system suspend, vs. ~17mW with the PHYs being on. This is
confirmed.

For SC7180/SC7280 Chrome OS boards in particular it would be ok to
power the PHYs off based on device_may_wakeup(), since Chrome OS
leaves USB wakeup enabled, hence the PHYs would remain powered as
desired.

However boards that opt for disabling USB wakeup could be impacted
by increased power consumption of USB peripherals, as seen with the
hub of SC7180/SC7280 Chrome OS boards.

2. with the PHYs on during system suspend allegedly some QC SoCs can't
reach their lowest power mode (commit c4a5153e87fd). I don't know
which SoCs are impacted.

Someone from QC claims that SC7280 has significantly lower power
consumption with "USB wakeup disabled", so far this has not been
confirmed by my colleague who takes power measurements, I'm in
the process of clarifying what "USB wakeup disabled" exactly
means in this context (e.g. no wakeup source flag vs. no wakeup
capable device plugged).

> 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.

Ok, I'll keep you posted on power findings on our side, though that will
only cover SC7180/SC7280.