Hi,
Magnus Lilja <lilja.magnus@xxxxxxxxx> 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.
>>>>
>>>> well, the easiest way is to add something like:
>>>>
>>>> if (!req->complete)
>>>> dump_stack();
>>>>
>>>> to fsl udc driver. Then you would know who queued the request without
>>>> ->complete. A slightly better approach would be to:
>>>>
>>>> if (WARN(!req->complete,
>>>> "%s: queueing request without ->complete\n", ep->name))
>>>> return;
>>>>
>>>> Or something like that.
>>>
>>> Well, I think I found it.
>>>
>>> fsl_udc_core.c:ep0_prime_status() sets req->req.complete = NULL before
>>> it queues a transfer and my printk()'s indicate that this is indeed
>>> the offending function.
>>>
>>> fsl_udc_core.c:ch9getstatus() also sets complete to NULL but in my
>>> tests right now I haven't seen that one.
>>>
>>> So it's an internal problem in the fsl_udc_core.c file.
>>
>> seems like it. It's rather odd that fsl_udc doesn't wanna know about
>> completion of Status stage. Oh well, I guess in this case it doesn't
>> matter if you add a complete function or reinstate the previous check
>> for valid complete.
>>
>> If you decide to reinstate the check, please add a comment above the
>> check explaining that fsl_udc itself queues requests with NULL
>> ->complete().
>>
>> I must say, however, that I would suggest adding a complete callback
>> since that will help us BUG with NULL pointer deref on bad gadget
>> drivers ;-)
>
> I can do that. Such a complete() callback function would be a no-op
> then I assume (with a comment in it why it is a no-op).
that's right :-)
--
balbi
Attachment:
signature.asc
Description: PGP signature
