diff mbox

[1/4,v3] usb: dwc2: Avoid sleeping while holding hsotg->lock

Message ID 1484180524-4087-2-git-send-email-john.stultz@linaro.org
State Accepted
Commit 6e6360b67d12370638ad1bc8943cc63d4c89da27
Headers show

Commit Message

John Stultz Jan. 12, 2017, 12:22 a.m. UTC 
Basically when plugging in various cables in different orders, I'm
occasionally seeing the following BUG splat:

[   86.215403] BUG: scheduling while atomic: kworker/u16:2/53/0x00000002
[   86.219164] usb 1-1: USB disconnect, device number 9
[   86.226845] Preemption disabled at:[   86.230218]
[<ffffff8008673558>] dwc2_conn_id_status_change+0x120/0x250
[   86.236894] CPU: 0 PID: 53 Comm: kworker/u16:2 Tainted: G        W
     4.9.0-rc8-00051-gd5a7979-dirty #1702
[   86.246836] Hardware name: HiKey Development Board (DT)
[   86.252100] Workqueue: dwc2 dwc2_conn_id_status_change
[   86.257279] Call trace:
[   86.259771] [<ffffff8008087c28>] dump_backtrace+0x0/0x1a0
[   86.265210] [<ffffff8008087ddc>] show_stack+0x14/0x20
[   86.270308] [<ffffff80084343f0>] dump_stack+0x90/0xb0
[   86.275401] [<ffffff80080d8d94>] __schedule_bug+0x6c/0xb8
[   86.280841] [<ffffff8008a07220>] __schedule+0x4f8/0x5b0
[   86.286099] [<ffffff8008a073e8>] schedule+0x38/0xa0
[   86.291017] [<ffffff8008a0a6cc>] schedule_hrtimeout_range_clock+0x8c/0xf0
[   86.297846] [<ffffff8008a0a740>] schedule_hrtimeout_range+0x10/0x18
[   86.304150] [<ffffff8008a0a4a0>] usleep_range+0x50/0x58
[   86.309418] [<ffffff800866d8dc>] dwc2_wait_for_mode.isra.4+0x54/0xd0
[   86.315815] [<ffffff800866f058>] dwc2_core_reset+0xe0/0x168
[   86.321431] [<ffffff800867e364>] dwc2_hsotg_core_init_disconnected+0x2c/0x310
[   86.328602] [<ffffff8008673568>] dwc2_conn_id_status_change+0x130/0x250
[   86.335254] [<ffffff80080ccd48>] process_one_work+0x118/0x370
[   86.341035] [<ffffff80080ccfe8>] worker_thread+0x48/0x498
[   86.346473] [<ffffff80080d2eb0>] kthread+0xd0/0xe8
[   86.351299] [<ffffff8008082e80>] ret_from_fork+0x10/0x50

This seems to be caused by the dwc2_wait_for_mode() calling
usleep_range() while the hstog->lock spinlock is held, since
we take that before calling dwc2_hsotg_core_init_disconnected().

This patch avoids the issue by adding an extra argument to
dwc2_core_reset(), as suggested by John Youn, which allows us to
skip the waiting, which should be unnecessary when calling from
dwc2_hsotg_core_init_disconnected().

Cc: Wei Xu <xuwei5@hisilicon.com>
Cc: Guodong Xu <guodong.xu@linaro.org>
Cc: Amit Pundir <amit.pundir@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: John Youn <johnyoun@synopsys.com>
Cc: Douglas Anderson <dianders@chromium.org>
Cc: Chen Yu <chenyu56@huawei.com>
Cc: Vardan Mikayelyan <mvardan@synopsys.com>
Cc: Kishon Vijay Abraham I <kishon@ti.com>
Cc: Felipe Balbi <felipe.balbi@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
Signed-off-by: John Stultz <john.stultz@linaro.org>

---
v3:
* Rebased to synopsys-usb/next branch

 drivers/usb/dwc2/core.c   | 6 +++---
 drivers/usb/dwc2/core.h   | 2 +-
 drivers/usb/dwc2/gadget.c | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

