No disconnection event for suspended device at v5.6

DISCUSSION Peter Chen 3 participants 8 messages Apr 15, 2020 – Apr 17, 2020 USB

Share & tools

Peter Chen

author

2020-04-15 10:35 UTC #0 lore raw

Hi Alan,

With commit e5d078af8e5f ("USB: hub: Don't record a connect-change event during reset-resume"), the disconnection event for the suspended device (eg, removable hub with no device connected, auto-suspended mouse) at roothub can't occur due to no hub->change_bits is set. Below is the debug message:

imx_usb 30b20000.usb: at imx_controller_resume usbmisc_imx 30b20200.usbmisc: wakeup int ci_hdrc ci_hdrc.1: at ci_controller_resume usb usb1: usb wakeup-resume
usb usb1: usb auto-resume
ci_hdrc ci_hdrc.1: resume root hub
hub 1-0:1.0: hub_resume
ci_hdrc ci_hdrc.1: GetStatus port:1 status 1c00100a 14 e0 PEC CSC
usb usb1-port1: status 0100 change 0003
hub 1-0:1.0: state 7 ports 1 chg 0000 evt 0000 hub 1-0:1.0: hub_suspend
usb usb1: bus auto-suspend, wakeup 1
ci_hdrc ci_hdrc.1: suspend root hub
ci_hdrc ci_hdrc.1: at ci_runtime_suspend imx_usb 30b20000.usb: at imx_controller_suspend

Signature · 2 lines

Thanks,
Peter Chen

Alan Stern

2020-04-15 19:40 UTC #1 lore raw

Quoted 4 lines · Peter Chen

Hi Alan,
With commit e5d078af8e5f ("USB: hub: Don't record a connect-change event during reset-resume"),

For those who care, this is commit 8099f58f1ecd in the upstream kernel.

Quoted 5 lines

the disconnection event for the suspended device
(eg, removable hub with no device connected, auto-suspended mouse) at roothub can't occur due to no hub->change_bits is set. Below is the debug message:

Quoted 16 lines

imx_usb 30b20000.usb: at imx_controller_resume usbmisc_imx 30b20200.usbmisc: wakeup int ci_hdrc ci_hdrc.1: at ci_controller_resume usb usb1: usb wakeup-resume
usb usb1: usb auto-resume
ci_hdrc ci_hdrc.1: resume root hub
hub 1-0:1.0: hub_resume
ci_hdrc ci_hdrc.1: GetStatus port:1 status 1c00100a 14 e0 PEC CSC
usb usb1-port1: status 0100 change 0003
hub 1-0:1.0: state 7 ports 1 chg 0000 evt 0000 hub 1-0:1.0: hub_suspend
usb usb1: bus auto-suspend, wakeup 1
ci_hdrc ci_hdrc.1: suspend root hub
ci_hdrc ci_hdrc.1: at ci_runtime_suspend imx_usb 30b20000.usb: at imx_controller_suspend

You know, I had a nagging feeling when I wrote that patch that there was some scenario it didn't take into account. This is it -- a device getting disconnected while the its parent hub is in runtime suspend.

Okay, clearly that commit needs to be reverted. Luckily, the commit's Changelog indicates that there is an alternate way of achieving the same goal. The patch below contains both the revert and the new fix. Please let me know if it works.

Paul, I trust this new patch won't mess up your Bluetooth adapter. It should still clear the hub->change_bits entry before the hub workqueue thread wakes up.

Alan Stern

drivers/usb/core/hub.c +14 -0

