| Message ID | 20190318170038.23914-1-nsaenzjulienne@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] xhci: clear port_remote_wakeup on device disconnection | expand |
On Mo, 2019-03-18 at 18:00 +0100, Nicolas Saenz Julienne wrote: > > This patch clears "port_remote_wakeup" upon detecting a device > disconnection. Making sure the above mentioned situation doesn't trigger > a PM busyloop. Hi, that is an interesting race condition. > > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > --- > drivers/usb/host/xhci-hub.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c > index e2eece693655..bea853f45aec 100644 > --- a/drivers/usb/host/xhci-hub.c > +++ b/drivers/usb/host/xhci-hub.c > @@ -942,6 +942,9 @@ static void xhci_get_usb3_port_status(struct xhci_port *port, u32 *status, > bus_state->suspended_ports &= ~(1 << portnum); > } > > + if (!(portsc & PORT_CONNECT)) > + bus_state->port_remote_wakeup &= ~(1 << portnum); > + > xhci_hub_report_usb3_link_state(xhci, status, portsc); > xhci_del_comp_mod_timer(xhci, portsc, portnum); > } Why are you putting that logic into xhci_get_usb3_port_status()? It looks to me like there is already something related in the caller /* USB2 and USB3 specific bits, including Port Link State */ if (hcd->speed >= HCD_USB3) xhci_get_usb3_port_status(port, &status, raw_port_status); else xhci_get_usb2_port_status(port, &status, raw_port_status, flags); /* * Clear stale usb2 resume signalling variables in case port changed * state during resume signalling. For example on error */ if ((bus_state->resume_done[wIndex] || test_bit(wIndex, &bus_state->resuming_ports)) && (raw_port_status & PORT_PLS_MASK) != XDEV_U3 && (raw_port_status & PORT_PLS_MASK) != XDEV_RESUME) { bus_state->resume_done[wIndex] = 0; clear_bit(wIndex, &bus_state->resuming_ports); usb_hcd_end_port_resume(&hcd->self, wIndex); } Otherwise very good catch! Regards Oliver
Hi Oliver, thanks for the review! On Tue, 2019-03-19 at 12:01 +0100, Oliver Neukum wrote: > On Mo, 2019-03-18 at 18:00 +0100, Nicolas Saenz Julienne wrote: > > This patch clears "port_remote_wakeup" upon detecting a device > > disconnection. Making sure the above mentioned situation doesn't trigger > > a PM busyloop. > > Hi, > > that is an interesting race condition. > > > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > > --- > > drivers/usb/host/xhci-hub.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c > > index e2eece693655..bea853f45aec 100644 > > --- a/drivers/usb/host/xhci-hub.c > > +++ b/drivers/usb/host/xhci-hub.c > > @@ -942,6 +942,9 @@ static void xhci_get_usb3_port_status(struct xhci_port > > *port, u32 *status, > > bus_state->suspended_ports &= ~(1 << portnum); > > } > > > > + if (!(portsc & PORT_CONNECT)) > > + bus_state->port_remote_wakeup &= ~(1 << portnum); > > + > > xhci_hub_report_usb3_link_state(xhci, status, portsc); > > xhci_del_comp_mod_timer(xhci, portsc, portnum); > > } > > Why are you putting that logic into xhci_get_usb3_port_status()? > It looks to me like there is already something related in the caller > > /* USB2 and USB3 specific bits, including Port Link State */ > if (hcd->speed >= HCD_USB3) > xhci_get_usb3_port_status(port, &status, raw_port_status); > else > xhci_get_usb2_port_status(port, &status, raw_port_status, > flags); > /* > * Clear stale usb2 resume signalling variables in case port changed > * state during resume signalling. For example on error > */ > if ((bus_state->resume_done[wIndex] || > test_bit(wIndex, &bus_state->resuming_ports)) && > (raw_port_status & PORT_PLS_MASK) != XDEV_U3 && > (raw_port_status & PORT_PLS_MASK) != XDEV_RESUME) { > bus_state->resume_done[wIndex] = 0; > clear_bit(wIndex, &bus_state->resuming_ports); > usb_hcd_end_port_resume(&hcd->self, wIndex); > } You're right, xhci_get_usb3_port_status() is not the ideal spot I'll move the code there. > > Otherwise very good catch! > > Regards > Oliver >
On 19.3.2019 16.54, Nicolas Saenz Julienne wrote: > Hi Oliver, thanks for the review! > > On Tue, 2019-03-19 at 12:01 +0100, Oliver Neukum wrote: >> On Mo, 2019-03-18 at 18:00 +0100, Nicolas Saenz Julienne wrote: >>> This patch clears "port_remote_wakeup" upon detecting a device >>> disconnection. Making sure the above mentioned situation doesn't trigger >>> a PM busyloop. >> >> Hi, >> >> that is an interesting race condition. >> Turns out that port_remote_wakeup was only properly cleared in case of a successful resume where port stays in U0, or in case of an error where port goes to inactive state. In all other cases port_remote_wakeup bit was left uncleared. >>> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> >>> --- >>> drivers/usb/host/xhci-hub.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c >>> index e2eece693655..bea853f45aec 100644 >>> --- a/drivers/usb/host/xhci-hub.c >>> +++ b/drivers/usb/host/xhci-hub.c >>> @@ -942,6 +942,9 @@ static void xhci_get_usb3_port_status(struct xhci_port >>> *port, u32 *status, >>> bus_state->suspended_ports &= ~(1 << portnum); >>> } >>> >>> + if (!(portsc & PORT_CONNECT)) >>> + bus_state->port_remote_wakeup &= ~(1 << portnum); >>> + A check like this should cover the failing resume cases, thanks We also saw a case when a successful resume was missed as port went into U1 Link power save state (resume->U0->U1) before xhci driver read the port status. A patch adding U1 and U2 to successful resume states is pending in my for-usb-linus branch for this. >> >> Why are you putting that logic into xhci_get_usb3_port_status()? >> It looks to me like there is already something related in the caller > > You're right, xhci_get_usb3_port_status() is not the ideal spot I'll move the > code there. > Sounds good, thanks -Mathias
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c index e2eece693655..bea853f45aec 100644 --- a/drivers/usb/host/xhci-hub.c +++ b/drivers/usb/host/xhci-hub.c @@ -942,6 +942,9 @@ static void xhci_get_usb3_port_status(struct xhci_port *port, u32 *status, bus_state->suspended_ports &= ~(1 << portnum); } + if (!(portsc & PORT_CONNECT)) + bus_state->port_remote_wakeup &= ~(1 << portnum); + xhci_hub_report_usb3_link_state(xhci, status, portsc); xhci_del_comp_mod_timer(xhci, portsc, portnum); }
This was reported on a Dell Precision 5520 using it's WD15 dock. The dock's Ethernet device interfaces with the laptop through one of it's USB3 ports. While idle, the Ethernet device and HCD are suspended by runtime PM, being the only active device connected to the bus. They are both generally resumed on behalf of the Ethernet device, which has remote wake-up capabilities. The Ethernet device was observed randomly disconnecting from the USB port shortly after submitting it's remote wake-up request. This causes the PM to busyloop, as the port is in the middle of a remote wake-up operation, waiting for the device to switch to U0. This leaves a bit set in "port_remote_wakeup", which stops any further suspend/resume operations. The logs look like this (I added the port status print in the first one): [28128.472101] xhci_hcd:handle_port_status: xhci_hcd 0000:3e:00.0: port resume event for port 3, port status 0xc4013e3 [28128.472165] xhci_hcd:xhci_hub_control: xhci_hcd 0000:3e:00.0: get port status, actual port 0 status = 0x1e0 [28128.472200] usbcore:hub_event: hub 4-0:1.0: state 7 ports 2 chg 0002 evt 0000 [28128.472221] usb 4-1: USB disconnect, device number 2 In between the first two logs we acknowledge the remote wake-up request, setting the link state as U0 and setting the relevant bit in "port_remote_wakeup". We also call usb_hcd_resume_root_hub() which will wake up the hub device. Note the CCS & PED bits on the port status are still set (see 5.4.8 in spec). In between the second and third log, the hub device finds out about the disconnection (CSS & PED are now 0), which triggers a call to hub_port_connect() which finds an already existing USB device and disconnects it. Runtime PM, seeing no devices connected to the HCD busyloops trying to suspend the device, which fails every time because of "port_remote_wakeup", generating a tangible CPU load. This patch clears "port_remote_wakeup" upon detecting a device disconnection. Making sure the above mentioned situation doesn't trigger a PM busyloop. Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> --- drivers/usb/host/xhci-hub.c | 3 +++ 1 file changed, 3 insertions(+)