RFC: Enable wakeup for intermediate USB bridge devices

RFC: Enable wakeup for intermediate USB bridge devices

[Sameer Nanda]

This is a follow-up from the patch I sent earlier enabling wakeup by
default in the ehci-pci driver.  That thread is here:
http://marc.info/?l=linux-usb&m=131595571619689&w=2

The current wake policy is to enable wake by default for a small set
of devices that the users expect (like keyboards, power buttons and
maybe LAN interfaces).  An example check of this can be seen in
usbhid_start where wakeup is enabled only if the interface subclass is
USB_INTERFACE_SUBCLASS_BOOT and protocol is
USB_INTERFACE_PROTOCOL_KEYBOARD.

However, this doesn't quite work out of the box today since the USB
host controllers that these devices connect to do not enable wakeup by
default.

The proposal is to enable wakeup by default on the intermediate bridge
devices such as ehci-pci and ohci-hcd.  This allows wake from USB
keyboard to work by default.  For non-keyboard devices, the user will
still need to explicitly enable them as a wakesources so there is no
change in the device wakeup policy from what is in place currently for
those devices.

Below is an example patch that enables wakeup for the ehci-pci bridge
device.  I can spin up a similar patch for the ohci-hcd also,
if there is interest in moving this forward.

=============

If the PORTWAK register is implemented and at least one of the physical
ports is enabled for wakeup, enable wakeup for the ehci-pci device.

Signed-off-by: Sameer Nanda <snanda@chromium.org>
---
 drivers/usb/host/ehci-pci.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
index 1102ce6..ce6f784 100644
--- a/drivers/usb/host/ehci-pci.c
+++ b/drivers/usb/host/ehci-pci.c
@@ -285,6 +285,16 @@ static int ehci_pci_setup(struct usb_hcd *hcd)
                      dev_warn(&pdev->dev, "Enabling legacy PCI PM\n");
                      device_set_wakeup_capable(&pdev->dev, 1);
              }
+       } else {
+               u16     port_wake;
+
+               pci_read_config_word(pdev, 0x62, &port_wake);
+
+               /* If PORTWAK register is implemented and at least one USB
+                * port is enabled for wakeup, enable wakeup.
+                */
+               if (port_wake & 0x0001 && port_wake & 0xfffe)
+                       device_set_wakeup_enable(&pdev->dev, 1);
      }

 #ifdef CONFIG_USB_SUSPEND

Re: RFC: Enable wakeup for intermediate USB bridge devices

[Oliver Neukum]

Am Donnerstag, 15. September 2011, 19:03:11 schrieb Sameer Nanda:
> The proposal is to enable wakeup by default on the intermediate bridge
> devices such as ehci-pci and ohci-hcd.  This allows wake from USB
> keyboard to work by default.  For non-keyboard devices, the user will
> still need to explicitly enable them as a wakesources so there is no
> change in the device wakeup policy from what is in place currently for
> those devices.

Have you tested what this means for power sessions on laptops?
Is power kept on under those conditions? How much testing has this
patch seen?

	Regards
		Oliver
-- 
- - - 
SUSE LINUX Products GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 16746 (AG Nürnberg) 
Maxfeldstraße 5                         
90409 Nürnberg 
Germany 
- - - 

Re: RFC: Enable wakeup for intermediate USB bridge devices

[Alan Stern]

On Thu, 15 Sep 2011, Sameer Nanda wrote:

