stable.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jiri Slaby <jslaby@suse.cz>
To: stable@vger.kernel.org
Cc: Mathias Nyman <mathias.nyman@linux.intel.com>,
	Jiri Slaby <jslaby@suse.cz>
Subject: [patch added to 3.12-stable] xhci: fix usb2 resume timing and races.
Date: Mon, 15 Feb 2016 17:12:35 +0100	[thread overview]
Message-ID: <1455552767-7413-7-git-send-email-jslaby@suse.cz> (raw)
In-Reply-To: <1455552767-7413-1-git-send-email-jslaby@suse.cz>

From: Mathias Nyman <mathias.nyman@linux.intel.com>

This patch has been added to the 3.12 stable tree. If you have any
objections, please let us know.

===============

commit f69115fdbc1ac0718e7d19ad3caa3da2ecfe1c96 upstream.

According to USB 2 specs ports need to signal resume for at least 20ms,
in practice even longer, before moving to U0 state.
Both host and devices can initiate resume.

On device initiated resume, a port status interrupt with the port in resume
state in issued. The interrupt handler tags a resume_done[port]
timestamp with current time + USB_RESUME_TIMEOUT, and kick roothub timer.
Root hub timer requests for port status, finds the port in resume state,
checks if resume_done[port] timestamp passed, and set port to U0 state.

On host initiated resume, current code sets the port to resume state,
sleep 20ms, and finally sets the port to U0 state. This should also
be changed to work in a similar way as the device initiated resume, with
timestamp tagging, but that is not yet tested and will be a separate
fix later.

There are a few issues with this approach

1. A host initiated resume will also generate a resume event. The event
   handler will find the port in resume state, believe it's a device
   initiated resume, and act accordingly.

2. A port status request might cut the resume signalling short if a
   get_port_status request is handled during the host resume signalling.
   The port will be found in resume state. The timestamp is not set leading
   to time_after_eq(jiffies, timestamp) returning true, as timestamp = 0.
   get_port_status will proceed with moving the port to U0.

3. If an error, or anything else happens to the port during device
   initiated resume signalling it will leave all the device resume
   parameters hanging uncleared, preventing further suspend, returning
   -EBUSY, and cause the pm thread to busyloop trying to enter suspend.

Fix this by using the existing resuming_ports bitfield to indicate that
resume signalling timing is taken care of.
Check if the resume_done[port] is set before using it for timestamp
comparison, and also clear out any resume signalling related variables
if port is not in U0 or Resume state

This issue was discovered when a PM thread busylooped, trying to runtime
suspend the xhci USB 2 roothub on a Dell XPS

