* [PATCH] usb: hub: Fix usb enumeration issue due to address0 race
@ 2021-11-15 22:16 Mathias Nyman
2021-11-16 8:22 ` Greg KH
[not found] ` <CGME20211118111915eucas1p2cf4a502442e7259c6c347daf0d87259e@eucas1p2.samsung.com>
0 siblings, 2 replies; 11+ messages in thread
From: Mathias Nyman @ 2021-11-15 22:16 UTC (permalink / raw)
To: gregkh, stern, kishon
Cc: hdegoede, chris.chiu, linux-usb, Mathias Nyman, stable
xHC hardware can only have one slot in default state with address 0
waiting for a unique address at a time, otherwise "undefined behavior
may occur" according to xhci spec 5.4.3.4
The address0_mutex exists to prevent this across both xhci roothubs.
If hub_port_init() fails, it may unlock the mutex and exit with a xhci
slot in default state. If the other xhci roothub calls hub_port_init()
at this point we end up with two slots in default state.
Make sure the address0_mutex protects the slot default state across
hub_port_init() retries, until slot is addressed or disabled.
Note, one known minor case is not fixed by this patch.
If device needs to be reset during resume, but fails all hub_port_init()
retries in usb_reset_and_verify_device(), then it's possible the slot is
still left in default state when address0_mutex is unlocked.
Cc: <stable@vger.kernel.org>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
drivers/usb/core/hub.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 86658a81d284..00c3506324e4 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -4700,8 +4700,6 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1,
if (oldspeed == USB_SPEED_LOW)
delay = HUB_LONG_RESET_TIME;
- mutex_lock(hcd->address0_mutex);
-
/* Reset the device; full speed may morph to high speed */
/* FIXME a USB 2.0 device may morph into SuperSpeed on reset. */
retval = hub_port_reset(hub, port1, udev, delay, false);
@@ -5016,7 +5014,6 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1,
hub_port_disable(hub, port1, 0);
update_devnum(udev, devnum); /* for disconnect processing */
}
- mutex_unlock(hcd->address0_mutex);
return retval;
}
@@ -5246,6 +5243,9 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
unit_load = 100;
status = 0;
+
+ mutex_lock(hcd->address0_mutex);
+
for (i = 0; i < PORT_INIT_TRIES; i++) {
/* reallocate for each attempt, since references
@@ -5282,6 +5282,8 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
if (status < 0)
goto loop;
+ mutex_unlock(hcd->address0_mutex);
+
if (udev->quirks & USB_QUIRK_DELAY_INIT)
msleep(2000);
@@ -5370,6 +5372,7 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
loop_disable:
hub_port_disable(hub, port1, 1);
+ mutex_lock(hcd->address0_mutex);
loop:
usb_ep0_reinit(udev);
release_devnum(udev);
@@ -5396,6 +5399,8 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
}
done:
+ mutex_unlock(hcd->address0_mutex);
+
hub_port_disable(hub, port1, 1);
if (hcd->driver->relinquish_port && !hub->hdev->parent) {
if (status != -ENOTCONN && status != -ENODEV)
@@ -5915,6 +5920,8 @@ static int usb_reset_and_verify_device(struct usb_device *udev)
bos = udev->bos;
udev->bos = NULL;
+ mutex_lock(hcd->address0_mutex);
+
for (i = 0; i < PORT_INIT_TRIES; ++i) {
/* ep0 maxpacket size may change; let the HCD know about it.
@@ -5924,6 +5931,7 @@ static int usb_reset_and_verify_device(struct usb_device *udev)
if (ret >= 0 || ret == -ENOTCONN || ret == -ENODEV)
break;
}
+ mutex_unlock(hcd->address0_mutex);
if (ret < 0)
goto re_enumerate;
--
2.25.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] usb: hub: Fix usb enumeration issue due to address0 race
2021-11-15 22:16 [PATCH] usb: hub: Fix usb enumeration issue due to address0 race Mathias Nyman
@ 2021-11-16 8:22 ` Greg KH
2021-11-16 8:39 ` Mathias Nyman
[not found] ` <CGME20211118111915eucas1p2cf4a502442e7259c6c347daf0d87259e@eucas1p2.samsung.com>
1 sibling, 1 reply; 11+ messages in thread
From: Greg KH @ 2021-11-16 8:22 UTC (permalink / raw)
To: Mathias Nyman; +Cc: stern, kishon, hdegoede, chris.chiu, linux-usb, stable
On Tue, Nov 16, 2021 at 12:16:30AM +0200, Mathias Nyman wrote:
> xHC hardware can only have one slot in default state with address 0
> waiting for a unique address at a time, otherwise "undefined behavior
> may occur" according to xhci spec 5.4.3.4
>
> The address0_mutex exists to prevent this across both xhci roothubs.
>
> If hub_port_init() fails, it may unlock the mutex and exit with a xhci
> slot in default state. If the other xhci roothub calls hub_port_init()
> at this point we end up with two slots in default state.
>
> Make sure the address0_mutex protects the slot default state across
> hub_port_init() retries, until slot is addressed or disabled.
>
> Note, one known minor case is not fixed by this patch.
> If device needs to be reset during resume, but fails all hub_port_init()
> retries in usb_reset_and_verify_device(), then it's possible the slot is
> still left in default state when address0_mutex is unlocked.
>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
What commit id does this "fix"?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] usb: hub: Fix usb enumeration issue due to address0 race
2021-11-16 8:22 ` Greg KH
@ 2021-11-16 8:39 ` Mathias Nyman
0 siblings, 0 replies; 11+ messages in thread
From: Mathias Nyman @ 2021-11-16 8:39 UTC (permalink / raw)
To: Greg KH; +Cc: stern, kishon, hdegoede, chris.chiu, linux-usb, stable
On 16.11.2021 10.22, Greg KH wrote:
> On Tue, Nov 16, 2021 at 12:16:30AM +0200, Mathias Nyman wrote:
>> xHC hardware can only have one slot in default state with address 0
>> waiting for a unique address at a time, otherwise "undefined behavior
>> may occur" according to xhci spec 5.4.3.4
>>
>> The address0_mutex exists to prevent this across both xhci roothubs.
>>
>> If hub_port_init() fails, it may unlock the mutex and exit with a xhci
>> slot in default state. If the other xhci roothub calls hub_port_init()
>> at this point we end up with two slots in default state.
>>
>> Make sure the address0_mutex protects the slot default state across
>> hub_port_init() retries, until slot is addressed or disabled.
>>
>> Note, one known minor case is not fixed by this patch.
>> If device needs to be reset during resume, but fails all hub_port_init()
>> retries in usb_reset_and_verify_device(), then it's possible the slot is
>> still left in default state when address0_mutex is unlocked.
>>
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>
> What commit id does this "fix"?
>
Looks like original cause could be:
638139eb95d2 ("usb: hub: allow to process more usb hub events in parallel")
which was partially fixed in 4.7 by:
feb26ac31a2a ("usb: core: hub: hub_port_init lock controller instead of bus")
And now improved by this patch
-Mathias
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] usb: hub: Fix usb enumeration issue due to address0 race
[not found] ` <CGME20211118111915eucas1p2cf4a502442e7259c6c347daf0d87259e@eucas1p2.samsung.com>
@ 2021-11-18 11:19 ` Marek Szyprowski
2021-11-18 13:50 ` Mathias Nyman
0 siblings, 1 reply; 11+ messages in thread
From: Marek Szyprowski @ 2021-11-18 11:19 UTC (permalink / raw)
To: Mathias Nyman, gregkh, stern, kishon
Cc: hdegoede, chris.chiu, linux-usb, stable
Hi,
On 15.11.2021 23:16, Mathias Nyman wrote:
> xHC hardware can only have one slot in default state with address 0
> waiting for a unique address at a time, otherwise "undefined behavior
> may occur" according to xhci spec 5.4.3.4
>
> The address0_mutex exists to prevent this across both xhci roothubs.
>
> If hub_port_init() fails, it may unlock the mutex and exit with a xhci
> slot in default state. If the other xhci roothub calls hub_port_init()
> at this point we end up with two slots in default state.
>
> Make sure the address0_mutex protects the slot default state across
> hub_port_init() retries, until slot is addressed or disabled.
>
> Note, one known minor case is not fixed by this patch.
> If device needs to be reset during resume, but fails all hub_port_init()
> retries in usb_reset_and_verify_device(), then it's possible the slot is
> still left in default state when address0_mutex is unlocked.
>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
This patch landed in linux next-20211118 as commit 6ae6dc22d2d1 ("usb:
hub: Fix usb enumeration issue due to address0 race"). On my test
systems it triggers the following deplock warning during system
suspend/resume cycle:
======================================================
WARNING: possible circular locking dependency detected
5.16.0-rc1-00014-g6ae6dc22d2d1 #4126 Not tainted
------------------------------------------------------
kworker/u16:8/738 is trying to acquire lock:
cf81f738 (hcd->address0_mutex){+.+.}-{3:3}, at:
usb_reset_and_verify_device+0xe8/0x3e4
but task is already holding lock:
cf80ab3c (&port_dev->status_lock){+.+.}-{3:3}, at:
usb_port_resume+0xa0/0x7e8
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (&port_dev->status_lock){+.+.}-{3:3}:
mutex_lock_nested+0x1c/0x24
hub_event+0x824/0x1758
process_one_work+0x2c8/0x7ec
worker_thread+0x50/0x584
kthread+0x13c/0x19c
ret_from_fork+0x14/0x2c
0x0
-> #0 (hcd->address0_mutex){+.+.}-{3:3}:
lock_acquire+0x2a0/0x42c
__mutex_lock+0x94/0xaa8
mutex_lock_nested+0x1c/0x24
usb_reset_and_verify_device+0xe8/0x3e4
usb_port_resume+0x4e0/0x7e8
usb_generic_driver_resume+0x18/0x40
usb_resume_both+0x120/0x164
usb_resume+0x14/0x60
usb_dev_resume+0xc/0x10
dpm_run_callback+0x98/0x32c
device_resume+0xb4/0x258
async_resume+0x20/0x64
async_run_entry_fn+0x40/0x15c
process_one_work+0x2c8/0x7ec
worker_thread+0x50/0x584
kthread+0x13c/0x19c
ret_from_fork+0x14/0x2c
0x0
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&port_dev->status_lock);
lock(hcd->address0_mutex);
lock(&port_dev->status_lock);
lock(hcd->address0_mutex);
*** DEADLOCK ***
4 locks held by kworker/u16:8/738:
#0: c1c08ca8 ((wq_completion)events_unbound){+.+.}-{0:0}, at:
process_one_work+0x21c/0x7ec
#1: cd9cff18 ((work_completion)(&entry->work)){+.+.}-{0:0}, at:
process_one_work+0x21c/0x7ec
#2: cf810148 (&dev->mutex){....}-{3:3}, at: device_resume+0x60/0x258
#3: cf80ab3c (&port_dev->status_lock){+.+.}-{3:3}, at:
usb_port_resume+0xa0/0x7e8
stack backtrace:
CPU: 4 PID: 738 Comm: kworker/u16:8 Not tainted
5.16.0-rc1-00014-g6ae6dc22d2d1 #4126
Hardware name: Samsung Exynos (Flattened Device Tree)
Workqueue: events_unbound async_run_entry_fn
[<c01110d0>] (unwind_backtrace) from [<c010cab8>] (show_stack+0x10/0x14)
[<c010cab8>] (show_stack) from [<c0b7427c>] (dump_stack_lvl+0x58/0x70)
[<c0b7427c>] (dump_stack_lvl) from [<c0192958>]
(check_noncircular+0x144/0x15c)
[<c0192958>] (check_noncircular) from [<c0195e68>]
(__lock_acquire+0x17e4/0x3180)
[<c0195e68>] (__lock_acquire) from [<c01983ec>] (lock_acquire+0x2a0/0x42c)
[<c01983ec>] (lock_acquire) from [<c0b7ba80>] (__mutex_lock+0x94/0xaa8)
[<c0b7ba80>] (__mutex_lock) from [<c0b7c4b0>] (mutex_lock_nested+0x1c/0x24)
[<c0b7c4b0>] (mutex_lock_nested) from [<c077bf9c>]
(usb_reset_and_verify_device+0xe8/0x3e4)
[<c077bf9c>] (usb_reset_and_verify_device) from [<c077e79c>]
(usb_port_resume+0x4e0/0x7e8)
[<c077e79c>] (usb_port_resume) from [<c07941f0>]
(usb_generic_driver_resume+0x18/0x40)
[<c07941f0>] (usb_generic_driver_resume) from [<c0789a40>]
(usb_resume_both+0x120/0x164)
[<c0789a40>] (usb_resume_both) from [<c078a854>] (usb_resume+0x14/0x60)
[<c078a854>] (usb_resume) from [<c0777fa0>] (usb_dev_resume+0xc/0x10)
[<c0777fa0>] (usb_dev_resume) from [<c06ca1b8>]
(dpm_run_callback+0x98/0x32c)
[<c06ca1b8>] (dpm_run_callback) from [<c06ca500>] (device_resume+0xb4/0x258)
[<c06ca500>] (device_resume) from [<c06ca6c4>] (async_resume+0x20/0x64)
[<c06ca6c4>] (async_resume) from [<c01548bc>]
(async_run_entry_fn+0x40/0x15c)
[<c01548bc>] (async_run_entry_fn) from [<c0148a68>]
(process_one_work+0x2c8/0x7ec)
[<c0148a68>] (process_one_work) from [<c0148fdc>] (worker_thread+0x50/0x584)
[<c0148fdc>] (worker_thread) from [<c0151274>] (kthread+0x13c/0x19c)
[<c0151274>] (kthread) from [<c0100108>] (ret_from_fork+0x14/0x2c)
Exception stack(0xcd9cffb0 to 0xcd9cfff8)
ffa0: 00000000 00000000 00000000
00000000
ffc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
00000000
ffe0: 00000000 00000000 00000000 00000000 00000013 00000000
usb 1-1: reset high-speed USB device number 2 using exynos-ehci
usb 1-1.1: reset high-speed USB device number 3 using exynos-ehci
usb usb3: root hub lost power or was reset
usb usb4: root hub lost power or was reset
s3c-rtc 101e0000.rtc: rtc disabled, re-enabling
smsc95xx 1-1.1:1.0 eth0: Link is Down
OOM killer enabled.
Restarting tasks ... done.
PM: suspend exit
Reverting it on top of next-20211118 fixes/hides this warning. Let me
know if I can help somehow fixing this issue.
> ---
> drivers/usb/core/hub.c | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 86658a81d284..00c3506324e4 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -4700,8 +4700,6 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1,
> if (oldspeed == USB_SPEED_LOW)
> delay = HUB_LONG_RESET_TIME;
>
> - mutex_lock(hcd->address0_mutex);
> -
> /* Reset the device; full speed may morph to high speed */
> /* FIXME a USB 2.0 device may morph into SuperSpeed on reset. */
> retval = hub_port_reset(hub, port1, udev, delay, false);
> @@ -5016,7 +5014,6 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1,
> hub_port_disable(hub, port1, 0);
> update_devnum(udev, devnum); /* for disconnect processing */
> }
> - mutex_unlock(hcd->address0_mutex);
> return retval;
> }
>
> @@ -5246,6 +5243,9 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
> unit_load = 100;
>
> status = 0;
> +
> + mutex_lock(hcd->address0_mutex);
> +
> for (i = 0; i < PORT_INIT_TRIES; i++) {
>
> /* reallocate for each attempt, since references
> @@ -5282,6 +5282,8 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
> if (status < 0)
> goto loop;
>
> + mutex_unlock(hcd->address0_mutex);
> +
> if (udev->quirks & USB_QUIRK_DELAY_INIT)
> msleep(2000);
>
> @@ -5370,6 +5372,7 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
>
> loop_disable:
> hub_port_disable(hub, port1, 1);
> + mutex_lock(hcd->address0_mutex);
> loop:
> usb_ep0_reinit(udev);
> release_devnum(udev);
> @@ -5396,6 +5399,8 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
> }
>
> done:
> + mutex_unlock(hcd->address0_mutex);
> +
> hub_port_disable(hub, port1, 1);
> if (hcd->driver->relinquish_port && !hub->hdev->parent) {
> if (status != -ENOTCONN && status != -ENODEV)
> @@ -5915,6 +5920,8 @@ static int usb_reset_and_verify_device(struct usb_device *udev)
> bos = udev->bos;
> udev->bos = NULL;
>
> + mutex_lock(hcd->address0_mutex);
> +
> for (i = 0; i < PORT_INIT_TRIES; ++i) {
>
> /* ep0 maxpacket size may change; let the HCD know about it.
> @@ -5924,6 +5931,7 @@ static int usb_reset_and_verify_device(struct usb_device *udev)
> if (ret >= 0 || ret == -ENOTCONN || ret == -ENODEV)
> break;
> }
> + mutex_unlock(hcd->address0_mutex);
>
> if (ret < 0)
> goto re_enumerate;
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] usb: hub: Fix usb enumeration issue due to address0 race
2021-11-18 11:19 ` Marek Szyprowski
@ 2021-11-18 13:50 ` Mathias Nyman
2021-11-22 10:44 ` Mathias Nyman
0 siblings, 1 reply; 11+ messages in thread
From: Mathias Nyman @ 2021-11-18 13:50 UTC (permalink / raw)
To: Marek Szyprowski, gregkh, stern, kishon
Cc: hdegoede, chris.chiu, linux-usb, stable
On 18.11.2021 13.19, Marek Szyprowski wrote:
> Hi,
>
> On 15.11.2021 23:16, Mathias Nyman wrote:
>> xHC hardware can only have one slot in default state with address 0
>> waiting for a unique address at a time, otherwise "undefined behavior
>> may occur" according to xhci spec 5.4.3.4
>>
>> The address0_mutex exists to prevent this across both xhci roothubs.
>>
>> If hub_port_init() fails, it may unlock the mutex and exit with a xhci
>> slot in default state. If the other xhci roothub calls hub_port_init()
>> at this point we end up with two slots in default state.
>>
>> Make sure the address0_mutex protects the slot default state across
>> hub_port_init() retries, until slot is addressed or disabled.
>>
>> Note, one known minor case is not fixed by this patch.
>> If device needs to be reset during resume, but fails all hub_port_init()
>> retries in usb_reset_and_verify_device(), then it's possible the slot is
>> still left in default state when address0_mutex is unlocked.
>>
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>
> This patch landed in linux next-20211118 as commit 6ae6dc22d2d1 ("usb:
> hub: Fix usb enumeration issue due to address0 race"). On my test
> systems it triggers the following deplock warning during system
> suspend/resume cycle:
>
> ======================================================
> WARNING: possible circular locking dependency detected
> 5.16.0-rc1-00014-g6ae6dc22d2d1 #4126 Not tainted
> ------------------------------------------------------
> kworker/u16:8/738 is trying to acquire lock:
> cf81f738 (hcd->address0_mutex){+.+.}-{3:3}, at:
> usb_reset_and_verify_device+0xe8/0x3e4
>
> but task is already holding lock:
> cf80ab3c (&port_dev->status_lock){+.+.}-{3:3}, at:
> usb_port_resume+0xa0/0x7e8
>
Thanks, I see it now.
Lock order is:
mutex_lock(&port_dev->status_lock)
mutex_lock(hcd->address0_mutex)
mutex_unlock(hcd->address0_mutex)
mutex_unlock(&port_dev->status_lock)
in hub_port_connect(), usb_port_resume() and usb_reset_device()
But patch changed the status_lock and address0_mutex lock order in hub_port_connect().
Lets see if we can take the status_lock a bit earlier in hub_port_connect() to
solve this.
-Mathias
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] usb: hub: Fix usb enumeration issue due to address0 race
2021-11-18 13:50 ` Mathias Nyman
@ 2021-11-22 10:44 ` Mathias Nyman
2021-11-22 10:50 ` [RFT PATCH] usb: hub: Fix locking issues with address0_mutex Mathias Nyman
0 siblings, 1 reply; 11+ messages in thread
From: Mathias Nyman @ 2021-11-22 10:44 UTC (permalink / raw)
To: Marek Szyprowski, gregkh, stern, kishon
Cc: hdegoede, chris.chiu, linux-usb, stable
On 18.11.2021 15.50, Mathias Nyman wrote:
> On 18.11.2021 13.19, Marek Szyprowski wrote:
>> Hi,
>>
>> On 15.11.2021 23:16, Mathias Nyman wrote:
>>> xHC hardware can only have one slot in default state with address 0
>>> waiting for a unique address at a time, otherwise "undefined behavior
>>> may occur" according to xhci spec 5.4.3.4
>>>
>>> The address0_mutex exists to prevent this across both xhci roothubs.
>>>
>>> If hub_port_init() fails, it may unlock the mutex and exit with a xhci
>>> slot in default state. If the other xhci roothub calls hub_port_init()
>>> at this point we end up with two slots in default state.
>>>
>>> Make sure the address0_mutex protects the slot default state across
>>> hub_port_init() retries, until slot is addressed or disabled.
>>>
>>> Note, one known minor case is not fixed by this patch.
>>> If device needs to be reset during resume, but fails all hub_port_init()
>>> retries in usb_reset_and_verify_device(), then it's possible the slot is
>>> still left in default state when address0_mutex is unlocked.
>>>
>>> Cc: <stable@vger.kernel.org>
>>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>>
>> This patch landed in linux next-20211118 as commit 6ae6dc22d2d1 ("usb:
>> hub: Fix usb enumeration issue due to address0 race"). On my test
>> systems it triggers the following deplock warning during system
>> suspend/resume cycle:
>>
>> ======================================================
>> WARNING: possible circular locking dependency detected
>> 5.16.0-rc1-00014-g6ae6dc22d2d1 #4126 Not tainted
>> ------------------------------------------------------
>> kworker/u16:8/738 is trying to acquire lock:
>> cf81f738 (hcd->address0_mutex){+.+.}-{3:3}, at:
>> usb_reset_and_verify_device+0xe8/0x3e4
>>
>> but task is already holding lock:
>> cf80ab3c (&port_dev->status_lock){+.+.}-{3:3}, at:
>> usb_port_resume+0xa0/0x7e8
>>
>
> Thanks, I see it now.
>
> Lock order is:
> mutex_lock(&port_dev->status_lock)
> mutex_lock(hcd->address0_mutex)
> mutex_unlock(hcd->address0_mutex)
> mutex_unlock(&port_dev->status_lock)
> in hub_port_connect(), usb_port_resume() and usb_reset_device()
>
> But patch changed the status_lock and address0_mutex lock order in hub_port_connect().
> Lets see if we can take the status_lock a bit earlier in hub_port_connect() to
> solve this.
>
I can easily reproduce this myself now.
I'll send a patch on top of this one to fix it.
Lockdep warnings are gone for me after applying it,
Also fixes an unbalanced address0_mutex unlock in error codepath.
Grateful if someone else could try it out as well.
Thanks
-Mathias
^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFT PATCH] usb: hub: Fix locking issues with address0_mutex
2021-11-22 10:44 ` Mathias Nyman
@ 2021-11-22 10:50 ` Mathias Nyman
2021-11-22 12:27 ` Marek Szyprowski
2021-11-22 15:41 ` Hans de Goede
0 siblings, 2 replies; 11+ messages in thread
From: Mathias Nyman @ 2021-11-22 10:50 UTC (permalink / raw)
To: m.szyprowski, gregkh, stern, kishon
Cc: hdegoede, chris.chiu, linux-usb, Mathias Nyman, stable
Fix the circular lock dependency and unbalanced unlock of addess0_mutex
introduced when fixing an address0_mutex enumeration retry race in commit
ae6dc22d2d1 ("usb: hub: Fix usb enumeration issue due to address0 race")
Make sure locking order between port_dev->status_lock and address0_mutex
is correct, and that address0_mutex is not unlocked in hub_port_connect
"done:" codepath which may be reached without locking address0_mutex
Fixes: 6ae6dc22d2d1 ("usb: hub: Fix usb enumeration issue due to address0 race")
Cc: <stable@vger.kernel.org>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
drivers/usb/core/hub.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 00c3506324e4..00070a8a6507 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -5188,6 +5188,7 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
struct usb_port *port_dev = hub->ports[port1 - 1];
struct usb_device *udev = port_dev->child;
static int unreliable_port = -1;
+ bool retry_locked;
/* Disconnect any existing devices under this port */
if (udev) {
@@ -5244,10 +5245,10 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
status = 0;
- mutex_lock(hcd->address0_mutex);
-
for (i = 0; i < PORT_INIT_TRIES; i++) {
-
+ usb_lock_port(port_dev);
+ mutex_lock(hcd->address0_mutex);
+ retry_locked = true;
/* reallocate for each attempt, since references
* to the previous one can escape in various ways
*/
@@ -5255,6 +5256,8 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
if (!udev) {
dev_err(&port_dev->dev,
"couldn't allocate usb_device\n");
+ mutex_unlock(hcd->address0_mutex);
+ usb_unlock_port(port_dev);
goto done;
}
@@ -5276,13 +5279,13 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
}
/* reset (non-USB 3.0 devices) and get descriptor */
- usb_lock_port(port_dev);
status = hub_port_init(hub, udev, port1, i);
- usb_unlock_port(port_dev);
if (status < 0)
goto loop;
mutex_unlock(hcd->address0_mutex);
+ usb_unlock_port(port_dev);
+ retry_locked = false;
if (udev->quirks & USB_QUIRK_DELAY_INIT)
msleep(2000);
@@ -5372,11 +5375,14 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
loop_disable:
hub_port_disable(hub, port1, 1);
- mutex_lock(hcd->address0_mutex);
loop:
usb_ep0_reinit(udev);
release_devnum(udev);
hub_free_dev(udev);
+ if (retry_locked) {
+ mutex_unlock(hcd->address0_mutex);
+ usb_unlock_port(port_dev);
+ }
usb_put_dev(udev);
if ((status == -ENOTCONN) || (status == -ENOTSUPP))
break;
@@ -5399,8 +5405,6 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
}
done:
- mutex_unlock(hcd->address0_mutex);
-
hub_port_disable(hub, port1, 1);
if (hcd->driver->relinquish_port && !hub->hdev->parent) {
if (status != -ENOTCONN && status != -ENODEV)
--
2.25.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [RFT PATCH] usb: hub: Fix locking issues with address0_mutex
2021-11-22 10:50 ` [RFT PATCH] usb: hub: Fix locking issues with address0_mutex Mathias Nyman
@ 2021-11-22 12:27 ` Marek Szyprowski
2021-11-23 9:18 ` Mathias Nyman
2021-11-22 15:41 ` Hans de Goede
1 sibling, 1 reply; 11+ messages in thread
From: Marek Szyprowski @ 2021-11-22 12:27 UTC (permalink / raw)
To: Mathias Nyman, gregkh, stern, kishon
Cc: hdegoede, chris.chiu, linux-usb, stable
Hi,
On 22.11.2021 11:50, Mathias Nyman wrote:
> Fix the circular lock dependency and unbalanced unlock of addess0_mutex
> introduced when fixing an address0_mutex enumeration retry race in commit
> ae6dc22d2d1 ("usb: hub: Fix usb enumeration issue due to address0 race")
>
> Make sure locking order between port_dev->status_lock and address0_mutex
> is correct, and that address0_mutex is not unlocked in hub_port_connect
> "done:" codepath which may be reached without locking address0_mutex
>
> Fixes: 6ae6dc22d2d1 ("usb: hub: Fix usb enumeration issue due to address0 race")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
This fixes the issue I've reported here:
https://lore.kernel.org/all/f3bfcbc7-f701-c74a-09bd-6491d4c8d863@samsung.com/
Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> drivers/usb/core/hub.c | 20 ++++++++++++--------
> 1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 00c3506324e4..00070a8a6507 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -5188,6 +5188,7 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
> struct usb_port *port_dev = hub->ports[port1 - 1];
> struct usb_device *udev = port_dev->child;
> static int unreliable_port = -1;
> + bool retry_locked;
>
> /* Disconnect any existing devices under this port */
> if (udev) {
> @@ -5244,10 +5245,10 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
>
> status = 0;
>
> - mutex_lock(hcd->address0_mutex);
> -
> for (i = 0; i < PORT_INIT_TRIES; i++) {
> -
> + usb_lock_port(port_dev);
> + mutex_lock(hcd->address0_mutex);
> + retry_locked = true;
> /* reallocate for each attempt, since references
> * to the previous one can escape in various ways
> */
> @@ -5255,6 +5256,8 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
> if (!udev) {
> dev_err(&port_dev->dev,
> "couldn't allocate usb_device\n");
> + mutex_unlock(hcd->address0_mutex);
> + usb_unlock_port(port_dev);
> goto done;
> }
>
> @@ -5276,13 +5279,13 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
> }
>
> /* reset (non-USB 3.0 devices) and get descriptor */
> - usb_lock_port(port_dev);
> status = hub_port_init(hub, udev, port1, i);
> - usb_unlock_port(port_dev);
> if (status < 0)
> goto loop;
>
> mutex_unlock(hcd->address0_mutex);
> + usb_unlock_port(port_dev);
> + retry_locked = false;
>
> if (udev->quirks & USB_QUIRK_DELAY_INIT)
> msleep(2000);
> @@ -5372,11 +5375,14 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
>
> loop_disable:
> hub_port_disable(hub, port1, 1);
> - mutex_lock(hcd->address0_mutex);
> loop:
> usb_ep0_reinit(udev);
> release_devnum(udev);
> hub_free_dev(udev);
> + if (retry_locked) {
> + mutex_unlock(hcd->address0_mutex);
> + usb_unlock_port(port_dev);
> + }
> usb_put_dev(udev);
> if ((status == -ENOTCONN) || (status == -ENOTSUPP))
> break;
> @@ -5399,8 +5405,6 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
> }
>
> done:
> - mutex_unlock(hcd->address0_mutex);
> -
> hub_port_disable(hub, port1, 1);
> if (hcd->driver->relinquish_port && !hub->hdev->parent) {
> if (status != -ENOTCONN && status != -ENODEV)
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFT PATCH] usb: hub: Fix locking issues with address0_mutex
2021-11-22 10:50 ` [RFT PATCH] usb: hub: Fix locking issues with address0_mutex Mathias Nyman
2021-11-22 12:27 ` Marek Szyprowski
@ 2021-11-22 15:41 ` Hans de Goede
2021-11-23 9:31 ` Mathias Nyman
1 sibling, 1 reply; 11+ messages in thread
From: Hans de Goede @ 2021-11-22 15:41 UTC (permalink / raw)
To: Mathias Nyman, m.szyprowski, gregkh, stern, kishon
Cc: chris.chiu, linux-usb, stable
Hi,
On 11/22/21 11:50, Mathias Nyman wrote:
> Fix the circular lock dependency and unbalanced unlock of addess0_mutex
> introduced when fixing an address0_mutex enumeration retry race in commit
> ae6dc22d2d1 ("usb: hub: Fix usb enumeration issue due to address0 race")
>
> Make sure locking order between port_dev->status_lock and address0_mutex
> is correct, and that address0_mutex is not unlocked in hub_port_connect
> "done:" codepath which may be reached without locking address0_mutex
>
> Fixes: 6ae6dc22d2d1 ("usb: hub: Fix usb enumeration issue due to address0 race")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
Oh, this is great, with this patch I can finally hot-plug my
thunderbolt dock (and thus a XHCI controller) without the XHCI
controller given a whole bunch of weird errors (and some USB
devices not working), which it does not when already connected at boot.
I also tried the hotplug thingy with the previous fix without
this locking fix and then I actually hit the deadlock and things
like lsusb would hang.
If we can get these 2 fixes together merged soon and also backported
to the stable series that would be great:
Acked-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Hans de Goede <hdegoede@redhat.com>
Regards,
Hans
> ---
> drivers/usb/core/hub.c | 20 ++++++++++++--------
> 1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 00c3506324e4..00070a8a6507 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -5188,6 +5188,7 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
> struct usb_port *port_dev = hub->ports[port1 - 1];
> struct usb_device *udev = port_dev->child;
> static int unreliable_port = -1;
> + bool retry_locked;
>
> /* Disconnect any existing devices under this port */
> if (udev) {
> @@ -5244,10 +5245,10 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
>
> status = 0;
>
> - mutex_lock(hcd->address0_mutex);
> -
> for (i = 0; i < PORT_INIT_TRIES; i++) {
> -
> + usb_lock_port(port_dev);
> + mutex_lock(hcd->address0_mutex);
> + retry_locked = true;
> /* reallocate for each attempt, since references
> * to the previous one can escape in various ways
> */
> @@ -5255,6 +5256,8 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
> if (!udev) {
> dev_err(&port_dev->dev,
> "couldn't allocate usb_device\n");
> + mutex_unlock(hcd->address0_mutex);
> + usb_unlock_port(port_dev);
> goto done;
> }
>
> @@ -5276,13 +5279,13 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
> }
>
> /* reset (non-USB 3.0 devices) and get descriptor */
> - usb_lock_port(port_dev);
> status = hub_port_init(hub, udev, port1, i);
> - usb_unlock_port(port_dev);
> if (status < 0)
> goto loop;
>
> mutex_unlock(hcd->address0_mutex);
> + usb_unlock_port(port_dev);
> + retry_locked = false;
>
> if (udev->quirks & USB_QUIRK_DELAY_INIT)
> msleep(2000);
> @@ -5372,11 +5375,14 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
>
> loop_disable:
> hub_port_disable(hub, port1, 1);
> - mutex_lock(hcd->address0_mutex);
> loop:
> usb_ep0_reinit(udev);
> release_devnum(udev);
> hub_free_dev(udev);
> + if (retry_locked) {
> + mutex_unlock(hcd->address0_mutex);
> + usb_unlock_port(port_dev);
> + }
> usb_put_dev(udev);
> if ((status == -ENOTCONN) || (status == -ENOTSUPP))
> break;
> @@ -5399,8 +5405,6 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
> }
>
> done:
> - mutex_unlock(hcd->address0_mutex);
> -
> hub_port_disable(hub, port1, 1);
> if (hcd->driver->relinquish_port && !hub->hdev->parent) {
> if (status != -ENOTCONN && status != -ENODEV)
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFT PATCH] usb: hub: Fix locking issues with address0_mutex
2021-11-22 12:27 ` Marek Szyprowski
@ 2021-11-23 9:18 ` Mathias Nyman
0 siblings, 0 replies; 11+ messages in thread
From: Mathias Nyman @ 2021-11-23 9:18 UTC (permalink / raw)
To: Marek Szyprowski, gregkh, stern, kishon
Cc: hdegoede, chris.chiu, linux-usb, stable
On 22.11.2021 14.27, Marek Szyprowski wrote:
> Hi,
>
> On 22.11.2021 11:50, Mathias Nyman wrote:
>> Fix the circular lock dependency and unbalanced unlock of addess0_mutex
>> introduced when fixing an address0_mutex enumeration retry race in commit
>> ae6dc22d2d1 ("usb: hub: Fix usb enumeration issue due to address0 race")
>>
>> Make sure locking order between port_dev->status_lock and address0_mutex
>> is correct, and that address0_mutex is not unlocked in hub_port_connect
>> "done:" codepath which may be reached without locking address0_mutex
>>
>> Fixes: 6ae6dc22d2d1 ("usb: hub: Fix usb enumeration issue due to address0 race")
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> This fixes the issue I've reported here:
> https://lore.kernel.org/all/f3bfcbc7-f701-c74a-09bd-6491d4c8d863@samsung.com/
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Thank you for testing, I'll add these tags
-Mathias
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFT PATCH] usb: hub: Fix locking issues with address0_mutex
2021-11-22 15:41 ` Hans de Goede
@ 2021-11-23 9:31 ` Mathias Nyman
0 siblings, 0 replies; 11+ messages in thread
From: Mathias Nyman @ 2021-11-23 9:31 UTC (permalink / raw)
To: Hans de Goede, m.szyprowski, gregkh, stern, kishon
Cc: chris.chiu, linux-usb, stable
On 22.11.2021 17.41, Hans de Goede wrote:
> Hi,
>
> On 11/22/21 11:50, Mathias Nyman wrote:
>> Fix the circular lock dependency and unbalanced unlock of addess0_mutex
>> introduced when fixing an address0_mutex enumeration retry race in commit
>> ae6dc22d2d1 ("usb: hub: Fix usb enumeration issue due to address0 race")
>>
>> Make sure locking order between port_dev->status_lock and address0_mutex
>> is correct, and that address0_mutex is not unlocked in hub_port_connect
>> "done:" codepath which may be reached without locking address0_mutex
>>
>> Fixes: 6ae6dc22d2d1 ("usb: hub: Fix usb enumeration issue due to address0 race")
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>
> Oh, this is great, with this patch I can finally hot-plug my
> thunderbolt dock (and thus a XHCI controller) without the XHCI
> controller given a whole bunch of weird errors (and some USB
> devices not working), which it does not when already connected at boot.
>
> I also tried the hotplug thingy with the previous fix without
> this locking fix and then I actually hit the deadlock and things
> like lsusb would hang.
>
> If we can get these 2 fixes together merged soon and also backported
> to the stable series that would be great:
>
> Acked-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Hans de Goede <hdegoede@redhat.com>
>
> Regards,
>
> Hans
>
Thanks for testing, I'll add your tags and submit this.
-Mathias
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-11-23 9:29 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-15 22:16 [PATCH] usb: hub: Fix usb enumeration issue due to address0 race Mathias Nyman
2021-11-16 8:22 ` Greg KH
2021-11-16 8:39 ` Mathias Nyman
[not found] ` <CGME20211118111915eucas1p2cf4a502442e7259c6c347daf0d87259e@eucas1p2.samsung.com>
2021-11-18 11:19 ` Marek Szyprowski
2021-11-18 13:50 ` Mathias Nyman
2021-11-22 10:44 ` Mathias Nyman
2021-11-22 10:50 ` [RFT PATCH] usb: hub: Fix locking issues with address0_mutex Mathias Nyman
2021-11-22 12:27 ` Marek Szyprowski
2021-11-23 9:18 ` Mathias Nyman
2021-11-22 15:41 ` Hans de Goede
2021-11-23 9:31 ` Mathias Nyman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).