Index: usb-devel/drivers/usb/core/hub.c
===================================================================
@@ -1219,6 +1219,11 @@ static void hub_activate(struct usb_hub
 #ifdef CONFIG_PM
 			udev->reset_resume = 1;
 #endif
+			/* Don't set the change_bits when the device
+			 * was powered off.
+			 */
+			if (test_bit(port1, hub->power_bits))
+				set_bit(port1, hub->change_bits);
 
 		} else {
 			/* The power session is gone; tell hub_wq */

@@ -3084,6 +3089,15 @@ static int check_port_resume_type(struct
 		if (portchange & USB_PORT_STAT_C_ENABLE)
 			usb_clear_port_feature(hub->hdev, port1,
 					USB_PORT_FEAT_C_ENABLE);
+
+		/*
+		 * Whatever made this reset-resume necessary may have
+		 * turned on the port1 bit in hub->change_bits.  But after
+		 * a successful reset-resume we want the bit to be clear;
+		 * if it was on it would indicate that something happened
+		 * following the reset-resume.
+		 */
+		clear_bit(port1, hub->change_bits);
 	}
 
 	return status;

Paul Zimmerman

2020-04-15 23:22 UTC #2 lore raw

Hi Alan,

Quoted 69 lines · On Wed, 15 Apr 2020 15:40:31 -0400 (EDT) Alan Stern…

Quoted 4 lines · Peter Chen
Hi Alan,
With commit e5d078af8e5f ("USB: hub: Don't record a connect-change event during reset-resume"),
For those who care, this is commit 8099f58f1ecd in the upstream kernel.
Quoted 5 lines
the disconnection event for the suspended device
(eg, removable hub with no device connected, auto-suspended mouse) at roothub can't occur due to no hub->change_bits is set. Below is the debug message:
Quoted 16 lines
imx_usb 30b20000.usb: at imx_controller_resume usbmisc_imx 30b20200.usbmisc: wakeup int ci_hdrc ci_hdrc.1: at ci_controller_resume usb usb1: usb wakeup-resume
usb usb1: usb auto-resume
ci_hdrc ci_hdrc.1: resume root hub
hub 1-0:1.0: hub_resume
ci_hdrc ci_hdrc.1: GetStatus port:1 status 1c00100a 14 e0 PEC CSC
usb usb1-port1: status 0100 change 0003
hub 1-0:1.0: state 7 ports 1 chg 0000 evt 0000 hub 1-0:1.0: hub_suspend
usb usb1: bus auto-suspend, wakeup 1
ci_hdrc ci_hdrc.1: suspend root hub
ci_hdrc ci_hdrc.1: at ci_runtime_suspend imx_usb 30b20000.usb: at imx_controller_suspend
You know, I had a nagging feeling when I wrote that patch that there was some scenario it didn't take into account. This is it -- a device getting disconnected while the its parent hub is in runtime suspend.
Okay, clearly that commit needs to be reverted. Luckily, the commit's Changelog indicates that there is an alternate way of achieving the same goal. The patch below contains both the revert and the new fix. Please let me know if it works.
Paul, I trust this new patch won't mess up your Bluetooth adapter. It should still clear the hub->change_bits entry before the hub workqueue thread wakes up.
Alan Stern
Index: usb-devel/drivers/usb/core/hub.c
===================================================================
drivers/usb/core/hub.c +7 -0
@@ -1219,6 +1219,11 @@ static void hub_activate(struct usb_hub
 #ifdef CONFIG_PM
 			udev->reset_resume = 1;
 #endif
+			/* Don't set the change_bits when the device
+			 * was powered off.
+			 */
+			if (test_bit(port1, hub->power_bits))
+				set_bit(port1, hub->change_bits);
 
 		} else {
 			/* The power session is gone; tell hub_wq */
@@ -3084,6 +3089,15 @@ static int check_port_resume_type(struct
 		if (portchange & USB_PORT_STAT_C_ENABLE)
 			usb_clear_port_feature(hub->hdev, port1,
 					USB_PORT_FEAT_C_ENABLE);
+
+		/*

drivers/usb/core/hub.c static int check_port_resume_type(struct · line 3103

Quoted 10 lines

drivers/usb/core/hub.c +7 -0

+		 * Whatever made this reset-resume necessary may have
+		 * turned on the port1 bit in hub->change_bits.  But after
+		 * a successful reset-resume we want the bit to be clear;
+		 * if it was on it would indicate that something happened
+		 * following the reset-resume.
+		 */
+		clear_bit(port1, hub->change_bits);
 	}
 
 	return status;

Unfortunately, my testing on this is somewhat inconclusive. I updated to the latest Linus kernel, then did about a half-dozen suspend/resume cycles to verify it was still working. Then I applied the patch and rebooted into the new kernel. At first I thought it was OK, but after about 5 or 6 suspend/resume cycles, the bluetooth stopped working (the desktop bluetooth manager in Linux Mint froze when I opened it). Another suspend/resume cycle brought it back to life, but after a couple more cycles it froze again.

But looking at the dmesg log, there were no errors concerning the bluetooth adapter. With the original failure, it would show errors before or during the firmware update of the bluetooth adapter, but now, the firmware update seemed to complete OK. And to top it off, after a reboot, I am no longer able to make it fail. I did more than a dozen suspend/resume cycles and have not seen any further failures.

So, make of that what you will :)

Thanks,
Paul

Alan Stern

2020-04-16 14:34 UTC #3 lore raw

Quoted 10 lines · Paul Zimmerman

Hi Alan,
Quoted 5 lines · On Wed, 15 Apr 2020 15:40:31 -0400 (EDT) Alan Stern…
Paul, I trust this new patch won't mess up your Bluetooth adapter. It should still clear the hub->change_bits entry before the hub workqueue thread wakes up.
Alan Stern

Quoted 8 lines

Unfortunately, my testing on this is somewhat inconclusive. I updated to the latest Linus kernel, then did about a half-dozen suspend/resume cycles to verify it was still working. Then I applied the patch and rebooted into the new kernel. At first I thought it was OK, but after about 5 or 6 suspend/resume cycles, the bluetooth stopped working (the desktop bluetooth manager in Linux Mint froze when I opened it). Another suspend/resume cycle brought it back to life, but after a couple more cycles it froze again.

That sounds different from your original bug report. Didn't the adapter fail in a significantly larger fraction of suspends?

Quoted 8 lines

But looking at the dmesg log, there were no errors concerning the bluetooth adapter. With the original failure, it would show errors before or during the firmware update of the bluetooth adapter, but now, the firmware update seemed to complete OK. And to top it off, after a reboot, I am no longer able to make it fail. I did more than a dozen suspend/resume cycles and have not seen any further failures.
So, make of that what you will :)

