Patchwork usb: handle warm-reset port requests on hub resume

login
register
mail settings
Submitter Jan-Marek Glogowski
Date Feb. 1, 2019, 12:52 p.m.
Message ID <1549025551-4306-1-git-send-email-glogow@fbihome.de>
Download mbox | patch
Permalink /patch/715757/
State New
Headers show

Comments

Jan-Marek Glogowski - Feb. 1, 2019, 12:52 p.m.
On plug-in of my USB-C device, its USB_SS_PORT_LS_SS_INACTIVE
link state bit is set. Greping all the kernel for this bit shows
that the port status requests a warm-reset this way.

This just happens, if its the only device on the root hub, the hub
therefore resumes and the HCDs status_urb isn't yet available.
If a warm-reset request is detected, this sets the hubs event_bits,
which will prevent any auto-suspend and allows the hubs workqueue
to warm-reset the port later in port_event.

Signed-off-by: Jan-Marek Glogowski <glogow@fbihome.de>
---

The original thread is "USB-C storage device not detected on USB 3.1 Gen 2
host when plugged in after boot". A different patch, suggested by Mathias
Nyman, didn't work for me. This patch was just rebased on usb-next, but not
re-tested. Original tests are based on 5.0-rc.

v1: This always warm-resets the ports in hub_activate, independent of the
"enum hub_activation_type". Just had a single device to test.

v2: I had the idea about the working device, if there is already a device
connected to the hub and that a resume only on "type == HUB_RESUME" should
be sufficient. This still works for me, but I didn't follow all the
hub_activate callers everywhere and I'm definitly still missing a lot of
knowledge about USB stuff. There is also HUB_RESET_RESUME with a slightly
different code path. I don't know how to trigger this.

v3: code unchanged to v2.

v4: instead of handling the warm-reset directly from hub_activate by calling
hub_port_reset, this defers the reset by setting the hubs event_bits of the
port.

---
 drivers/usb/core/hub.c | 7 +++++++
 1 file changed, 7 insertions(+)
Jan-Marek Glogowski - Feb. 1, 2019, 12:58 p.m.
This is actually v4.

With this patch applied, dmesg on device plug looks like this and has no more suspend-resume-cycle:

