[RFC][PATCH] musb: Avoid musb_gadget_pullup "Unhandled fault" oops on omap4

[RFC][PATCH] musb: Avoid musb_gadget_pullup "Unhandled fault" oops on omap4

[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.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!


Reported-by: Zach Pfeffer <zach.pfeffer@linaro.org>
Signed-off-by: John Stultz <john.stultz@linaro.org>

diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index 6aeb363..548338c 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -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;
 }
 

Re: [RFC][PATCH] musb: Avoid musb_gadget_pullup "Unhandled fault" oops on omap4

[john stultz]

On Wed, 2011-07-20 at 17:09 -0700, John Stultz wrote:
> 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


> Reported-by: Zach Pfeffer <zach.pfeffer@linaro.org>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> 
> diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
> index 6aeb363..548338c 100644
> --- a/drivers/usb/musb/musb_gadget.c
> +++ b/drivers/usb/musb/musb_gadget.c
> @@ -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;
>  }
>  
> 

Re: [RFC][PATCH] musb: Avoid musb_gadget_pullup "Unhandled fault" oops on omap4

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 509 bytes --]

On Mon, Jul 25, 2011 at 08:43:54AM -0700, john stultz wrote:
> On Wed, 2011-07-20 at 17:09 -0700, John Stultz wrote:
> > 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

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [RFC][PATCH] musb: Avoid musb_gadget_pullup "Unhandled fault" oops on omap4

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 466 bytes --]

On Mon, Jul 25, 2011 at 08:43:54AM -0700, john stultz wrote:
> On Wed, 2011-07-20 at 17:09 -0700, John Stultz wrote:
> > 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:

Cc: stable@kernel.org

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [RFC][PATCH] musb: Avoid musb_gadget_pullup "Unhandled fault" oops on omap4

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2426 bytes --]

hi,

On Wed, Jul 20, 2011 at 05:09:34PM -0700, John Stultz wrote:
> 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!
> 
> 
> Reported-by: Zach Pfeffer <zach.pfeffer@linaro.org>
> Signed-off-by: John Stultz <john.stultz@linaro.org>

Applied, thanks.

improved commit log slightly:

commit 7e1bb0fdcc9b51ebec0a1e5e06ff075aab55c941
Author: John Stultz <john.stultz@linaro.org>
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.
    
    [ balbi@ti.com : simplified commit log; removed few things
        which didn't belong there ]
    
    Cc: stable@kernel.org
    Reported-by: Zach Pfeffer <zach.pfeffer@linaro.org>
    Signed-off-by: John Stultz <john.stultz@linaro.org>
    Signed-off-by: Felipe Balbi <balbi@ti.com>

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [RFC][PATCH] musb: Avoid musb_gadget_pullup "Unhandled fault" oops on omap4

[Colin Cross]

On Thu, Jul 28, 2011 at 10:04 PM, Felipe Balbi <balbi@ti.com> wrote:
> hi,
>
> On Wed, Jul 20, 2011 at 05:09:34PM -0700, John Stultz wrote:
>> 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!
>>
>>
>> Reported-by: Zach Pfeffer <zach.pfeffer@linaro.org>
>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>
> Applied, thanks.
>
> improved commit log slightly:
>
> commit 7e1bb0fdcc9b51ebec0a1e5e06ff075aab55c941
> Author: John Stultz <john.stultz@linaro.org>
> 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.
>
>    [ balbi@ti.com : simplified commit log; removed few things
>        which didn't belong there ]
>
>    Cc: stable@kernel.org
>    Reported-by: Zach Pfeffer <zach.pfeffer@linaro.org>
>    Signed-off-by: John Stultz <john.stultz@linaro.org>
>    Signed-off-by: Felipe Balbi <balbi@ti.com>
>
> --
> 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.