Overall, I guess we can call it a success. Do you want to collect a usbmon trace to verify that the patch behaves as expected, or are you happy with the testing so far?

Alan Stern

Paul Zimmerman

2020-04-16 19:47 UTC #4 lore raw

Quoted 21 lines · Alan Stern

Quoted 10 lines · Paul Zimmerman
Hi Alan,
Quoted 5 lines · On Wed, 15 Apr 2020 15:40:31 -0400 (EDT) Alan Stern…
Paul, I trust this new patch won't mess up your Bluetooth adapter. It should still clear the hub->change_bits entry before the hub workqueue thread wakes up.
Alan Stern
Quoted 8 lines
Unfortunately, my testing on this is somewhat inconclusive. I updated to the latest Linus kernel, then did about a half-dozen suspend/resume cycles to verify it was still working. Then I applied the patch and rebooted into the new kernel. At first I thought it was OK, but after about 5 or 6 suspend/resume cycles, the bluetooth stopped working (the desktop bluetooth manager in Linux Mint froze when I opened it). Another suspend/resume cycle brought it back to life, but after a couple more cycles it froze again.

Quoted 2 lines

That sounds different from your original bug report. Didn't the adapter fail in a significantly larger fraction of suspends?

Yes it did.

Quoted 7 lines

Quoted 7 lines
But looking at the dmesg log, there were no errors concerning the bluetooth adapter. With the original failure, it would show errors before or during the firmware update of the bluetooth adapter, but now, the firmware update seemed to complete OK. And to top it off, after a reboot, I am no longer able to make it fail. I did more than a dozen suspend/resume cycles and have not seen any further failures.

Quoted 5 lines

Quoted 1 line
So, make of that what you will :)
Overall, I guess we can call it a success. Do you want to collect a usbmon trace to verify that the patch behaves as expected, or are you happy with the testing so far?

I doubt a usbmon trace would show us anything unless I could get the bluetooth to fail again. And since I was the only one who saw the original problem anyway, I think it's fine to proceed with this patch as-is.

Thanks,
Paul

Alan Stern

2020-04-16 20:20 UTC #5 lore raw

Quoted 36 lines · Paul Zimmerman

