From 9b37596a2e860404503a3f2a6513db60c296bfdc Mon Sep 17 00:00:00 2001 From: Alan Stern Date: Mon, 7 Mar 2011 11:11:52 -0500 Subject: USB: move usbcore away from hcd->state 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 Tested-by: Alex Terekhov CC: Signed-off-by: Greg Kroah-Hartman --- drivers/usb/core/hcd-pci.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) (limited to 'drivers/usb/core/hcd-pci.c') diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c index f71e8e307e0f..d37088591d9a 100644 --- a/drivers/usb/core/hcd-pci.c +++ b/drivers/usb/core/hcd-pci.c @@ -363,8 +363,7 @@ static int check_root_hub_suspended(struct device *dev) 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 *dev, bool do_wakeup) 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 *dev, int event) 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 *dev, int event) 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 device *dev) 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)); -- cgit v1.2.3 From 8766c815607e572556b04075d4545330123c4f27 Mon Sep 17 00:00:00 2001 From: Sarah Sharp Date: Fri, 15 Oct 2010 10:33:48 -0700 Subject: usb: Make usb_hcd_pci_probe labels more descriptive. Make the labels for the goto statements in usb_hcd_pci_probe() describe the cleanup they do, rather than being numbered err[1-4]. This makes it easier to add error handling later. The error handling for this function looks a little fishy, since set_hs_companion() isn't called until the very end of the function, and clear_hs_companion() is called if this function fails earlier than that. But it should be harmless to clear a NULL pointer, so leave the error handling as-is. Signed-off-by: Sarah Sharp --- drivers/usb/core/hcd-pci.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) (limited to 'drivers/usb/core/hcd-pci.c') diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c index d37088591d9a..88d9109b4b45 100644 --- a/drivers/usb/core/hcd-pci.c +++ b/drivers/usb/core/hcd-pci.c @@ -192,13 +192,13 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id) "Found HC with no IRQ. Check BIOS/PCI %s setup!\n", pci_name(dev)); retval = -ENODEV; - goto err1; + goto disable_pci; } hcd = usb_create_hcd(driver, &dev->dev, pci_name(dev)); if (!hcd) { retval = -ENOMEM; - goto err1; + goto disable_pci; } if (driver->flags & HCD_MEMORY) { @@ -209,13 +209,13 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id) driver->description)) { dev_dbg(&dev->dev, "controller already in use\n"); retval = -EBUSY; - goto err2; + goto clear_companion; } hcd->regs = ioremap_nocache(hcd->rsrc_start, hcd->rsrc_len); if (hcd->regs == NULL) { dev_dbg(&dev->dev, "error mapping memory\n"); retval = -EFAULT; - goto err3; + goto release_mem_region; } } else { @@ -236,7 +236,7 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id) if (region == PCI_ROM_RESOURCE) { dev_dbg(&dev->dev, "no i/o regions available\n"); retval = -EBUSY; - goto err2; + goto clear_companion; } } @@ -244,24 +244,24 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id) retval = usb_add_hcd(hcd, dev->irq, IRQF_DISABLED | IRQF_SHARED); if (retval != 0) - goto err4; + goto unmap_registers; set_hs_companion(dev, hcd); if (pci_dev_run_wake(dev)) pm_runtime_put_noidle(&dev->dev); return retval; - err4: +unmap_registers: if (driver->flags & HCD_MEMORY) { iounmap(hcd->regs); - err3: +release_mem_region: release_mem_region(hcd->rsrc_start, hcd->rsrc_len); } else release_region(hcd->rsrc_start, hcd->rsrc_len); - err2: +clear_companion: clear_hs_companion(dev, hcd); usb_put_hcd(hcd); - err1: +disable_pci: pci_disable_device(dev); dev_err(&dev->dev, "init %s fail, %d\n", pci_name(dev), retval); return retval; -- cgit v1.2.3 From ff9d78b36f76687c91c67b9f4c5c33bc888ed2f9 Mon Sep 17 00:00:00 2001 From: Sarah Sharp Date: Thu, 2 Dec 2010 19:10:02 -0800 Subject: USB: Set usb_hcd->state and flags for shared roothubs. The hcd->flags are in a sorry state. Some of them are clearly specific to the particular roothub (HCD_POLL_RH, HCD_POLL_PENDING, and HCD_WAKEUP_PENDING), but some flags are related to PCI device state (HCD_HW_ACCESSIBLE and HCD_SAW_IRQ). This is an issue when one PCI device can have two roothubs that share the same IRQ line and hardware. Make sure to set HCD_FLAG_SAW_IRQ for both roothubs when an interrupt is serviced, or an URB is unlinked without an interrupt. (We can't tell if the host actually serviced an interrupt for a particular bus, but we can tell it serviced some interrupt.) HCD_HW_ACCESSIBLE is set once by usb_add_hcd(), which is set for both roothubs as they are added, so it doesn't need to be modified. HCD_POLL_RH and HCD_POLL_PENDING are only checked by the USB core, and they are never set by the xHCI driver, since the roothub never needs to be polled. The usb_hcd's state field is a similar mess. Sometimes the state applies to the underlying hardware: HC_STATE_HALT, HC_STATE_RUNNING, and HC_STATE_QUIESCING. But sometimes the state refers to the roothub state: HC_STATE_RESUMING and HC_STATE_SUSPENDED. Alan Stern recently made the USB core not rely on the hcd->state variable. Internally, the xHCI driver still checks for HC_STATE_SUSPENDED, so leave that code in. Remove all references to HC_STATE_HALT, since the xHCI driver only sets and doesn't test those variables. We still have to set HC_STATE_RUNNING, since Alan's patch has a bug that means the roothub won't get registered if we don't set that. Alan's patch made the USB core check a different variable when trying to determine whether to suspend a roothub. The xHCI host has a split roothub, where two buses are registered for one PCI device. Each bus in the xHCI split roothub can be suspended separately, but both buses must be suspended before the PCI device can be suspended. Therefore, make sure that the USB core checks HCD_RH_RUNNING() for both roothubs before suspending the PCI host. Signed-off-by: Sarah Sharp --- drivers/usb/core/hcd-pci.c | 27 +++++++++++++++++++++++---- drivers/usb/core/hcd.c | 4 ++++ drivers/usb/host/xhci-ring.c | 2 ++ 3 files changed, 29 insertions(+), 4 deletions(-) (limited to 'drivers/usb/core/hcd-pci.c') diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c index 88d9109b4b45..b992a886f05c 100644 --- a/drivers/usb/core/hcd-pci.c +++ b/drivers/usb/core/hcd-pci.c @@ -367,6 +367,13 @@ static int check_root_hub_suspended(struct device *dev) dev_warn(dev, "Root hub is not suspended\n"); return -EBUSY; } + if (hcd->shared_hcd) { + hcd = hcd->shared_hcd; + if (HCD_RH_RUNNING(hcd)) { + dev_warn(dev, "Secondary root hub is not suspended\n"); + return -EBUSY; + } + } return 0; } @@ -391,11 +398,16 @@ static int suspend_common(struct device *dev, bool do_wakeup) */ if (do_wakeup && HCD_WAKEUP_PENDING(hcd)) return -EBUSY; + if (do_wakeup && hcd->shared_hcd && + HCD_WAKEUP_PENDING(hcd->shared_hcd)) + return -EBUSY; retval = hcd->driver->pci_suspend(hcd, do_wakeup); suspend_report_result(hcd->driver->pci_suspend, retval); /* Check again in case wakeup raced with pci_suspend */ - if (retval == 0 && do_wakeup && HCD_WAKEUP_PENDING(hcd)) { + if ((retval == 0 && do_wakeup && HCD_WAKEUP_PENDING(hcd)) || + (retval == 0 && do_wakeup && hcd->shared_hcd && + HCD_WAKEUP_PENDING(hcd->shared_hcd))) { if (hcd->driver->pci_resume) hcd->driver->pci_resume(hcd, false); retval = -EBUSY; @@ -426,7 +438,9 @@ static int resume_common(struct device *dev, int event) struct usb_hcd *hcd = pci_get_drvdata(pci_dev); int retval; - if (HCD_RH_RUNNING(hcd)) { + if (HCD_RH_RUNNING(hcd) || + (hcd->shared_hcd && + HCD_RH_RUNNING(hcd->shared_hcd))) { dev_dbg(dev, "can't resume, not suspended!\n"); return 0; } @@ -440,6 +454,8 @@ static int resume_common(struct device *dev, int event) pci_set_master(pci_dev); clear_bit(HCD_FLAG_SAW_IRQ, &hcd->flags); + if (hcd->shared_hcd) + clear_bit(HCD_FLAG_SAW_IRQ, &hcd->shared_hcd->flags); if (hcd->driver->pci_resume && !HCD_DEAD(hcd)) { if (event != PM_EVENT_AUTO_RESUME) @@ -449,6 +465,8 @@ static int resume_common(struct device *dev, int event) event == PM_EVENT_RESTORE); if (retval) { dev_err(dev, "PCI post-resume error %d!\n", retval); + if (hcd->shared_hcd) + usb_hc_died(hcd->shared_hcd); usb_hc_died(hcd); } } @@ -474,8 +492,9 @@ static int hcd_pci_suspend_noirq(struct device *dev) pci_save_state(pci_dev); - /* If the root hub is dead rather than suspended, - * disallow remote wakeup. + /* If the root hub is dead rather than suspended, disallow remote + * wakeup. usb_hc_died() should ensure that both hosts are marked as + * dying, so we only need to check the primary roothub. */ if (HCD_DEAD(hcd)) device_set_wakeup_enable(dev, 0); diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c index ba15eeab824e..02b4dbfa488a 100644 --- a/drivers/usb/core/hcd.c +++ b/drivers/usb/core/hcd.c @@ -1153,6 +1153,8 @@ int usb_hcd_check_unlink_urb(struct usb_hcd *hcd, struct urb *urb, dev_warn(hcd->self.controller, "Unlink after no-IRQ? " "Controller is probably using the wrong IRQ.\n"); set_bit(HCD_FLAG_SAW_IRQ, &hcd->flags); + if (hcd->shared_hcd) + set_bit(HCD_FLAG_SAW_IRQ, &hcd->shared_hcd->flags); } return 0; @@ -2124,6 +2126,8 @@ irqreturn_t usb_hcd_irq (int irq, void *__hcd) rc = IRQ_NONE; } else { set_bit(HCD_FLAG_SAW_IRQ, &hcd->flags); + if (hcd->shared_hcd) + set_bit(HCD_FLAG_SAW_IRQ, &hcd->shared_hcd->flags); if (unlikely(hcd->state == HC_STATE_HALT)) usb_hc_died(hcd); diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index 9e2b26c3da40..47763bed378a 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -2181,6 +2181,8 @@ irqreturn_t xhci_msi_irq(int irq, struct usb_hcd *hcd) irqreturn_t ret; set_bit(HCD_FLAG_SAW_IRQ, &hcd->flags); + if (hcd->shared_hcd) + set_bit(HCD_FLAG_SAW_IRQ, &hcd->shared_hcd->flags); ret = xhci_irq(hcd); -- cgit v1.2.3