> This is a follow-up from the patch I sent earlier enabling wakeup by
> default in the ehci-pci driver.  That thread is here:
> http://marc.info/?l=linux-usb&m=131595571619689&w=2
> 
> The current wake policy is to enable wake by default for a small set
> of devices that the users expect (like keyboards, power buttons and
> maybe LAN interfaces).  An example check of this can be seen in
> usbhid_start where wakeup is enabled only if the interface subclass is
> USB_INTERFACE_SUBCLASS_BOOT and protocol is
> USB_INTERFACE_PROTOCOL_KEYBOARD.
> 
> However, this doesn't quite work out of the box today since the USB
> host controllers that these devices connect to do not enable wakeup by
> default.
> 
> The proposal is to enable wakeup by default on the intermediate bridge
> devices such as ehci-pci and ohci-hcd.  This allows wake from USB
> keyboard to work by default.  For non-keyboard devices, the user will
> still need to explicitly enable them as a wakesources so there is no
> change in the device wakeup policy from what is in place currently for
> those devices.
> 
> Below is an example patch that enables wakeup for the ehci-pci bridge
> device.  I can spin up a similar patch for the ohci-hcd also,
> if there is interest in moving this forward.
> 
> =============
> 
> If the PORTWAK register is implemented and at least one of the physical
> ports is enabled for wakeup, enable wakeup for the ehci-pci device.
> 
> Signed-off-by: Sameer Nanda <snanda@chromium.org>
> ---
>  drivers/usb/host/ehci-pci.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
> index 1102ce6..ce6f784 100644
> --- a/drivers/usb/host/ehci-pci.c
> +++ b/drivers/usb/host/ehci-pci.c
> @@ -285,6 +285,16 @@ static int ehci_pci_setup(struct usb_hcd *hcd)
>                       dev_warn(&pdev->dev, "Enabling legacy PCI PM\n");
>                       device_set_wakeup_capable(&pdev->dev, 1);
>               }
> +       } else {
> +               u16     port_wake;
> +
> +               pci_read_config_word(pdev, 0x62, &port_wake);
> +
> +               /* If PORTWAK register is implemented and at least one USB
> +                * port is enabled for wakeup, enable wakeup.
> +                */
> +               if (port_wake & 0x0001 && port_wake & 0xfffe)
> +                       device_set_wakeup_enable(&pdev->dev, 1);
>       }
> 
>  #ifdef CONFIG_USB_SUSPEND

This is not the right way to do what you want.  Assuming the change to
the kernel's default wakeup-enable policy is accepted, the patch below
will enable remote wakeup for _all_ host controllers that support it.

At the same time, the patch will disable remote wakeup by default for
root hubs.  I think this is unlikely to cause any problems; people
probably don't want their sleeping laptops to wake up when a USB device
is plugged in or unplugged.  And besides, this is what we should have
been doing all along, according to the current policy.

Note: I haven't tested this patch.  You ought to check and make sure it 
behaves the way it's supposed to.

Alan Stern


Index: usb-3.1/drivers/usb/core/hcd.c
===================================================================
--- usb-3.1.orig/drivers/usb/core/hcd.c
+++ usb-3.1/drivers/usb/core/hcd.c
@@ -2429,7 +2429,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
 	 * but drivers can override it in reset() if needed, along with
 	 * recording the overall controller's system wakeup capability.
 	 */
-	device_init_wakeup(&rhdev->dev, 1);
+	device_set_wakeup_capable(&rhdev->dev, 1);
 
 	/* HCD_FLAG_RH_RUNNING doesn't matter until the root hub is
 	 * registered.  But since the controller can die at any time,
@@ -2478,6 +2478,13 @@ int usb_add_hcd(struct usb_hcd *hcd,
 	}
 	if (hcd->uses_new_polling && HCD_POLL_RH(hcd))
 		usb_hcd_poll_rh_status(hcd);
+
+	/*
+	 * Host controllers don't generate their own wakeup requests;
+	 * they only forward requests from the root hub.  Therefore
+	 * controllers should always be enabled for remote wakeup.
+	 */
+	device_wakeup_enable(hcd->self.controller);
 	return retval;
 
 error_create_attr_group:

Re: RFC: Enable wakeup for intermediate USB bridge devices

[Sameer Nanda]