-- 
2.7.4

Comments

Felipe Balbi Jan. 16, 2017, 10:36 a.m. UTC | #1 
Hi,

John Stultz <john.stultz@linaro.org> writes:
> Basically when plugging in various cables in different orders, I'm

> occasionally seeing the following BUG splat:

>

> [   86.215403] BUG: scheduling while atomic: kworker/u16:2/53/0x00000002

> [   86.219164] usb 1-1: USB disconnect, device number 9

> [   86.226845] Preemption disabled at:[   86.230218]

> [<ffffff8008673558>] dwc2_conn_id_status_change+0x120/0x250

> [   86.236894] CPU: 0 PID: 53 Comm: kworker/u16:2 Tainted: G        W

>      4.9.0-rc8-00051-gd5a7979-dirty #1702

> [   86.246836] Hardware name: HiKey Development Board (DT)

> [   86.252100] Workqueue: dwc2 dwc2_conn_id_status_change

> [   86.257279] Call trace:

> [   86.259771] [<ffffff8008087c28>] dump_backtrace+0x0/0x1a0

> [   86.265210] [<ffffff8008087ddc>] show_stack+0x14/0x20

> [   86.270308] [<ffffff80084343f0>] dump_stack+0x90/0xb0

> [   86.275401] [<ffffff80080d8d94>] __schedule_bug+0x6c/0xb8

> [   86.280841] [<ffffff8008a07220>] __schedule+0x4f8/0x5b0

> [   86.286099] [<ffffff8008a073e8>] schedule+0x38/0xa0

> [   86.291017] [<ffffff8008a0a6cc>] schedule_hrtimeout_range_clock+0x8c/0xf0

> [   86.297846] [<ffffff8008a0a740>] schedule_hrtimeout_range+0x10/0x18

> [   86.304150] [<ffffff8008a0a4a0>] usleep_range+0x50/0x58

> [   86.309418] [<ffffff800866d8dc>] dwc2_wait_for_mode.isra.4+0x54/0xd0

> [   86.315815] [<ffffff800866f058>] dwc2_core_reset+0xe0/0x168

> [   86.321431] [<ffffff800867e364>] dwc2_hsotg_core_init_disconnected+0x2c/0x310

> [   86.328602] [<ffffff8008673568>] dwc2_conn_id_status_change+0x130/0x250

> [   86.335254] [<ffffff80080ccd48>] process_one_work+0x118/0x370

> [   86.341035] [<ffffff80080ccfe8>] worker_thread+0x48/0x498

> [   86.346473] [<ffffff80080d2eb0>] kthread+0xd0/0xe8

> [   86.351299] [<ffffff8008082e80>] ret_from_fork+0x10/0x50

>

> This seems to be caused by the dwc2_wait_for_mode() calling

> usleep_range() while the hstog->lock spinlock is held, since

> we take that before calling dwc2_hsotg_core_init_disconnected().

>

> This patch avoids the issue by adding an extra argument to

> dwc2_core_reset(), as suggested by John Youn, which allows us to

> skip the waiting, which should be unnecessary when calling from

> dwc2_hsotg_core_init_disconnected().

>

> Cc: Wei Xu <xuwei5@hisilicon.com>

> Cc: Guodong Xu <guodong.xu@linaro.org>

> Cc: Amit Pundir <amit.pundir@linaro.org>

> Cc: Rob Herring <robh+dt@kernel.org>

> Cc: John Youn <johnyoun@synopsys.com>

> Cc: Douglas Anderson <dianders@chromium.org>

> Cc: Chen Yu <chenyu56@huawei.com>

> Cc: Vardan Mikayelyan <mvardan@synopsys.com>

> Cc: Kishon Vijay Abraham I <kishon@ti.com>

> Cc: Felipe Balbi <felipe.balbi@linux.intel.com>

> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

> Cc: linux-usb@vger.kernel.org

> Signed-off-by: John Stultz <john.stultz@linaro.org>

> ---


doesn't apply to my testing/next. Please rebase

