musb: Avoid musb_gadget_pullup "Unhandled fault" oops on omap4
Subsystems (2)
RFC PATCH 1/1 musb: Avoid musb_gadget_pullup "Unhandled fault" oops on omap4
I've recently run across an "Unhandled fault: imprecise external abort" oops that is caused when a driver called usb_gadget_connect() when there was no cable plugged into the musb gadget port.
You can see the oops message here:
https://launchpadlibrarian.net/75635123/minicom.txt
Doing some digging, it seemed the problem was triggered when reading from the musb registers in musb_pullup() when the device controller is powered down.
Looking at other examples of where the registers were accessed, I noticed they were always enclosed by pm_runtime_get/put calls. So I added such calls to the musb_gadget_pullup() function and it seemed to resolve the problem.
Now, full disclosure: this was triggered with the out-of-tree Android adb gadget driver. However, I suspect the same behavior could be triggered using the composite gadget driver as well, so I think this is a generic issue. However, if I'm wrong, let me know and I'll try to make sure the fix is done in the right place.
If this is the right fix, it probably should be queued for 3.1 and 3.0-stable.
Comments and feedback would be greatly appreciated!
drivers/usb/musb/musb_gadget.c +5 -0
@@ -1698,6 +1698,8 @@ static int musb_gadget_pullup(struct usb_gadget *gadget, int is_on) is_on = !!is_on; +pm_runtime_get_sync (musb->controller); +/* NOTE: this assumes we are sensing vbus; we'd rather * not pullup unless the B-session is active. */
@@ -1707,6 +1709,9 @@ static int musb_gadget_pullup(struct usb_gadget *gadget, int is_on)musb_pullup (musb, is_on); }spin_unlock_irqrestore (&musb->lock, flags); + +pm_runtime_put (musb->controller); +return 0 ; }
Quoted 3 lines · John Stultz
I've recently run across an "Unhandled fault: imprecise external abort" oops that is caused when a driver called
usb_gadget_connect()when there was no cable plugged into the musb gadget port.
Felipe: Ping? Any thoughts on this patch?
thanks
-john
Quoted 27 lines
Reported-by Zach Pfeffer <[email protected]>Signed-off-by John Stultz <[email protected]>drivers/usb/musb/musb_gadget.c +5
-0@@ -1698,6 +1698,8 @@ static int musb_gadget_pullup(struct usb_gadget *gadget, int is_on) is_on = !!is_on; +pm_runtime_get_sync (musb->controller); +/* NOTE: this assumes we are sensing vbus; we'd rather * not pullup unless the B-session is active. */ @@ -1707,6 +1709,9 @@ static int musb_gadget_pullup(struct usb_gadget *gadget, int is_on)musb_pullup (musb, is_on); }spin_unlock_irqrestore (&musb->lock, flags); + +pm_runtime_put (musb->controller); +return 0 ; }
Quoted 6 lines · john stultz
Quoted 3 lines · John Stultz
I've recently run across an "Unhandled fault: imprecise external abort" oops that is caused when a driver called
usb_gadget_connect()when there was no cable plugged into the musb gadget port.Felipe: Ping? Any thoughts on this patch?
I'll start looking into fixes once linus tags v3.1-rc0 ;-) This is in my queue though, I'll start testing soon :-D
Signature · 1 line
balbi
Quoted 6 lines · john stultz
Quoted 3 lines · John Stultz
I've recently run across an "Unhandled fault: imprecise external abort" oops that is caused when a driver called
usb_gadget_connect()when there was no cable plugged into the musb gadget port.Felipe: Ping? Any thoughts on this patch?
I'll be queueing this one. Can you resend with:
Signature · 1 line
balbi
Felipe Balbi
Appliedhi,
Quoted 22 lines · John Stultz
I've recently run across an "Unhandled fault: imprecise external abort" oops that is caused when a driver called
usb_gadget_connect()when there was no cable plugged into the musb gadget port.You can see the oops message here:
https://launchpadlibrarian.net/75635123/minicom.txtDoing some digging, it seemed the problem was triggered when reading from the musb registers in
musb_pullup()when the device controller is powered down.Looking at other examples of where the registers were accessed, I noticed they were always enclosed by pm_runtime_get/put calls. So I added such calls to the
musb_gadget_pullup()function and it seemed to resolve the problem.Now, full disclosure: this was triggered with the out-of-tree Android adb gadget driver. However, I suspect the same behavior could be triggered using the composite gadget driver as well, so I think this is a generic issue. However, if I'm wrong, let me know and I'll try to make sure the fix is done in the right place.
Quoted 8 lines
If this is the right fix, it probably should be queued for 3.1 and 3.0-stable.
Comments and feedback would be greatly appreciated!
Reported-by Zach Pfeffer <[email protected]>Signed-off-by John Stultz <[email protected]>
Applied, thanks.
improved commit log slightly:
commit 7e1bb0fdcc9b51ebec0a1e5e06ff075aab55c941
Author: John Stultz <[email protected]> Date: Wed Jul 20 17:09:34 2011 -0700
usb: musb: fix oops on musb_gadget_pullup
an 'unhandled fault' is causes when a gadget driver calls usb_gadget_connect() while the USB cable isn't plugged into the OTG port.
the fault is caused by an access to MUSB's memory space while its clock is turned off due to pm_runtime kicking in.
in order to fix the fault, we enclose musb_gadget_pullup() with pm_runtime_get_sync() ... pm_runtime_put() calls to be sure we will always reach that path with clock turned on.
[ [email protected] : simplified commit log; removed few things
which didn't belong there ]
Signature · 1 line
balbi
Quoted 59 lines · Felipe Balbi
hi,
Quoted 22 lines · John Stultz
I've recently run across an "Unhandled fault: imprecise external abort" oops that is caused when a driver called
usb_gadget_connect()when there was no cable plugged into the musb gadget port.You can see the oops message here:
https://launchpadlibrarian.net/75635123/minicom.txtDoing some digging, it seemed the problem was triggered when reading from the musb registers in
musb_pullup()when the device controller is powered down.Looking at other examples of where the registers were accessed, I noticed they were always enclosed by pm_runtime_get/put calls. So I added such calls to the
musb_gadget_pullup()function and it seemed to resolve the problem.Now, full disclosure: this was triggered with the out-of-tree Android adb gadget driver. However, I suspect the same behavior could be triggered using the composite gadget driver as well, so I think this is a generic issue. However, if I'm wrong, let me know and I'll try to make sure the fix is done in the right place.
Quoted 8 lines
If this is the right fix, it probably should be queued for 3.1 and 3.0-stable.
Comments and feedback would be greatly appreciated!
Reported-by Zach Pfeffer <[email protected]>Signed-off-by John Stultz <[email protected]>Applied, thanks.
improved commit log slightly:
commit
7e1bb0fdcc9b51ebec0a1e5e06ff075aab55c941
Author: John Stultz <[email protected]>
Date: Wed Jul 20 17:09:34 2011 -0700usb: musb: fix oops on
musb_gadget_pullupan 'unhandled fault' is causes when a gadget driver calls
usb_gadget_connect()while the USB cable isn't plugged into the OTG port.the fault is caused by an access to MUSB's memory space while its clock is turned off due to
pm_runtimekicking in.in order to fix the fault, we enclose
musb_gadget_pullup()withpm_runtime_get_sync()...pm_runtime_put()calls to be sure we will always reach that path with clock turned on.[ [email protected] : simplified commit log; removed few things which didn't belong there ]
Quoted 8 lines
Reported-by Zach Pfeffer <[email protected]>Signed-off-by John Stultz <[email protected]>Signed-off-by Felipe Balbi <[email protected]>--
balbi
musb_pullup can get called from usb_function_deactivate in atomic context (usb_function_deactivate -> usb_gadget_disconnect -> musb_pullup), and pm_runtime_get_sync is not safe in atomic context unless pm_runtime_irq_safe has been called in musb-core.c (which I'm not sure it is safe to do).
Since this already gets called in atomic context, you might as well put the irq-safe pm_runtime calls inside the if statement, and avoid unnecessary wakeups.