On Thu, Sep 15, 2011 at 7:36 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Thu, 15 Sep 2011, Sameer Nanda wrote:
>
> > This is a follow-up from the patch I sent earlier enabling wakeup by
> > default in the ehci-pci driver.  That thread is here:
> > http://marc.info/?l=linux-usb&m=131595571619689&w=2
> >
> > The current wake policy is to enable wake by default for a small set
> > of devices that the users expect (like keyboards, power buttons and
> > maybe LAN interfaces).  An example check of this can be seen in
> > usbhid_start where wakeup is enabled only if the interface subclass is
> > USB_INTERFACE_SUBCLASS_BOOT and protocol is
> > USB_INTERFACE_PROTOCOL_KEYBOARD.
> >
> > However, this doesn't quite work out of the box today since the USB
> > host controllers that these devices connect to do not enable wakeup by
> > default.
> >
> > The proposal is to enable wakeup by default on the intermediate bridge
> > devices such as ehci-pci and ohci-hcd.  This allows wake from USB
> > keyboard to work by default.  For non-keyboard devices, the user will
> > still need to explicitly enable them as a wakesources so there is no
> > change in the device wakeup policy from what is in place currently for
> > those devices.
> >
> > Below is an example patch that enables wakeup for the ehci-pci bridge
> > device.  I can spin up a similar patch for the ohci-hcd also,
> > if there is interest in moving this forward.
> >
> > =============
> >
> > If the PORTWAK register is implemented and at least one of the physical
> > ports is enabled for wakeup, enable wakeup for the ehci-pci device.
> >
> > Signed-off-by: Sameer Nanda <snanda@chromium.org>
> > ---
> >  drivers/usb/host/ehci-pci.c |   10 ++++++++++
> >  1 files changed, 10 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
> > index 1102ce6..ce6f784 100644
> > --- a/drivers/usb/host/ehci-pci.c
> > +++ b/drivers/usb/host/ehci-pci.c
> > @@ -285,6 +285,16 @@ static int ehci_pci_setup(struct usb_hcd *hcd)
> >                       dev_warn(&pdev->dev, "Enabling legacy PCI PM\n");
> >                       device_set_wakeup_capable(&pdev->dev, 1);
> >               }
> > +       } else {
> > +               u16     port_wake;
> > +
> > +               pci_read_config_word(pdev, 0x62, &port_wake);
> > +
> > +               /* If PORTWAK register is implemented and at least one USB
> > +                * port is enabled for wakeup, enable wakeup.
> > +                */
> > +               if (port_wake & 0x0001 && port_wake & 0xfffe)
> > +                       device_set_wakeup_enable(&pdev->dev, 1);
> >       }
> >
> >  #ifdef CONFIG_USB_SUSPEND
>
> This is not the right way to do what you want.  Assuming the change to
> the kernel's default wakeup-enable policy is accepted, the patch below
> will enable remote wakeup for _all_ host controllers that support it.
>
> At the same time, the patch will disable remote wakeup by default for
> root hubs.  I think this is unlikely to cause any problems; people
> probably don't want their sleeping laptops to wake up when a USB device
> is plugged in or unplugged.  And besides, this is what we should have
> been doing all along, according to the current policy.
>
> Note: I haven't tested this patch.  You ought to check and make sure it
> behaves the way it's supposed to.

Tested the patch and it works for me -- I can wake the system up from
S3 state from a USB keyboard without needing to manually enable wakeup
on intermediate bridges.  Thanks for the patch!

>
> Alan Stern
>
>
> Index: usb-3.1/drivers/usb/core/hcd.c
> ===================================================================
> --- usb-3.1.orig/drivers/usb/core/hcd.c
> +++ usb-3.1/drivers/usb/core/hcd.c
> @@ -2429,7 +2429,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
>         * but drivers can override it in reset() if needed, along with
>         * recording the overall controller's system wakeup capability.
>         */
> -       device_init_wakeup(&rhdev->dev, 1);
> +       device_set_wakeup_capable(&rhdev->dev, 1);
>
>        /* HCD_FLAG_RH_RUNNING doesn't matter until the root hub is
>         * registered.  But since the controller can die at any time,
> @@ -2478,6 +2478,13 @@ int usb_add_hcd(struct usb_hcd *hcd,
>        }
>        if (hcd->uses_new_polling && HCD_POLL_RH(hcd))
>                usb_hcd_poll_rh_status(hcd);
> +
> +       /*
> +        * Host controllers don't generate their own wakeup requests;
> +        * they only forward requests from the root hub.  Therefore
> +        * controllers should always be enabled for remote wakeup.
> +        */
> +       device_wakeup_enable(hcd->self.controller);
>        return retval;
>
>  error_create_attr_group:
>



--
Sameer