All of lore.kernel.org
 help / color / mirror / Atom feed
From: Mathias Nyman <mathias.nyman@linux.intel.com>
To: daniel@quora.org
Cc: <linux-usb@vger.kernel.org>, <stern@rowland.harvard.edu>,
	linux-kernel@vger.kernel.org,
	Mathias Nyman <mathias.nyman@linux.intel.com>
Subject: [TESTPATCH] xhci: fix usb2 resume timing and races.
Date: Mon, 30 Nov 2015 17:16:32 +0200	[thread overview]
Message-ID: <1448896592-26421-1-git-send-email-mathias.nyman@linux.intel.com> (raw)
In-Reply-To: <CAMVG2sus03xnbCSikFGG+t2fXM_gME_jMeGpGHxVuVRks07WCg@mail.gmail.com>

usb2 ports need to signal resume for 20ms before moving to U0 state.
Both device and host can initiate resume.

On host initated resume port is set to resume state, sleep 20ms,
and finally set port to U0 state.

On device initated resume a port status interrupt with a port in resume
state in issued. The interrupt handler tags a resume_done[port]
timestamp with current time + 20ms, 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.

There are a few issues with this approach,
1. A host initated resume will also generate a resume event, the event
   handler will find the port in resume state, believe it's a device
   initated and act accordingly.

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

3. If an error, or anything else happends to the port during device
   initated 20ms resume signalling it will leave all device resume
   parameters hanging uncleared preventing further resume.

Fix this by using the existing resuming_ports bitfield to indicate if
resume signalling timing is taken care of.
Also check if the resume_done[port] is set  before using it in time
comparison. Also clear out any resume signalling related variables if port
is not in U0 or Resume state.

Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-hub.c  | 38 ++++++++++++++++++++++++++++++++++++--
 drivers/usb/host/xhci-ring.c |  3 ++-
 2 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 78241b5..c4f5e41 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -616,8 +616,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 20ms 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, do nothing
+				 */
+			} 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 20ms? yet? */
+		} else if (time_after_eq(jiffies,
+					 bus_state->resume_done[wIndex])) {
 			int time_left;
 
 			xhci_dbg(xhci, "Resume USB2 port %d\n",
@@ -665,6 +687,16 @@ static u32 xhci_get_port_status(struct usb_hcd *hcd,
 			status |= USB_PORT_STAT_SUSPEND;
 		}
 	}
+	/* Clear stale usb2 resume signalling variables in case port changed
+	 * state during 20ms 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))) {
@@ -995,6 +1027,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);
@@ -1002,6 +1035,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 97ffe39..3743bb2 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1583,7 +1583,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);
-- 
1.9.1


       reply	other threads:[~2015-11-30 15:11 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CAMVG2sus03xnbCSikFGG+t2fXM_gME_jMeGpGHxVuVRks07WCg@mail.gmail.com>
2015-11-30 15:16 ` Mathias Nyman [this message]
2015-11-30 15:59   ` [TESTPATCH] xhci: fix usb2 resume timing and races kbuild test robot
2015-12-01 15:47   ` Alan Stern
2015-12-02  8:50     ` Mathias Nyman
2015-12-01  8:26 ` [TESTPATCH v2] " Mathias Nyman
2015-12-01 14:32   ` Felipe Balbi
2015-12-01 15:08     ` Mathias Nyman
2015-12-01 15:11       ` Felipe Balbi
2015-12-05  4:02   ` Daniel J Blueman
2015-12-08 11:16     ` Mathias Nyman

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=1448896592-26421-1-git-send-email-mathias.nyman@linux.intel.com \
    --to=mathias.nyman@linux.intel.com \
    --cc=daniel@quora.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=stern@rowland.harvard.edu \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.