From 1b42ae6d4355328dc4406b6f0188adcf8c566435 Mon Sep 17 00:00:00 2001 From: Alan Stern Date: Tue, 13 Mar 2007 11:10:52 -0400 Subject: USB: fix race in HCD removal This patch (as865) fixes a race in the HCD removal code discovered by Milan Plzik. Arrival of an interrupt after the root hub was unregistered could cause the root-hub status timer to start up, even after it was supposed to have been shut down. The problem is fixed by moving the del_timer_sync() call to after the HCD's stop() method, at which time IRQ generation should be disabled. Cc: Milan Plzik Signed-off-by: Alan Stern Signed-off-by: Greg Kroah-Hartman --- drivers/usb/core/hcd.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) (limited to 'drivers/usb/core/hcd.c') diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c index b26c19e8d19f..af7aed11398b 100644 --- a/drivers/usb/core/hcd.c +++ b/drivers/usb/core/hcd.c @@ -544,6 +544,8 @@ void usb_hcd_poll_rh_status(struct usb_hcd *hcd) unsigned long flags; char buffer[4]; /* Any root hubs with > 31 ports? */ + if (unlikely(!hcd->rh_registered)) + return; if (!hcd->uses_new_polling && !hcd->status_urb) return; @@ -1670,12 +1672,12 @@ void usb_remove_hcd(struct usb_hcd *hcd) usb_disconnect(&hcd->self.root_hub); mutex_unlock(&usb_bus_list_lock); - hcd->poll_rh = 0; - del_timer_sync(&hcd->rh_timer); - hcd->driver->stop(hcd); hcd->state = HC_STATE_HALT; + hcd->poll_rh = 0; + del_timer_sync(&hcd->rh_timer); + if (hcd->irq >= 0) free_irq(hcd->irq, hcd); usb_deregister_bus(&hcd->self); -- cgit v1.2.3 From 6b157c9bf3bace6eeb4a973da63923ef24995cce Mon Sep 17 00:00:00 2001 From: Alan Stern Date: Tue, 13 Mar 2007 16:37:30 -0400 Subject: USB: separate autosuspend from external suspend This patch (as866) adds new entry points for external USB device suspend and resume requests, as opposed to internally-generated autosuspend or autoresume. It also changes the existing remote-wakeup code paths to use the new routines, since remote wakeup is not the same as autoresume. As part of the change, it turns out to be necessary to do remote wakeup of root hubs from a workqueue. We had been using khubd, but it does autoresume rather than an external resume. Using the ksuspend_usb_wq workqueue for this purpose seemed a logical choice. Signed-off-by: Alan Stern Signed-off-by: Greg Kroah-Hartman --- drivers/usb/core/driver.c | 82 ++++++++++++++++++++++++++++++++++------------- drivers/usb/core/hcd.c | 25 +++++++++++++-- drivers/usb/core/hcd.h | 3 ++ drivers/usb/core/hub.c | 14 +------- drivers/usb/core/usb.c | 3 +- drivers/usb/core/usb.h | 4 ++- 6 files changed, 90 insertions(+), 41 deletions(-) (limited to 'drivers/usb/core/hcd.c') diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c index 8c0a7de61228..abea48de8766 100644 --- a/drivers/usb/core/driver.c +++ b/drivers/usb/core/driver.c @@ -1424,48 +1424,84 @@ void usb_autosuspend_work(struct work_struct *work) #endif /* CONFIG_USB_SUSPEND */ -static int usb_suspend(struct device *dev, pm_message_t message) +/** + * usb_external_suspend_device - external suspend of a USB device and its interfaces + * @udev: the usb_device to suspend + * @msg: Power Management message describing this state transition + * + * This routine handles external suspend requests: ones not generated + * internally by a USB driver (autosuspend) but rather coming from the user + * (via sysfs) or the PM core (system sleep). The suspend will be carried + * out regardless of @udev's usage counter or those of its interfaces, + * and regardless of whether or not remote wakeup is enabled. Of course, + * interface drivers still have the option of failing the suspend (if + * there are unsuspended children, for example). + * + * The caller must hold @udev's device lock. + */ +int usb_external_suspend_device(struct usb_device *udev, pm_message_t msg) { int status; - if (is_usb_device(dev)) { - struct usb_device *udev = to_usb_device(dev); - - usb_pm_lock(udev); - udev->auto_pm = 0; - status = usb_suspend_both(udev, message); - usb_pm_unlock(udev); - } else - status = 0; + usb_pm_lock(udev); + udev->auto_pm = 0; + status = usb_suspend_both(udev, msg); + usb_pm_unlock(udev); return status; } -static int usb_resume(struct device *dev) +/** + * usb_external_resume_device - external resume of a USB device and its interfaces + * @udev: the usb_device to resume + * + * This routine handles external resume requests: ones not generated + * internally by a USB driver (autoresume) but rather coming from the user + * (via sysfs), the PM core (system resume), or the device itself (remote + * wakeup). @udev's usage counter is unaffected. + * + * The caller must hold @udev's device lock. + */ +int usb_external_resume_device(struct usb_device *udev) { int status; - if (is_usb_device(dev)) { - struct usb_device *udev = to_usb_device(dev); - - usb_pm_lock(udev); - udev->auto_pm = 0; - status = usb_resume_both(udev); - usb_pm_unlock(udev); + usb_pm_lock(udev); + udev->auto_pm = 0; + status = usb_resume_both(udev); + usb_pm_unlock(udev); - /* Rebind drivers that had no suspend method? */ - } else - status = 0; + /* Now that the device is awake, we can start trying to autosuspend + * it again. */ + if (status == 0) + usb_try_autosuspend_device(udev); return status; } +static int usb_suspend(struct device *dev, pm_message_t message) +{ + if (!is_usb_device(dev)) /* Ignore PM for interfaces */ + return 0; + return usb_external_suspend_device(to_usb_device(dev), message); +} + +static int usb_resume(struct device *dev) +{ + if (!is_usb_device(dev)) /* Ignore PM for interfaces */ + return 0; + return usb_external_resume_device(to_usb_device(dev)); +} + +#else + +#define usb_suspend NULL +#define usb_resume NULL + #endif /* CONFIG_PM */ struct bus_type usb_bus_type = { .name = "usb", .match = usb_device_match, .uevent = usb_uevent, -#ifdef CONFIG_PM .suspend = usb_suspend, .resume = usb_resume, -#endif }; diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c index af7aed11398b..8bc3ce6d9666 100644 --- a/drivers/usb/core/hcd.c +++ b/drivers/usb/core/hcd.c @@ -37,6 +37,7 @@ #include #include #include +#include #include @@ -1298,14 +1299,25 @@ int hcd_bus_resume (struct usb_bus *bus) return status; } +/* Workqueue routine for root-hub remote wakeup */ +static void hcd_resume_work(struct work_struct *work) +{ + struct usb_hcd *hcd = container_of(work, struct usb_hcd, wakeup_work); + struct usb_device *udev = hcd->self.root_hub; + + usb_lock_device(udev); + usb_external_resume_device(udev); + usb_unlock_device(udev); +} + /** * usb_hcd_resume_root_hub - called by HCD to resume its root hub * @hcd: host controller for this root hub * * The USB host controller calls this function when its root hub is * suspended (with the remote wakeup feature enabled) and a remote - * wakeup request is received. It queues a request for khubd to - * resume the root hub (that is, manage its downstream ports again). + * wakeup request is received. The routine submits a workqueue request + * to resume the root hub (that is, manage its downstream ports again). */ void usb_hcd_resume_root_hub (struct usb_hcd *hcd) { @@ -1313,7 +1325,7 @@ void usb_hcd_resume_root_hub (struct usb_hcd *hcd) spin_lock_irqsave (&hcd_root_hub_lock, flags); if (hcd->rh_registered) - usb_resume_root_hub (hcd->self.root_hub); + queue_work(ksuspend_usb_wq, &hcd->wakeup_work); spin_unlock_irqrestore (&hcd_root_hub_lock, flags); } EXPORT_SYMBOL_GPL(usb_hcd_resume_root_hub); @@ -1502,6 +1514,9 @@ struct usb_hcd *usb_create_hcd (const struct hc_driver *driver, init_timer(&hcd->rh_timer); hcd->rh_timer.function = rh_timer_func; hcd->rh_timer.data = (unsigned long) hcd; +#ifdef CONFIG_PM + INIT_WORK(&hcd->wakeup_work, hcd_resume_work); +#endif hcd->driver = driver; hcd->product_desc = (driver->product_desc) ? driver->product_desc : @@ -1668,6 +1683,10 @@ void usb_remove_hcd(struct usb_hcd *hcd) hcd->rh_registered = 0; spin_unlock_irq (&hcd_root_hub_lock); +#ifdef CONFIG_PM + flush_workqueue(ksuspend_usb_wq); +#endif + mutex_lock(&usb_bus_list_lock); usb_disconnect(&hcd->self.root_hub); mutex_unlock(&usb_bus_list_lock); diff --git a/drivers/usb/core/hcd.h b/drivers/usb/core/hcd.h index 2a269ca20517..ef50fa494e47 100644 --- a/drivers/usb/core/hcd.h +++ b/drivers/usb/core/hcd.h @@ -68,6 +68,9 @@ struct usb_hcd { struct timer_list rh_timer; /* drives root-hub polling */ struct urb *status_urb; /* the current status urb */ +#ifdef CONFIG_PM + struct work_struct wakeup_work; /* for remote wakeup */ +#endif /* * hardware info/state diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index 7a6028599d62..19abe81babd5 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -1855,12 +1855,7 @@ static int remote_wakeup(struct usb_device *udev) usb_lock_device(udev); if (udev->state == USB_STATE_SUSPENDED) { dev_dbg(&udev->dev, "usb %sresume\n", "wakeup-"); - status = usb_autoresume_device(udev); - - /* Give the interface drivers a chance to do something, - * then autosuspend the device again. */ - if (status == 0) - usb_autosuspend_device(udev); + status = usb_external_resume_device(udev); } usb_unlock_device(udev); return status; @@ -1984,13 +1979,6 @@ static inline int remote_wakeup(struct usb_device *udev) #define hub_resume NULL #endif -void usb_resume_root_hub(struct usb_device *hdev) -{ - struct usb_hub *hub = hdev_to_hub(hdev); - - kick_khubd(hub); -} - /* USB 2.0 spec, 7.1.7.3 / fig 7-29: * diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c index 82338f497860..138252e0a1cf 100644 --- a/drivers/usb/core/usb.c +++ b/drivers/usb/core/usb.c @@ -49,7 +49,8 @@ const char *usbcore_name = "usbcore"; static int nousb; /* Disable USB when built into kernel image */ -struct workqueue_struct *ksuspend_usb_wq; /* For autosuspend */ +/* Workqueue for autosuspend and for remote wakeup of root hubs */ +struct workqueue_struct *ksuspend_usb_wq; #ifdef CONFIG_USB_SUSPEND static int usb_autosuspend_delay = 2; /* Default delay value, diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h index b98bc0d381c0..c94379e55f2d 100644 --- a/drivers/usb/core/usb.h +++ b/drivers/usb/core/usb.h @@ -21,7 +21,6 @@ extern char *usb_cache_string(struct usb_device *udev, int index); extern int usb_set_configuration(struct usb_device *dev, int configuration); extern void usb_kick_khubd(struct usb_device *dev); -extern void usb_resume_root_hub(struct usb_device *dev); extern int usb_match_device(struct usb_device *dev, const struct usb_device_id *id); @@ -37,6 +36,9 @@ extern void usb_host_cleanup(void); extern void usb_autosuspend_work(struct work_struct *work); extern int usb_port_suspend(struct usb_device *dev); extern int usb_port_resume(struct usb_device *dev); +extern int usb_external_suspend_device(struct usb_device *udev, + pm_message_t msg); +extern int usb_external_resume_device(struct usb_device *udev); static inline void usb_pm_lock(struct usb_device *udev) { -- cgit v1.2.3 From 1941044aa9632aa8debbb94a3c8a5ed0ebddade8 Mon Sep 17 00:00:00 2001 From: Alan Stern Date: Tue, 27 Mar 2007 13:33:59 -0400 Subject: USB: add "last_busy" field for use in autosuspend This patch (as877) adds a "last_busy" field to struct usb_device, for use by the autosuspend framework. Now if an autosuspend call comes at a time when the device isn't busy but hasn't yet been idle for long enough, the timer can be set to exactly the desired value. And we will be ready to handle things like HID drivers, which can't maintain a useful usage count and must rely on the time-of-last-use to decide when to autosuspend. The patch also makes some related minor improvements: Move the calls to the autosuspend condition-checking routine into usb_suspend_both(), which is the only place where it really matters. If the autosuspend timer is already running, don't stop and restart it. Replace immediate returns with gotos so that the optional debugging ouput won't be bypassed. If autoresume is disabled but the device is already awake, don't return an error for an autoresume call. Don't try to autoresume a device if it isn't suspended. (Yes, this undercuts the previous change -- so sue me.) Don't duplicate existing code in the autosuspend work routine. Fix the kerneldoc in usb_autopm_put_interface(): If an autoresume call fails, the usage counter is left unchanged. Signed-off-by: Alan Stern Signed-off-by: Greg Kroah-Hartman --- drivers/usb/core/driver.c | 102 +++++++++++++++++++++++++++++----------------- drivers/usb/core/hcd.c | 1 + drivers/usb/core/hub.c | 1 + include/linux/usb.h | 8 ++++ 4 files changed, 75 insertions(+), 37 deletions(-) (limited to 'drivers/usb/core/hcd.c') diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c index 593386eb974d..631f30582481 100644 --- a/drivers/usb/core/driver.c +++ b/drivers/usb/core/driver.c @@ -932,6 +932,7 @@ static int autosuspend_check(struct usb_device *udev) { int i; struct usb_interface *intf; + long suspend_time; /* For autosuspend, fail fast if anything is in use or autosuspend * is disabled. Also fail if any interfaces require remote wakeup @@ -943,6 +944,7 @@ static int autosuspend_check(struct usb_device *udev) if (udev->autosuspend_delay < 0 || udev->autosuspend_disabled) return -EPERM; + suspend_time = udev->last_busy + udev->autosuspend_delay; if (udev->actconfig) { for (i = 0; i < udev->actconfig->desc.bNumInterfaces; i++) { intf = udev->actconfig->interface[i]; @@ -958,6 +960,17 @@ static int autosuspend_check(struct usb_device *udev) } } } + + /* If everything is okay but the device hasn't been idle for long + * enough, queue a delayed autosuspend request. + */ + suspend_time -= jiffies; + if (suspend_time > 0) { + if (!timer_pending(&udev->autosuspend.timer)) + queue_delayed_work(ksuspend_usb_wq, &udev->autosuspend, + suspend_time); + return -EAGAIN; + } return 0; } @@ -1010,19 +1023,18 @@ static int usb_suspend_both(struct usb_device *udev, pm_message_t msg) struct usb_interface *intf; struct usb_device *parent = udev->parent; - cancel_delayed_work(&udev->autosuspend); - if (udev->state == USB_STATE_NOTATTACHED) - return 0; - if (udev->state == USB_STATE_SUSPENDED) - return 0; + if (udev->state == USB_STATE_NOTATTACHED || + udev->state == USB_STATE_SUSPENDED) + goto done; udev->do_remote_wakeup = device_may_wakeup(&udev->dev); if (udev->auto_pm) { status = autosuspend_check(udev); if (status < 0) - return status; + goto done; } + cancel_delayed_work(&udev->autosuspend); /* Suspend all the interfaces and then udev itself */ if (udev->actconfig) { @@ -1047,6 +1059,7 @@ static int usb_suspend_both(struct usb_device *udev, pm_message_t msg) } else if (parent) usb_autosuspend_device(parent); + done: // dev_dbg(&udev->dev, "%s: status %d\n", __FUNCTION__, status); return status; } @@ -1086,14 +1099,18 @@ static int usb_resume_both(struct usb_device *udev) struct usb_interface *intf; struct usb_device *parent = udev->parent; - if (udev->auto_pm && udev->autoresume_disabled) - return -EPERM; cancel_delayed_work(&udev->autosuspend); - if (udev->state == USB_STATE_NOTATTACHED) - return -ENODEV; + if (udev->state == USB_STATE_NOTATTACHED) { + status = -ENODEV; + goto done; + } /* Propagate the resume up the tree, if necessary */ if (udev->state == USB_STATE_SUSPENDED) { + if (udev->auto_pm && udev->autoresume_disabled) { + status = -EPERM; + goto done; + } if (parent) { status = usb_autoresume_device(parent); if (status == 0) { @@ -1139,24 +1156,13 @@ static int usb_resume_both(struct usb_device *udev) } } + done: // dev_dbg(&udev->dev, "%s: status %d\n", __FUNCTION__, status); return status; } #ifdef CONFIG_USB_SUSPEND -/* usb_autosuspend_work - callback routine to autosuspend a USB device */ -void usb_autosuspend_work(struct work_struct *work) -{ - struct usb_device *udev = - container_of(work, struct usb_device, autosuspend.work); - - usb_pm_lock(udev); - udev->auto_pm = 1; - usb_suspend_both(udev, PMSG_SUSPEND); - usb_pm_unlock(udev); -} - /* Internal routine to adjust a device's usage counter and change * its autosuspend state. */ @@ -1165,20 +1171,34 @@ static int usb_autopm_do_device(struct usb_device *udev, int inc_usage_cnt) int status = 0; usb_pm_lock(udev); + udev->auto_pm = 1; udev->pm_usage_cnt += inc_usage_cnt; WARN_ON(udev->pm_usage_cnt < 0); if (inc_usage_cnt >= 0 && udev->pm_usage_cnt > 0) { - udev->auto_pm = 1; - status = usb_resume_both(udev); + if (udev->state == USB_STATE_SUSPENDED) + status = usb_resume_both(udev); if (status != 0) udev->pm_usage_cnt -= inc_usage_cnt; - } else if (inc_usage_cnt <= 0 && autosuspend_check(udev) == 0) - queue_delayed_work(ksuspend_usb_wq, &udev->autosuspend, - udev->autosuspend_delay); + else if (inc_usage_cnt) + udev->last_busy = jiffies; + } else if (inc_usage_cnt <= 0 && udev->pm_usage_cnt <= 0) { + if (inc_usage_cnt) + udev->last_busy = jiffies; + status = usb_suspend_both(udev, PMSG_SUSPEND); + } usb_pm_unlock(udev); return status; } +/* usb_autosuspend_work - callback routine to autosuspend a USB device */ +void usb_autosuspend_work(struct work_struct *work) +{ + struct usb_device *udev = + container_of(work, struct usb_device, autosuspend.work); + + usb_autopm_do_device(udev, 0); +} + /** * usb_autosuspend_device - delayed autosuspend of a USB device and its interfaces * @udev: the usb_device to autosuspend @@ -1270,15 +1290,20 @@ static int usb_autopm_do_interface(struct usb_interface *intf, if (intf->condition == USB_INTERFACE_UNBOUND) status = -ENODEV; else { + udev->auto_pm = 1; intf->pm_usage_cnt += inc_usage_cnt; if (inc_usage_cnt >= 0 && intf->pm_usage_cnt > 0) { - udev->auto_pm = 1; - status = usb_resume_both(udev); + if (udev->state == USB_STATE_SUSPENDED) + status = usb_resume_both(udev); if (status != 0) intf->pm_usage_cnt -= inc_usage_cnt; - } else if (inc_usage_cnt <= 0 && autosuspend_check(udev) == 0) - queue_delayed_work(ksuspend_usb_wq, &udev->autosuspend, - udev->autosuspend_delay); + else if (inc_usage_cnt) + udev->last_busy = jiffies; + } else if (inc_usage_cnt <= 0 && intf->pm_usage_cnt <= 0) { + if (inc_usage_cnt) + udev->last_busy = jiffies; + status = usb_suspend_both(udev, PMSG_SUSPEND); + } } usb_pm_unlock(udev); return status; @@ -1337,11 +1362,14 @@ EXPORT_SYMBOL_GPL(usb_autopm_put_interface); * or @intf is unbound. A typical example would be a character-device * driver when its device file is opened. * - * The routine increments @intf's usage counter. So long as the counter - * is greater than 0, autosuspend will not be allowed for @intf or its - * usb_device. When the driver is finished using @intf it should call - * usb_autopm_put_interface() to decrement the usage counter and queue - * a delayed autosuspend request (if the counter is <= 0). + * + * The routine increments @intf's usage counter. (However if the + * autoresume fails then the counter is re-decremented.) So long as the + * counter is greater than 0, autosuspend will not be allowed for @intf + * or its usb_device. When the driver is finished using @intf it should + * call usb_autopm_put_interface() to decrement the usage counter and + * queue a delayed autosuspend request (if the counter is <= 0). + * * * Note that @intf->pm_usage_cnt is owned by the interface driver. The * core will not change its value other than the increment and decrement diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c index 8bc3ce6d9666..40cf882293e6 100644 --- a/drivers/usb/core/hcd.c +++ b/drivers/usb/core/hcd.c @@ -1306,6 +1306,7 @@ static void hcd_resume_work(struct work_struct *work) struct usb_device *udev = hcd->self.root_hub; usb_lock_device(udev); + usb_mark_last_busy(udev); usb_external_resume_device(udev); usb_unlock_device(udev); } diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index 2a0b15e42bc7..bde29ab2b504 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -1859,6 +1859,7 @@ static int remote_wakeup(struct usb_device *udev) usb_lock_device(udev); if (udev->state == USB_STATE_SUSPENDED) { dev_dbg(&udev->dev, "usb %sresume\n", "wakeup-"); + usb_mark_last_busy(udev); status = usb_external_resume_device(udev); } usb_unlock_device(udev); diff --git a/include/linux/usb.h b/include/linux/usb.h index f9e4445d5b53..cfbd2bb8fa2c 100644 --- a/include/linux/usb.h +++ b/include/linux/usb.h @@ -398,6 +398,7 @@ struct usb_device { struct delayed_work autosuspend; /* for delayed autosuspends */ struct mutex pm_mutex; /* protects PM operations */ + unsigned long last_busy; /* time of last use */ int autosuspend_delay; /* in jiffies */ unsigned auto_pm:1; /* autosuspend/resume in progress */ @@ -443,6 +444,11 @@ static inline void usb_autopm_disable(struct usb_interface *intf) usb_autopm_set_interface(intf); } +static inline void usb_mark_last_busy(struct usb_device *udev) +{ + udev->last_busy = jiffies; +} + #else static inline int usb_autopm_set_interface(struct usb_interface *intf) @@ -457,6 +463,8 @@ static inline void usb_autopm_enable(struct usb_interface *intf) { } static inline void usb_autopm_disable(struct usb_interface *intf) { } +static inline void usb_mark_last_busy(struct usb_device *udev) +{ } #endif /*-------------------------------------------------------------------------*/ -- cgit v1.2.3