Hi Matthias, On Mon, Apr 25, 2022 at 06:33:03PM +0530, Pavan Kondeti wrote: > Hi Matthias, > > On Fri, Apr 22, 2022 at 11:44:36AM -0700, Matthias Kaehlcke wrote: > > On Fri, Apr 22, 2022 at 01:57:17PM +0200, Rafael J. Wysocki wrote: > > > On Tue, Apr 19, 2022 at 9:11 PM Sandeep Maheswaram > > > <quic_c_sanm@xxxxxxxxxxx> wrote: > > > > > > > > From: Matthias Kaehlcke <mka@xxxxxxxxxxxx> > > > > > > > > Add device_children_wakeup_capable() which checks whether the device itself > > > > or one if its descendants is wakeup capable. > > > > > > device_wakeup_path() exists for a very similar purpose. > > > > > > Is it not usable for whatever you need the new function introduced here? > > > > I wasn't aware of it's function, there are no doc comments and the > > name isn't really self explanatory. > > > > In a quick test device_wakeup_path() returned inconsistent values for the > > root hub, sometimes true, others false when a wakeup capable USB device was > > connected. > > We will also test the same to double confirm the behavior of > device_wakeup_path(). I am assuming that you checked device_wakeup_path() > only during system suspend path. > > Here is what I understood by looking at __device_suspend(). Please share > your thoughts on this. > > power.wakeup_path is set to true for the parent *after* a wakeup capable > device is suspended. This means when the root hub(s) is suspended, it is > propagated to xhci-plat and when xhci-plat is suspended, it is propagated > to dwc3. bottom up propgation during system suspend. > > I believe we can directly check something like this in the dwc3 driver > instead of having another wrapper like device_children_wakeup_capable(). > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 1170b80..a783257 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -1878,8 +1878,14 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > break; > case DWC3_GCTL_PRTCAP_HOST: > if (!PMSG_IS_AUTO(msg)) { > + /* > + * Don't kill the host when dwc3 is wakeup capable and > + * its children needs wakeup. > + */ > + if (device_may_wakeup(dwc->dev) && device_wakeup_path(dwc->dev)) > + handle_it(); > + } else { > dwc3_core_exit(dwc); > - break; > } > > /* Let controller to suspend HSPHY before PHY driver suspends */ > device_wakeup_path(dwc->dev) is returning true all the time irrespective of the wakeup capability (and enabled status) of the connected USB devices. That is because xhci-plat device is configured to wakeup all the time. Since the child is wakeup capable, its parent i.e dwc3 has device_wakeup_path() set. device_children_wakeup_capable() will also suffer the problem. However, device_children_wakeup_capable(&hcd->self.root_hub->dev) is what Sandeep's patch is using. That is not correct. we have two root hubs (HS and SS) associated with a USB3 controller and calling it on one root hub is incorrect. device_children_wakeup_capable() must be called on xhci-plat so that it covers both HS and SS root hubs I am thinking of dynamically enabling/disabling xhci-plat wakeup capability so that the wakeup path is correctly propagated to dwc3. something like below. Does it make sense to you? diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c index 649ffd8..be0c55b 100644 --- a/drivers/usb/host/xhci-plat.c +++ b/drivers/usb/host/xhci-plat.c @@ -412,6 +412,9 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev) struct xhci_hcd *xhci = hcd_to_xhci(hcd); int ret; + if (!device_wakeup_path(dev)) + device_wakeup_disable(dev); + if (pm_runtime_suspended(dev)) pm_runtime_resume(dev); @@ -443,6 +446,8 @@ static int __maybe_unused xhci_plat_resume(struct device *dev) pm_runtime_set_active(dev); pm_runtime_enable(dev); + device_wakeup_enable(dev); + return 0; } Thanks, Pavan