[v14,2/7] PM / wakeup: Add device_children_wakeup_capable()

Commit Message

Sandeep Maheswaram April 19, 2022, 7:11 p.m. UTC

From: Matthias Kaehlcke <[e-mail]>

Add device_children_wakeup_capable() which checks whether the device itself
or one if its descendants is wakeup capable.

Suggested-by: Felipe Balbi <[e-mail]>
Signed-off-by: Sandeep Maheswaram <[e-mail]>
Signed-off-by: Matthias Kaehlcke <[e-mail]>
---
 drivers/base/power/wakeup.c | 18 ++++++++++++++++++
 include/linux/pm_wakeup.h   |  7 +++++++
 2 files changed, 25 insertions(+)

Comments

Rafael J. Wysocki April 22, 2022, 11:57 a.m. UTC | #1

On Tue, Apr 19, 2022 at 9:11 PM Sandeep Maheswaram
<[e-mail]> wrote:
>
> From: Matthias Kaehlcke <[e-mail]>
>
> 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?

> Suggested-by: Felipe Balbi <[e-mail]>
> Signed-off-by: Sandeep Maheswaram <[e-mail]>
> Signed-off-by: Matthias Kaehlcke <[e-mail]>
> ---
>  drivers/base/power/wakeup.c | 18 ++++++++++++++++++
>  include/linux/pm_wakeup.h   |  7 +++++++
>  2 files changed, 25 insertions(+)
>
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index a57d469..1900637 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -541,6 +541,24 @@ int device_set_wakeup_enable(struct device *dev, bool enable)
>  }
>  EXPORT_SYMBOL_GPL(device_set_wakeup_enable);
>
> +static int __device_children_wakeup_capable(struct device *dev, void *dummy)
> +{
> +       return device_may_wakeup(dev) ||
> +               device_for_each_child(dev, NULL, __device_children_wakeup_capable);
> +}
> +
> +/**
> + * device_children_wakeup_capable - Check whether a device or one of its descendants is
> + *                                  wakeup capable.
> + * @dev: Device to handle.
> + */
> +bool device_children_wakeup_capable(struct device *dev)
> +{
> +       return __device_children_wakeup_capable(dev, NULL);
> +}
> +EXPORT_SYMBOL_GPL(device_children_wakeup_capable);
> +
> +
>  /**
>   * wakeup_source_not_registered - validate the given wakeup source.
>   * @ws: Wakeup source to be validated.
> diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h
> index 196a157..9a3005b 100644
> --- a/include/linux/pm_wakeup.h
> +++ b/include/linux/pm_wakeup.h
> @@ -109,6 +109,7 @@ extern struct wakeup_source *wakeup_sources_walk_next(struct wakeup_source *ws);
>  extern int device_wakeup_enable(struct device *dev);
>  extern int device_wakeup_disable(struct device *dev);
>  extern void device_set_wakeup_capable(struct device *dev, bool capable);
> +extern bool device_children_wakeup_capable(struct device *dev);
>  extern int device_init_wakeup(struct device *dev, bool val);
>  extern int device_set_wakeup_enable(struct device *dev, bool enable);
>  extern void __pm_stay_awake(struct wakeup_source *ws);
> @@ -186,6 +187,12 @@ static inline bool device_wakeup_path(struct device *dev)
>
>  static inline void device_set_wakeup_path(struct device *dev) {}
>
> +static inline bool device_children_wakeup_capable(struct device *dev)
> +{
> +       return false;
> +}
> +
> +
>  static inline void __pm_stay_awake(struct wakeup_source *ws) {}
>
>  static inline void pm_stay_awake(struct device *dev) {}
> --
> 2.7.4
>

Matthias Kaehlcke April 22, 2022, 6:44 p.m. UTC | #2

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
> <[e-mail]> wrote:
> >
> > From: Matthias Kaehlcke <[e-mail]>
> >
> > 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.

Pavan Kondeti April 25, 2022, 1:03 p.m. UTC | #3

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
> > <[e-mail]> wrote:
> > >
> > > From: Matthias Kaehlcke <[e-mail]>
> > >
> > > 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 */

Thanks,
Pavan

Pavan Kondeti April 29, 2022, 12:59 p.m. UTC | #4

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
> > > <[e-mail]> wrote:
> > > >
> > > > From: Matthias Kaehlcke <[e-mail]>
> > > >
> > > > 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

Matthias Kaehlcke April 29, 2022, 7:19 p.m. UTC | #5

Hi Pavan,

On Fri, Apr 29, 2022 at 06:29:56PM +0530, Pavan Kondeti wrote:
> 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
> > > > <[e-mail]> wrote:
> > > > >
> > > > > From: Matthias Kaehlcke <[e-mail]>
> > > > >
> > > > > 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