-- 
balbi
John Stultz Jan. 16, 2017, 8:37 p.m. UTC | #2 
On Mon, Jan 16, 2017 at 2:36 AM, Felipe Balbi
<felipe.balbi@linux.intel.com> wrote:
>

> Hi,

>

> John Stultz <john.stultz@linaro.org> writes:

>> Basically when plugging in various cables in different orders, I'm

>> occasionally seeing the following BUG splat:

>>

>> [   86.215403] BUG: scheduling while atomic: kworker/u16:2/53/0x00000002

>> [   86.219164] usb 1-1: USB disconnect, device number 9

>> [   86.226845] Preemption disabled at:[   86.230218]

>> [<ffffff8008673558>] dwc2_conn_id_status_change+0x120/0x250

>> [   86.236894] CPU: 0 PID: 53 Comm: kworker/u16:2 Tainted: G        W

>>      4.9.0-rc8-00051-gd5a7979-dirty #1702

>> [   86.246836] Hardware name: HiKey Development Board (DT)

>> [   86.252100] Workqueue: dwc2 dwc2_conn_id_status_change

>> [   86.257279] Call trace:

>> [   86.259771] [<ffffff8008087c28>] dump_backtrace+0x0/0x1a0

>> [   86.265210] [<ffffff8008087ddc>] show_stack+0x14/0x20

>> [   86.270308] [<ffffff80084343f0>] dump_stack+0x90/0xb0

>> [   86.275401] [<ffffff80080d8d94>] __schedule_bug+0x6c/0xb8

>> [   86.280841] [<ffffff8008a07220>] __schedule+0x4f8/0x5b0

>> [   86.286099] [<ffffff8008a073e8>] schedule+0x38/0xa0

>> [   86.291017] [<ffffff8008a0a6cc>] schedule_hrtimeout_range_clock+0x8c/0xf0

>> [   86.297846] [<ffffff8008a0a740>] schedule_hrtimeout_range+0x10/0x18

>> [   86.304150] [<ffffff8008a0a4a0>] usleep_range+0x50/0x58

>> [   86.309418] [<ffffff800866d8dc>] dwc2_wait_for_mode.isra.4+0x54/0xd0

>> [   86.315815] [<ffffff800866f058>] dwc2_core_reset+0xe0/0x168

>> [   86.321431] [<ffffff800867e364>] dwc2_hsotg_core_init_disconnected+0x2c/0x310

>> [   86.328602] [<ffffff8008673568>] dwc2_conn_id_status_change+0x130/0x250

>> [   86.335254] [<ffffff80080ccd48>] process_one_work+0x118/0x370

>> [   86.341035] [<ffffff80080ccfe8>] worker_thread+0x48/0x498

>> [   86.346473] [<ffffff80080d2eb0>] kthread+0xd0/0xe8

>> [   86.351299] [<ffffff8008082e80>] ret_from_fork+0x10/0x50

>>

>> This seems to be caused by the dwc2_wait_for_mode() calling

>> usleep_range() while the hstog->lock spinlock is held, since

>> we take that before calling dwc2_hsotg_core_init_disconnected().

>>

>> This patch avoids the issue by adding an extra argument to

>> dwc2_core_reset(), as suggested by John Youn, which allows us to

>> skip the waiting, which should be unnecessary when calling from

>> dwc2_hsotg_core_init_disconnected().

>>

>> Cc: Wei Xu <xuwei5@hisilicon.com>

>> Cc: Guodong Xu <guodong.xu@linaro.org>

>> Cc: Amit Pundir <amit.pundir@linaro.org>

>> Cc: Rob Herring <robh+dt@kernel.org>

>> Cc: John Youn <johnyoun@synopsys.com>

>> Cc: Douglas Anderson <dianders@chromium.org>

>> Cc: Chen Yu <chenyu56@huawei.com>

>> Cc: Vardan Mikayelyan <mvardan@synopsys.com>

>> Cc: Kishon Vijay Abraham I <kishon@ti.com>

