| Message ID | f984e50b-0e64-88ef-2f53-b8da2a62daf0@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, Magnus Lilja <lilja.magnus@gmail.com> writes: > Hi > > 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?
Hi On 23 January 2017 at 12:51, Felipe Balbi <balbi@kernel.org> wrote: > > Hi, > > Magnus Lilja <lilja.magnus@gmail.com> writes: >> Hi >> >> 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 Regards, Magnus
Hi, Magnus Lilja <lilja.magnus@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.
On 24 January 2017 at 09:52, Felipe Balbi <balbi@kernel.org> wrote: > > Hi, > > Magnus Lilja <lilja.magnus@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
Hi, Magnus Lilja <lilja.magnus@gmail.com> writes: >> Magnus Lilja <lilja.magnus@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. 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.
Hi On 24 January 2017 at 11:54, Felipe Balbi <balbi@kernel.org> wrote: > > Hi, > > Magnus Lilja <lilja.magnus@gmail.com> writes: >>> Magnus Lilja <lilja.magnus@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. > > 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. Regards, Magnus¨
Hi, Magnus Lilja <lilja.magnus@gmail.com> writes: >> Magnus Lilja <lilja.magnus@gmail.com> writes: >>>> Magnus Lilja <lilja.magnus@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. >> >> 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 ;-)
On 24 January 2017 at 19:34, Felipe Balbi <balbi@kernel.org> wrote: > > Hi, > > Magnus Lilja <lilja.magnus@gmail.com> writes: >>> Magnus Lilja <lilja.magnus@gmail.com> writes: >>>>> Magnus Lilja <lilja.magnus@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. >>> >>> 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). Regards, Magnus
Hi, Magnus Lilja <lilja.magnus@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. >>>> >>>> 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 :-)
--- 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;
Hi 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: 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. Thanks, Magnus