Thanks for pointing that out!

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

I think this also needs to be done conditionally, otherwise it would
create a new wake source on every resume when wakeup is already
enabled.

Other than that this seems to do the trick and keeps the USB layer out of
the dwc3 code.

Pavan Kondeti April 30, 2022, 3:11 a.m. UTC | #6

Hi Matthias,

On Fri, Apr 29, 2022 at 12:19:22PM -0700, Matthias Kaehlcke wrote:
> Hi Pavan,
> 
> On Fri, Apr 29, 2022 at 06:29:56PM +0530, Pavan Kondeti wrote:
> > 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
> > > > > <[e-mail]> wrote:
> > > > > >
> > > > > > From: Matthias Kaehlcke <[e-mail]>
> > > > > >
> > > > > > 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
> 
> Thanks for pointing that out!
> 
> > 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);
> 
> I think this also needs to be done conditionally, otherwise it would
> create a new wake source on every resume when wakeup is already
> enabled.
> 
Right, this needs to be done conditionally. However, there is a silent
warning inside device_wakeup_enable() if it is called during system
transition. Not sure if we really need to worry about that or not.

Thanks,
Pavan

Matthias Kaehlcke May 3, 2022, 12:57 a.m. UTC | #7

Hi Pavan,

On Sat, Apr 30, 2022 at 08:41:30AM +0530, Pavan Kondeti wrote:
> Hi Matthias,
> 
> On Fri, Apr 29, 2022 at 12:19:22PM -0700, Matthias Kaehlcke wrote:
> > Hi Pavan,
> > 
> > On Fri, Apr 29, 2022 at 06:29:56PM +0530, Pavan Kondeti wrote:
> > > 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
> > > > > > <[e-mail]> wrote:
> > > > > > >
> > > > > > > From: Matthias Kaehlcke <[e-mail]>
> > > > > > >
> > > > > > > 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
> > 
> > Thanks for pointing that out!
> > 
> > > 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);
> > 
> > I think this also needs to be done conditionally, otherwise it would
> > create a new wake source on every resume when wakeup is already
> > enabled.
> > 
> Right, this needs to be done conditionally. However, there is a silent
> warning inside device_wakeup_enable() if it is called during system
> transition. Not sure if we really need to worry about that or not.

I guess it's up to the maintainers. Removing and adding the wakeup source on
suspend/resume is a bit of a hack, but it might be acceptable if it addresses
the issue and doesn't have negative side effects.

Patch

diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index a57d469..1900637 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -541,6 +541,24 @@  int device_set_wakeup_enable(struct device *dev, bool enable)
 }
 EXPORT_SYMBOL_GPL(device_set_wakeup_enable);
 
+static int __device_children_wakeup_capable(struct device *dev, void *dummy)
+{
+	return device_may_wakeup(dev) ||
+		device_for_each_child(dev, NULL, __device_children_wakeup_capable);
+}
+
+/**
+ * device_children_wakeup_capable - Check whether a device or one of its descendants is
+ *                                  wakeup capable.
+ * @dev: Device to handle.
+ */
+bool device_children_wakeup_capable(struct device *dev)
+{
+	return __device_children_wakeup_capable(dev, NULL);
+}
+EXPORT_SYMBOL_GPL(device_children_wakeup_capable);
+
+
 /**
  * wakeup_source_not_registered - validate the given wakeup source.
  * @ws: Wakeup source to be validated.
diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h
index 196a157..9a3005b 100644
--- a/include/linux/pm_wakeup.h
+++ b/include/linux/pm_wakeup.h
@@ -109,6 +109,7 @@  extern struct wakeup_source *wakeup_sources_walk_next(struct wakeup_source *ws);
 extern int device_wakeup_enable(struct device *dev);
 extern int device_wakeup_disable(struct device *dev);
 extern void device_set_wakeup_capable(struct device *dev, bool capable);
+extern bool device_children_wakeup_capable(struct device *dev);
 extern int device_init_wakeup(struct device *dev, bool val);
 extern int device_set_wakeup_enable(struct device *dev, bool enable);
 extern void __pm_stay_awake(struct wakeup_source *ws);
@@ -186,6 +187,12 @@  static inline bool device_wakeup_path(struct device *dev)
 
 static inline void device_set_wakeup_path(struct device *dev) {}
 
+static inline bool device_children_wakeup_capable(struct device *dev)
+{
+	return false;
+}
+
+
 static inline void __pm_stay_awake(struct wakeup_source *ws) {}
 
 static inline void pm_stay_awake(struct device *dev) {}