[  454.845205] xhci_hcd:handle_port_status: xhci_hcd 0000:00:14.0: Port Status Change Event for port 19
[  454.845215] xhci_hcd:handle_port_status: xhci_hcd 0000:00:14.0: resume root hub
[  454.845231] xhci_hcd:handle_port_status: xhci_hcd 0000:00:14.0: handle_port_status: starting port polling.
[  454.845295] usbcore:usb_remote_wakeup: usb usb2: usb wakeup-resume
[  454.845303] usbcore:hcd_bus_resume: usb usb2: usb auto-resume
[  454.845324] usbcore:hub_resume: hub 2-0:1.0: hub_resume
[  454.845343] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: get port status, actual port 0 status  = 0x2a0
[  454.845351] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: Get port status returned 0x2a0
[  454.845382] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: get port status, actual port 1 status  = 0x2a0
[  454.845386] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: Get port status returned 0x2a0
[  454.845402] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: get port status, actual port 2 status  = 0x4002c0
[  454.845406] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: Get port status returned 0x4002c0
[  454.845423] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: get port status, actual port 3 status  = 0x2a0
[  454.845432] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: Get port status returned 0x2a0
[  454.845489] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: get port status, actual port 4 status  = 0x2a0
[  454.845495] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: Get port status returned 0x2a0
[  454.845510] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: get port status, actual port 5 status  = 0x2a0
[  454.845514] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: Get port status returned 0x2a0
[  454.845539] usbcore:hub_event: hub 2-0:1.0: state 7 ports 6 chg 0000 evt 0008
[  454.845549] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: get port status, actual port 2 status  = 0x4002c0
[  454.845553] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: Get port status returned 0x4002c0
[  454.845565] usbcore:port_event: usb usb2-port3: link state change
[  454.845575] xhci_hcd:xhci_clear_port_change_bit: xhci_hcd 0000:00:14.0: clear port link state change, actual port 2 status  = 0x2c0
[  454.845586] usbcore:port_event: usb usb2-port3: do warm reset
[  454.853463] xhci_hcd:xhci_hub_status_data: xhci_hcd 0000:00:14.0: xhci_hub_status_data: stopping port polling.
[  454.905757] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: get port status, actual port 2 status  = 0x2b0
[  454.905763] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: Get port status returned 0x2b0
[  454.905793] usbcore:hub_port_wait_reset: usb usb2-port3: not warm reset yet, waiting 50ms
[  454.950999] xhci_hcd:handle_port_status: xhci_hcd 0000:00:14.0: Port Status Change Event for port 19
[  454.951010] xhci_hcd:handle_port_status: xhci_hcd 0000:00:14.0: handle_port_status: starting port polling.
[  454.965676] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: get port status, actual port 2 status  = 0x2a1203
[  454.965682] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: Get port status returned 0x310203
[  454.965724] xhci_hcd:xhci_clear_port_change_bit: xhci_hcd 0000:00:14.0: clear port reset change, actual port 2 status  = 0xa1203
[  454.965742] xhci_hcd:xhci_clear_port_change_bit: xhci_hcd 0000:00:14.0: clear port warm(BH) reset change, actual port 2 status  = 0x21203
[  454.965760] xhci_hcd:xhci_clear_port_change_bit: xhci_hcd 0000:00:14.0: clear port link state change, actual port 2 status  = 0x21203
[  454.965773] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: get port status, actual port 2 status  = 0x21203
[  454.965777] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: Get port status returned 0x10203
[  455.025707] usbcore:hub_event: hub 2-0:1.0: state 7 ports 6 chg 0000 evt 0008
[  455.025726] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: get port status, actual port 2 status  = 0x21203
[  455.025731] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: Get port status returned 0x10203
[  455.025763] xhci_hcd:xhci_clear_port_change_bit: xhci_hcd 0000:00:14.0: clear port connect change, actual port 2 status  = 0x1203
[  455.025777] usbcore:hub_port_connect_change: usb usb2-port3: status 0203, change 0001, 10.0 Gb/s
[  455.025785] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: get port status, actual port 2 status  = 0x1203
[  455.025790] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: Get port status returned 0x203
[  455.061704] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: get port status, actual port 2 status  = 0x1203
[  455.061710] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: Get port status returned 0x203
[  455.097673] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: get port status, actual port 2 status  = 0x1203
[  455.097679] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: Get port status returned 0x203
[  455.121663] xhci_hcd:xhci_hub_status_data: xhci_hcd 0000:00:14.0: xhci_hub_status_data: stopping port polling.
[  455.133520] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: get port status, actual port 2 status  = 0x1203
[  455.133526] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: Get port status returned 0x203
[  455.169674] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: get port status, actual port 2 status  = 0x1203
[  455.169680] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: Get port status returned 0x203
[  455.169706] usbcore:hub_port_debounce: usb usb2-port3: debounce total 100ms stable 100ms status 0x203
[  455.169718] xhci_hcd:xhci_ring_cmd_db: xhci_hcd 0000:00:14.0: // Ding dong!
[  455.169796] xhci_hcd:xhci_alloc_virt_device: xhci_hcd 0000:00:14.0: Slot 1 output ctx = 0x44e420000 (dma)
[  455.169802] xhci_hcd:xhci_alloc_virt_device: xhci_hcd 0000:00:14.0: Slot 1 input ctx = 0x456743000 (dma)
[  455.169814] xhci_hcd:xhci_alloc_virt_device: xhci_hcd 0000:00:14.0: Set slot id 1 dcbaa entry 000000008f69af20 to 0x44e420000
[  455.169861] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: get port status, actual port 2 status  = 0x1203
[  455.169866] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: Get port status returned 0x203
[  455.169886] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: set port reset, actual port 2 status  = 0x1311
[  455.169895] xhci_hcd:handle_port_status: xhci_hcd 0000:00:14.0: Port Status Change Event for port 19
[  455.169900] xhci_hcd:handle_port_status: xhci_hcd 0000:00:14.0: handle_port_status: starting port polling.
[  455.237702] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: get port status, actual port 2 status  = 0x201203
[  455.237708] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: Get port status returned 0x100203
[  455.237743] xhci_hcd:xhci_clear_port_change_bit: xhci_hcd 0000:00:14.0: clear port reset change, actual port 2 status  = 0x1203
[  455.237759] xhci_hcd:xhci_clear_port_change_bit: xhci_hcd 0000:00:14.0: clear port warm(BH) reset change, actual port 2 status  = 0x1203
[  455.237778] xhci_hcd:xhci_clear_port_change_bit: xhci_hcd 0000:00:14.0: clear port link state change, actual port 2 status  = 0x1203
[  455.237792] xhci_hcd:xhci_clear_port_change_bit: xhci_hcd 0000:00:14.0: clear port connect change, actual port 2 status  = 0x1203
[  455.237807] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: get port status, actual port 2 status  = 0x1203
[  455.237811] xhci_hcd:xhci_hub_control: xhci_hcd 0000:00:14.0: Get port status returned 0x203
[  455.297761] xhci_hcd:xhci_setup_addressable_virt_dev: xhci_hcd 0000:00:14.0: Set root hub portnum to 19
[  455.297768] xhci_hcd:xhci_setup_addressable_virt_dev: xhci_hcd 0000:00:14.0: Set fake root hub portnum to 3
[  455.297773] xhci_hcd:xhci_setup_addressable_virt_dev: xhci_hcd 0000:00:14.0: udev->tt =           (null)
[  455.297778] xhci_hcd:xhci_setup_addressable_virt_dev: xhci_hcd 0000:00:14.0: udev->ttport = 0x0
[  455.297786] xhci_hcd:xhci_ring_cmd_db: xhci_hcd 0000:00:14.0: // Ding dong!
[  455.297856] xhci_hcd:xhci_dbg_trace: xhci_hcd 0000:00:14.0: Successful setup address command
[  455.297868] xhci_hcd:xhci_dbg_trace: xhci_hcd 0000:00:14.0: Op regs DCBAA ptr = 0x0000045673a000
[  455.297875] xhci_hcd:xhci_dbg_trace: xhci_hcd 0000:00:14.0: Slot ID 1 dcbaa entry @000000008f69af20 = 0x0000044e420000
[  455.297880] xhci_hcd:xhci_dbg_trace: xhci_hcd 0000:00:14.0: Output Context DMA address = 0x44e420000
[  455.297885] xhci_hcd:xhci_dbg_trace: xhci_hcd 0000:00:14.0: Internal device address = 0
[  455.297895] usb 2-3: new SuperSpeed Gen 1 USB device number 2 using xhci_hcd
[  455.318310] usbcore:usb_parse_endpoint: usb 2-3: skipped 1 descriptor after endpoint
[  455.318316] usbcore:usb_parse_endpoint: usb 2-3: skipped 1 descriptor after endpoint
[  455.318347] xhci_hcd:process_ctrl_td: xhci_hcd 0000:00:14.0: Waiting for status stage event
[  455.318393] usbcore:usb_get_langid: usb 2-3: default language 0x0409
[  455.318425] xhci_hcd:process_ctrl_td: xhci_hcd 0000:00:14.0: Waiting for status stage event
[  455.318504] xhci_hcd:process_ctrl_td: xhci_hcd 0000:00:14.0: Waiting for status stage event
[  455.318584] xhci_hcd:process_ctrl_td: xhci_hcd 0000:00:14.0: Waiting for status stage event
[  455.318634] usbcore:usb_new_device: usb 2-3: udev 2, busnum 2, minor = 129
[  455.318639] usb 2-3: New USB device found, idVendor=0781, idProduct=5596, bcdDevice= 1.00
[  455.318643] usb 2-3: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[  455.318647] usb 2-3: Product: Ultra T C
[  455.318650] usb 2-3: Manufacturer: SanDisk
[  455.318653] usb 2-3: SerialNumber: 4C530001090830111403
...

