On Wed, Jul 11, 2012 at 11:23 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> This patch (as1589) resolves some unlikely races involving system
> shutdown or controller death in ehci-hcd:
>
> Shutdown races with both root-hub resume and controller
> resume.
>
> Controller death races with root-hub suspend.
>
> A new bitflag is added to indicate that the controller has been shut
> down (whether for system shutdown or because it died). Tests are
> added in the suspend and resume pathways to avoid reactivating the
> controller after any sort of shutdown.
>
> Signed-off-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
>
> ---
>
> drivers/usb/host/ehci-hcd.c | 20 +++++++++++++++++++-
> drivers/usb/host/ehci-hub.c | 27 +++++++++++++++++++++++----
> drivers/usb/host/ehci.h | 1 +
> 3 files changed, 43 insertions(+), 5 deletions(-)
>
> Index: usb-3.4/drivers/usb/host/ehci.h
> ===================================================================
> --- usb-3.4.orig/drivers/usb/host/ehci.h
> +++ usb-3.4/drivers/usb/host/ehci.h
> @@ -118,6 +118,7 @@ struct ehci_hcd { /* one per controlle
> bool need_rescan:1;
> bool intr_unlinking:1;
> bool async_unlinking:1;
> + bool shutdown:1;
> struct ehci_qh *qh_scan_next;
>
> /* async schedule support */
> Index: usb-3.4/drivers/usb/host/ehci-hcd.c
> ===================================================================
> --- usb-3.4.orig/drivers/usb/host/ehci-hcd.c
> +++ usb-3.4/drivers/usb/host/ehci-hcd.c
> @@ -343,6 +343,7 @@ static void ehci_shutdown(struct usb_hcd
> struct ehci_hcd *ehci = hcd_to_ehci(hcd);
>
> spin_lock_irq(&ehci->lock);
> + ehci->shutdown = true;
> ehci->rh_state = EHCI_RH_STOPPING;
> ehci->enabled_hrtimer_events = 0;
> spin_unlock_irq(&ehci->lock);
> @@ -823,6 +824,7 @@ dead:
> usb_hc_died(hcd);
>
> /* Don't let the controller do anything more */
> + ehci->shutdown = true;
> ehci->rh_state = EHCI_RH_STOPPING;
> ehci->command &= ~(CMD_RUN | CMD_ASE | CMD_PSE);
> ehci_writel(ehci, ehci->command, &ehci->regs->command);
> @@ -1129,6 +1131,9 @@ static int __maybe_unused ehci_resume(st
> /* Mark hardware accessible again as we are back to full power by now */
> set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
>
> + if (ehci->shutdown)
> + return 0; /* Controller is dead */
> +
> /*
> * If CF is still set and we aren't resuming from hibernation
> * then we maintained suspend power.
> @@ -1139,10 +1144,17 @@ static int __maybe_unused ehci_resume(st
> int mask = INTR_MASK;
>
> ehci_prepare_ports_for_controller_resume(ehci);
> +
> + spin_lock_irq(&ehci->lock);
> + if (ehci->shutdown)
> + goto skip;
> +
> if (!hcd->self.root_hub->do_remote_wakeup)
> mask &= ~STS_PCD;
> ehci_writel(ehci, mask, &ehci->regs->intr_enable);
> ehci_readl(ehci, &ehci->regs->intr_enable);
> + skip:
> + spin_unlock_irq(&ehci->lock);
> return 0;
> }
>
> @@ -1154,14 +1166,20 @@ static int __maybe_unused ehci_resume(st
> (void) ehci_halt(ehci);
> (void) ehci_reset(ehci);
>
> + spin_lock_irq(&ehci->lock);
> + if (ehci->shutdown)
> + goto skip;
> +
> ehci_writel(ehci, ehci->command, &ehci->regs->command);
> ehci_writel(ehci, FLAG_CF, &ehci->regs->configured_flag);
> ehci_readl(ehci, &ehci->regs->command); /* unblock posted writes */
>
> + ehci->rh_state = EHCI_RH_SUSPENDED;
> + spin_unlock_irq(&ehci->lock);
> +
> /* here we "know" root ports should always stay powered */
> ehci_port_power(ehci, 1);
>
> - ehci->rh_state = EHCI_RH_SUSPENDED;
> return 1;
> }
>
> Index: usb-3.4/drivers/usb/host/ehci-hub.c
> ===================================================================
> --- usb-3.4.orig/drivers/usb/host/ehci-hub.c
> +++ usb-3.4/drivers/usb/host/ehci-hub.c
> @@ -221,6 +221,8 @@ static int ehci_bus_suspend (struct usb_
> ehci_quiesce(ehci);
>
> spin_lock_irq (&ehci->lock);
> + if (ehci->rh_state < EHCI_RH_RUNNING)
> + goto done;
>
> /* Once the controller is stopped, port resumes that are already
> * in progress won't complete. Hence if remote wakeup is enabled
> @@ -306,6 +308,10 @@ static int ehci_bus_suspend (struct usb_
> ehci_halt (ehci);
>
> spin_lock_irq(&ehci->lock);
> + if (ehci->enabled_hrtimer_events & BIT(EHCI_HRTIMER_POLL_DEAD))
> + ehci_handle_controller_death(ehci);
I am wondering that why the above two lines are added, since ehci_halt has been
completed, and the hrtimer will be canceled soon.
> + if (ehci->rh_state != EHCI_RH_RUNNING)
> + goto done;
> ehci->rh_state = EHCI_RH_SUSPENDED;
>
> end_unlink_async(ehci);
> @@ -320,6 +326,7 @@ static int ehci_bus_suspend (struct usb_
> ehci_writel(ehci, mask, &ehci->regs->intr_enable);
> ehci_readl(ehci, &ehci->regs->intr_enable);
>
> + done:
> ehci->next_statechange = jiffies + msecs_to_jiffies(10);
> ehci->enabled_hrtimer_events = 0;
> ehci->next_hrtimer_event = EHCI_HRTIMER_NO_EVENT;
> @@ -342,10 +349,8 @@ static int ehci_bus_resume (struct usb_h
> if (time_before (jiffies, ehci->next_statechange))
> msleep(5);
> spin_lock_irq (&ehci->lock);
> - if (!HCD_HW_ACCESSIBLE(hcd)) {
> - spin_unlock_irq(&ehci->lock);
> - return -ESHUTDOWN;
> - }
> + if (!HCD_HW_ACCESSIBLE(hcd) || ehci->shutdown)
> + goto shutdown;
>
> if (unlikely(ehci->debug)) {
> if (!dbgp_reset_prep())
> @@ -384,6 +389,8 @@ static int ehci_bus_resume (struct usb_h
> spin_unlock_irq(&ehci->lock);
> msleep(8);
> spin_lock_irq(&ehci->lock);
> + if (ehci->shutdown)
> + goto shutdown;
>
> /* clear phy low-power mode before resume */
> if (ehci->bus_suspended && ehci->has_hostpc) {
> @@ -401,6 +408,8 @@ static int ehci_bus_resume (struct usb_h
> spin_unlock_irq(&ehci->lock);
> msleep(5);
> spin_lock_irq(&ehci->lock);
> + if (ehci->shutdown)
> + goto shutdown;
> }
>
> /* manually resume the ports we suspended during bus_suspend() */
> @@ -421,6 +430,8 @@ static int ehci_bus_resume (struct usb_h
> spin_unlock_irq(&ehci->lock);
> msleep(20);
> spin_lock_irq(&ehci->lock);
> + if (ehci->shutdown)
> + goto shutdown;
> }
>
> i = HCS_N_PORTS (ehci->hcs_params);
> @@ -439,10 +450,18 @@ static int ehci_bus_resume (struct usb_h
> ehci_handover_companion_ports(ehci);
>
> /* Now we can safely re-enable irqs */
> + spin_lock_irq(&ehci->lock);
> + if (ehci->shutdown)
> + goto shutdown;
> ehci_writel(ehci, INTR_MASK, &ehci->regs->intr_enable);
> (void) ehci_readl(ehci, &ehci->regs->intr_enable);
> + spin_unlock_irq(&ehci->lock);
>
> return 0;
> +
> + shutdown:
> + spin_unlock_irq(&ehci->lock);
> + return -ESHUTDOWN;
> }
>
> #else
>
>
> --
> 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
Thanks,
--
Ming Lei
--
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
