USB: EHCI: fix up locking

This patch (as1588) adjusts the locking in ehci-hcd's various halt,
shutdown, and suspend/resume pathways.  We want to hold the spinlock
while writing device registers and accessing shared variables, but not
while polling in a loop.

In addition, there's no need to call ehci_work() at times when no URBs
can be active, i.e., in ehci_stop() and ehci_bus_suspend().

Finally, ehci_adjust_port_wakeup_flags() is called only in situations
where interrupts are enabled; therefore it can use spin_lock_irq
rather than spin_lock_irqsave.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 340c9c4894bf59e296cf32ff24a8eec39918c5eb..ac4c8ddde20a624dadb7f3b0ced3f31aa4d601a4 100644 (file)
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -167,21 +167,24 @@ static int tdi_in_host_mode (struct ehci_hcd *ehci)
        return (tmp & 3) == USBMODE_CM_HC;
 }
 
-/* force HC to halt state from unknown (EHCI spec section 2.3) */
+/*
+ * Force HC to halt state from unknown (EHCI spec section 2.3).
+ * Must be called with interrupts enabled and the lock not held.
+ */
 static int ehci_halt (struct ehci_hcd *ehci)
 {
-       u32     temp = ehci_readl(ehci, &ehci->regs->status);
+       u32     temp;
+
+       spin_lock_irq(&ehci->lock);
 
        /* disable any irqs left enabled by previous code */
        ehci_writel(ehci, 0, &ehci->regs->intr_enable);
 
-       if (ehci_is_TDI(ehci) && tdi_in_host_mode(ehci) == 0) {
+       if (ehci_is_TDI(ehci) && !tdi_in_host_mode(ehci)) {
+               spin_unlock_irq(&ehci->lock);
                return 0;
        }
 
-       if ((temp & STS_HALT) != 0)
-               return 0;
-
        /*
         * This routine gets called during probe before ehci->command
         * has been initialized, so we can't rely on its value.
@@ -190,7 +193,11 @@ static int ehci_halt (struct ehci_hcd *ehci)
        temp = ehci_readl(ehci, &ehci->regs->command);
        temp &= ~(CMD_RUN | CMD_IAAD);
        ehci_writel(ehci, temp, &ehci->regs->command);
-       return handshake (ehci, &ehci->regs->status,
+
+       spin_unlock_irq(&ehci->lock);
+       synchronize_irq(ehci_to_hcd(ehci)->irq);
+
+       return handshake(ehci, &ehci->regs->status,
                          STS_HALT, STS_HALT, 16 * 125);
 }
 
@@ -210,7 +217,10 @@ static void tdi_reset (struct ehci_hcd *ehci)
        ehci_writel(ehci, tmp, &ehci->regs->usbmode);
 }
 
-/* reset a non-running (STS_HALT == 1) controller */
+/*
+ * Reset a non-running (STS_HALT == 1) controller.
+ * Must be called with interrupts enabled and the lock not held.
+ */
 static int ehci_reset (struct ehci_hcd *ehci)
 {
        int     retval;
@@ -248,7 +258,10 @@ static int ehci_reset (struct ehci_hcd *ehci)
        return retval;
 }
 
-/* idle the controller (from running) */
+/*
+ * Idle the controller (turn off the schedules).
+ * Must be called with interrupts enabled and the lock not held.
+ */
 static void ehci_quiesce (struct ehci_hcd *ehci)
 {
        u32     temp;
@@ -261,8 +274,10 @@ static void ehci_quiesce (struct ehci_hcd *ehci)
        handshake(ehci, &ehci->regs->status, STS_ASS | STS_PSS, temp, 16 * 125);
 
        /* then disable anything that's still active */
+       spin_lock_irq(&ehci->lock);
        ehci->command &= ~(CMD_ASE | CMD_PSE);
        ehci_writel(ehci, ehci->command, &ehci->regs->command);
+       spin_unlock_irq(&ehci->lock);
 
        /* hardware can take 16 microframes to turn off ... */
        handshake(ehci, &ehci->regs->status, STS_ASS | STS_PSS, 0, 16 * 125);
@@ -301,11 +316,14 @@ static void ehci_turn_off_all_ports(struct ehci_hcd *ehci)
 
 /*
  * Halt HC, turn off all ports, and let the BIOS use the companion controllers.
- * Should be called with ehci->lock held.
+ * Must be called with interrupts enabled and the lock not held.
  */
 static void ehci_silence_controller(struct ehci_hcd *ehci)
 {
        ehci_halt(ehci);
+
+       spin_lock_irq(&ehci->lock);
+       ehci->rh_state = EHCI_RH_HALTED;
        ehci_turn_off_all_ports(ehci);
 
        /* make BIOS/etc use companion controller during reboot */
@@ -313,6 +331,7 @@ static void ehci_silence_controller(struct ehci_hcd *ehci)
 
        /* unblock posted writes */
        ehci_readl(ehci, &ehci->regs->configured_flag);
+       spin_unlock_irq(&ehci->lock);
 }
 
 /* ehci_shutdown kick in for silicon on any bus (not just pci, etc).
@@ -325,10 +344,11 @@ static void ehci_shutdown(struct usb_hcd *hcd)
 
        spin_lock_irq(&ehci->lock);
        ehci->rh_state = EHCI_RH_STOPPING;
-       ehci_silence_controller(ehci);
        ehci->enabled_hrtimer_events = 0;
        spin_unlock_irq(&ehci->lock);
 
+       ehci_silence_controller(ehci);
+
        hrtimer_cancel(&ehci->hrtimer);
 }
 
@@ -400,11 +420,11 @@ static void ehci_stop (struct usb_hcd *hcd)
 
        spin_lock_irq(&ehci->lock);
        ehci->enabled_hrtimer_events = 0;
-       ehci_quiesce(ehci);
+       spin_unlock_irq(&ehci->lock);
 
+       ehci_quiesce(ehci);
        ehci_silence_controller(ehci);
        ehci_reset (ehci);
-       spin_unlock_irq(&ehci->lock);
 
        hrtimer_cancel(&ehci->hrtimer);
        remove_sysfs_files(ehci);
@@ -412,8 +432,6 @@ static void ehci_stop (struct usb_hcd *hcd)
 
        /* root hub is shut down separately (first, when possible) */
        spin_lock_irq (&ehci->lock);
-       if (ehci->async)
-               ehci_work (ehci);
        end_free_itds(ehci);
        spin_unlock_irq (&ehci->lock);
        ehci_mem_cleanup (ehci);

diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
index 05490d387fd27c86195590f2545207172d9dac53..ffc5f27df725bff5edc12df99bd05098dadff9b0 100644 (file)
--- a/drivers/usb/host/ehci-hub.c
+++ b/drivers/usb/host/ehci-hub.c
@@ -59,6 +59,7 @@ static void ehci_handover_companion_ports(struct ehci_hcd *ehci)
        /* Give the connections some time to appear */
        msleep(20);
 
+       spin_lock_irq(&ehci->lock);
        port = HCS_N_PORTS(ehci->hcs_params);
        while (port--) {
                if (test_bit(port, &ehci->owned_ports)) {
@@ -70,23 +71,30 @@ static void ehci_handover_companion_ports(struct ehci_hcd *ehci)
                                clear_bit(port, &ehci->owned_ports);
                        else if (test_bit(port, &ehci->companion_ports))
                                ehci_writel(ehci, status & ~PORT_PE, reg);
-                       else
+                       else {
+                               spin_unlock_irq(&ehci->lock);
                                ehci_hub_control(hcd, SetPortFeature,
                                                USB_PORT_FEAT_RESET, port + 1,
                                                NULL, 0);
+                               spin_lock_irq(&ehci->lock);
+                       }
                }
        }
+       spin_unlock_irq(&ehci->lock);
 
        if (!ehci->owned_ports)
                return;
        msleep(90);             /* Wait for resets to complete */
 
+       spin_lock_irq(&ehci->lock);
        port = HCS_N_PORTS(ehci->hcs_params);
        while (port--) {
                if (test_bit(port, &ehci->owned_ports)) {
+                       spin_unlock_irq(&ehci->lock);
                        ehci_hub_control(hcd, GetPortStatus,
                                        0, port + 1,
                                        (char *) &buf, sizeof(buf));
+                       spin_lock_irq(&ehci->lock);
 
                        /* The companion should now own the port,
                         * but if something went wrong the port must not
@@ -105,6 +113,7 @@ static void ehci_handover_companion_ports(struct ehci_hcd *ehci)
        }
 
        ehci->owned_ports = 0;
+       spin_unlock_irq(&ehci->lock);
 }
 
 static int ehci_port_change(struct ehci_hcd *ehci)
@@ -133,7 +142,6 @@ static void ehci_adjust_port_wakeup_flags(struct ehci_hcd *ehci,
 {
        int             port;
        u32             temp;
-       unsigned long   flags;
 
        /* If remote wakeup is enabled for the root hub but disabled
         * for the controller, we must adjust all the port wakeup flags
@@ -143,7 +151,7 @@ static void ehci_adjust_port_wakeup_flags(struct ehci_hcd *ehci,
        if (!ehci_to_hcd(ehci)->self.root_hub->do_remote_wakeup || do_wakeup)
                return;
 
-       spin_lock_irqsave(&ehci->lock, flags);
+       spin_lock_irq(&ehci->lock);
 
        /* clear phy low-power mode before changing wakeup flags */
        if (ehci->has_hostpc) {
@@ -154,9 +162,9 @@ static void ehci_adjust_port_wakeup_flags(struct ehci_hcd *ehci,
                        temp = ehci_readl(ehci, hostpc_reg);
                        ehci_writel(ehci, temp & ~HOSTPC_PHCD, hostpc_reg);
                }
-               spin_unlock_irqrestore(&ehci->lock, flags);
+               spin_unlock_irq(&ehci->lock);
                msleep(5);
-               spin_lock_irqsave(&ehci->lock, flags);
+               spin_lock_irq(&ehci->lock);
        }
 
        port = HCS_N_PORTS(ehci->hcs_params);
@@ -194,7 +202,7 @@ static void ehci_adjust_port_wakeup_flags(struct ehci_hcd *ehci,
        if (!suspending && ehci_port_change(ehci))
                usb_hcd_resume_root_hub(ehci_to_hcd(ehci));
 
-       spin_unlock_irqrestore(&ehci->lock, flags);
+       spin_unlock_irq(&ehci->lock);
 }
 
 static int ehci_bus_suspend (struct usb_hcd *hcd)
@@ -209,6 +217,9 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)
        if (time_before (jiffies, ehci->next_statechange))
                msleep(5);
 
+       /* stop the schedules */
+       ehci_quiesce(ehci);
+
        spin_lock_irq (&ehci->lock);
 
        /* Once the controller is stopped, port resumes that are already
@@ -224,10 +235,6 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)
                }
        }
 
-       /* stop schedules, clean any completed work */
-       ehci_quiesce(ehci);
-       ehci_work(ehci);
-
        /* Unlike other USB host controller types, EHCI doesn't have
         * any notion of "global" or bus-wide suspend.  The driver has
         * to manually suspend all the active unsuspended ports, and
@@ -289,6 +296,7 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)
                                        "succeeded" : "failed");
                }
        }
+       spin_unlock_irq(&ehci->lock);
 
        /* Apparently some devices need a >= 1-uframe delay here */
        if (ehci->bus_suspended)
@@ -296,6 +304,8 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)
 
        /* turn off now-idle HC */
        ehci_halt (ehci);
+
+       spin_lock_irq(&ehci->lock);
        ehci->rh_state = EHCI_RH_SUSPENDED;
 
        end_unlink_async(ehci);
@@ -424,13 +434,14 @@ static int ehci_bus_resume (struct usb_hcd *hcd)
        }
 
        ehci->next_statechange = jiffies + msecs_to_jiffies(5);
+       spin_unlock_irq(&ehci->lock);
+
+       ehci_handover_companion_ports(ehci);
 
        /* Now we can safely re-enable irqs */
        ehci_writel(ehci, INTR_MASK, &ehci->regs->intr_enable);
        (void) ehci_readl(ehci, &ehci->regs->intr_enable);
 
-       spin_unlock_irq (&ehci->lock);
-       ehci_handover_companion_ports(ehci);
        return 0;
 }
 
@@ -1018,7 +1029,9 @@ static int ehci_hub_control (
                case USB_PORT_FEAT_TEST:
                        if (!selector || selector > 5)
                                goto error;
+                       spin_unlock_irqrestore(&ehci->lock, flags);
                        ehci_quiesce(ehci);
+                       spin_lock_irqsave(&ehci->lock, flags);
 
                        /* Put all enabled ports into suspend */
                        while (ports--) {
@@ -1030,7 +1043,11 @@ static int ehci_hub_control (
                                        ehci_writel(ehci, temp | PORT_SUSPEND,
                                                        sreg);
                        }
+
+                       spin_unlock_irqrestore(&ehci->lock, flags);
                        ehci_halt(ehci);
+                       spin_lock_irqsave(&ehci->lock, flags);
+
                        temp = ehci_readl(ehci, status_reg);
                        temp |= selector << 16;
                        ehci_writel(ehci, temp, status_reg);

diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index f7f3ce3275b89560e31bc97103a0776975e6d420..65360945df78016cf2231e8224061e5a841be0b5 100644 (file)
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -445,12 +445,11 @@ static int controller_suspend(struct device *dev)
        if (time_before(jiffies, ehci->next_statechange))
                msleep(10);
 
-       spin_lock_irqsave(&ehci->lock, flags);
+       ehci_halt(ehci);
 
+       spin_lock_irqsave(&ehci->lock, flags);
        tegra->port_speed = (readl(&hw->port_status[0]) >> 26) & 0x3;
-       ehci_halt(ehci);
        clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
-
        spin_unlock_irqrestore(&ehci->lock, flags);
 
        tegra_ehci_power_down(hcd);