Do we still want / need to fix the status_urb == NULL problem from xhci?

Jan-Marek
Alan Stern - Feb. 1, 2019, 3:53 p.m.
On Fri, 1 Feb 2019, Jan-Marek Glogowski wrote:

> On plug-in of my USB-C device, its USB_SS_PORT_LS_SS_INACTIVE
> link state bit is set. Greping all the kernel for this bit shows
> that the port status requests a warm-reset this way.
> 
> This just happens, if its the only device on the root hub, the hub
> therefore resumes and the HCDs status_urb isn't yet available.
> If a warm-reset request is detected, this sets the hubs event_bits,
> which will prevent any auto-suspend and allows the hubs workqueue
> to warm-reset the port later in port_event.
> 
> Signed-off-by: Jan-Marek Glogowski <glogow@fbihome.de>
> ---
> 
> The original thread is "USB-C storage device not detected on USB 3.1 Gen 2
> host when plugged in after boot". A different patch, suggested by Mathias
> Nyman, didn't work for me. This patch was just rebased on usb-next, but not
> re-tested. Original tests are based on 5.0-rc.
> 
> v1: This always warm-resets the ports in hub_activate, independent of the
> "enum hub_activation_type". Just had a single device to test.
> 
> v2: I had the idea about the working device, if there is already a device
> connected to the hub and that a resume only on "type == HUB_RESUME" should
> be sufficient. This still works for me, but I didn't follow all the
> hub_activate callers everywhere and I'm definitly still missing a lot of
> knowledge about USB stuff. There is also HUB_RESET_RESUME with a slightly
> different code path. I don't know how to trigger this.
> 
> v3: code unchanged to v2.
> 
> v4: instead of handling the warm-reset directly from hub_activate by calling
> hub_port_reset, this defers the reset by setting the hubs event_bits of the
> port.
> 
> ---
>  drivers/usb/core/hub.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index bb0830c..8d4631c 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -108,6 +108,8 @@ EXPORT_SYMBOL_GPL(ehci_cf_port_reset_rwsem);
>  static void hub_release(struct kref *kref);
>  static int usb_reset_and_verify_device(struct usb_device *udev);
>  static int hub_port_disable(struct usb_hub *hub, int port1, int set_state);
> +static bool hub_port_warm_reset_required(struct usb_hub *hub, int port1,
> +		u16 portstatus);
>  
>  static inline char *portspeed(struct usb_hub *hub, int portstatus)
>  {
> @@ -1137,6 +1139,11 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
>  						   USB_PORT_FEAT_ENABLE);
>  		}
>  
> +		/* Make sure a warm-reset request is handled by port_event */
> +		if (type == HUB_RESUME &&
> +		    hub_port_warm_reset_required(hub, port1, portstatus))
> +			set_bit(port1, hub->event_bits);
> +
>  		/*
>  		 * Add debounce if USB3 link is in polling/link training state.
>  		 * Link will automatically transition to Enabled state after

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Patch

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index bb0830c..8d4631c 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -108,6 +108,8 @@  EXPORT_SYMBOL_GPL(ehci_cf_port_reset_rwsem);
 static void hub_release(struct kref *kref);
 static int usb_reset_and_verify_device(struct usb_device *udev);
 static int hub_port_disable(struct usb_hub *hub, int port1, int set_state);
+static bool hub_port_warm_reset_required(struct usb_hub *hub, int port1,
+		u16 portstatus);
 
 static inline char *portspeed(struct usb_hub *hub, int portstatus)
 {
@@ -1137,6 +1139,11 @@  static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
 						   USB_PORT_FEAT_ENABLE);
 		}
 
+		/* Make sure a warm-reset request is handled by port_event */
+		if (type == HUB_RESUME &&
+		    hub_port_warm_reset_required(hub, port1, portstatus))
+			set_bit(port1, hub->event_bits);
+
 		/*
 		 * Add debounce if USB3 link is in polling/link training state.
 		 * Link will automatically transition to Enabled state after