Re: [PATCH] usb: ehci: Only sleep for post-resume handover if devices use persist

2013-04-24 Thread Alan Stern
On Tue, 23 Apr 2013, Julius Werner wrote:

 The current EHCI code sleeps a flat 110ms in the resume path if there
 was a USB 1.1 device connected to its companion controller during
 suspend, waiting for the device to reappear and reset so that it can be
 handed back to the companion. This is necessary if the device uses
 persist, so that the companion controller can actually see it during its
 own resume path.
 
 However, if the device doesn't use persist, this is entirely
 unnecessary. We might just as well ignore it and have the normal device
 detection/reset/handoff code handle it asynchronously when it eventually
 shows up. As USB 1.1 devices are almost exclusively HIDs these days (for
 which persist has no value), this can allow distros to shave another
 tenth of a second off their resume time.

This is a good idea.  The patch needs some fixes, though.

 Signed-off-by: Julius Werner jwer...@chromium.org
 ---
  drivers/usb/host/ehci-hub.c | 21 +
  1 file changed, 21 insertions(+)
 
 diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
 index 7d06e77..2507afb 100644
 --- a/drivers/usb/host/ehci-hub.c
 +++ b/drivers/usb/host/ehci-hub.c
 @@ -29,6 +29,8 @@
  /*-*/
  #include linux/usb/otg.h
  
 +#include ../core/usb.h

Unfortunately, this won't work if ehci-hcd is built as a module.  
usbcore does not export usb_device_type, which is used by 
is_usb_device().

Also, including a core header into a non-core driver is ugly.  Perhaps
a better approach would be to have usbcore export an iterator that runs
over devices only -- a for_each_usb_device() routine.

 +
  #define  PORT_WAKE_BITS  (PORT_WKOC_E|PORT_WKDISC_E|PORT_WKCONN_E)
  
  #ifdef   CONFIG_PM
 @@ -42,6 +44,18 @@ static int ehci_hub_control(
   u16 wLength
  );
  
 +
 +static int companion_persist(struct device *dev, void *unused)

This name isn't so great.  Something like
persistent_device_on_usb11_bus would be more accurate, if not more
readable.  Maybe you can think of something better.

 +{
 + struct usb_device *udev = container_of(dev, struct usb_device, dev);
 +
 + if (!is_usb_device(dev))
 + return 0;  /* Filter out struct usb_interface on the same bus */
 +
 + return !udev-maxchild  udev-persist_enabled 
 + udev-bus-root_hub-speed  USB_SPEED_HIGH;
 +}
 +
  /* After a power loss, ports that were owned by the companion must be
   * reset so that the companion can still own them.
   */
 @@ -56,6 +70,13 @@ static void ehci_handover_companion_ports(struct ehci_hcd 
 *ehci)
   if (!ehci-owned_ports)
   return;
  
 + /* Check if any USB 1.1 controller in the system uses a non-hub device
 +  * with persist enabled. If they don't, the device will get reenumerated
 +  * anyway and there is no need to slow resume down to wait for it.
 +  */

/*
 * New comments should be formatted like this.
 */

Also, the comment is grammatically unsound: It talks about non-existent
devices getting reenumerated.  What you really mean is more like this:

If all devices (aside from hubs) on USB-1.1 buses have persist
disabled then we don't need to perform the handover, with its 
 100 ms delay.  Any such devices will get reenumerated
normally; typically they are only HID anyway.


 + if (!bus_for_each_dev(usb_bus_type, NULL, NULL, companion_persist))
 + return;
 +
   /* Make sure the ports are powered */
   port = HCS_N_PORTS(ehci-hcs_params);
   while (port--) {

Alan Stern

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH] usb: ehci: Only sleep for post-resume handover if devices use persist

2013-04-23 Thread Julius Werner
The current EHCI code sleeps a flat 110ms in the resume path if there
was a USB 1.1 device connected to its companion controller during
suspend, waiting for the device to reappear and reset so that it can be
handed back to the companion. This is necessary if the device uses
persist, so that the companion controller can actually see it during its
own resume path.

However, if the device doesn't use persist, this is entirely
unnecessary. We might just as well ignore it and have the normal device
detection/reset/handoff code handle it asynchronously when it eventually
shows up. As USB 1.1 devices are almost exclusively HIDs these days (for
which persist has no value), this can allow distros to shave another
tenth of a second off their resume time.

Signed-off-by: Julius Werner jwer...@chromium.org
---
 drivers/usb/host/ehci-hub.c | 21 +
 1 file changed, 21 insertions(+)

diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
index 7d06e77..2507afb 100644
--- a/drivers/usb/host/ehci-hub.c
+++ b/drivers/usb/host/ehci-hub.c
@@ -29,6 +29,8 @@
 /*-*/
 #include linux/usb/otg.h
 
+#include ../core/usb.h
+
 #definePORT_WAKE_BITS  (PORT_WKOC_E|PORT_WKDISC_E|PORT_WKCONN_E)
 
 #ifdef CONFIG_PM
@@ -42,6 +44,18 @@ static int ehci_hub_control(
u16 wLength
 );
 
+
+static int companion_persist(struct device *dev, void *unused)
+{
+   struct usb_device *udev = container_of(dev, struct usb_device, dev);
+
+   if (!is_usb_device(dev))
+   return 0;  /* Filter out struct usb_interface on the same bus */
+
+   return !udev-maxchild  udev-persist_enabled 
+   udev-bus-root_hub-speed  USB_SPEED_HIGH;
+}
+
 /* After a power loss, ports that were owned by the companion must be
  * reset so that the companion can still own them.
  */
@@ -56,6 +70,13 @@ static void ehci_handover_companion_ports(struct ehci_hcd 
*ehci)
if (!ehci-owned_ports)
return;
 
+   /* Check if any USB 1.1 controller in the system uses a non-hub device
+* with persist enabled. If they don't, the device will get reenumerated
+* anyway and there is no need to slow resume down to wait for it.
+*/
+   if (!bus_for_each_dev(usb_bus_type, NULL, NULL, companion_persist))
+   return;
+
/* Make sure the ports are powered */
port = HCS_N_PORTS(ehci-hcs_params);
while (port--) {
-- 
1.8.2.1

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html