Quoted 21 lines · Alan Stern
Quoted 10 lines · Paul Zimmerman
Hi Alan,
Quoted 5 lines · On Wed, 15 Apr 2020 15:40:31 -0400 (EDT) Alan Stern…
Paul, I trust this new patch won't mess up your Bluetooth adapter. It should still clear the hub->change_bits entry before the hub workqueue thread wakes up.
Alan Stern
Quoted 8 lines
Unfortunately, my testing on this is somewhat inconclusive. I updated to the latest Linus kernel, then did about a half-dozen suspend/resume cycles to verify it was still working. Then I applied the patch and rebooted into the new kernel. At first I thought it was OK, but after about 5 or 6 suspend/resume cycles, the bluetooth stopped working (the desktop bluetooth manager in Linux Mint froze when I opened it). Another suspend/resume cycle brought it back to life, but after a couple more cycles it froze again.
Quoted 2 lines
That sounds different from your original bug report. Didn't the adapter fail in a significantly larger fraction of suspends?
Yes it did.
Quoted 9 lines
Quoted 8 lines
But looking at the dmesg log, there were no errors concerning the bluetooth adapter. With the original failure, it would show errors before or during the firmware update of the bluetooth adapter, but now, the firmware update seemed to complete OK. And to top it off, after a reboot, I am no longer able to make it fail. I did more than a dozen suspend/resume cycles and have not seen any further failures.
So, make of that what you will :)

Quoted 8 lines

Quoted 3 lines
Overall, I guess we can call it a success. Do you want to collect a usbmon trace to verify that the patch behaves as expected, or are you happy with the testing so far?
I doubt a usbmon trace would show us anything unless I could get the bluetooth to fail again. And since I was the only one who saw the original problem anyway, I think it's fine to proceed with this patch as-is.

Okay, then I'll add your Tested-by if you don't mind.

Peter, have you tested the patch?

Alan Stern

Peter Chen

author Tested-by

2020-04-17 02:42 UTC #6 lore raw

Quoted 42 lines · Alan Stern

Quoted 40 lines · Paul Zimmerman
Quoted 21 lines · Alan Stern
Quoted 10 lines · Paul Zimmerman
Hi Alan,
Quoted 5 lines · On Wed, 15 Apr 2020 15:40:31 -0400 (EDT) Alan Stern…
Paul, I trust this new patch won't mess up your Bluetooth adapter. It should still clear the hub->change_bits entry before the hub workqueue thread wakes up.
Alan Stern
Quoted 8 lines
Unfortunately, my testing on this is somewhat inconclusive. I updated to the latest Linus kernel, then did about a half-dozen suspend/resume cycles to verify it was still working. Then I applied the patch and rebooted into the new kernel. At first I thought it was OK, but after about 5 or 6 suspend/resume cycles, the bluetooth stopped working (the desktop bluetooth manager in Linux Mint froze when I opened it). Another suspend/resume cycle brought it back to life, but after a couple more cycles it froze again.
Quoted 2 lines
That sounds different from your original bug report. Didn't the adapter fail in a significantly larger fraction of suspends?
Yes it did.
Quoted 12 lines
Quoted 8 lines
But looking at the dmesg log, there were no errors concerning the bluetooth adapter. With the original failure, it would show errors before or during the firmware update of the bluetooth adapter, but now, the firmware update seemed to complete OK. And to top it off, after a reboot, I am no longer able to make it fail. I did more than a dozen suspend/resume cycles and have not seen any further failures.
So, make of that what you will :)
Overall, I guess we can call it a success. Do you want to collect a usbmon trace to verify that the patch behaves as expected, or are you happy with the testing so far?

Quoted 9 lines

Quoted 4 lines
I doubt a usbmon trace would show us anything unless I could get the bluetooth to fail again. And since I was the only one who saw the original problem anyway, I think it's fine to proceed with this patch as-is.
Okay, then I'll add your Tested-by if you don't mind.
Peter, have you tested the patch?

I tested the reported issue and reset-resume case, both are OK.

Tested-by Peter Chen <[email protected]>

Signature · 2 lines

Thanks,
Peter Chen

Peter Chen

author

2020-04-16 01:19 UTC #7 lore raw

In reply to Alan Stern

Quoted 9 lines · Alan Stern

Quoted 4 lines · Peter Chen
Hi Alan,
With commit e5d078af8e5f ("USB: hub: Don't record a connect-change event during reset-resume"),
For those who care, this is commit 8099f58f1ecd in the upstream kernel.

Sorry, that commit was from v5.4 stable tree. I ran this issue out at both v5.6-rc4/7 and v5.4 stable tree.

commit e5d078af8e5fb0896706af855f52e9c0c69627b1

Author: Alan Stern <[email protected]>
Date:   Fri Jan 31 10:39:26 2020 -0500

USB: hub: Don't record a connect-change event during reset-resume

commit 8099f58f1ecddf4f374f4828a3dff8397c7cbd74 upstream.

Paul Zimmerman reports that his USB Bluetooth adapter sometimes

Signature · 2 lines

Thanks,
Peter Chen