| Message ID | 20181121154504.13052-1-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect" | expand |
Hi Marek, On 11/21/2018 7:45 PM, Marek Szyprowski wrote: > This reverts commit dccf1bad4be7eaa096c1f3697bd37883f9a08ecb. > > The reverted commit breaks locking in the DWC2 driver. It causes random > crashes or memory corruption related issues on SMP machines. Here is an > example of the observed reproducible issue (other are a bit more random): > > ===================================== > 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; > } > > Could you please apply "[PATCH v2] usb: dwc2: Disable all EP's on disconnect" and fix for that patch "[PATCH] usb: dwc2: Fix ep disable spinlock flow.". Let me know on test results. Thanks, Minas
Hi Minas, On 2018-11-22 07:53, Minas Harutyunyan wrote: > On 11/21/2018 7:45 PM, Marek Szyprowski wrote: >> This reverts commit dccf1bad4be7eaa096c1f3697bd37883f9a08ecb. >> >> The reverted commit breaks locking in the DWC2 driver. It causes random >> crashes or memory corruption related issues on SMP machines. Here is an >> example of the observed reproducible issue (other are a bit more random): >> >> ===================================== >> 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; >> } >> >> > Could you please apply "[PATCH v2] usb: dwc2: Disable all EP's on > disconnect" and fix for that patch "[PATCH] usb: dwc2: Fix ep disable > spinlock flow.". Let me know on test results. I've checked and even with your fix "[PATCH] usb: dwc2: Fix ep disable spinlock flow.", the issue is there. Playing conditionally with spinlocks seems to be very bad idea. When spinlock is taken, the other CPU is touching DWC2 registers. Don't expect that if you skip spinlock and forcibly change DWC2 registers, the hardware will do what you really expects... There random crashes and lockdep still prints a warning: ===================================== WARNING: bad unlock balance detected! 4.20.0-rc3-00005-g1246b0e2e509 #5096 Not tainted ------------------------------------- ip/1491 is trying to release lock (&(&hsotg->lock)->rlock) at: [<c061c2ec>] dwc2_hsotg_complete_request+0x84/0x214 but there are no more locks to release! other info that might help us debug this: 2 locks held by ip/1491: #0: e2180f30 (rtnl_mutex){+.+.}, at: rtnetlink_rcv_msg+0x224/0x488 #1: 68ab9014 (&(&dev->lock)->rlock){-.-.}, at: eth_stop+0x24/0xa8 stack backtrace: CPU: 1 PID: 1491 Comm: ip Not tainted 4.20.0-rc3-00005-g1246b0e2e509 #5096 Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) [<c0111a0c>] (unwind_backtrace) from [<c010d8f4>] (show_stack+0x10/0x14) [<c010d8f4>] (show_stack) from [<c09e4198>] (dump_stack+0x90/0xc8) [<c09e4198>] (dump_stack) from [<c017d2c8>] (print_unlock_imbalance_bug+0xb0/0xe0) [<c017d2c8>] (print_unlock_imbalance_bug) from [<c0180404>] (lock_release+0x190/0x380) [<c0180404>] (lock_release) from [<c0a042a4>] (_raw_spin_unlock+0x18/0x54) [<c0a042a4>] (_raw_spin_unlock) from [<c061c2ec>] (dwc2_hsotg_complete_request+0x84/0x214) [<c061c2ec>] (dwc2_hsotg_complete_request) from [<c061e378>] (kill_all_requests+0x44/0xb4) [<c061e378>] (kill_all_requests) from [<c061e4fc>] (dwc2_hsotg_ep_disable+0x114/0x21c) [<c061e4fc>] (dwc2_hsotg_ep_disable) from [<c066085c>] (usb_ep_disable+0xd0/0x1c8) [<c066085c>] (usb_ep_disable) from [<c0664664>] (eth_stop+0x68/0xa8) [<c0664664>] (eth_stop) from [<c07871b8>] (__dev_close_many+0x94/0xfc) [<c07871b8>] (__dev_close_many) from [<c07931b0>] (__dev_change_flags+0xa0/0x198) [<c07931b0>] (__dev_change_flags) from [<c07932c8>] (dev_change_flags+0x18/0x48) [<c07932c8>] (dev_change_flags) from [<c07ab798>] (do_setlink+0x298/0x990) [<c07ab798>] (do_setlink) from [<c07ac760>] (rtnl_newlink+0x4a8/0x70c) [<c07ac760>] (rtnl_newlink) from [<c07a7688>] (rtnetlink_rcv_msg+0x254/0x488) [<c07a7688>] (rtnetlink_rcv_msg) from [<c07cf844>] (netlink_rcv_skb+0xe0/0x118) [<c07cf844>] (netlink_rcv_skb) from [<c07cf0e4>] (netlink_unicast+0x180/0x1c8) [<c07cf0e4>] (netlink_unicast) from [<c07cf478>] (netlink_sendmsg+0x2bc/0x348) [<c07cf478>] (netlink_sendmsg) from [<c0769a38>] (sock_sendmsg+0x14/0x24) [<c0769a38>] (sock_sendmsg) from [<c076a9b0>] (___sys_sendmsg+0x22c/0x248) [<c076a9b0>] (___sys_sendmsg) from [<c076b5f8>] (__sys_sendmsg+0x40/0x6c) [<c076b5f8>] (__sys_sendmsg) from [<c0101000>] (ret_fast_syscall+0x0/0x28) Exception stack(0xedec5fa8 to 0xedec5ff0) ... Best regards
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 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
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, 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. >> } >> >> 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. > 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. Thanks, Minas
Hi Marek, I'm surprised you don't get deadlocks when you apply this patch. On Wed, Nov 21, 2018 at 04:45:04PM +0100, Marek Szyprowski wrote: > @@ -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); > One of the callers is already holding the hsotg->log. The spin_is_locked() test would avoid the deadlock. regards, dan carpenter
Dear Minas, On 2018-12-04 13:34, 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 :-) Okay, I finally managed to find some time to check this. Your diff is mangled, so I had to manually apply it. Frankly, it is very similar to the revert I proposed. I've checked it on my test machines and it fixes the issues. I'm not very happy about the unlock/lock design, but it should be safe in this case and doesn't make the code even more complex. Feel free to add a following tag to the final patch: Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > 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 > > Best regards
Hi Dan, On 2018-12-06 15:52, Dan Carpenter wrote: > Hi Marek, > > I'm surprised you don't get deadlocks when you apply this patch. Why should I get it? It is just a revert to the state before applying the mentioned incorrect patch. > On Wed, Nov 21, 2018 at 04:45:04PM +0100, Marek Szyprowski wrote: > >> @@ -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); >> > One of the callers is already holding the hsotg->log. The > spin_is_locked() test would avoid the deadlock. Before that broken patch, there was no call to any function which takes spinlock again, so no deadlock. Best regards
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
Hi Maynard, On 12/7/2018 3:09 AM, Maynard CABIENTE wrote: > 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 > Thank you for testing. Issue which described looks like not related to these patches. For that issue please open another mail thread with more details. We will work on it. Thanks, Minas > 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 Marek, On 12/6/2018 7:04 PM, Marek Szyprowski wrote: > Dear Minas, > > On 2018-12-04 13:34, 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 :-) > > Okay, I finally managed to find some time to check this. Your diff is > mangled, so I had to manually apply it. Frankly, it is very similar to > the revert I proposed. I've checked it on my test machines and it fixes > the issues. I'm not very happy about the unlock/lock design, but it > should be safe in this case and doesn't make the code even more complex. > Feel free to add a following tag to the final patch: > > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> Thanks for testing. > >> 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 >> >> > Best regards > Thanks, Minas
Hi, Minas Harutyunyan <minas.harutyunyan@synopsys.com> writes: > Hi Marek, > > On 12/6/2018 7:04 PM, Marek Szyprowski wrote: >> Dear Minas, >> >> On 2018-12-04 13:34, 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 :-) >> >> Okay, I finally managed to find some time to check this. Your diff is >> mangled, so I had to manually apply it. Frankly, it is very similar to >> the revert I proposed. I've checked it on my test machines and it fixes >> the issues. I'm not very happy about the unlock/lock design, but it >> should be safe in this case and doesn't make the code even more complex. >> Feel free to add a following tag to the final patch: >> >> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > Thanks for testing. should $Subject be applied? Can you resend rebased on testing/next with Tested-by tags collected if you're happy with it? thanks
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
Hi, Minas Harutyunyan <minas.harutyunyan@synopsys.com> writes: > 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". I would say it's best to go with option 2. Just send a fix on top :-)
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
Hi Dan, On 12/7/2018 2:16 PM, Dan Carpenter wrote: > 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. > Yeah. I'll try deeper investigate driver locking flow and fix it later. Actually, I like your idea with introducing dwc2_hsotg_ep_disable_lock() function. Maybe you yourself will submit new patch for safe locking fixes? But please just after my patch will applied :-) Currently there are 2-3 high priority issues reported by community and I should find solutions/fixes. Thank you very much for your time and useful feedback. Thanks, Minas > regards, > dan carpenter > >
Hi Dan, On 12/7/2018 3:20 PM, Minas Harutyunyan wrote: > Hi Dan, > > On 12/7/2018 2:16 PM, Dan Carpenter wrote: >> 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. >> > Yeah. I'll try deeper investigate driver locking flow and fix it later. > Actually, I like your idea with introducing dwc2_hsotg_ep_disable_lock() > function. Maybe you yourself will submit new patch for safe locking > fixes? But please just after my patch will applied :-) > Currently there are 2-3 high priority issues reported by community and I > should find solutions/fixes. > Thank you very much for your time and useful feedback. > > Thanks, > Minas > > >> regards, >> dan carpenter >> >> > > 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? Thanks, Minas
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; }