>> Cc: Felipe Balbi <felipe.balbi@linux.intel.com>

>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

>> Cc: linux-usb@vger.kernel.org

>> Signed-off-by: John Stultz <john.stultz@linaro.org>

>> ---

>

> doesn't apply to my testing/next. Please rebase


So these were rebased onto JohnY's tree here:
  https://github.com/synopsys-usb/linux.git next

And apparently have been merged there. I suspect he's going to submit
his entire tree there to you?

JohnY: Is this right?

thanks
-john
John Youn Jan. 16, 2017, 8:57 p.m. UTC | #3 
> On Jan 16, 2017, at 12:37 PM, John Stultz <john.stultz@linaro.org> wrote:

> 

> On Mon, Jan 16, 2017 at 2:36 AM, Felipe Balbi

> <felipe.balbi@linux.intel.com> wrote:

>> 

>> Hi,

>> 

>> John Stultz <john.stultz@linaro.org> writes:

>>> Basically when plugging in various cables in different orders, I'm

>>> occasionally seeing the following BUG splat:

>>> 

>>> [   86.215403] BUG: scheduling while atomic: kworker/u16:2/53/0x00000002

>>> [   86.219164] usb 1-1: USB disconnect, device number 9

>>> [   86.226845] Preemption disabled at:[   86.230218]

>>> [<ffffff8008673558>] dwc2_conn_id_status_change+0x120/0x250

>>> [   86.236894] CPU: 0 PID: 53 Comm: kworker/u16:2 Tainted: G        W

>>>     4.9.0-rc8-00051-gd5a7979-dirty #1702

>>> [   86.246836] Hardware name: HiKey Development Board (DT)

>>> [   86.252100] Workqueue: dwc2 dwc2_conn_id_status_change

>>> [   86.257279] Call trace:

>>> [   86.259771] [<ffffff8008087c28>] dump_backtrace+0x0/0x1a0

>>> [   86.265210] [<ffffff8008087ddc>] show_stack+0x14/0x20

>>> [   86.270308] [<ffffff80084343f0>] dump_stack+0x90/0xb0

>>> [   86.275401] [<ffffff80080d8d94>] __schedule_bug+0x6c/0xb8

>>> [   86.280841] [<ffffff8008a07220>] __schedule+0x4f8/0x5b0

>>> [   86.286099] [<ffffff8008a073e8>] schedule+0x38/0xa0

>>> [   86.291017] [<ffffff8008a0a6cc>] schedule_hrtimeout_range_clock+0x8c/0xf0

>>> [   86.297846] [<ffffff8008a0a740>] schedule_hrtimeout_range+0x10/0x18

>>> [   86.304150] [<ffffff8008a0a4a0>] usleep_range+0x50/0x58

>>> [   86.309418] [<ffffff800866d8dc>] dwc2_wait_for_mode.isra.4+0x54/0xd0

>>> [   86.315815] [<ffffff800866f058>] dwc2_core_reset+0xe0/0x168

>>> [   86.321431] [<ffffff800867e364>] dwc2_hsotg_core_init_disconnected+0x2c/0x310

>>> [   86.328602] [<ffffff8008673568>] dwc2_conn_id_status_change+0x130/0x250

>>> [   86.335254] [<ffffff80080ccd48>] process_one_work+0x118/0x370

>>> [   86.341035] [<ffffff80080ccfe8>] worker_thread+0x48/0x498

>>> [   86.346473] [<ffffff80080d2eb0>] kthread+0xd0/0xe8

>>> [   86.351299] [<ffffff8008082e80>] ret_from_fork+0x10/0x50

>>> 

>>> This seems to be caused by the dwc2_wait_for_mode() calling

>>> usleep_range() while the hstog->lock spinlock is held, since

>>> we take that before calling dwc2_hsotg_core_init_disconnected().

>>> 

>>> This patch avoids the issue by adding an extra argument to

>>> dwc2_core_reset(), as suggested by John Youn, which allows us to

>>> skip the waiting, which should be unnecessary when calling from

>>> dwc2_hsotg_core_init_disconnected().

