| Message ID | 20181121154504.13052-1-m.szyprowski@samsung.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect" | expand |
Ugh... We also had a long thread about the v2 patch but it turns out the list was not CC'd. I should have checked for that. You have to pass a flag to say if the caller holds the lock or not... regards, dan carpenter
On Tue, Dec 04, 2018 at 12:34:08PM +0000, Minas Harutyunyan wrote: > @@ -3185,12 +3183,13 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg) > hsotg->connected = 0; > hsotg->test_mode = 0; > > - /* all endpoints should be shutdown */ > for (ep = 0; ep < hsotg->num_of_eps; ep++) { > if (hsotg->eps_in[ep]) > - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep); > + kill_all_requests(hsotg, hsotg->eps_in[ep], > + -ESHUTDOWN); > if (hsotg->eps_out[ep]) > - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep); > + kill_all_requests(hsotg, hsotg->eps_out[ep], > + -ESHUTDOWN); Should this part be in a separate patch? I'm not trying to be rhetorical at all. I literally don't know the code very well. Hopefully the full commit message will explain it. > } > > call_gadget(hsotg, disconnect); > @@ -3234,6 +3233,8 @@ static void dwc2_hsotg_irq_fifoempty(struct > dwc2_hsotg *hsotg, bool periodic) > GINTSTS_PTXFEMP | \ > GINTSTS_RXFLVL) > > +static int dwc2_hsotg_ep_disable(struct usb_ep *ep); > + > /** > * dwc2_hsotg_core_init - issue softreset to the core > * @hsotg: The device state > @@ -3258,12 +3259,14 @@ void dwc2_hsotg_core_init_disconnected(struct > dwc2_hsotg *hsotg, > return; > } else { > /* all endpoints should be shutdown */ > + spin_unlock(&hsotg->lock); > for (ep = 1; ep < hsotg->num_of_eps; ep++) { > if (hsotg->eps_in[ep]) > > dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep); > if (hsotg->eps_out[ep]) > > dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep); > } > + spin_lock(&hsotg->lock); > } > > /* The idea here is that this is the only caller which is holding the lock and we drop it here and take it again inside dwc2_hsotg_ep_disable(). I don't know the code very well and can't totally swear that this doesn't introduce a small race condition... Another option would be to introduce a new function which takes the lock and change all the other callers instead. To me that would be easier to review... See below for how it might look: regards, dan carpenter diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index 94f3ba995580..b17a5dbefd5f 100644 --- a/drivers/usb/dwc2/gadget.c +++ b/drivers/usb/dwc2/gadget.c @@ -3166,6 +3166,7 @@ static void kill_all_requests(struct dwc2_hsotg *hsotg, } static int dwc2_hsotg_ep_disable(struct usb_ep *ep); +static int dwc2_hsotg_ep_disable_lock(struct usb_ep *ep); /** * dwc2_hsotg_disconnect - disconnect service @@ -3188,9 +3189,9 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg) /* all endpoints should be shutdown */ for (ep = 0; ep < hsotg->num_of_eps; ep++) { if (hsotg->eps_in[ep]) - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep); + dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep); if (hsotg->eps_out[ep]) - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep); + dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep); } call_gadget(hsotg, disconnect); @@ -4069,10 +4070,8 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep) struct dwc2_hsotg *hsotg = hs_ep->parent; int dir_in = hs_ep->dir_in; int index = hs_ep->index; - unsigned long flags; u32 epctrl_reg; u32 ctrl; - int locked; dev_dbg(hsotg->dev, "%s(ep %p)\n", __func__, ep); @@ -4088,10 +4087,6 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep) epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index); - locked = spin_is_locked(&hsotg->lock); - if (!locked) - spin_lock_irqsave(&hsotg->lock, flags); - ctrl = dwc2_readl(hsotg, epctrl_reg); if (ctrl & DXEPCTL_EPENA) @@ -4114,12 +4109,23 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep) hs_ep->fifo_index = 0; hs_ep->fifo_size = 0; - if (!locked) - spin_unlock_irqrestore(&hsotg->lock, flags); - return 0; } +static int dwc2_hsotg_ep_disable_lock(struct usb_ep *ep) +{ + struct dwc2_hsotg_ep *hs_ep = our_ep(ep); + struct dwc2_hsotg *hsotg = hs_ep->parent; + unsigned long flags; + int ret; + + spin_lock_irqsave(&hsotg->lock, flags); + ret = dwc2_hsotg_ep_disable(ep); + spin_unlock_irqrestore(&hsotg->lock, flags); + + return ret; +} + /** * on_list - check request is on the given endpoint * @ep: The endpoint to check. @@ -4267,7 +4273,7 @@ static int dwc2_hsotg_ep_sethalt_lock(struct usb_ep *ep, int value) static const struct usb_ep_ops dwc2_hsotg_ep_ops = { .enable = dwc2_hsotg_ep_enable, - .disable = dwc2_hsotg_ep_disable, + .disable = dwc2_hsotg_ep_disable_lock, .alloc_request = dwc2_hsotg_ep_alloc_request, .free_request = dwc2_hsotg_ep_free_request, .queue = dwc2_hsotg_ep_queue_lock, @@ -4407,9 +4413,9 @@ static int dwc2_hsotg_udc_stop(struct usb_gadget *gadget) /* all endpoints should be shutdown */ for (ep = 1; ep < hsotg->num_of_eps; ep++) { if (hsotg->eps_in[ep]) - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep); + dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep); if (hsotg->eps_out[ep]) - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep); + dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep); } spin_lock_irqsave(&hsotg->lock, flags); @@ -4857,9 +4863,9 @@ int dwc2_hsotg_suspend(struct dwc2_hsotg *hsotg) for (ep = 0; ep < hsotg->num_of_eps; ep++) { if (hsotg->eps_in[ep]) - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep); + dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep); if (hsotg->eps_out[ep]) - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep); + dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep); } }
Hi Minas, I tried your new patch on top of the other 2 patches for a couple of days now and I do not see the issue that Marek encountered. Of course, I did not see it also on the original two patches you created. I also do not see the original FIFO map warning issue that I have with all 3 patches intact. However, I do have 2 issues that I'm not certain if this was created by these patches or already there in the beginning (prior to applying all your 3 patches). Again my system is an Altera Cyclone V SoC FPGA running on linux kernel 4.14.44. We are using both USB HID and mass storage gadget. The 2 issues may occur when USB mass storage is in use. First issue I am encountering is (sometimes continuous) multiple USB resets occurring when using the USB mass storage gadget. When I used a USB sniffer (Ellisys) to see the cause of the USB reset, I am seeing an out-of-order USB mass storage transfer. That is, the transfer is supposed to follow (1) Command Block Wrapper (CBW), (2) Data, (3) Command Status Wrapper (CSW). When the USB reset occur, I see either one of the following prior to the reset: - Data comes in first before the CBW - Request Sense CBW, Data then no CSW followed by a Test Unit Ready CBW and Data with no CSW again - CBW, CSW and then Data Second issue that I see is a deadlock then my system will reboot. This happens when I re-enable the USB mass storage gadget after disabling it before. The two issues may not be related to your changes. I will revert back all your patches to see if the issue is present already. However, I will hit my original problem when I don't apply at least 2 of your patches, which is the WARNING: CPU: 0 PID: 0 at ../drivers/usb/dwc2/gadget.c:300 dwc2_hsotg_init_fifo+0x34/0x1b4. I will get back to you once I test it without your patches. Regards, Maynard On Tuesday, December 04, 2018 7:34 AM, Minas Harutyunyan wrote: > On 11/23/2018 6:43 PM, Dan Carpenter wrote: > > Ugh... We also had a long thread about the v2 patch but it turns out > > the list was not CC'd. I should have checked for that. > > > > You have to pass a flag to say if the caller holds the lock or not... > > > > regards, > > dan carpenter > > > > Hi Dan, Marek, Maynard, > > Could you please apply bellow patch over follow patches: > > dccf1bad4be7 usb: dwc2: Disable all EP's on disconnect > 6f774b501928 usb: dwc2: Fix ep disable spinlock flow. > > Please review and test. Feedback is appreciated :-) > > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index > 8eb25e3597b0..db438d13c36a 100644 > --- a/drivers/usb/dwc2/gadget.c > +++ b/drivers/usb/dwc2/gadget.c > @@ -3165,8 +3165,6 @@ static void kill_all_requests(struct dwc2_hsotg > *hsotg, > dwc2_hsotg_txfifo_flush(hsotg, ep->fifo_index); > } > > -static int dwc2_hsotg_ep_disable(struct usb_ep *ep); > - > /** > * dwc2_hsotg_disconnect - disconnect service > * @hsotg: The device state. > @@ -3185,12 +3183,13 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg > *hsotg) > hsotg->connected = 0; > hsotg->test_mode = 0; > > - /* all endpoints should be shutdown */ > for (ep = 0; ep < hsotg->num_of_eps; ep++) { > if (hsotg->eps_in[ep]) > - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep); > + kill_all_requests(hsotg, hsotg->eps_in[ep], > + -ESHUTDOWN); > if (hsotg->eps_out[ep]) > - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep); > + kill_all_requests(hsotg, hsotg->eps_out[ep], > + -ESHUTDOWN); > } > > call_gadget(hsotg, disconnect); @@ -3234,6 +3233,8 @@ static void > dwc2_hsotg_irq_fifoempty(struct dwc2_hsotg *hsotg, bool periodic) > GINTSTS_PTXFEMP | \ > GINTSTS_RXFLVL) > > +static int dwc2_hsotg_ep_disable(struct usb_ep *ep); > + > /** > * dwc2_hsotg_core_init - issue softreset to the core > * @hsotg: The device state > @@ -3258,12 +3259,14 @@ void dwc2_hsotg_core_init_disconnected(struct > dwc2_hsotg *hsotg, > return; > } else { > /* all endpoints should be shutdown */ > + spin_unlock(&hsotg->lock); > for (ep = 1; ep < hsotg->num_of_eps; ep++) { > if (hsotg->eps_in[ep]) > > dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep); > if (hsotg->eps_out[ep]) > > dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep); > } > + spin_lock(&hsotg->lock); > } > > /* > @@ -4072,7 +4075,6 @@ static int dwc2_hsotg_ep_disable(struct usb_ep > *ep) > unsigned long flags; > u32 epctrl_reg; > u32 ctrl; > - int locked; > > dev_dbg(hsotg->dev, "%s(ep %p)\n", __func__, ep); > > @@ -4088,7 +4090,7 @@ static int dwc2_hsotg_ep_disable(struct usb_ep > *ep) > > epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index); > > - locked = spin_trylock_irqsave(&hsotg->lock, flags); > + spin_lock_irqsave(&hsotg->lock, flags); > > ctrl = dwc2_readl(hsotg, epctrl_reg); > > @@ -4112,8 +4114,7 @@ static int dwc2_hsotg_ep_disable(struct usb_ep > *ep) > hs_ep->fifo_index = 0; > hs_ep->fifo_size = 0; > > - if (locked) > - spin_unlock_irqrestore(&hsotg->lock, flags); > + spin_unlock_irqrestore(&hsotg->lock, flags); > > return 0; > } > > Thanks, > Minas ________________________________ Ce message, ainsi que tous les fichiers joints à ce message, peuvent contenir des informations sensibles et/ ou confidentielles ne devant pas être divulguées. Si vous n'êtes pas le destinataire de ce message (ou que vous recevez ce message par erreur), nous vous remercions de le notifier immédiatement à son expéditeur, et de détruire ce message. Toute copie, divulgation, modification, utilisation ou diffusion, non autorisée, directe ou indirecte, de tout ou partie de ce message, est strictement interdite. This e-mail, and any document attached hereby, may contain confidential and/or privileged information. If you are not the intended recipient (or have received this e-mail in error) please notify the sender immediately and destroy this e-mail. Any unauthorized, direct or indirect, copying, disclosure, distribution or other use of the material or parts thereof is strictly forbidden.
Hi Filipe, My patch dccf1bad4be7eaa096c1f3697bd37883f9a08ecb "usb: dwc2: Disable all EP's on disconnect" applied to 4.20-rc1. I need to update this patch. What I should do. There are 2 options: 1. Ack Marek Szyprowski <m.szyprowski@samsung.com> [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect" then submit final fixed version of patch? 2. Or submit new patch to fix existing "usb: dwc2: Disable all EP's on disconnect". Please advise. Thanks, Minas
On Wed, Dec 05, 2018 at 12:52:22PM +0000, Minas Harutyunyan wrote: > Hi, > > On 12/4/2018 5:29 PM, Dan Carpenter wrote: > > On Tue, Dec 04, 2018 at 12:34:08PM +0000, Minas Harutyunyan wrote: > >> @@ -3185,12 +3183,13 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg) > >> hsotg->connected = 0; > >> hsotg->test_mode = 0; > >> > >> - /* all endpoints should be shutdown */ > >> for (ep = 0; ep < hsotg->num_of_eps; ep++) { > >> if (hsotg->eps_in[ep]) > >> - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep); > >> + kill_all_requests(hsotg, hsotg->eps_in[ep], > >> + -ESHUTDOWN); > >> if (hsotg->eps_out[ep]) > >> - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep); > >> + kill_all_requests(hsotg, hsotg->eps_out[ep], > >> + -ESHUTDOWN); > > > > > > Should this part be in a separate patch? > > > > I'm not trying to be rhetorical at all. I literally don't know the > > code very well. Hopefully the full commit message will explain it. > > > > Actually, this fragment of patch revert changes from V2 and keep > untouched dwc2_hsotg_disconnect() function. > To me it feels like there are two issues. The first is this change, and the second is fixing the lockdep warning. > >> } > >> > >> call_gadget(hsotg, disconnect); > >> @@ -3234,6 +3233,8 @@ static void dwc2_hsotg_irq_fifoempty(struct > >> dwc2_hsotg *hsotg, bool periodic) > >> GINTSTS_PTXFEMP | \ > >> GINTSTS_RXFLVL) > >> > >> +static int dwc2_hsotg_ep_disable(struct usb_ep *ep); > >> + > >> /** > >> * dwc2_hsotg_core_init - issue softreset to the core > >> * @hsotg: The device state > >> @@ -3258,12 +3259,14 @@ void dwc2_hsotg_core_init_disconnected(struct > >> dwc2_hsotg *hsotg, > >> return; > >> } else { > >> /* all endpoints should be shutdown */ > >> + spin_unlock(&hsotg->lock); > >> for (ep = 1; ep < hsotg->num_of_eps; ep++) { > >> if (hsotg->eps_in[ep]) > >> > >> dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep); > >> if (hsotg->eps_out[ep]) > >> > >> dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep); > >> } > >> + spin_lock(&hsotg->lock); > >> } > >> > >> /* > > > > The idea here is that this is the only caller which is holding the > > lock and we drop it here and take it again inside dwc2_hsotg_ep_disable(). > > I don't know the code very well and can't totally swear that this > > doesn't introduce a small race condition... > > > Above fragment of patch allow to keep untouched dwc2_hsotg_ep_disble() > function also, without changing spin_lock/_unlock stuff inside function. > > My approach here minimally update code to add any races. Just in > dwc2_hsotg_core_init_disconnected() function on USB reset interrupt > perform disabling all EP's. Because on USB reset interrupt, called from interrupt > handler with acquired lock and dwc2_hsotg_ep_disble() function (without > changes) acquire lock, just need to unlock lock to avoid any troubles. > Yes. I understand that. I just don't like it. Although your patch is more "minimal" in that it touches fewer lines of code it's actually more complicated because we have to verify that it's safe to drop the lock. > > Another option would be to introduce a new function which takes the lock > > and change all the other callers instead. To me that would be easier to > > review... See below for how it might look: > > > > regards, > > dan carpenter > > > > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c > > index 94f3ba995580..b17a5dbefd5f 100644 > > --- a/drivers/usb/dwc2/gadget.c > > +++ b/drivers/usb/dwc2/gadget.c > > @@ -3166,6 +3166,7 @@ static void kill_all_requests(struct dwc2_hsotg *hsotg, > > } > > > > static int dwc2_hsotg_ep_disable(struct usb_ep *ep); > > +static int dwc2_hsotg_ep_disable_lock(struct usb_ep *ep); > > > > /** > > * dwc2_hsotg_disconnect - disconnect service > > @@ -3188,9 +3189,9 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg) > > /* all endpoints should be shutdown */ > > for (ep = 0; ep < hsotg->num_of_eps; ep++) { > > if (hsotg->eps_in[ep]) > > - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep); > > + dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep); > > if (hsotg->eps_out[ep]) > > - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep); > > + dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep); > > } > > > > call_gadget(hsotg, disconnect); > > @@ -4069,10 +4070,8 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep) > > struct dwc2_hsotg *hsotg = hs_ep->parent; > > int dir_in = hs_ep->dir_in; > > int index = hs_ep->index; > > - unsigned long flags; > > u32 epctrl_reg; > > u32 ctrl; > > - int locked; > > > > dev_dbg(hsotg->dev, "%s(ep %p)\n", __func__, ep); > > > > @@ -4088,10 +4087,6 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep) > > > > epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index); > > > > - locked = spin_is_locked(&hsotg->lock); > > - if (!locked) > > - spin_lock_irqsave(&hsotg->lock, flags); > > - > > ctrl = dwc2_readl(hsotg, epctrl_reg); > > > > if (ctrl & DXEPCTL_EPENA) > > @@ -4114,12 +4109,23 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep) > > hs_ep->fifo_index = 0; > > hs_ep->fifo_size = 0; > > > > - if (!locked) > > - spin_unlock_irqrestore(&hsotg->lock, flags); > > - > > return 0; > > } > > > > +static int dwc2_hsotg_ep_disable_lock(struct usb_ep *ep) > > +{ > > + struct dwc2_hsotg_ep *hs_ep = our_ep(ep); > > + struct dwc2_hsotg *hsotg = hs_ep->parent; > > + unsigned long flags; > > + int ret; > > + > > + spin_lock_irqsave(&hsotg->lock, flags); > > + ret = dwc2_hsotg_ep_disable(ep); > > + spin_unlock_irqrestore(&hsotg->lock, flags); > > + > > + return ret; > > +} > > + > > /** > > * on_list - check request is on the given endpoint > > * @ep: The endpoint to check. > > @@ -4267,7 +4273,7 @@ static int dwc2_hsotg_ep_sethalt_lock(struct usb_ep *ep, int value) > > > > static const struct usb_ep_ops dwc2_hsotg_ep_ops = { > > .enable = dwc2_hsotg_ep_enable, > > - .disable = dwc2_hsotg_ep_disable, > > + .disable = dwc2_hsotg_ep_disable_lock, > > .alloc_request = dwc2_hsotg_ep_alloc_request, > > .free_request = dwc2_hsotg_ep_free_request, > > .queue = dwc2_hsotg_ep_queue_lock, > > @@ -4407,9 +4413,9 @@ static int dwc2_hsotg_udc_stop(struct usb_gadget *gadget) > > /* all endpoints should be shutdown */ > > for (ep = 1; ep < hsotg->num_of_eps; ep++) { > > if (hsotg->eps_in[ep]) > > - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep); > > + dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep); > > if (hsotg->eps_out[ep]) > > - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep); > > + dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep); > > } > > > > spin_lock_irqsave(&hsotg->lock, flags); > > @@ -4857,9 +4863,9 @@ int dwc2_hsotg_suspend(struct dwc2_hsotg *hsotg) > > > > for (ep = 0; ep < hsotg->num_of_eps; ep++) { > > if (hsotg->eps_in[ep]) > > - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep); > > + dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep); > > if (hsotg->eps_out[ep]) > > - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep); > > + dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep); > > } > > } > > > > > > Your code doesn't take care about fifo_map warnings from > dwc2_hsotg_init_fifo() function. Before calling dwc2_hsotg_init_fifo() > from dwc2_hsotg_core_init_disconnected() function all Ep's should > disabled and fifo bitmap should be cleared. > Correct. I am only trying to fix the locking. I hope you can fix the rest in a separate patch. regards, dan carpenter
On Fri, Dec 07, 2018 at 02:13:18PM +0000, Minas Harutyunyan wrote: > > My patch doesn't pass sparse checking: "warning: context imbalance in > 'dwc2_hsotg_core_init_disconnected' - unexpected unlock". Sparse persist! > So, I need to re-work patch. Can I use your idea with > dwc2_hsotg_ep_disable_lock() function to prepare new one? > Sparse lock checking is pretty crap, and I wouldn't go out of my way normally to make it happy... But of course you can use my idea. regards, dan carpenter
===================================== WARNING: bad unlock balance detected! 4.19.0-rc6-00027-gdccf1bad4be7-dirty #1119 Not tainted ------------------------------------- ip/1464 is trying to release lock (&(&hsotg->lock)->rlock) at: [<c0615494>] dwc2_hsotg_complete_request+0x84/0x218 but there are no more locks to release! other info that might help us debug this: 2 locks held by ip/1464: #0: d69babd3 (rtnl_mutex){+.+.}, at: rtnetlink_rcv_msg+0x224/0x488 #1: 5fb350d2 (&(&dev->lock)->rlock){-.-.}, at: eth_stop+0x24/0xa8 stack backtrace: CPU: 1 PID: 1464 Comm: ip Not tainted 4.19.0-rc6-00027-gdccf1bad4be7-dirty #1119 Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) [<c0111f9c>] (unwind_backtrace) from [<c010deb8>] (show_stack+0x10/0x14) [<c010deb8>] (show_stack) from [<c09d3398>] (dump_stack+0x90/0xc8) [<c09d3398>] (dump_stack) from [<c017d02c>] (print_unlock_imbalance_bug+0xb0/0xe0) [<c017d02c>] (print_unlock_imbalance_bug) from [<c01800dc>] (lock_release+0x1a4/0x374) [<c01800dc>] (lock_release) from [<c09ef7fc>] (_raw_spin_unlock+0x18/0x54) [<c09ef7fc>] (_raw_spin_unlock) from [<c0615494>] (dwc2_hsotg_complete_request+0x84/0x218) [<c0615494>] (dwc2_hsotg_complete_request) from [<c0617530>] (kill_all_requests+0x44/0xb4) [<c0617530>] (kill_all_requests) from [<c0617690>] (dwc2_hsotg_ep_disable+0xf0/0x200) [<c0617690>] (dwc2_hsotg_ep_disable) from [<c0659698>] (usb_ep_disable+0xd0/0x1c8) [<c0659698>] (usb_ep_disable) from [<c065d4a8>] (eth_stop+0x68/0xa8) [<c065d4a8>] (eth_stop) from [<c077e0b8>] (__dev_close_many+0x94/0xfc) [<c077e0b8>] (__dev_close_many) from [<c078a064>] (__dev_change_flags+0xa0/0x198) [<c078a064>] (__dev_change_flags) from [<c078a17c>] (dev_change_flags+0x18/0x48) [<c078a17c>] (dev_change_flags) from [<c07a1a98>] (do_setlink+0x298/0x990) [<c07a1a98>] (do_setlink) from [<c07a29f0>] (rtnl_newlink+0x4a0/0x6fc) [<c07a29f0>] (rtnl_newlink) from [<c079dd74>] (rtnetlink_rcv_msg+0x254/0x488) [<c079dd74>] (rtnetlink_rcv_msg) from [<c07c47f4>] (netlink_rcv_skb+0xe0/0x118) [<c07c47f4>] (netlink_rcv_skb) from [<c07c4094>] (netlink_unicast+0x180/0x1c8) [<c07c4094>] (netlink_unicast) from [<c07c4428>] (netlink_sendmsg+0x2bc/0x348) [<c07c4428>] (netlink_sendmsg) from [<c0760b9c>] (sock_sendmsg+0x14/0x24) [<c0760b9c>] (sock_sendmsg) from [<c0761af8>] (___sys_sendmsg+0x22c/0x248) [<c0761af8>] (___sys_sendmsg) from [<c0762740>] (__sys_sendmsg+0x40/0x6c) [<c0762740>] (__sys_sendmsg) from [<c0101000>] (ret_fast_syscall+0x0/0x28) Exception stack(0xede65fa8 to 0xede65ff0) ... Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- The suspicious locking has been already pointed in the review of the first version of this patch, but sadly the v2 didn't change it and landed in v4.20-rc1. --- drivers/usb/dwc2/gadget.c | 30 +++++++----------------------- 1 file changed, 7 insertions(+), 23 deletions(-) diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index 79189db4bf17..220c0f9b89b0 100644 --- a/drivers/usb/dwc2/gadget.c +++ b/drivers/usb/dwc2/gadget.c @@ -3109,8 +3109,6 @@ static void kill_all_requests(struct dwc2_hsotg *hsotg, dwc2_hsotg_txfifo_flush(hsotg, ep->fifo_index); } -static int dwc2_hsotg_ep_disable(struct usb_ep *ep); - /** * dwc2_hsotg_disconnect - disconnect service * @hsotg: The device state. @@ -3129,12 +3127,13 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg) hsotg->connected = 0; hsotg->test_mode = 0; - /* all endpoints should be shutdown */ for (ep = 0; ep < hsotg->num_of_eps; ep++) { if (hsotg->eps_in[ep]) - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep); + kill_all_requests(hsotg, hsotg->eps_in[ep], + -ESHUTDOWN); if (hsotg->eps_out[ep]) - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep); + kill_all_requests(hsotg, hsotg->eps_out[ep], + -ESHUTDOWN); } call_gadget(hsotg, disconnect); @@ -3192,23 +3191,13 @@ void dwc2_hsotg_core_init_disconnected(struct dwc2_hsotg *hsotg, u32 val; u32 usbcfg; u32 dcfg = 0; - int ep; /* Kill any ep0 requests as controller will be reinitialized */ kill_all_requests(hsotg, hsotg->eps_out[0], -ECONNRESET); - if (!is_usb_reset) { + if (!is_usb_reset) if (dwc2_core_reset(hsotg, true)) return; - } else { - /* all endpoints should be shutdown */ - for (ep = 1; ep < hsotg->num_of_eps; ep++) { - if (hsotg->eps_in[ep]) - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep); - if (hsotg->eps_out[ep]) - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep); - } - } /* * we must now enable ep0 ready for host detection and then @@ -4004,7 +3993,6 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep) unsigned long flags; u32 epctrl_reg; u32 ctrl; - int locked; dev_dbg(hsotg->dev, "%s(ep %p)\n", __func__, ep); @@ -4020,9 +4008,7 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep) epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index); - locked = spin_is_locked(&hsotg->lock); - if (!locked) - spin_lock_irqsave(&hsotg->lock, flags); + spin_lock_irqsave(&hsotg->lock, flags); ctrl = dwc2_readl(hsotg, epctrl_reg); @@ -4046,9 +4032,7 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep) hs_ep->fifo_index = 0; hs_ep->fifo_size = 0; - if (!locked) - spin_unlock_irqrestore(&hsotg->lock, flags); - + spin_unlock_irqrestore(&hsotg->lock, flags); return 0; }