Skip to contentSkip to conversation

[Fwd: patch usb-ehci-ensure-all-watchdog-timer-events-are-deleted-when-suspending-usb.patch added to gregkh-2.6 tree]

PATCH Jon Hunter 2 participants 3 messages Aug 14, 2009

Subsystems (2)
lore
Share & tools
lore

Jon Hunter

author Applied
#0 lore raw

Hi All,

I just thought I would send out this patch as an FYI. We recently ran into an issue where we were getting a kernel panic in the ehci driver when entering suspend on the omap3. We discussed this with the linux-usb folks and they recommended the below fix. This problem is hard to reproduce, but has not been seen since adding this fix. Anyway the details are below.

Cheers
Jon

-------- Original Message --------
Subject: patch usb-ehci-ensure-all-watchdog-timer-events-are-deleted-when-suspending-usb.patch added to gregkh-2.6 tree
Date: Fri, 14 Aug 2009 11:48:45 -0500
From: [email protected] <[email protected]>
To: Hunter, Jon <[email protected]>, [email protected] <[email protected]>, [email protected] <[email protected]>, [email protected] <[email protected]>, [email protected] <[email protected]>

This is a note to let you know that I've just added the patch titled

Subject: USB: EHCI: ensure all watchdog timer events are deleted
when suspending usb

to my gregkh-2.6 tree. Its filename is

usb-ehci-ensure-all-watchdog-timer-events-are-deleted-when-suspending-usb.patch

This tree can be found at

http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/

From [email protected] Thu Aug 13 16:37:16 2009
From: Jon Hunter <[email protected]>
Date: Wed, 12 Aug 2009 11:57:59 -0400 (EDT)
Subject: USB: EHCI: ensure all watchdog timer events are deleted when suspending usb
To: Greg KH <[email protected]>, Jon Hunter <[email protected]>
Cc: Fei Yang <[email protected]>
Message-ID: <[email protected]>

From: Jon Hunter <[email protected]>

This patch was previously discussed in the following thread:
http://thread.gmane.org/gmane.linux.usb.general/19472/focus=19484

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.

To avoid this panic, per recommendation from Alan Stern (see above thread), we make sure any pending timer events (that may have been scheduled by calling ehci_work within the ehci_bus_suspend() function) are deleted before returning.

---
 drivers/usb/host/ehci-hub.c |    5 +++++
 1 file changed, 5 insertions(+)
drivers/usb/host/ehci-hub.c +5 -0
@@ -209,6 +209,11 @@ static int ehci_bus_suspend (struct usb_
 
 	ehci->next_statechange = jiffies + msecs_to_jiffies(10);
 	spin_unlock_irq (&ehci->lock);
+
+	/* ehci_work() may have re-enabled the watchdog timer, which we do not
+	 * want, and so we must delete any pending watchdog timer events.
+	 */
+	del_timer_sync(&ehci->watchdog);
 	return 0;
 }
 
#1 lore raw
Quoted 9 lines · Jon Hunter

Hi All,

I just thought I would send out this patch as an FYI. We recently ran into an issue where we were getting a kernel panic in the ehci driver when entering suspend on the omap3. We discussed this with the linux-usb folks and they recommended the below fix. This problem is hard to reproduce, but has not been seen since adding this fix. Anyway the details are below.

Jon,

Do you know if this will be queued as a fix for .31? or if it will wait for the .32 merge window.

Kevin

Quoted 89 lines

-------- Original Message --------
Subject: patch usb-ehci-ensure-all-watchdog-timer-events-are-deleted-when-suspending-usb.patch added to gregkh-2.6 tree
Date: Fri, 14 Aug 2009 11:48:45 -0500
From: [email protected] <[email protected]>
To: Hunter, Jon <[email protected]>, [email protected] <[email protected]>, [email protected] <[email protected]>,
[email protected] <[email protected]>, [email protected]
<[email protected]>

This is a note to let you know that I've just added the patch titled

Subject: USB: EHCI: ensure all watchdog timer events are deleted
when suspending usb

to my gregkh-2.6 tree. Its filename is

usb-ehci-ensure-all-watchdog-timer-events-are-deleted-when-suspending-usb.patch

This tree can be found at

http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/

From [email protected] Thu Aug 13 16:37:16 2009
From: Jon Hunter <[email protected]>
Date: Wed, 12 Aug 2009 11:57:59 -0400 (EDT)
Subject: USB: EHCI: ensure all watchdog timer events are deleted when suspending usb
To: Greg KH <[email protected]>, Jon Hunter <[email protected]>
Cc: Fei Yang <[email protected]>
Message-ID: <[email protected]>

From: Jon Hunter <[email protected]>

This patch was previously discussed in the following thread:
http://thread.gmane.org/gmane.linux.usb.general/19472/focus=19484

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.

To avoid this panic, per recommendation from Alan Stern (see above thread), we
make sure any pending timer events (that may have been scheduled by calling ehci_work within the ehci_bus_suspend() function) are deleted before returning.

---
 drivers/usb/host/ehci-hub.c |    5 +++++
 1 file changed, 5 insertions(+)
drivers/usb/host/ehci-hub.c +5 -1
@@ -209,6 +209,11 @@ static int ehci_bus_suspend (struct usb_
 
 	ehci->next_statechange = jiffies + msecs_to_jiffies(10);
 	spin_unlock_irq (&ehci->lock);
+
+	/* ehci_work() may have re-enabled the watchdog timer, which we do not
+	 * want, and so we must delete any pending watchdog timer events.
+	 */
+	del_timer_sync(&ehci->watchdog);
 	return 0;
 }
 
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#2 lore raw
Quoted 6 lines · Kevin Hilman

Jon,

Do you know if this will be queued as a fix for .31? or if it will wait for the .32 merge window.

Kevin

I am not sure. I was just browsing Greg's page and I found the following text file.

http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/series

Here Greg has a list of patches for .31. This patch is not part of what he has planned for .31. The patch is listed under "USB stuff (after 2.6.30 is out)", however, I believe that this is a typo, as this is under the main heading "Stuff to be merged after 2.6.31 is out". So I would presume this would come after .31 in .32 if there are no issues.

Cheers
Jon