* USB-C device hotplug issue @ 2018-10-25 12:20 Dennis Wassenberg 2018-10-25 12:28 ` Greg Kroah-Hartman 2018-10-25 14:46 ` Alan Stern 0 siblings, 2 replies; 12+ messages in thread From: Dennis Wassenberg @ 2018-10-25 12:20 UTC (permalink / raw) To: Greg Kroah-Hartman, Alan Stern, Ravi Chandra Sadineni, Kuppuswamy Sathyanarayanan, Bin Liu, Maxim Moseychuk, Mike Looijmans, Dominik Bozek, linux-usb, linux-kernel Hi all, I have a question regarding the usb hub driver (drivers/usb/core/hub.c). There is the following scenario. I am using the Lenovo T580 device with the Lenovo UltraDock CS18 docking station. If I plug an usb-c device to the docking station there is one device which will not be recognized (SanDisk Ultra USB-C Flash Drive, https://www.sandisk.com/home/mobile-device-storage/ultra-usb-type-c). An other usb-c devices (SanDisk Extreme 900 SSD, https://www.sandisk.com/home/ssd/extreme-900-ssd) works fine. I don’t have that much USB-C devices available, so there is one device working and the other one not. I made some analysis of the situation where I attached the SanDisk Ultra USB-C Flash Drive. I added some debug logs in drivers/usb/core/hub.c in port_event and hub_port_reset and activated all dynamic debug prints in hub.c. The output is the following: [ 724.110784] usb 4-1-port1: XXX: port_event: portstatus: 0x2c0, portchange: 0x40! [ 724.110789] usb 4-1-port1: link state change [ 724.114953] usb 4-1-port1: do warm reset [ 724.168109] usb 4-1-port1: not warm reset yet, waiting 50ms [ 724.220768] usb 4-1-port1: not warm reset yet, waiting 200ms [ 724.425188] usb 4-1-port1: XXX: hub_port_reset (before clear USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x1! [ 724.425906] usb 4-1-port1: XXX: hub_port_reset (after clear USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x0! [ 724.477429] hub 4-1:1.0: state 7 ports 4 chg 0000 evt 0002 [ 724.477980] usb 4-1-port1: XXX: port_event: portstatus: 0x203, portchange: 0x0! The same situation with SanDisk Extreme 900 SSD: [ 863.647484] hub 4-1:1.0: state 7 ports 4 chg 0000 evt 0002 [ 863.647965] usb 4-1-port1: XXX: port_event: portstatus: 0x203, portchange: 0x1! [ 863.648305] usb 4-1-port1: status 0203, change 0001, 10.0 Gb/s [ 863.758573] usb 4-1-port1: debounce total 100ms stable 100ms status 0x203 [ 863.773495] usb 4-1-port1: XXX: hub_port_reset (before clear USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x0! [ 863.773699] usb 4-1-port1: XXX: hub_port_reset (after clear USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x0! [ 863.826311] usb 4-1.1: new SuperSpeedPlus USB device number 6 using xhci_hcd [ 863.840002] usb 4-1.1: udev 6, busnum 4, minor = 389 [ 863.840010] usb 4-1.1: New USB device found, idVendor=0781, idProduct=5593 [ 863.840014] usb 4-1.1: New USB device strings: Mfr=2, Product=3, SerialNumber=1 [ 863.840018] usb 4-1.1: Product: EXTREME900 [ 863.840022] usb 4-1.1: Manufacturer: SanDisk [ 863.840026] usb 4-1.1: SerialNumber: 10000019DF56 So, at first I am wondering if the usb hub port was in USB_SS_PORT_LS_U3 if I attach the SanDisk Ultra USB-C Flash Drive. In case of the SanDisk Extreme 900 the usb hub port is in USB_SS_PORT_LS_U0. What’ the reason for that? I read https://www.kernel.org/doc/html/v4.14/driver-api/usb/power-management.html. In this test usb core was bootet wird autosuspend=-1.Additionally the <hubdev-portX>/power/pm_qos_no_power_off sysfs variable is always set to 1. Nevertheless the hub port is in LS U3, but only using SanDisk Ultra USB-C Flash Drive. I looked through the code and wondered why the USB_PORT_FEAT_C_CONNECTION bit is cleared at successful warm usb 3 reset in hub_port_reset. In case of the Ultra USB-C Flash Drive at first the link state change is detected. Directly after executing the actual hub_port_reset the USB_PORT_FEAT_C_CONNECTION bit will change to 1. But the hub_port_reset code will set this bit to 0. At the next run the bit remains 0 and the connect_change flag in port_event will not be set. That means the USB_PORT_FEAT_C_CONNECTION flag will be cleared without taking it into account. That’s why this device will not be detected correctly. If I modify the code in this way that I did’t clear the USB_PORT_FEAT_C_CONNECTION bit in hub_port_reset on warm usb3 reset this flag is still set during the next run. This makes sure that the connect_change flag in port_event is set and the usb device is detected correctly. Is this code change correct or will it break other use cases? Are there any other ways to make the kernel detect that usb device at the docking station? Please refer to the output of the modified usb_hub_reset code: [ 121.566344] hub 4-1:1.0: state 7 ports 4 chg 0000 evt 0002 [ 121.566805] usb 4-1-port1: XXX: port_event: portstatus: 0x2c0, portchange: 0x40! [ 121.566810] usb 4-1-port1: link state change [ 121.573481] usb 4-1-port1: do warm reset [ 121.625297] usb 4-1-port1: not warm reset yet, waiting 50ms [ 121.677854] usb 4-1-port1: not warm reset yet, waiting 200ms [ 121.881091] usb 4-1-port1: XXX: hub_port_reset (before clear USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x1! [ 121.881454] usb 4-1-port1: XXX: hub_port_reset (after clear USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x1! [ 121.933109] hub 4-1:1.0: state 7 ports 4 chg 0000 evt 0002 [ 121.933407] usb 4-1-port1: XXX: port_event: portstatus: 0x203, portchange: 0x1! [ 121.933861] usb 4-1-port1: status 0203, change 0001, 10.0 Gb/s [ 122.042540] usb 4-1-port1: debounce total 100ms stable 100ms status 0x203 [ 122.056865] usb 4-1-port1: XXX: hub_port_reset (before clear USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x0! [ 122.057230] usb 4-1-port1: XXX: hub_port_reset (after clear USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x0! [ 122.109166] usb 4-1.1: new SuperSpeed USB device number 5 using xhci_hcd [ 122.122392] usb 4-1.1: udev 5, busnum 4, minor = 388 [ 122.122399] usb 4-1.1: New USB device found, idVendor=0781, idProduct=5596 [ 122.122404] usb 4-1.1: New USB device strings: Mfr=1, Product=2, SerialNumber=3 [ 122.122408] usb 4-1.1: Product: Ultra T C [ 122.122412] usb 4-1.1: Manufacturer: SanDisk [ 122.122415] usb 4-1.1: SerialNumber: 4C531001400624115002 [ 122.124151] hub 4-1:1.0: state 7 ports 4 chg 0000 evt 0002 [ 122.125042] usb 4-1-port1: XXX: port_event: portstatus: 0x203, portchange: 0x0! The code change for Kernel 4.14 look as follows: Subject: [PATCH] usb: core: do not clear USB_PORT_FEAT_C_CONNECTION on warm hub port reset Signed-off-by: Dennis Wassenberg <dennis.wassenberg@secunet.com> --- drivers/usb/core/hub.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index a9db0887edca..9b4dfe6c8829 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -2815,7 +2815,9 @@ static int hub_port_reset(struct usb_hub *hub, int port1, USB_PORT_FEAT_C_BH_PORT_RESET); usb_clear_port_feature(hub->hdev, port1, USB_PORT_FEAT_C_PORT_LINK_STATE); - usb_clear_port_feature(hub->hdev, port1, + + if (!warm) + usb_clear_port_feature(hub->hdev, port1, USB_PORT_FEAT_C_CONNECTION); /* -- 2.17.1 Thank you for your help! Best regards, Dennis ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: USB-C device hotplug issue 2018-10-25 12:20 USB-C device hotplug issue Dennis Wassenberg @ 2018-10-25 12:28 ` Greg Kroah-Hartman 2018-10-25 12:38 ` Dennis Wassenberg 2018-10-25 14:46 ` Alan Stern 1 sibling, 1 reply; 12+ messages in thread From: Greg Kroah-Hartman @ 2018-10-25 12:28 UTC (permalink / raw) To: Dennis Wassenberg Cc: Alan Stern, Ravi Chandra Sadineni, Kuppuswamy Sathyanarayanan, Bin Liu, Maxim Moseychuk, Mike Looijmans, Dominik Bozek, linux-usb, linux-kernel On Thu, Oct 25, 2018 at 02:20:50PM +0200, Dennis Wassenberg wrote: > The code change for Kernel 4.14 look as follows: 4.14 is a year old now, can you try 4.19 to see if it works better? Lots of good xhci fixes and features have been added since then. thanks, greg k-h ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: USB-C device hotplug issue 2018-10-25 12:28 ` Greg Kroah-Hartman @ 2018-10-25 12:38 ` Dennis Wassenberg 0 siblings, 0 replies; 12+ messages in thread From: Dennis Wassenberg @ 2018-10-25 12:38 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Alan Stern, Ravi Chandra Sadineni, Kuppuswamy Sathyanarayanan, Bin Liu, Maxim Moseychuk, Mike Looijmans, Dominik Bozek, linux-usb, linux-kernel Hi, Using kernel 4.19 its exactly the same behavior. Best regards, Dennis On 25.10.18 14:28, Greg Kroah-Hartman wrote: > On Thu, Oct 25, 2018 at 02:20:50PM +0200, Dennis Wassenberg wrote: >> The code change for Kernel 4.14 look as follows: > > 4.14 is a year old now, can you try 4.19 to see if it works better? > Lots of good xhci fixes and features have been added since then. > > thanks, > > greg k-h > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: USB-C device hotplug issue 2018-10-25 12:20 USB-C device hotplug issue Dennis Wassenberg 2018-10-25 12:28 ` Greg Kroah-Hartman @ 2018-10-25 14:46 ` Alan Stern 2018-10-26 9:44 ` Dennis Wassenberg 1 sibling, 1 reply; 12+ messages in thread From: Alan Stern @ 2018-10-25 14:46 UTC (permalink / raw) To: Dennis Wassenberg, Mathias Nyman Cc: Greg Kroah-Hartman, Ravi Chandra Sadineni, Kuppuswamy Sathyanarayanan, Bin Liu, Maxim Moseychuk, Mike Looijmans, Dominik Bozek, USB list, Kernel development list On Thu, 25 Oct 2018, Dennis Wassenberg wrote: > Hi all, > > I have a question regarding the usb hub driver (drivers/usb/core/hub.c). > > There is the following scenario. I am using the Lenovo T580 device with the Lenovo UltraDock CS18 docking station. If I plug an usb-c device to the docking station there is one device which will not be recognized (SanDisk Ultra USB-C Flash Drive, https://www.sandisk.com/home/mobile-device-storage/ultra-usb-type-c). An other usb-c devices (SanDisk Extreme 900 SSD, https://www.sandisk.com/home/ssd/extreme-900-ssd) works fine. I don’t have that much USB-C devices available, so there is one device working and the other one not. > > I made some analysis of the situation where I attached the SanDisk Ultra USB-C Flash Drive. > I added some debug logs in drivers/usb/core/hub.c in port_event and hub_port_reset and activated all dynamic debug prints in hub.c. The output is the following: > > [ 724.110784] usb 4-1-port1: XXX: port_event: portstatus: 0x2c0, portchange: 0x40! > [ 724.110789] usb 4-1-port1: link state change > [ 724.114953] usb 4-1-port1: do warm reset > [ 724.168109] usb 4-1-port1: not warm reset yet, waiting 50ms > [ 724.220768] usb 4-1-port1: not warm reset yet, waiting 200ms > [ 724.425188] usb 4-1-port1: XXX: hub_port_reset (before clear USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x1! > [ 724.425906] usb 4-1-port1: XXX: hub_port_reset (after clear USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x0! > [ 724.477429] hub 4-1:1.0: state 7 ports 4 chg 0000 evt 0002 > [ 724.477980] usb 4-1-port1: XXX: port_event: portstatus: 0x203, portchange: 0x0! > > The same situation with SanDisk Extreme 900 SSD: > > [ 863.647484] hub 4-1:1.0: state 7 ports 4 chg 0000 evt 0002 > [ 863.647965] usb 4-1-port1: XXX: port_event: portstatus: 0x203, portchange: 0x1! > [ 863.648305] usb 4-1-port1: status 0203, change 0001, 10.0 Gb/s > [ 863.758573] usb 4-1-port1: debounce total 100ms stable 100ms status 0x203 > [ 863.773495] usb 4-1-port1: XXX: hub_port_reset (before clear USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x0! > [ 863.773699] usb 4-1-port1: XXX: hub_port_reset (after clear USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x0! > [ 863.826311] usb 4-1.1: new SuperSpeedPlus USB device number 6 using xhci_hcd > [ 863.840002] usb 4-1.1: udev 6, busnum 4, minor = 389 > [ 863.840010] usb 4-1.1: New USB device found, idVendor=0781, idProduct=5593 > [ 863.840014] usb 4-1.1: New USB device strings: Mfr=2, Product=3, SerialNumber=1 > [ 863.840018] usb 4-1.1: Product: EXTREME900 > [ 863.840022] usb 4-1.1: Manufacturer: SanDisk > [ 863.840026] usb 4-1.1: SerialNumber: 10000019DF56 > > So, at first I am wondering if the usb hub port was in USB_SS_PORT_LS_U3 if I attach the SanDisk Ultra USB-C Flash Drive. In case of the SanDisk Extreme 900 the usb hub port is in USB_SS_PORT_LS_U0. What’ the reason for that? > > I read https://www.kernel.org/doc/html/v4.14/driver-api/usb/power-management.html. In this test usb core was bootet wird autosuspend=-1.Additionally the <hubdev-portX>/power/pm_qos_no_power_off sysfs variable is always set to 1. Nevertheless the hub port is in LS U3, but only using SanDisk Ultra USB-C Flash Drive. > > I looked through the code and wondered why the USB_PORT_FEAT_C_CONNECTION bit is cleared at successful warm usb 3 reset in hub_port_reset. > In case of the Ultra USB-C Flash Drive at first the link state change is detected. Directly after executing the actual hub_port_reset the USB_PORT_FEAT_C_CONNECTION bit will change to 1. But the hub_port_reset code will set this bit to 0. At the next run the bit remains 0 and the connect_change flag in port_event will not be set. That means the USB_PORT_FEAT_C_CONNECTION flag will be cleared without taking it into account. That’s why this device will not be detected correctly. > > If I modify the code in this way that I did’t clear the USB_PORT_FEAT_C_CONNECTION bit in hub_port_reset on warm usb3 reset this flag is still set during the next run. This makes sure that the connect_change flag in port_event is set and the usb device is detected correctly. > Is this code change correct or will it break other use cases? Are there any other ways to make the kernel detect that usb device at the docking station? > > Please refer to the output of the modified usb_hub_reset code: > > [ 121.566344] hub 4-1:1.0: state 7 ports 4 chg 0000 evt 0002 > [ 121.566805] usb 4-1-port1: XXX: port_event: portstatus: 0x2c0, portchange: 0x40! > [ 121.566810] usb 4-1-port1: link state change > [ 121.573481] usb 4-1-port1: do warm reset > [ 121.625297] usb 4-1-port1: not warm reset yet, waiting 50ms > [ 121.677854] usb 4-1-port1: not warm reset yet, waiting 200ms > [ 121.881091] usb 4-1-port1: XXX: hub_port_reset (before clear USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x1! > [ 121.881454] usb 4-1-port1: XXX: hub_port_reset (after clear USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x1! > [ 121.933109] hub 4-1:1.0: state 7 ports 4 chg 0000 evt 0002 > [ 121.933407] usb 4-1-port1: XXX: port_event: portstatus: 0x203, portchange: 0x1! > [ 121.933861] usb 4-1-port1: status 0203, change 0001, 10.0 Gb/s > [ 122.042540] usb 4-1-port1: debounce total 100ms stable 100ms status 0x203 > [ 122.056865] usb 4-1-port1: XXX: hub_port_reset (before clear USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x0! > [ 122.057230] usb 4-1-port1: XXX: hub_port_reset (after clear USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x0! > [ 122.109166] usb 4-1.1: new SuperSpeed USB device number 5 using xhci_hcd > [ 122.122392] usb 4-1.1: udev 5, busnum 4, minor = 388 > [ 122.122399] usb 4-1.1: New USB device found, idVendor=0781, idProduct=5596 > [ 122.122404] usb 4-1.1: New USB device strings: Mfr=1, Product=2, SerialNumber=3 > [ 122.122408] usb 4-1.1: Product: Ultra T C > [ 122.122412] usb 4-1.1: Manufacturer: SanDisk > [ 122.122415] usb 4-1.1: SerialNumber: 4C531001400624115002 > [ 122.124151] hub 4-1:1.0: state 7 ports 4 chg 0000 evt 0002 > [ 122.125042] usb 4-1-port1: XXX: port_event: portstatus: 0x203, portchange: 0x0! > > > The code change for Kernel 4.14 look as follows: > > Subject: [PATCH] usb: core: do not clear USB_PORT_FEAT_C_CONNECTION on warm > hub port reset > > Signed-off-by: Dennis Wassenberg <dennis.wassenberg@secunet.com> > --- > drivers/usb/core/hub.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > index a9db0887edca..9b4dfe6c8829 100644 > --- a/drivers/usb/core/hub.c > +++ b/drivers/usb/core/hub.c > @@ -2815,7 +2815,9 @@ static int hub_port_reset(struct usb_hub *hub, int port1, > USB_PORT_FEAT_C_BH_PORT_RESET); > usb_clear_port_feature(hub->hdev, port1, > USB_PORT_FEAT_C_PORT_LINK_STATE); > - usb_clear_port_feature(hub->hdev, port1, > + > + if (!warm) > + usb_clear_port_feature(hub->hdev, port1, > USB_PORT_FEAT_C_CONNECTION); > > /* The key fact is that connection events can get lost if they happen to occur during a port reset. I'm not entirely certain of the logic here, but it looks like the correct test to add should be "if (udev != NULL)", not "if (!warm)". Perhaps Mathias can confirm this. Are there any reasons for resetting a port when there's no attached device, besides a link-state change? If there are, we might need to check for a connect-change when they occur too. Alan Stern ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: USB-C device hotplug issue 2018-10-25 14:46 ` Alan Stern @ 2018-10-26 9:44 ` Dennis Wassenberg 2018-10-26 14:07 ` Alan Stern 0 siblings, 1 reply; 12+ messages in thread From: Dennis Wassenberg @ 2018-10-26 9:44 UTC (permalink / raw) To: Alan Stern, Mathias Nyman Cc: Greg Kroah-Hartman, Ravi Chandra Sadineni, Kuppuswamy Sathyanarayanan, Bin Liu, Maxim Moseychuk, Mike Looijmans, Dominik Bozek, USB list, Kernel development list Hi all, On 10/25/18 4:46 PM, Alan Stern wrote: > On Thu, 25 Oct 2018, Dennis Wassenberg wrote: > >> Hi all, >> >> I have a question regarding the usb hub driver (drivers/usb/core/hub.c). >> >> There is the following scenario. I am using the Lenovo T580 device with the Lenovo UltraDock CS18 docking station. If I plug an usb-c device to the docking station there is one device which will not be recognized (SanDisk Ultra USB-C Flash Drive, https://www.sandisk.com/home/mobile-device-storage/ultra-usb-type-c). An other usb-c devices (SanDisk Extreme 900 SSD, https://www.sandisk.com/home/ssd/extreme-900-ssd) works fine. I don’t have that much USB-C devices available, so there is one device working and the other one not. >> >> I made some analysis of the situation where I attached the SanDisk Ultra USB-C Flash Drive. >> I added some debug logs in drivers/usb/core/hub.c in port_event and hub_port_reset and activated all dynamic debug prints in hub.c. The output is the following: >> >> [ 724.110784] usb 4-1-port1: XXX: port_event: portstatus: 0x2c0, portchange: 0x40! >> [ 724.110789] usb 4-1-port1: link state change >> [ 724.114953] usb 4-1-port1: do warm reset >> [ 724.168109] usb 4-1-port1: not warm reset yet, waiting 50ms >> [ 724.220768] usb 4-1-port1: not warm reset yet, waiting 200ms >> [ 724.425188] usb 4-1-port1: XXX: hub_port_reset (before clear USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x1! >> [ 724.425906] usb 4-1-port1: XXX: hub_port_reset (after clear USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x0! >> [ 724.477429] hub 4-1:1.0: state 7 ports 4 chg 0000 evt 0002 >> [ 724.477980] usb 4-1-port1: XXX: port_event: portstatus: 0x203, portchange: 0x0! >> >> The same situation with SanDisk Extreme 900 SSD: >> >> [ 863.647484] hub 4-1:1.0: state 7 ports 4 chg 0000 evt 0002 >> [ 863.647965] usb 4-1-port1: XXX: port_event: portstatus: 0x203, portchange: 0x1! >> [ 863.648305] usb 4-1-port1: status 0203, change 0001, 10.0 Gb/s >> [ 863.758573] usb 4-1-port1: debounce total 100ms stable 100ms status 0x203 >> [ 863.773495] usb 4-1-port1: XXX: hub_port_reset (before clear USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x0! >> [ 863.773699] usb 4-1-port1: XXX: hub_port_reset (after clear USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x0! >> [ 863.826311] usb 4-1.1: new SuperSpeedPlus USB device number 6 using xhci_hcd >> [ 863.840002] usb 4-1.1: udev 6, busnum 4, minor = 389 >> [ 863.840010] usb 4-1.1: New USB device found, idVendor=0781, idProduct=5593 >> [ 863.840014] usb 4-1.1: New USB device strings: Mfr=2, Product=3, SerialNumber=1 >> [ 863.840018] usb 4-1.1: Product: EXTREME900 >> [ 863.840022] usb 4-1.1: Manufacturer: SanDisk >> [ 863.840026] usb 4-1.1: SerialNumber: 10000019DF56 >> >> So, at first I am wondering if the usb hub port was in USB_SS_PORT_LS_U3 if I attach the SanDisk Ultra USB-C Flash Drive. In case of the SanDisk Extreme 900 the usb hub port is in USB_SS_PORT_LS_U0. What’ the reason for that? >> >> I read https://www.kernel.org/doc/html/v4.14/driver-api/usb/power-management.html. In this test usb core was bootet wird autosuspend=-1.Additionally the <hubdev-portX>/power/pm_qos_no_power_off sysfs variable is always set to 1. Nevertheless the hub port is in LS U3, but only using SanDisk Ultra USB-C Flash Drive. >> >> I looked through the code and wondered why the USB_PORT_FEAT_C_CONNECTION bit is cleared at successful warm usb 3 reset in hub_port_reset. >> In case of the Ultra USB-C Flash Drive at first the link state change is detected. Directly after executing the actual hub_port_reset the USB_PORT_FEAT_C_CONNECTION bit will change to 1. But the hub_port_reset code will set this bit to 0. At the next run the bit remains 0 and the connect_change flag in port_event will not be set. That means the USB_PORT_FEAT_C_CONNECTION flag will be cleared without taking it into account. That’s why this device will not be detected correctly. >> >> If I modify the code in this way that I did’t clear the USB_PORT_FEAT_C_CONNECTION bit in hub_port_reset on warm usb3 reset this flag is still set during the next run. This makes sure that the connect_change flag in port_event is set and the usb device is detected correctly. >> Is this code change correct or will it break other use cases? Are there any other ways to make the kernel detect that usb device at the docking station? >> >> Please refer to the output of the modified usb_hub_reset code: >> >> [ 121.566344] hub 4-1:1.0: state 7 ports 4 chg 0000 evt 0002 >> [ 121.566805] usb 4-1-port1: XXX: port_event: portstatus: 0x2c0, portchange: 0x40! >> [ 121.566810] usb 4-1-port1: link state change >> [ 121.573481] usb 4-1-port1: do warm reset >> [ 121.625297] usb 4-1-port1: not warm reset yet, waiting 50ms >> [ 121.677854] usb 4-1-port1: not warm reset yet, waiting 200ms >> [ 121.881091] usb 4-1-port1: XXX: hub_port_reset (before clear USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x1! >> [ 121.881454] usb 4-1-port1: XXX: hub_port_reset (after clear USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x1! >> [ 121.933109] hub 4-1:1.0: state 7 ports 4 chg 0000 evt 0002 >> [ 121.933407] usb 4-1-port1: XXX: port_event: portstatus: 0x203, portchange: 0x1! >> [ 121.933861] usb 4-1-port1: status 0203, change 0001, 10.0 Gb/s >> [ 122.042540] usb 4-1-port1: debounce total 100ms stable 100ms status 0x203 >> [ 122.056865] usb 4-1-port1: XXX: hub_port_reset (before clear USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x0! >> [ 122.057230] usb 4-1-port1: XXX: hub_port_reset (after clear USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x0! >> [ 122.109166] usb 4-1.1: new SuperSpeed USB device number 5 using xhci_hcd >> [ 122.122392] usb 4-1.1: udev 5, busnum 4, minor = 388 >> [ 122.122399] usb 4-1.1: New USB device found, idVendor=0781, idProduct=5596 >> [ 122.122404] usb 4-1.1: New USB device strings: Mfr=1, Product=2, SerialNumber=3 >> [ 122.122408] usb 4-1.1: Product: Ultra T C >> [ 122.122412] usb 4-1.1: Manufacturer: SanDisk >> [ 122.122415] usb 4-1.1: SerialNumber: 4C531001400624115002 >> [ 122.124151] hub 4-1:1.0: state 7 ports 4 chg 0000 evt 0002 >> [ 122.125042] usb 4-1-port1: XXX: port_event: portstatus: 0x203, portchange: 0x0! >> >> >> The code change for Kernel 4.14 look as follows: >> >> Subject: [PATCH] usb: core: do not clear USB_PORT_FEAT_C_CONNECTION on warm >> hub port reset >> >> Signed-off-by: Dennis Wassenberg <dennis.wassenberg@secunet.com> >> --- >> drivers/usb/core/hub.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c >> index a9db0887edca..9b4dfe6c8829 100644 >> --- a/drivers/usb/core/hub.c >> +++ b/drivers/usb/core/hub.c >> @@ -2815,7 +2815,9 @@ static int hub_port_reset(struct usb_hub *hub, int port1, >> USB_PORT_FEAT_C_BH_PORT_RESET); >> usb_clear_port_feature(hub->hdev, port1, >> USB_PORT_FEAT_C_PORT_LINK_STATE); >> - usb_clear_port_feature(hub->hdev, port1, >> + >> + if (!warm) >> + usb_clear_port_feature(hub->hdev, port1, >> USB_PORT_FEAT_C_CONNECTION); >> >> /* > > The key fact is that connection events can get lost if they happen to > occur during a port reset. Yes. > > I'm not entirely certain of the logic here, but it looks like the > correct test to add should be "if (udev != NULL)", not "if (!warm)". > Perhaps Mathias can confirm thisI don't know if clearing the USB_PORT_FEAT_C_CONNECTION bit is correct in case of a non warm reset. In my case I always observed a warm reset because of the link state change. Thats why I checked the warm variable to not change the behavoir for cases a didn't checked for the first shot. During the first run of usb_hub_reset the udev is NULL because in this situation the device is not attached logically. [ 112.889810] usb 4-1-port1: XXX: port_event: portstatus: 0x2c0, portchange: 0x40! [ 113.201192] usb 4-1-port1: XXX: hub_port_reset: udev: (nil)! [ 113.201198] usb 4-1-port1: XXX: hub_port_reset (not clearing USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x1! [ 113.253612] usb 4-1-port1: XXX: port_event: portstatus: 0x203, portchange: 0x1! [ 113.377208] usb 4-1-port1: XXX: hub_port_reset: udev: ffff88046b302800! [ 113.377214] usb 4-1-port1: XXX: hub_port_reset (not clearing USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x0! [ 113.429478] usb 4-1.1: new SuperSpeed USB device number 7 using xhci_hcd [ 113.442370] usb 4-1.1: New USB device found, idVendor=0781, idProduct=5596 [ 113.442376] usb 4-1.1: New USB device strings: Mfr=1, Product=2, SerialNumber=3 [ 113.442381] usb 4-1.1: Product: Ultra T C [ 113.442385] usb 4-1.1: Manufacturer: SanDisk [ 113.442388] usb 4-1.1: SerialNumber: 4C530001131013121031 Or maybe we can skip clearing the USB_PORT_FEAT_C_CONNECTION bit in case of hub_port_reset completely without any other check? Dennis > > Are there any reasons for resetting a port when there's no attached > device, besides a link-state change? If there are, we might need to > check for a connect-change when they occur too. > > Alan Stern > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: USB-C device hotplug issue 2018-10-26 9:44 ` Dennis Wassenberg @ 2018-10-26 14:07 ` Alan Stern 2018-11-05 15:35 ` Mathias Nyman 0 siblings, 1 reply; 12+ messages in thread From: Alan Stern @ 2018-10-26 14:07 UTC (permalink / raw) To: Dennis Wassenberg Cc: Mathias Nyman, Greg Kroah-Hartman, Ravi Chandra Sadineni, Kuppuswamy Sathyanarayanan, Bin Liu, Maxim Moseychuk, Mike Looijmans, Dominik Bozek, USB list, Kernel development list On Fri, 26 Oct 2018, Dennis Wassenberg wrote: > >> --- a/drivers/usb/core/hub.c > >> +++ b/drivers/usb/core/hub.c > >> @@ -2815,7 +2815,9 @@ static int hub_port_reset(struct usb_hub *hub, int port1, > >> USB_PORT_FEAT_C_BH_PORT_RESET); > >> usb_clear_port_feature(hub->hdev, port1, > >> USB_PORT_FEAT_C_PORT_LINK_STATE); > >> - usb_clear_port_feature(hub->hdev, port1, > >> + > >> + if (!warm) > >> + usb_clear_port_feature(hub->hdev, port1, > >> USB_PORT_FEAT_C_CONNECTION); > >> > >> /* > > > > The key fact is that connection events can get lost if they happen to > > occur during a port reset. > Yes. > > > > I'm not entirely certain of the logic here, but it looks like the > > correct test to add should be "if (udev != NULL)", not "if (!warm)". > > Perhaps Mathias can confirm this > I don't know if clearing the USB_PORT_FEAT_C_CONNECTION bit is > correct in case of a non warm reset. In my case I always observed a > warm reset because of the link state change. > Thats why I checked the warm variable to not change the behavoir for > cases a didn't checked for the first shot. (The syntax of that last sentence is pretty mangled; I can't understand it.) Think of it this way: If a device was not attached before the reset, we don't want to clear the connect-change status because then we wouldn't recognize a device that was plugged in during the reset. If a device was attached before the reset, we don't want any connect-change status which might be provoked by the reset to last, because we don't want the core to think that the device was unplugged and replugged when all that happened was a reset. So the important criterion is whether or not a device was attached to the port when the reset started. It's something of a coincidence that you only observe warm resets when there's nothing attached. > During the first run of usb_hub_reset the udev is NULL because in > this situation the device is not attached logically. > > [ 112.889810] usb 4-1-port1: XXX: port_event: portstatus: 0x2c0, portchange: 0x40! > [ 113.201192] usb 4-1-port1: XXX: hub_port_reset: udev: (nil)! > [ 113.201198] usb 4-1-port1: XXX: hub_port_reset (not clearing USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x1! > [ 113.253612] usb 4-1-port1: XXX: port_event: portstatus: 0x203, portchange: 0x1! > [ 113.377208] usb 4-1-port1: XXX: hub_port_reset: udev: ffff88046b302800! > [ 113.377214] usb 4-1-port1: XXX: hub_port_reset (not clearing USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x0! > [ 113.429478] usb 4-1.1: new SuperSpeed USB device number 7 using xhci_hcd > [ 113.442370] usb 4-1.1: New USB device found, idVendor=0781, idProduct=5596 > [ 113.442376] usb 4-1.1: New USB device strings: Mfr=1, Product=2, SerialNumber=3 > [ 113.442381] usb 4-1.1: Product: Ultra T C > [ 113.442385] usb 4-1.1: Manufacturer: SanDisk > [ 113.442388] usb 4-1.1: SerialNumber: 4C530001131013121031 > > Or maybe we can skip clearing the USB_PORT_FEAT_C_CONNECTION bit in > case of hub_port_reset completely without any other check? See above. Alan Stern ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: USB-C device hotplug issue 2018-10-26 14:07 ` Alan Stern @ 2018-11-05 15:35 ` Mathias Nyman 2018-11-07 9:08 ` Dennis Wassenberg 0 siblings, 1 reply; 12+ messages in thread From: Mathias Nyman @ 2018-11-05 15:35 UTC (permalink / raw) To: Alan Stern, Dennis Wassenberg Cc: Mathias Nyman, Greg Kroah-Hartman, Ravi Chandra Sadineni, Kuppuswamy Sathyanarayanan, Bin Liu, Maxim Moseychuk, Mike Looijmans, Dominik Bozek, USB list, Kernel development list On 26.10.2018 17:07, Alan Stern wrote: > On Fri, 26 Oct 2018, Dennis Wassenberg wrote: > >>>> --- a/drivers/usb/core/hub.c >>>> +++ b/drivers/usb/core/hub.c >>>> @@ -2815,7 +2815,9 @@ static int hub_port_reset(struct usb_hub *hub, int port1, >>>> USB_PORT_FEAT_C_BH_PORT_RESET); >>>> usb_clear_port_feature(hub->hdev, port1, >>>> USB_PORT_FEAT_C_PORT_LINK_STATE); >>>> - usb_clear_port_feature(hub->hdev, port1, >>>> + >>>> + if (!warm) >>>> + usb_clear_port_feature(hub->hdev, port1, >>>> USB_PORT_FEAT_C_CONNECTION); >>>> >>>> /* >>> >>> The key fact is that connection events can get lost if they happen to >>> occur during a port reset. >> Yes. >>> >>> I'm not entirely certain of the logic here, but it looks like the >>> correct test to add should be "if (udev != NULL)", not "if (!warm)". >>> Perhaps Mathias can confirm this Sorry about the late response, got distracted while performing git archeology. "if (udev != NULL)" would seem more reasonable. Logs show that clearing the FEAT_C_CONNECTION was originally added after a hot reset failed, and before issuing a warm reset to a SS.Inactive link. (see 10d674a USB: When hot reset for USB3 fails, try warm reset.) Apparently all the change flags needed to be cleared for some specific host + device combination before issuing a warm reset for the enumeration to work properly. The change to always clear FEAT_C_CONNECTION for USB3 was done later in patch: a24a607 USB: Rip out recursive call on warm port reset. Motivation was: "In hub_port_finish_reset, unconditionally clear the connect status change (CSC) bit for USB 3.0 hubs when the port reset is done. If we had to issue multiple warm resets for a device, that bit may have been set if the device went into SS.Inactive and then was successfully warm reset." >> I don't know if clearing the USB_PORT_FEAT_C_CONNECTION bit is >> correct in case of a non warm reset. In my case I always observed a >> warm reset because of the link state change. >> Thats why I checked the warm variable to not change the behavoir for >> cases a didn't checked for the first shot. > > (The syntax of that last sentence is pretty mangled; I can't understand > it.) > > Think of it this way: > > If a device was not attached before the reset, we don't want > to clear the connect-change status because then we wouldn't > recognize a device that was plugged in during the reset. > > If a device was attached before the reset, we don't want any > connect-change status which might be provoked by the reset to > last, because we don't want the core to think that the device > was unplugged and replugged when all that happened was a reset. > > So the important criterion is whether or not a device was attached to > the port when the reset started. It's something of a coincidence that > you only observe warm resets when there's nothing attached. > >> During the first run of usb_hub_reset the udev is NULL because in >> this situation the device is not attached logically. >> >> [ 112.889810] usb 4-1-port1: XXX: port_event: portstatus: 0x2c0, portchange: 0x40! >> [ 113.201192] usb 4-1-port1: XXX: hub_port_reset: udev: (nil)! >> [ 113.201198] usb 4-1-port1: XXX: hub_port_reset (not clearing USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x1! >> [ 113.253612] usb 4-1-port1: XXX: port_event: portstatus: 0x203, portchange: 0x1! >> [ 113.377208] usb 4-1-port1: XXX: hub_port_reset: udev: ffff88046b302800! >> [ 113.377214] usb 4-1-port1: XXX: hub_port_reset (not clearing USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x0! >> [ 113.429478] usb 4-1.1: new SuperSpeed USB device number 7 using xhci_hcd >> [ 113.442370] usb 4-1.1: New USB device found, idVendor=0781, idProduct=5596 >> [ 113.442376] usb 4-1.1: New USB device strings: Mfr=1, Product=2, SerialNumber=3 >> [ 113.442381] usb 4-1.1: Product: Ultra T C >> [ 113.442385] usb 4-1.1: Manufacturer: SanDisk >> [ 113.442388] usb 4-1.1: SerialNumber: 4C530001131013121031 >> >> Or maybe we can skip clearing the USB_PORT_FEAT_C_CONNECTION bit in >> case of hub_port_reset completely without any other check? > > See above. Checking for udev sounds reasonable to me. Not sure if we should worry about the specific host+device combo that needed flags cleared before warm reset. See patch: 10d674a USB: When hot reset for USB3 fails, try warm reset. https://marc.info/?l=linux-usb&m=131603549603799&w=2 -Mathias ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: USB-C device hotplug issue 2018-11-05 15:35 ` Mathias Nyman @ 2018-11-07 9:08 ` Dennis Wassenberg 2018-11-09 13:47 ` Mathias Nyman 0 siblings, 1 reply; 12+ messages in thread From: Dennis Wassenberg @ 2018-11-07 9:08 UTC (permalink / raw) To: Mathias Nyman, Alan Stern Cc: Mathias Nyman, Greg Kroah-Hartman, Ravi Chandra Sadineni, Kuppuswamy Sathyanarayanan, Bin Liu, Maxim Moseychuk, Mike Looijmans, Dominik Bozek, USB list, Kernel development list On 05.11.18 16:35, Mathias Nyman wrote: > On 26.10.2018 17:07, Alan Stern wrote: >> On Fri, 26 Oct 2018, Dennis Wassenberg wrote: >> >>>>> --- a/drivers/usb/core/hub.c >>>>> +++ b/drivers/usb/core/hub.c >>>>> @@ -2815,7 +2815,9 @@ static int hub_port_reset(struct usb_hub *hub, int port1, >>>>> USB_PORT_FEAT_C_BH_PORT_RESET); >>>>> usb_clear_port_feature(hub->hdev, port1, >>>>> USB_PORT_FEAT_C_PORT_LINK_STATE); >>>>> - usb_clear_port_feature(hub->hdev, port1, >>>>> + >>>>> + if (!warm) >>>>> + usb_clear_port_feature(hub->hdev, port1, >>>>> USB_PORT_FEAT_C_CONNECTION); >>>>> /* >>>> >>>> The key fact is that connection events can get lost if they happen to >>>> occur during a port reset. >>> Yes. >>>> >>>> I'm not entirely certain of the logic here, but it looks like the >>>> correct test to add should be "if (udev != NULL)", not "if (!warm)". >>>> Perhaps Mathias can confirm this > > Sorry about the late response, got distracted while performing git > archeology. > > "if (udev != NULL)" would seem more reasonable. > > Logs show that clearing the FEAT_C_CONNECTION was originally added > after a hot reset failed, and before issuing a warm reset to a SS.Inactive > link. (see 10d674a USB: When hot reset for USB3 fails, try warm reset.) > > Apparently all the change flags needed to be cleared for some specific > host + device combination before issuing a warm reset for the enumeration > to work properly. > > The change to always clear FEAT_C_CONNECTION for USB3 was done later in patch: > a24a607 USB: Rip out recursive call on warm port reset. > > Motivation was: > > "In hub_port_finish_reset, unconditionally clear the connect status > change (CSC) bit for USB 3.0 hubs when the port reset is done. If we > had to issue multiple warm resets for a device, that bit may have been > set if the device went into SS.Inactive and then was successfully warm > reset." > >>> I don't know if clearing the USB_PORT_FEAT_C_CONNECTION bit is >>> correct in case of a non warm reset. In my case I always observed a >>> warm reset because of the link state change. >>> Thats why I checked the warm variable to not change the behavoir for >>> cases a didn't checked for the first shot. >> >> (The syntax of that last sentence is pretty mangled; I can't understand >> it.) >> >> Think of it this way: >> >> If a device was not attached before the reset, we don't want >> to clear the connect-change status because then we wouldn't >> recognize a device that was plugged in during the reset. >> >> If a device was attached before the reset, we don't want any >> connect-change status which might be provoked by the reset to >> last, because we don't want the core to think that the device >> was unplugged and replugged when all that happened was a reset. >> >> So the important criterion is whether or not a device was attached to >> the port when the reset started. It's something of a coincidence that >> you only observe warm resets when there's nothing attached. >> >>> During the first run of usb_hub_reset the udev is NULL because in >>> this situation the device is not attached logically. >>> >>> [ 112.889810] usb 4-1-port1: XXX: port_event: portstatus: 0x2c0, portchange: 0x40! >>> [ 113.201192] usb 4-1-port1: XXX: hub_port_reset: udev: (nil)! >>> [ 113.201198] usb 4-1-port1: XXX: hub_port_reset (not clearing USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x1! >>> [ 113.253612] usb 4-1-port1: XXX: port_event: portstatus: 0x203, portchange: 0x1! >>> [ 113.377208] usb 4-1-port1: XXX: hub_port_reset: udev: ffff88046b302800! >>> [ 113.377214] usb 4-1-port1: XXX: hub_port_reset (not clearing USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x0! >>> [ 113.429478] usb 4-1.1: new SuperSpeed USB device number 7 using xhci_hcd >>> [ 113.442370] usb 4-1.1: New USB device found, idVendor=0781, idProduct=5596 >>> [ 113.442376] usb 4-1.1: New USB device strings: Mfr=1, Product=2, SerialNumber=3 >>> [ 113.442381] usb 4-1.1: Product: Ultra T C >>> [ 113.442385] usb 4-1.1: Manufacturer: SanDisk >>> [ 113.442388] usb 4-1.1: SerialNumber: 4C530001131013121031 >>> >>> Or maybe we can skip clearing the USB_PORT_FEAT_C_CONNECTION bit in >>> case of hub_port_reset completely without any other check? >> >> See above. > > Checking for udev sounds reasonable to me. > Not sure if we should worry about the specific host+device combo that needed flags > cleared before warm reset. See patch: > > 10d674a USB: When hot reset for USB3 fails, try warm reset. > https://marc.info/?l=linux-usb&m=131603549603799&w=2 > > -Mathias Checking for udev works well too in my case. So there is no need to check the warm flag. Regarding the other point about the specific host+device combo which needs the flags cleared, I don't know how to proceed. Best regards, Dennis ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: USB-C device hotplug issue 2018-11-07 9:08 ` Dennis Wassenberg @ 2018-11-09 13:47 ` Mathias Nyman 2018-11-13 13:38 ` Dennis Wassenberg 2018-11-13 13:40 ` [PATCH] usb: core: Fix hub port connection events lost Dennis Wassenberg 0 siblings, 2 replies; 12+ messages in thread From: Mathias Nyman @ 2018-11-09 13:47 UTC (permalink / raw) To: Dennis Wassenberg, Alan Stern Cc: Mathias Nyman, Greg Kroah-Hartman, Ravi Chandra Sadineni, Kuppuswamy Sathyanarayanan, Bin Liu, Maxim Moseychuk, Mike Looijmans, Dominik Bozek, USB list, Kernel development list On 07.11.2018 11:08, Dennis Wassenberg wrote: > > On 05.11.18 16:35, Mathias Nyman wrote: >> On 26.10.2018 17:07, Alan Stern wrote: >>> On Fri, 26 Oct 2018, Dennis Wassenberg wrote: >>> >>>>>> --- a/drivers/usb/core/hub.c >>>>>> +++ b/drivers/usb/core/hub.c >>>>>> @@ -2815,7 +2815,9 @@ static int hub_port_reset(struct usb_hub *hub, int port1, >>>>>> USB_PORT_FEAT_C_BH_PORT_RESET); >>>>>> usb_clear_port_feature(hub->hdev, port1, >>>>>> USB_PORT_FEAT_C_PORT_LINK_STATE); >>>>>> - usb_clear_port_feature(hub->hdev, port1, >>>>>> + >>>>>> + if (!warm) >>>>>> + usb_clear_port_feature(hub->hdev, port1, >>>>>> USB_PORT_FEAT_C_CONNECTION); >>>>>> /* >>>>> >>>>> The key fact is that connection events can get lost if they happen to >>>>> occur during a port reset. >>>> Yes. >>>>> >>>>> I'm not entirely certain of the logic here, but it looks like the >>>>> correct test to add should be "if (udev != NULL)", not "if (!warm)". >>>>> Perhaps Mathias can confirm this >> >> Sorry about the late response, got distracted while performing git >> archeology. >> >> "if (udev != NULL)" would seem more reasonable. >> >> Logs show that clearing the FEAT_C_CONNECTION was originally added >> after a hot reset failed, and before issuing a warm reset to a SS.Inactive >> link. (see 10d674a USB: When hot reset for USB3 fails, try warm reset.) >> >> Apparently all the change flags needed to be cleared for some specific >> host + device combination before issuing a warm reset for the enumeration >> to work properly. >> >> The change to always clear FEAT_C_CONNECTION for USB3 was done later in patch: >> a24a607 USB: Rip out recursive call on warm port reset. >> >> Motivation was: >> >> "In hub_port_finish_reset, unconditionally clear the connect status >> change (CSC) bit for USB 3.0 hubs when the port reset is done. If we >> had to issue multiple warm resets for a device, that bit may have been >> set if the device went into SS.Inactive and then was successfully warm >> reset." >> >>>> I don't know if clearing the USB_PORT_FEAT_C_CONNECTION bit is >>>> correct in case of a non warm reset. In my case I always observed a >>>> warm reset because of the link state change. >>>> Thats why I checked the warm variable to not change the behavoir for >>>> cases a didn't checked for the first shot. >>> >>> (The syntax of that last sentence is pretty mangled; I can't understand >>> it.) >>> >>> Think of it this way: >>> >>> If a device was not attached before the reset, we don't want >>> to clear the connect-change status because then we wouldn't >>> recognize a device that was plugged in during the reset. >>> >>> If a device was attached before the reset, we don't want any >>> connect-change status which might be provoked by the reset to >>> last, because we don't want the core to think that the device >>> was unplugged and replugged when all that happened was a reset. >>> >>> So the important criterion is whether or not a device was attached to >>> the port when the reset started. It's something of a coincidence that >>> you only observe warm resets when there's nothing attached. >>> >>>> During the first run of usb_hub_reset the udev is NULL because in >>>> this situation the device is not attached logically. >>>> >>>> [ 112.889810] usb 4-1-port1: XXX: port_event: portstatus: 0x2c0, portchange: 0x40! >>>> [ 113.201192] usb 4-1-port1: XXX: hub_port_reset: udev: (nil)! >>>> [ 113.201198] usb 4-1-port1: XXX: hub_port_reset (not clearing USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x1! >>>> [ 113.253612] usb 4-1-port1: XXX: port_event: portstatus: 0x203, portchange: 0x1! >>>> [ 113.377208] usb 4-1-port1: XXX: hub_port_reset: udev: ffff88046b302800! >>>> [ 113.377214] usb 4-1-port1: XXX: hub_port_reset (not clearing USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x0! >>>> [ 113.429478] usb 4-1.1: new SuperSpeed USB device number 7 using xhci_hcd >>>> [ 113.442370] usb 4-1.1: New USB device found, idVendor=0781, idProduct=5596 >>>> [ 113.442376] usb 4-1.1: New USB device strings: Mfr=1, Product=2, SerialNumber=3 >>>> [ 113.442381] usb 4-1.1: Product: Ultra T C >>>> [ 113.442385] usb 4-1.1: Manufacturer: SanDisk >>>> [ 113.442388] usb 4-1.1: SerialNumber: 4C530001131013121031 >>>> >>>> Or maybe we can skip clearing the USB_PORT_FEAT_C_CONNECTION bit in >>>> case of hub_port_reset completely without any other check? >>> >>> See above. >> >> Checking for udev sounds reasonable to me. >> Not sure if we should worry about the specific host+device combo that needed flags >> cleared before warm reset. See patch: >> >> 10d674a USB: When hot reset for USB3 fails, try warm reset. >> https://marc.info/?l=linux-usb&m=131603549603799&w=2 >> >> -Mathias > Checking for udev works well too in my case. So there is no need to check the warm flag. > > Regarding the other point about the specific host+device combo which needs the flags cleared, I don't know how to proceed. > I support just adding a udev check patch, want to send one? Current hub port reset code is wrong, causing real life issues today. The issue with the specific host+device is from 2011, The port reset code has changed completely since then. If it turns out to still be a issue we can make a host/device specific quirk. -Mathias ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: USB-C device hotplug issue 2018-11-09 13:47 ` Mathias Nyman @ 2018-11-13 13:38 ` Dennis Wassenberg 2018-11-13 13:40 ` [PATCH] usb: core: Fix hub port connection events lost Dennis Wassenberg 1 sibling, 0 replies; 12+ messages in thread From: Dennis Wassenberg @ 2018-11-13 13:38 UTC (permalink / raw) To: Mathias Nyman, Alan Stern Cc: Mathias Nyman, Greg Kroah-Hartman, Ravi Chandra Sadineni, Kuppuswamy Sathyanarayanan, Bin Liu, Maxim Moseychuk, Mike Looijmans, Dominik Bozek, USB list, Kernel development list On 09.11.18 14:47, Mathias Nyman wrote: > On 07.11.2018 11:08, Dennis Wassenberg wrote: >> >> On 05.11.18 16:35, Mathias Nyman wrote: >>> On 26.10.2018 17:07, Alan Stern wrote: >>>> On Fri, 26 Oct 2018, Dennis Wassenberg wrote: >>>> >>>>>>> --- a/drivers/usb/core/hub.c >>>>>>> +++ b/drivers/usb/core/hub.c >>>>>>> @@ -2815,7 +2815,9 @@ static int hub_port_reset(struct usb_hub *hub, int port1, >>>>>>> USB_PORT_FEAT_C_BH_PORT_RESET); >>>>>>> usb_clear_port_feature(hub->hdev, port1, >>>>>>> USB_PORT_FEAT_C_PORT_LINK_STATE); >>>>>>> - usb_clear_port_feature(hub->hdev, port1, >>>>>>> + >>>>>>> + if (!warm) >>>>>>> + usb_clear_port_feature(hub->hdev, port1, >>>>>>> USB_PORT_FEAT_C_CONNECTION); >>>>>>> /* >>>>>> >>>>>> The key fact is that connection events can get lost if they happen to >>>>>> occur during a port reset. >>>>> Yes. >>>>>> >>>>>> I'm not entirely certain of the logic here, but it looks like the >>>>>> correct test to add should be "if (udev != NULL)", not "if (!warm)". >>>>>> Perhaps Mathias can confirm this >>> >>> Sorry about the late response, got distracted while performing git >>> archeology. >>> >>> "if (udev != NULL)" would seem more reasonable. >>> >>> Logs show that clearing the FEAT_C_CONNECTION was originally added >>> after a hot reset failed, and before issuing a warm reset to a SS.Inactive >>> link. (see 10d674a USB: When hot reset for USB3 fails, try warm reset.) >>> >>> Apparently all the change flags needed to be cleared for some specific >>> host + device combination before issuing a warm reset for the enumeration >>> to work properly. >>> >>> The change to always clear FEAT_C_CONNECTION for USB3 was done later in patch: >>> a24a607 USB: Rip out recursive call on warm port reset. >>> >>> Motivation was: >>> >>> "In hub_port_finish_reset, unconditionally clear the connect status >>> change (CSC) bit for USB 3.0 hubs when the port reset is done. If we >>> had to issue multiple warm resets for a device, that bit may have been >>> set if the device went into SS.Inactive and then was successfully warm >>> reset." >>> >>>>> I don't know if clearing the USB_PORT_FEAT_C_CONNECTION bit is >>>>> correct in case of a non warm reset. In my case I always observed a >>>>> warm reset because of the link state change. >>>>> Thats why I checked the warm variable to not change the behavoir for >>>>> cases a didn't checked for the first shot. >>>> >>>> (The syntax of that last sentence is pretty mangled; I can't understand >>>> it.) >>>> >>>> Think of it this way: >>>> >>>> If a device was not attached before the reset, we don't want >>>> to clear the connect-change status because then we wouldn't >>>> recognize a device that was plugged in during the reset. >>>> >>>> If a device was attached before the reset, we don't want any >>>> connect-change status which might be provoked by the reset to >>>> last, because we don't want the core to think that the device >>>> was unplugged and replugged when all that happened was a reset. >>>> >>>> So the important criterion is whether or not a device was attached to >>>> the port when the reset started. It's something of a coincidence that >>>> you only observe warm resets when there's nothing attached. >>>> >>>>> During the first run of usb_hub_reset the udev is NULL because in >>>>> this situation the device is not attached logically. >>>>> >>>>> [ 112.889810] usb 4-1-port1: XXX: port_event: portstatus: 0x2c0, portchange: 0x40! >>>>> [ 113.201192] usb 4-1-port1: XXX: hub_port_reset: udev: (nil)! >>>>> [ 113.201198] usb 4-1-port1: XXX: hub_port_reset (not clearing USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x1! >>>>> [ 113.253612] usb 4-1-port1: XXX: port_event: portstatus: 0x203, portchange: 0x1! >>>>> [ 113.377208] usb 4-1-port1: XXX: hub_port_reset: udev: ffff88046b302800! >>>>> [ 113.377214] usb 4-1-port1: XXX: hub_port_reset (not clearing USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x0! >>>>> [ 113.429478] usb 4-1.1: new SuperSpeed USB device number 7 using xhci_hcd >>>>> [ 113.442370] usb 4-1.1: New USB device found, idVendor=0781, idProduct=5596 >>>>> [ 113.442376] usb 4-1.1: New USB device strings: Mfr=1, Product=2, SerialNumber=3 >>>>> [ 113.442381] usb 4-1.1: Product: Ultra T C >>>>> [ 113.442385] usb 4-1.1: Manufacturer: SanDisk >>>>> [ 113.442388] usb 4-1.1: SerialNumber: 4C530001131013121031 >>>>> >>>>> Or maybe we can skip clearing the USB_PORT_FEAT_C_CONNECTION bit in >>>>> case of hub_port_reset completely without any other check? >>>> >>>> See above. >>> >>> Checking for udev sounds reasonable to me. >>> Not sure if we should worry about the specific host+device combo that needed flags >>> cleared before warm reset. See patch: >>> >>> 10d674a USB: When hot reset for USB3 fails, try warm reset. >>> https://marc.info/?l=linux-usb&m=131603549603799&w=2 >>> >>> -Mathias >> Checking for udev works well too in my case. So there is no need to check the warm flag. >> >> Regarding the other point about the specific host+device combo which needs the flags cleared, I don't know how to >> proceed. >> > > I support just adding a udev check patch, want to send one? Ok, I will do so. > > Current hub port reset code is wrong, causing real life issues today. > > The issue with the specific host+device is from 2011, > The port reset code has changed completely since then. > If it turns out to still be a issue we can make a host/device specific quirk. >> -Mathias ok, understood. Dennis ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] usb: core: Fix hub port connection events lost 2018-11-09 13:47 ` Mathias Nyman 2018-11-13 13:38 ` Dennis Wassenberg @ 2018-11-13 13:40 ` Dennis Wassenberg 2018-11-13 13:55 ` Mathias Nyman 1 sibling, 1 reply; 12+ messages in thread From: Dennis Wassenberg @ 2018-11-13 13:40 UTC (permalink / raw) To: Mathias Nyman, Alan Stern, Mathias Nyman, Greg Kroah-Hartman Cc: Ravi Chandra Sadineni, Kuppuswamy Sathyanarayanan, Bin Liu, Maxim Moseychuk, Mike Looijmans, Dominik Bozek, USB list, Kernel development list This will clear the USB_PORT_FEAT_C_CONNECTION bit in case of a hub port reset only if a device is was attached to the hub port before resetting the hub port. Using a Lenovo T480s attached to the ultra dock it was not possible to detect some usb-c devices at the dock usb-c ports because the hub_port_reset code will clear the USB_PORT_FEAT_C_CONNECTION bit after the actual hub port reset. Using this device combo the USB_PORT_FEAT_C_CONNECTION bit was set between the actual hub port reset and the clear of the USB_PORT_FEAT_C_CONNECTION bit. This ends up with clearing the USB_PORT_FEAT_C_CONNECTION bit after the new device was attached such that it was not detected. This patch will not clear the USB_PORT_FEAT_C_CONNECTION bit if there is currently no device attached to the port before the hub port reset. This will avoid clearing the connection bit for new attached devices. Signed-off-by: Dennis Wassenberg <dennis.wassenberg@secunet.com> --- drivers/usb/core/hub.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index c6077d582d29..2731fad6f659 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -2849,7 +2849,9 @@ static int hub_port_reset(struct usb_hub *hub, int port1, USB_PORT_FEAT_C_BH_PORT_RESET); usb_clear_port_feature(hub->hdev, port1, USB_PORT_FEAT_C_PORT_LINK_STATE); - usb_clear_port_feature(hub->hdev, port1, + + if (udev) + usb_clear_port_feature(hub->hdev, port1, USB_PORT_FEAT_C_CONNECTION); /* -- 2.17.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] usb: core: Fix hub port connection events lost 2018-11-13 13:40 ` [PATCH] usb: core: Fix hub port connection events lost Dennis Wassenberg @ 2018-11-13 13:55 ` Mathias Nyman 0 siblings, 0 replies; 12+ messages in thread From: Mathias Nyman @ 2018-11-13 13:55 UTC (permalink / raw) To: Dennis Wassenberg, Alan Stern, Mathias Nyman, Greg Kroah-Hartman Cc: Ravi Chandra Sadineni, Kuppuswamy Sathyanarayanan, Bin Liu, Maxim Moseychuk, Mike Looijmans, Dominik Bozek, USB list, Kernel development list On 13.11.2018 15:40, Dennis Wassenberg wrote: > This will clear the USB_PORT_FEAT_C_CONNECTION bit in case of a hub port reset > only if a device is was attached to the hub port before resetting the hub port. > > Using a Lenovo T480s attached to the ultra dock it was not possible to detect > some usb-c devices at the dock usb-c ports because the hub_port_reset code > will clear the USB_PORT_FEAT_C_CONNECTION bit after the actual hub port reset. > Using this device combo the USB_PORT_FEAT_C_CONNECTION bit was set between the > actual hub port reset and the clear of the USB_PORT_FEAT_C_CONNECTION bit. > This ends up with clearing the USB_PORT_FEAT_C_CONNECTION bit after the > new device was attached such that it was not detected. > > This patch will not clear the USB_PORT_FEAT_C_CONNECTION bit if there is > currently no device attached to the port before the hub port reset. > This will avoid clearing the connection bit for new attached devices. > > Signed-off-by: Dennis Wassenberg <dennis.wassenberg@secunet.com> > --- > drivers/usb/core/hub.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > index c6077d582d29..2731fad6f659 100644 > --- a/drivers/usb/core/hub.c > +++ b/drivers/usb/core/hub.c > @@ -2849,7 +2849,9 @@ static int hub_port_reset(struct usb_hub *hub, int port1, > USB_PORT_FEAT_C_BH_PORT_RESET); > usb_clear_port_feature(hub->hdev, port1, > USB_PORT_FEAT_C_PORT_LINK_STATE); > - usb_clear_port_feature(hub->hdev, port1, > + > + if (udev) > + usb_clear_port_feature(hub->hdev, port1, > USB_PORT_FEAT_C_CONNECTION); > > /* > Acked-by: Mathias Nyman <mathias.nyman@linux.intel.com> ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-11-13 13:51 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-25 12:20 USB-C device hotplug issue Dennis Wassenberg 2018-10-25 12:28 ` Greg Kroah-Hartman 2018-10-25 12:38 ` Dennis Wassenberg 2018-10-25 14:46 ` Alan Stern 2018-10-26 9:44 ` Dennis Wassenberg 2018-10-26 14:07 ` Alan Stern 2018-11-05 15:35 ` Mathias Nyman 2018-11-07 9:08 ` Dennis Wassenberg 2018-11-09 13:47 ` Mathias Nyman 2018-11-13 13:38 ` Dennis Wassenberg 2018-11-13 13:40 ` [PATCH] usb: core: Fix hub port connection events lost Dennis Wassenberg 2018-11-13 13:55 ` 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).