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@xxxxxx> 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@xxxxxxxxxx>
>> Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx>
>
> Applied, thanks.
>
> improved commit log slightly:
>
> commit 7e1bb0fdcc9b51ebec0a1e5e06ff075aab55c941
> Author: John Stultz <john.stultz@xxxxxxxxxx>
> 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@xxxxxx : simplified commit log; removed few things
>        which didn't belong there ]
>
>    Cc: stable@xxxxxxxxxx
>    Reported-by: Zach Pfeffer <zach.pfeffer@xxxxxxxxxx>
>    Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx>
>    Signed-off-by: Felipe Balbi <balbi@xxxxxx>
>
> --
> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/