Hi Alan,
I haven't had a chance to look this over too closely, but I really
appreciate the hcd->state cleanup. I was actually just going to post
the updated USB 3.0 hub/split roothub patches today, so I'll have to
respin them on top of this patch later.
Sarah Sharp
On Mon, Mar 07, 2011 at 11:11:52AM -0500, Alan Stern wrote:
> The hcd->state variable is a disaster. It's not clearly owned by
> either usbcore or the host controller drivers, and they both change it
> from time to time, potentially stepping on each other's toes. It's
> not protected by any locks. And there's no mechanism to prevent it
> from going through an invalid transition.
>
> This patch (as1451) takes a first step toward fixing these problems.
> As it turns out, usbcore uses hcd->state for essentially only two
> things: checking whether the controller's root hub is running and
> checking whether the controller has died. Therefore the patch adds
> two new atomic bitflags to the hcd structure, to store these pieces of
> information. The new flags are used only by usbcore, and a private
> spinlock prevents invalid combinations (a dead controller's root hub
> cannot be running).
>
> The patch does not change the places where usbcore sets hcd->state,
> since HCDs may depend on them. Furthermore, there is one place in
> usb_hcd_irq() where usbcore still must use hcd->state: An HCD's
> interrupt handler can implicitly indicate that the controller died by
> setting hcd->state to HC_STATE_HALT. Nevertheless, the new code is a
> big improvement over the current code.
>
> The patch makes one other change. The hcd_bus_suspend() and
> hcd_bus_resume() routines now check first whether the host controller
> has died; if it has then they return immediately without calling the
> HCD's bus_suspend or bus_resume methods.
>
> This fixes the major problem reported in Bugzilla #29902: The system
> fails to suspend after a host controller dies during system resume.
>
> Signed-off-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> Tested-by: Alex Terekhov <a.terekhov@xxxxxxxxx>
> CC: <stable@xxxxxxxxxx>
>
> ---
>
> drivers/usb/core/hcd-pci.c | 13 ++++------
> drivers/usb/core/hcd.c | 55 +++++++++++++++++++++++++++++++++------------
> include/linux/usb/hcd.h | 4 +++
> 3 files changed, 51 insertions(+), 21 deletions(-)
>
> Index: usb-2.6/include/linux/usb/hcd.h
> ===================================================================
> --- usb-2.6.orig/include/linux/usb/hcd.h
> +++ usb-2.6/include/linux/usb/hcd.h
> @@ -99,6 +99,8 @@ struct usb_hcd {
> #define HCD_FLAG_POLL_RH 2 /* poll for rh status? */
> #define HCD_FLAG_POLL_PENDING 3 /* status has changed? */
> #define HCD_FLAG_WAKEUP_PENDING 4 /* root hub is resuming? */
> +#define HCD_FLAG_RH_RUNNING 5 /* root hub is running? */
> +#define HCD_FLAG_DEAD 6 /* controller has died? */
>
> /* The flags can be tested using these macros; they are likely to
> * be slightly faster than test_bit().
> @@ -108,6 +110,8 @@ struct usb_hcd {
> #define HCD_POLL_RH(hcd) ((hcd)->flags & (1U << HCD_FLAG_POLL_RH))
> #define HCD_POLL_PENDING(hcd) ((hcd)->flags & (1U << HCD_FLAG_POLL_PENDING))
> #define HCD_WAKEUP_PENDING(hcd) ((hcd)->flags & (1U << HCD_FLAG_WAKEUP_PENDING))
> +#define HCD_RH_RUNNING(hcd) ((hcd)->flags & (1U << HCD_FLAG_RH_RUNNING))
> +#define HCD_DEAD(hcd) ((hcd)->flags & (1U << HCD_FLAG_DEAD))
>
> /* Flags that get set only during HCD registration or removal. */
> unsigned rh_registered:1;/* is root hub registered? */
> Index: usb-2.6/drivers/usb/core/hcd.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/core/hcd.c
> +++ usb-2.6/drivers/usb/core/hcd.c
> @@ -983,7 +983,7 @@ static int register_root_hub(struct usb_
> spin_unlock_irq (&hcd_root_hub_lock);
>
> /* Did the HC die before the root hub was registered? */
> - if (hcd->state == HC_STATE_HALT)
> + if (HCD_DEAD(hcd) || hcd->state == HC_STATE_HALT)
> usb_hc_died (hcd); /* This time clean up */
> }
>
> @@ -1089,13 +1089,10 @@ int usb_hcd_link_urb_to_ep(struct usb_hc
> * Check the host controller's state and add the URB to the
> * endpoint's queue.
> */
> - switch (hcd->state) {
> - case HC_STATE_RUNNING:
> - case HC_STATE_RESUMING:
> + if (HCD_RH_RUNNING(hcd)) {
> urb->unlinked = 0;
> list_add_tail(&urb->urb_list, &urb->ep->urb_list);
> - break;
> - default:
> + } else {
> rc = -ESHUTDOWN;
> goto done;
> }
> @@ -1913,7 +1910,7 @@ int usb_hcd_get_frame_number (struct usb
> {
> struct usb_hcd *hcd = bus_to_hcd(udev->bus);
>
> - if (!HC_IS_RUNNING (hcd->state))
> + if (!HCD_RH_RUNNING(hcd))
> return -ESHUTDOWN;
> return hcd->driver->get_frame_number (hcd);
> }
> @@ -1930,9 +1927,15 @@ int hcd_bus_suspend(struct usb_device *r
>
> dev_dbg(&rhdev->dev, "bus %s%s\n",
> (msg.event & PM_EVENT_AUTO ? "auto-" : ""), "suspend");
> + if (HCD_DEAD(hcd)) {
> + dev_dbg(&rhdev->dev, "skipped %s of dead bus\n", "suspend");
> + return 0;
> + }
> +
> if (!hcd->driver->bus_suspend) {
> status = -ENOENT;
> } else {
> + clear_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
> hcd->state = HC_STATE_QUIESCING;
> status = hcd->driver->bus_suspend(hcd);
> }
> @@ -1940,7 +1943,12 @@ int hcd_bus_suspend(struct usb_device *r
> usb_set_device_state(rhdev, USB_STATE_SUSPENDED);
> hcd->state = HC_STATE_SUSPENDED;
> } else {
> - hcd->state = old_state;
> + spin_lock_irq(&hcd_root_hub_lock);
> + if (!HCD_DEAD(hcd)) {
> + set_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
> + hcd->state = old_state;
> + }
> + spin_unlock_irq(&hcd_root_hub_lock);
> dev_dbg(&rhdev->dev, "bus %s fail, err %d\n",
> "suspend", status);
> }
> @@ -1955,9 +1963,13 @@ int hcd_bus_resume(struct usb_device *rh
>
> dev_dbg(&rhdev->dev, "usb %s%s\n",
> (msg.event & PM_EVENT_AUTO ? "auto-" : ""), "resume");
> + if (HCD_DEAD(hcd)) {
> + dev_dbg(&rhdev->dev, "skipped %s of dead bus\n", "resume");
> + return 0;
> + }
> if (!hcd->driver->bus_resume)
> return -ENOENT;
> - if (hcd->state == HC_STATE_RUNNING)
> + if (HCD_RH_RUNNING(hcd))
> return 0;
>
> hcd->state = HC_STATE_RESUMING;
> @@ -1966,10 +1978,15 @@ int hcd_bus_resume(struct usb_device *rh
> if (status == 0) {
> /* TRSMRCY = 10 msec */
> msleep(10);
> - usb_set_device_state(rhdev, rhdev->actconfig
> - ? USB_STATE_CONFIGURED
> - : USB_STATE_ADDRESS);
> - hcd->state = HC_STATE_RUNNING;
> + spin_lock_irq(&hcd_root_hub_lock);
> + if (!HCD_DEAD(hcd)) {
> + usb_set_device_state(rhdev, rhdev->actconfig
> + ? USB_STATE_CONFIGURED
> + : USB_STATE_ADDRESS);
> + set_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
> + hcd->state = HC_STATE_RUNNING;
> + }
> + spin_unlock_irq(&hcd_root_hub_lock);
> } else {
> hcd->state = old_state;
> dev_dbg(&rhdev->dev, "bus %s fail, err %d\n",
> @@ -2080,7 +2097,7 @@ irqreturn_t usb_hcd_irq (int irq, void *
> */
> local_irq_save(flags);
>
> - if (unlikely(hcd->state == HC_STATE_HALT || !HCD_HW_ACCESSIBLE(hcd))) {
> + if (unlikely(HCD_DEAD(hcd) || !HCD_HW_ACCESSIBLE(hcd))) {
> rc = IRQ_NONE;
> } else if (hcd->driver->irq(hcd) == IRQ_NONE) {
> rc = IRQ_NONE;
> @@ -2114,6 +2131,8 @@ void usb_hc_died (struct usb_hcd *hcd)
> dev_err (hcd->self.controller, "HC died; cleaning up\n");
>
> spin_lock_irqsave (&hcd_root_hub_lock, flags);
> + clear_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
> + set_bit(HCD_FLAG_DEAD, &hcd->flags);
> if (hcd->rh_registered) {
> clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
>
> @@ -2256,6 +2275,12 @@ int usb_add_hcd(struct usb_hcd *hcd,
> */
> device_init_wakeup(&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,
> + * let's initialize the flag before touching the hardware.
> + */
> + set_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
> +
> /* "reset" is misnamed; its role is now one-time init. the controller
> * should already have been reset (and boot firmware kicked off etc).
> */
> @@ -2323,6 +2348,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
> return retval;
>
> error_create_attr_group:
> + clear_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
> if (HC_IS_RUNNING(hcd->state))
> hcd->state = HC_STATE_QUIESCING;
> spin_lock_irq(&hcd_root_hub_lock);
> @@ -2375,6 +2401,7 @@ void usb_remove_hcd(struct usb_hcd *hcd)
> usb_get_dev(rhdev);
> sysfs_remove_group(&rhdev->dev.kobj, &usb_bus_attr_group);
>
> + clear_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
> if (HC_IS_RUNNING (hcd->state))
> hcd->state = HC_STATE_QUIESCING;
>
> Index: usb-2.6/drivers/usb/core/hcd-pci.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/core/hcd-pci.c
> +++ usb-2.6/drivers/usb/core/hcd-pci.c
> @@ -363,8 +363,7 @@ static int check_root_hub_suspended(stru
> struct pci_dev *pci_dev = to_pci_dev(dev);
> struct usb_hcd *hcd = pci_get_drvdata(pci_dev);
>
> - if (!(hcd->state == HC_STATE_SUSPENDED ||
> - hcd->state == HC_STATE_HALT)) {
> + if (HCD_RH_RUNNING(hcd)) {
> dev_warn(dev, "Root hub is not suspended\n");
> return -EBUSY;
> }
> @@ -386,7 +385,7 @@ static int suspend_common(struct device
> if (retval)
> return retval;
>
> - if (hcd->driver->pci_suspend) {
> + if (hcd->driver->pci_suspend && !HCD_DEAD(hcd)) {
> /* Optimization: Don't suspend if a root-hub wakeup is
> * pending and it would cause the HCD to wake up anyway.
> */
> @@ -427,7 +426,7 @@ static int resume_common(struct device *
> struct usb_hcd *hcd = pci_get_drvdata(pci_dev);
> int retval;
>
> - if (hcd->state != HC_STATE_SUSPENDED) {
> + if (HCD_RH_RUNNING(hcd)) {
> dev_dbg(dev, "can't resume, not suspended!\n");
> return 0;
> }
> @@ -442,7 +441,7 @@ static int resume_common(struct device *
>
> clear_bit(HCD_FLAG_SAW_IRQ, &hcd->flags);
>
> - if (hcd->driver->pci_resume) {
> + if (hcd->driver->pci_resume && !HCD_DEAD(hcd)) {
> if (event != PM_EVENT_AUTO_RESUME)
> wait_for_companions(pci_dev, hcd);
>
> @@ -475,10 +474,10 @@ static int hcd_pci_suspend_noirq(struct
>
> pci_save_state(pci_dev);
>
> - /* If the root hub is HALTed rather than SUSPENDed,
> + /* If the root hub is dead rather than suspended,
> * disallow remote wakeup.
> */
> - if (hcd->state == HC_STATE_HALT)
> + if (HCD_DEAD(hcd))
> device_set_wakeup_enable(dev, 0);
> dev_dbg(dev, "wakeup: %d\n", device_may_wakeup(dev));
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
