linux-kernel - Re: RFC: Enable wakeup for intermediate USB bridge devices

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CANMivWZUj-JP7h2b-UaKes6RGVhGT2Fsv1pD+n_zkjevMcPjRw@mail.gmail.com>
Date:	Fri, 16 Sep 2011 09:14:37 -0700
From:	Sameer Nanda <snanda@...omium.org>
To:	Alan Stern <stern@...land.harvard.edu>
Cc:	gregkh@...e.de, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Vincent Palatin <vpalatin@...omium.org>, olofj@...omium.org
Subject: Re: RFC: Enable wakeup for intermediate USB bridge devices

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