usb: gadget: Kernel panic (NULL pointer dereference) when using fsl_udc2_core on i.MX31 PDK

[Magnus Lilja]

On 24 January 2017 at 09:52, Felipe Balbi <balbi at kernel.org> wrote:
>
> Hi,
>
> Magnus Lilja <lilja.magnus at gmail.com> writes:
>>>> I tried the fsl_udc_core gadget driver on the i.MX31 PDK board and got a
>>>> kernel panic (NULL pointer dereference) when connecting the USB cable. I
>>>> had the g_serial module loaded as well.
>>>>
>>>> The NULL pointer panic comes from gadget/udc/core.c
>>>> usb_gadget_giveback_request() which calls req->complete() and in some
>>>> cases req->complete is NULL.
>>>>
>>>> Commit 304f7e5e1d08 ("usb: gadget: Refactor request completion") changed
>>>> fsl_udc2_core.c (and several other files) and in fsl_udc2_core.c a check
>>>> that req->complete is non-NULL was removed:
>>>>
>>>> --- a/drivers/usb/gadget/udc/fsl_udc_core.c
>>>> +++ b/drivers/usb/gadget/udc/fsl_udc_core.c
>>>> @@ -197,10 +197,8 @@ __acquires(ep->udc->lock)
>>>>          ep->stopped = 1;
>>>>
>>>>          spin_unlock(&ep->udc->lock);
>>>> -       /* complete() is from gadget layer,
>>>> -        * eg fsg->bulk_in_complete() */
>>>> -       if (req->req.complete)
>>>> -               req->req.complete(&ep->ep, &req->req);
>>>> +
>>>> +       usb_gadget_giveback_request(&ep->ep, &req->req);
>>>>
>>>>          spin_lock(&ep->udc->lock);
>>>>          ep->stopped = stopped;
>>>>
>>>> If I re-introduce the check (either in fsl_udc_core.c or core.c) at
>>>> least USB gadget operation using g_serial seems to work just fine.
>>>>
>>>> I don't know the logic in detail to understand whether this is a proper
>>>> fix or if there is some other more problem with the fls_udc_core driver.
>>>> Does anyone have input in this matter?
>>>>
>>>> I can produce a proper patch that fixes this problem by re-introducing
>>>> the check (in either fsl_udc_core.c or core.c) if that is a proper
>>>> solution and I can also assist in testing other fixes to the problem.
>>>
>>> ->complete() is supposed to be mandatory. Which gadget do you have that
>>> ->doesn't set ->complete() to a valid function pointer?
>>
>> I'm modprobing g_serial so the following modules are loaded (using my patch):
>>
>> ~ # lsmod
>> usb_f_acm
>> u_serial
>> g_serial
>> libcomposite
>> configfs
>> fsl_usb2_udc
>
> okay, can you figure out which request is coming without ->complete()
> set? To which endpoint is this request being queued? It would be nice to
> know these details. Maybe this is an old bug which ought to be fixed.

Sure, I can try figure that out. Any input to make the debug of the
faster is appreciated if you have any.

Regards, Magnus