Re: RFC: Enable wakeup for intermediate USB bridge devices

[Sameer Nanda]

On Thu, Sep 15, 2011 at 7:36 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > ---
> > Â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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/