Alan, Thanks you for your quick response. Is it possible that the watchdog be scheduled as a result of some activity on the device side. I'm not sure if there is a race condition when the host is trying to suspend the bus while the device is sending something. Thanks, Fei -----Original Message----- From: Alan Stern [mailto:stern@xxxxxxxxxxxxxxxxxxx] Sent: Monday, June 29, 2009 10:48 AM To: Yang Fei-AFY095 Cc: linux-usb@xxxxxxxxxxxxxxx; Gadiyar, Anand; Pandita, Vikram; Ghongdemath, Girish Subject: Re: FW: ehci->watchdog timer panic 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
