Hi Felipe, Please drop this patch from your next branch. I will provide another fix based on Marek's suggestion. Thanks, Grigor. On 5/17/2018 4:18 PM, Marek Szyprowski wrote: > Hi All, > > I've just noticed this patch has been applied to linux-next. > > It breaks DWC2 driver initialization on Samsung Exynos-based boards > I've use for kernel daily tests. > > Here is an example of the call stack: > > Unable to handle kernel NULL pointer dereference at virtual address 0000000c > pgd = (ptrval) > [0000000c] *pgd=00000000 > Internal error: Oops: 5 [#1] PREEMPT SMP ARM > Modules linked in: > CPU: 0 PID: 25 Comm: kworker/0:1 Not tainted 4.17.0-rc5-next-20180517 #782 > Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) > Workqueue: events deferred_probe_work_func > PC is at usb_ep_alloc_request+0x1c/0x200 > LR is at composite_dev_prepare+0x20/0xec > pc : [<c06976a8>] lr : [<c0694744>] psr: 60000153 > sp : d95f1d10 ip : 00000000 fp : 00000002 > r10: c1072d58 r9 : d82443d0 r8 : 00000000 > r7 : c1074180 r6 : c10ad460 r5 : c100746c r4 : d8253980 > r3 : 00000000 r2 : d82539b8 r1 : 014000c0 r0 : d82443d0 > Flags: nZCv IRQs on FIQs off Mode SVC_32 ISA ARM Segment none > Control: 10c5387d Table: 4000406a DAC: 00000051 > Process kworker/0:1 (pid: 25, stack limit = 0x(ptrval)) > Stack: (0xd95f1d10 to 0xd95f2000) > ... > [<c06976a8>] (usb_ep_alloc_request) from [<c0694744>] > (composite_dev_prepare+0x20/0xec) > [<c0694744>] (composite_dev_prepare) from [<c0694b4c>] > (composite_bind+0x7c/0x1c8) > [<c0694b4c>] (composite_bind) from [<c069a8e0>] > (udc_bind_to_driver+0x74/0x134) > [<c069a8e0>] (udc_bind_to_driver) from [<c069aa14>] > (check_pending_gadget_drivers+0x74/0xac) > [<c069aa14>] (check_pending_gadget_drivers) from [<c069abdc>] > (usb_add_gadget_udc_release+0x190/0x1ec) > [<c069abdc>] (usb_add_gadget_udc_release) from [<c0655a84>] > (dwc2_gadget_init+0x274/0x464) > [<c0655a84>] (dwc2_gadget_init) from [<c0642fb0>] > (dwc2_driver_probe+0x488/0x50c) > [<c0642fb0>] (dwc2_driver_probe) from [<c057a1e4>] > (platform_drv_probe+0x48/0x9c) > [<c057a1e4>] (platform_drv_probe) from [<c0577d54>] > (driver_probe_device+0x2dc/0x4ac) > [<c0577d54>] (driver_probe_device) from [<c0575d54>] > (bus_for_each_drv+0x74/0xb8) > [<c0575d54>] (bus_for_each_drv) from [<c057792c>] > (__device_attach+0xd4/0x168) > [<c057792c>] (__device_attach) from [<c0576bf8>] > (bus_probe_device+0x88/0x90) > [<c0576bf8>] (bus_probe_device) from [<c05771b0>] > (deferred_probe_work_func+0x60/0x180) > [<c05771b0>] (deferred_probe_work_func) from ad+0x28c/0x58c) > [<c0147f2c>] (worker_thread) from [<c014dc5c>] (kthread+0x138/0x168) > [<c014dc5c>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20) > > > A proper fix for the potential memory leak is to add free to error path > instead of reordering usb_add_gadget_udc() call. The issue happens if DWC2 > driver is initialized from deferred probe (in such case the gadget driver > is already registered). > > Felipe: please drop or revert this patch in your -next branch. > > > Best regards > Marek Szyprowski, PhD > Samsung R&D Institute Poland > > On 2018-04-16 12:16, Grigor Tovmasyan wrote: >> In dwc2_gadget_init() we allocate EP0 request via >> dwc2_hsotg_ep_alloc_request(). After that there are >> usb_add_gadget_udc() call and if it failed, then >> ctrl_req will not be freed and will cause memory leak. >> >> Reordered function calls in gadget_init: moved up usb_add_gadget_udc() >> before dwc2_hsotg_ep_alloc_request(). >> >> Tested using kmemleak. >> >> Cc: Stefan Wahren <stefan.wahren@xxxxxxxx> >> Signed-off-by: Grigor Tovmasyan <tovmasya@xxxxxxxxxxxx> >> Acked-by: Minas Harutyunyan <hminas@xxxxxxxxxxxx> >> --- >> drivers/usb/dwc2/gadget.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c >> index 6c32bf26e48e..24000bda5c20 100644 >> --- a/drivers/usb/dwc2/gadget.c >> +++ b/drivers/usb/dwc2/gadget.c >> @@ -4679,6 +4679,10 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg) >> INIT_LIST_HEAD(&hsotg->gadget.ep_list); >> hsotg->gadget.ep0 = &hsotg->eps_out[0]->ep; >> >> + ret = usb_add_gadget_udc(dev, &hsotg->gadget); >> + if (ret) >> + return ret; >> + >> /* allocate EP0 request */ >> >> hsotg->ctrl_req = dwc2_hsotg_ep_alloc_request(&hsotg->eps_out[0]->ep, >> @@ -4698,10 +4702,6 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg) >> epnum, 0); >> } >> >> - ret = usb_add_gadget_udc(dev, &hsotg->gadget); >> - if (ret) >> - return ret; >> - >> dwc2_hsotg_dump(hsotg); >> >> return 0; >> > > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
