On Mon, 29 Jun 2009, Yang Fei-AFY095 wrote:
> Hello All,
>
> I am looking for some advice on the correct method for utlising the
> ehci_bus_suspend() function on an OMAP3 device. On the OMAP3 device the
> usbhost controller is in a separate internal power-domain. So when the
> usbhost is inactive or suspend is called, we can disable clocks and
> power-down the usbhost to save power.
>
> Recently we found that after calling ehci_bus_suspend() and disabling
> the usbhost clocks we would see the ehci watchdog timer event fire. This
> was causing a kernel panic because the usbhost controllers clocks were
> disabled and inside the watchdog timer function the clocks were not
> being re-enabled, so when the ehci registers were accessed this resulted
> in a CPU data-abort.
>
> From reviewing the ehci_bus_suspend() function, I do see that after
> deleting any pending watchdog timer events, there is a called made to
> ehci_work() (see below code snippet). From looking at the ehci_work()
> function it does appear to be possible that another watchdog timer event
> could be scheduled, but I have been unable to confirm this.
ehci_work can enable the watchdog timer.
> Unfortunately, this failure is not easily reproduced.
>
> --- snip ---
>
> static int ehci_bus_suspend (struct usb_hcd *hcd) {
> struct ehci_hcd *ehci = hcd_to_ehci (hcd);
> int port;
> int mask;
>
> ehci_dbg(ehci, "suspend root hub\n");
>
> if (time_before (jiffies, ehci->next_statechange))
> msleep(5);
> del_timer_sync(&ehci->watchdog);
> del_timer_sync(&ehci->iaa_watchdog);
>
> port = HCS_N_PORTS (ehci->hcs_params);
> spin_lock_irq (&ehci->lock);
>
> /* stop schedules, clean any completed work */
> if (HC_IS_RUNNING(hcd->state)) {
> ehci_quiesce (ehci);
> hcd->state = HC_STATE_QUIESCING;
> }
> ehci->command = ehci_readl(ehci, &ehci->regs->command);
> ehci_work(ehci);
>
> --- snip ---
>
> Hence, I wanted to ask the following questions:
>
> 1). Theoretically is it possible that a call to ehci_work() during the
> suspend could cause another watchdog timer event to be scheduled?
In theory, maybe. It's hard to examine all the possible code paths to
tell for sure. But whatever timer event is pending, it can't be
important.
> 2). If there is a watchdog event pending after calling
> ehci_bus_suspend(), should we:
> a). Make the function, ehci_bus_suspend, return an error code -EBUSY to
> indicate there is still work pending? See below.
No.
> b). Make the function calling the ehci_bus_suspend, returns an error to
> indicate that we could not disable the clocks and suspend failed?
No. Simply add another call to del_timer_sync at the very end of
ehci_bus_suspend (after the spin_unlock_irq). Include a comment
explaining that ehci_work may have re-enabled the timer, so it has to
be turned off again.
Alan Stern
--
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