Reported-by: Daniel J Blueman <daniel@quora.org>
Tested-by: Daniel J Blueman <daniel@quora.org>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/usb/host/xhci-hub.c  | 45 +++++++++++++++++++++++++++++++++++++++-----
 drivers/usb/host/xhci-ring.c |  3 ++-
 2 files changed, 42 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 55b3aa33bc06..66a7641dfff1 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -611,8 +611,30 @@ static u32 xhci_get_port_status(struct usb_hcd *hcd,
 		if ((raw_port_status & PORT_RESET) ||
 				!(raw_port_status & PORT_PE))
 			return 0xffffffff;
-		if (time_after_eq(jiffies,
-					bus_state->resume_done[wIndex])) {
+		/* did port event handler already start resume timing? */
+		if (!bus_state->resume_done[wIndex]) {
+			/* If not, maybe we are in a host initated resume? */
+			if (test_bit(wIndex, &bus_state->resuming_ports)) {
+				/* Host initated resume doesn't time the resume
+				 * signalling using resume_done[].
+				 * It manually sets RESUME state, sleeps 20ms
+				 * and sets U0 state. This should probably be
+				 * changed, but not right now.
+				 */
+			} else {
+				/* port resume was discovered now and here,
+				 * start resume timing
+				 */
+				unsigned long timeout = jiffies +
+					msecs_to_jiffies(USB_RESUME_TIMEOUT);
+
+				set_bit(wIndex, &bus_state->resuming_ports);
+				bus_state->resume_done[wIndex] = timeout;
+				mod_timer(&hcd->rh_timer, timeout);
+			}
+		/* Has resume been signalled for USB_RESUME_TIME yet? */
+		} else if (time_after_eq(jiffies,
+					 bus_state->resume_done[wIndex])) {
 			int time_left;
 
 			xhci_dbg(xhci, "Resume USB2 port %d\n",
@@ -654,13 +676,24 @@ static u32 xhci_get_port_status(struct usb_hcd *hcd,
 		} else {
 			/*
 			 * The resume has been signaling for less than
-			 * 20ms. Report the port status as SUSPEND,
-			 * let the usbcore check port status again
-			 * and clear resume signaling later.
+			 * USB_RESUME_TIME. Report the port status as SUSPEND,
+			 * let the usbcore check port status again and clear
+			 * resume signaling later.
 			 */
 			status |= USB_PORT_STAT_SUSPEND;
 		}
 	}
+	/*
+	 * 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);
+	}
 	if ((raw_port_status & PORT_PLS_MASK) == XDEV_U0
 			&& (raw_port_status & PORT_POWER)
 			&& (bus_state->suspended_ports & (1 << wIndex))) {
@@ -991,6 +1024,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 				if ((temp & PORT_PE) == 0)
 					goto error;
 
+				set_bit(wIndex, &bus_state->resuming_ports);
 				xhci_set_link_state(xhci, port_array, wIndex,
 							XDEV_RESUME);
 				spin_unlock_irqrestore(&xhci->lock, flags);
@@ -998,6 +1032,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 				spin_lock_irqsave(&xhci->lock, flags);
 				xhci_set_link_state(xhci, port_array, wIndex,
 							XDEV_U0);
+				clear_bit(wIndex, &bus_state->resuming_ports);
 			}
 			bus_state->port_c_suspend |= 1 << wIndex;
 
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 2c9d2c33b834..68a02abd74ef 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1738,7 +1738,8 @@ static void handle_port_status(struct xhci_hcd *xhci,
 			 */
 			bogus_port_status = true;
 			goto cleanup;
-		} else {
+		} else if (!test_bit(faked_port_index,
+				     &bus_state->resuming_ports)) {
 			xhci_dbg(xhci, "resume HS port %d\n", port_id);
 			bus_state->resume_done[faked_port_index] = jiffies +
 				msecs_to_jiffies(USB_RESUME_TIMEOUT);
-- 
2.7.1


  parent reply	other threads:[~2016-02-15 16:12 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-02-15 16:12 [patch added to 3.12-stable] USB: serial: ftdi_sio: add support for Yaesu SCU-18 cable Jiri Slaby
2016-02-15 16:12 ` [patch added to 3.12-stable] USB: cp210x: add ID for IAI USB to RS485 adaptor Jiri Slaby
2016-02-15 16:12 ` [patch added to 3.12-stable] USB: serial: option: Adding support for Telit LE922 Jiri Slaby
2016-02-15 16:12 ` [patch added to 3.12-stable] USB: option: fix Cinterion AHxx enumeration Jiri Slaby
2016-02-15 16:12 ` [patch added to 3.12-stable] tty: Fix GPF in flush_to_ldisc() Jiri Slaby
2016-02-15 16:12 ` [patch added to 3.12-stable] tty: Fix unsafe ldisc reference via ioctl(TIOCGETD) Jiri Slaby
2016-02-15 16:12 ` Jiri Slaby [this message]
2016-02-15 16:12 ` [patch added to 3.12-stable] ext4: Fix handling of extended tv_sec Jiri Slaby
2016-02-15 16:12 ` [patch added to 3.12-stable] crypto: af_alg - Disallow bind/setkey/... after accept(2) Jiri Slaby
2016-02-15 16:12 ` [patch added to 3.12-stable] crypto: af_alg - Fix socket double-free when accept fails Jiri Slaby
2016-02-15 16:12 ` [patch added to 3.12-stable] AHCI: Fix softreset failed issue of Port Multiplier Jiri Slaby
2016-02-15 16:12 ` [patch added to 3.12-stable] libata: disable forced PORTS_IMPL for >= AHCI 1.3 Jiri Slaby
2016-02-15 16:12 ` [patch added to 3.12-stable] ahci: Intel DNV device IDs SATA Jiri Slaby
2016-02-15 16:12 ` [patch added to 3.12-stable] crypto: algif_hash - wait for crypto_ahash_init() to complete Jiri Slaby
2016-02-15 16:12 ` [patch added to 3.12-stable] EVM: Use crypto_memneq() for digest comparisons Jiri Slaby
2016-02-15 16:12 ` [patch added to 3.12-stable] crypto: user - lock crypto_alg_list on alg dump Jiri Slaby
2016-02-15 16:12 ` [patch added to 3.12-stable] FS-Cache: Increase reference of parent after registering, netfs success Jiri Slaby
2016-02-15 16:12 ` [patch added to 3.12-stable] FS-Cache: Don't override netfs's primary_index if registering failed Jiri Slaby
2016-02-15 16:12 ` [patch added to 3.12-stable] binfmt_elf: Don't clobber passed executable's file header Jiri Slaby

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1455552767-7413-7-git-send-email-jslaby@suse.cz \
    --to=jslaby@suse.cz \
    --cc=mathias.nyman@linux.intel.com \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).