>>> 

>>> Cc: Wei Xu <xuwei5@hisilicon.com>

>>> Cc: Guodong Xu <guodong.xu@linaro.org>

>>> Cc: Amit Pundir <amit.pundir@linaro.org>

>>> Cc: Rob Herring <robh+dt@kernel.org>

>>> Cc: John Youn <johnyoun@synopsys.com>

>>> Cc: Douglas Anderson <dianders@chromium.org>

>>> Cc: Chen Yu <chenyu56@huawei.com>

>>> Cc: Vardan Mikayelyan <mvardan@synopsys.com>

>>> Cc: Kishon Vijay Abraham I <kishon@ti.com>

>>> Cc: Felipe Balbi <felipe.balbi@linux.intel.com>

>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

>>> Cc: linux-usb@vger.kernel.org

>>> Signed-off-by: John Stultz <john.stultz@linaro.org>

>>> ---

>> 

>> doesn't apply to my testing/next. Please rebase

> 

> So these were rebased onto JohnY's tree here:

>  https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_synopsys-2Dusb_linux.git&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=U3o8uKoKhWme5_V9D-eeCkB11BFwt4KvWztBgdE9ZpA&m=eBJTBuJyU21iKJvHdy5FxqtxsVARo0iIqJGxMHrlbyQ&s=VIiAT32aG7s04G5NoOOthNdm2JX0eWjJpg62neY_-KI&e=  next

> 

> And apparently have been merged there. I suspect he's going to submit

> his entire tree there to you?

> 

> JohnY: Is this right?



Yeah I'll get these issues sorted out with Felipe. Which may mean resubmitting everything the the proper order.

Thanks,
John
diff mbox

Patch

diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index c987547..7195366 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -313,7 +313,7 @@  static bool dwc2_iddig_filter_enabled(struct dwc2_hsotg *hsotg)
  * Do core a soft reset of the core.  Be careful with this because it
  * resets all the internal state machines of the core.
  */
-int dwc2_core_reset(struct dwc2_hsotg *hsotg)
+int dwc2_core_reset(struct dwc2_hsotg *hsotg, bool skip_wait)
 {
 	u32 greset;
 	int count = 0;
@@ -369,7 +369,7 @@  int dwc2_core_reset(struct dwc2_hsotg *hsotg)
 		}
 	} while (!(greset & GRSTCTL_AHBIDLE));
 
-	if (wait_for_host_mode)
+	if (wait_for_host_mode && !skip_wait)
 		dwc2_wait_for_mode(hsotg, true);
 
 	return 0;
@@ -500,7 +500,7 @@  int dwc2_core_reset_and_force_dr_mode(struct dwc2_hsotg *hsotg)
 {
 	int retval;
 
-	retval = dwc2_core_reset(hsotg);
+	retval = dwc2_core_reset(hsotg, false);
 	if (retval)
 		return retval;
 
diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 013feb9..05ce52c 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -1089,7 +1089,7 @@  static inline bool dwc2_is_hs_iot(struct dwc2_hsotg *hsotg)
  * The following functions support initialization of the core driver component
  * and the DWC_otg controller
  */
-int dwc2_core_reset(struct dwc2_hsotg *hsotg);
+int dwc2_core_reset(struct dwc2_hsotg *hsotg, bool skip_wait);
 int dwc2_core_reset_and_force_dr_mode(struct dwc2_hsotg *hsotg);
 int dwc2_enter_hibernation(struct dwc2_hsotg *hsotg);
 int dwc2_exit_hibernation(struct dwc2_hsotg *hsotg, bool restore);
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 242930e..e9cf162 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -3161,7 +3161,7 @@  void dwc2_hsotg_core_init_disconnected(struct dwc2_hsotg *hsotg,
 	kill_all_requests(hsotg, hsotg->eps_out[0], -ECONNRESET);
 
 	if (!is_usb_reset)
-		if (dwc2_core_reset(hsotg))
+		if (dwc2_core_reset(hsotg, true))
 			return;
 
 	/*