[PATCH 0/5] usb: hub: Fail fast on USB3 LPM requests issues

[PATCH 0/5] usb: hub: Fail fast on USB3 LPM requests issues

[Mathias Nyman]

Stress testing suspend and resume with several Link Power Management
(LPM) capable USB3 hubs and devices reveal that hub workqueue may be
blocked from handling device disconnects due to slow processing LPM
requests over a bad link.

Enabling and disabling USB3 LPM requires sending four USB requests.
These requests are sent in the hub workqueue. If the link is bad then
the requests need to fail first before other port issues are handled.

Fail fast if these USB3 LPM requests fail.
Don't try to recover a failure to disable LPM with enabling LPM.
This just causes even more blocking requests to be sent over a
possibly broken link.

Mathias Nyman (5):
  usb: hub: Block less in USB3 link power management LPM disable path
  usb: hub: verify device is configured in usb_device_may_initiate_lpm()
  usb: hub: Don't disable LPM completely if device initiated LPM fails
  usb: hub: reorder USB3 link power management enable requests
  usb: hub: Fail fast in USB3 link power management enable path

 drivers/usb/core/hub.c | 74 ++++++++++++++++++++++--------------------
 1 file changed, 39 insertions(+), 35 deletions(-)

-- 
2.43.0

[PATCH 1/5] usb: hub: Block less in USB3 link power management LPM disable path

[Mathias Nyman]

Several usb requests are needed to allow or forbid a USB3 link from
going into U1 or U2 hardware link power management (LPM) states.

Fail fast on issues in LPM disabling path. LPM disabling is done in hub
workqueue paths that are often already handling possible link issues.

Enabling and disabling LPM involves four usb requests.
Two requests sent to the upstream hub of the connected device:
SetPortFeature(U1_TIMEOUT)
SetPortFeature(U2_TIMEOUT)

And two to the device itself:
SetFeature(U1_ENABLE)
SetFeature(U2_ENABLE)

The requests to the hub sets the inactivity timeout used by the hub to
know when to initiate U1 and U2 LPM link state transitions.
These requests are also used prevent U1/U2 LPM transitions completely
by passing zero timeout value.

The requsts sent to the device only controls if device is allowed to
initiate U1/U2 transitions. If not enabled then only hub initiates U1/U2
transitions. Hub may block these device initiated attempts.

Reorder and send the hub requests first, these are more likely to succeed
due to shorter path, and we can consider LPM disabled if these succeed
as U1/U2 link state can not be entered after that.

Fail immediately if a request fails, and don't try to enable back LPM
after a failed request, that will just send more LPM requests over a
bad link.

If a device request controlling device initiateed LPM fails then exit
immediately, but consider LPM disabled at this stage.

Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/core/hub.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 8c7f9cc785bb..a901d1b55856 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -4393,8 +4393,6 @@ static int usb_disable_link_state(struct usb_hcd *hcd, struct usb_device *udev,
 	if (usb_set_lpm_timeout(udev, state, 0))
 		return -EBUSY;
 
-	usb_set_device_initiated_lpm(udev, state, false);
-
 	if (hcd->driver->disable_usb3_lpm_timeout(hcd, udev, state))
 		dev_warn(&udev->dev, "Could not disable xHCI %s timeout, "
 				"bus schedule bandwidth may be impacted.\n",
@@ -4424,6 +4422,7 @@ static int usb_disable_link_state(struct usb_hcd *hcd, struct usb_device *udev,
 int usb_disable_lpm(struct usb_device *udev)
 {
 	struct usb_hcd *hcd;
+	int err;
 
 	if (!udev || !udev->parent ||
 			udev->speed < USB_SPEED_SUPER ||
@@ -4441,14 +4440,19 @@ int usb_disable_lpm(struct usb_device *udev)
 
 	/* If LPM is enabled, attempt to disable it. */
 	if (usb_disable_link_state(hcd, udev, USB3_LPM_U1))
-		goto enable_lpm;
+		goto disable_failed;
 	if (usb_disable_link_state(hcd, udev, USB3_LPM_U2))
-		goto enable_lpm;
+		goto disable_failed;
+
+	err = usb_set_device_initiated_lpm(udev, USB3_LPM_U1, false);
+	if (!err)
+		usb_set_device_initiated_lpm(udev, USB3_LPM_U2, false);
 
 	return 0;
 
-enable_lpm:
-	usb_enable_lpm(udev);
+disable_failed:
+	udev->lpm_disable_count--;
+
 	return -EBUSY;
 }
 EXPORT_SYMBOL_GPL(usb_disable_lpm);
-- 
2.43.0

[PATCH 2/5] usb: hub: verify device is configured in usb_device_may_initiate_lpm()

[Mathias Nyman]

Move device configured check into usb_device_may_initiate_lpm() instead
of calling it before the function.

No functional changes, helps rework to fail faster during link power
management (LPM) enabling.

Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/core/hub.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index a901d1b55856..10ef9f51fcfd 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -4234,9 +4234,9 @@ static int usb_set_lpm_timeout(struct usb_device *udev,
 }
 
 /*
- * Don't allow device intiated U1/U2 if the system exit latency + one bus
- * interval is greater than the minimum service interval of any active
- * periodic endpoint. See USB 3.2 section 9.4.9
+ * Don't allow device intiated U1/U2 if device isn't in the configured state,
+ * or the system exit latency + one bus interval is greater than the minimum
+ * service interval of any active periodic endpoint. See USB 3.2 section 9.4.9
  */
 static bool usb_device_may_initiate_lpm(struct usb_device *udev,
 					enum usb3_link_state state)
@@ -4244,7 +4244,7 @@ static bool usb_device_may_initiate_lpm(struct usb_device *udev,
 	unsigned int sel;		/* us */
 	int i, j;
 
-	if (!udev->lpm_devinit_allow)
+	if (!udev->lpm_devinit_allow || !udev->actconfig)
 		return false;
 
 	if (state == USB3_LPM_U1)
@@ -4341,11 +4341,11 @@ static void usb_enable_link_state(struct usb_hcd *hcd, struct usb_device *udev,
 		return;
 	}
 
-	/* Only a configured device will accept the Set Feature
-	 * U1/U2_ENABLE
+	/*
+	 * Enable device initiated U1/U2 with a SetFeature(U1/U2_ENABLE) request
+	 * if system exit latency is short enough and device is configured
 	 */
-	if (udev->actconfig &&
-	    usb_device_may_initiate_lpm(udev, state)) {
+	if (usb_device_may_initiate_lpm(udev, state)) {
 		if (usb_set_device_initiated_lpm(udev, state, true)) {
 			/*
 			 * Request to enable device initiated U1/U2 failed,
-- 
2.43.0

[PATCH 3/5] usb: hub: Don't disable LPM completely if device initiated LPM fails

[Mathias Nyman]

Enabling device initiated USB3 link power management (LPM) may fail for
various reasons such as too long system exit latency, or link issues.

These are not good reason to disable hub initiated LPM U1/U2
states, especially as it requires sending more requests over a
possibly broken link, causing the hub work to block for even longer.

Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/core/hub.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 10ef9f51fcfd..ccf21bb49038 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -4345,17 +4345,8 @@ static void usb_enable_link_state(struct usb_hcd *hcd, struct usb_device *udev,
 	 * Enable device initiated U1/U2 with a SetFeature(U1/U2_ENABLE) request
 	 * if system exit latency is short enough and device is configured
 	 */
-	if (usb_device_may_initiate_lpm(udev, state)) {
-		if (usb_set_device_initiated_lpm(udev, state, true)) {
-			/*
-			 * Request to enable device initiated U1/U2 failed,
-			 * better to turn off lpm in this case.
-			 */
-			usb_set_lpm_timeout(udev, state, 0);
-			hcd->driver->disable_usb3_lpm_timeout(hcd, udev, state);
-			return;
-		}
-	}
+	if (usb_device_may_initiate_lpm(udev, state))
+		usb_set_device_initiated_lpm(udev, state, true);
 
 	if (state == USB3_LPM_U1)
 		udev->usb3_lpm_u1_enabled = 1;
-- 
2.43.0

[PATCH 4/5] usb: hub: reorder USB3 link power management enable requests

[Mathias Nyman]

Several usb requests are needed to allow a USB3 link to enter U1/U2
hardware link power management LPM states. Reorder these requests
and send the more significant and likely to succeed first.
This is similar to the change done for disabling LPM

Enable LPM by first sending requests to the upstream hub of the device
SetPortFeature(U1_TIMEOUT)
SetPortFeature(U2_TIMEOUT)

These are more likely to succeed due to the shorter path, and LPM can
be considered enabled as link may go to U1/U2 LPM states after those.

Send the requests to the device after this, they allow the device
to initialte U1/U2 link transitions. Hub can already initiate U1/U2
SetFeature(U1_ENABLE)
SetFeature(U2_ENABLE)

Fail fast and bail out if a requests to the device fails.

This changes device initated LPM policy a bit. Device is no longer
able to initiate U2 if it failed or is not allowed to initiate
U1.

Enabling and disabling Link power management is done as part of
hub work. Avoid trying to send additional USB requests to a device
when there are known issues. It just causes hub work to block for
even longer.

Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/core/hub.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index ccf21bb49038..d534d7b4606c 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -4160,7 +4160,7 @@ static int usb_set_device_initiated_lpm(struct usb_device *udev,
 				"for unconfigured device.\n",
 				__func__, str_enable_disable(enable),
 				usb3_lpm_names[state]);
-		return 0;
+		return -EINVAL;
 	}
 
 	if (enable) {
@@ -4341,13 +4341,6 @@ static void usb_enable_link_state(struct usb_hcd *hcd, struct usb_device *udev,
 		return;
 	}
 
-	/*
-	 * Enable device initiated U1/U2 with a SetFeature(U1/U2_ENABLE) request
-	 * if system exit latency is short enough and device is configured
-	 */
-	if (usb_device_may_initiate_lpm(udev, state))
-		usb_set_device_initiated_lpm(udev, state, true);
-
 	if (state == USB3_LPM_U1)
 		udev->usb3_lpm_u1_enabled = 1;
 	else if (state == USB3_LPM_U2)
@@ -4508,6 +4501,18 @@ void usb_enable_lpm(struct usb_device *udev)
 
 	if (port_dev->usb3_lpm_u2_permit)
 		usb_enable_link_state(hcd, udev, USB3_LPM_U2);
+
+	/*
+	 * Enable device initiated U1/U2 with a SetFeature(U1/U2_ENABLE) request
+	 * if system exit latency is short enough and device is configured
+	 */
+	if (usb_device_may_initiate_lpm(udev, USB3_LPM_U1)) {
+		if (usb_set_device_initiated_lpm(udev, USB3_LPM_U1, true))
+			return;
+
+		if (usb_device_may_initiate_lpm(udev, USB3_LPM_U2))
+			usb_set_device_initiated_lpm(udev, USB3_LPM_U2, true);
+	}
 }
 EXPORT_SYMBOL_GPL(usb_enable_lpm);
 
-- 
2.43.0

[PATCH 5/5] usb: hub: Fail fast in USB3 link power management enable path

[Mathias Nyman]

Enabling LPM is done in hub workqueue, often in paths handling possible
link issues. So fail immediately on USB3 LPM issues and avoid hub wq
from unnecessary blocking, thus allowing it to handle other port events
faster.

Detect errors when enabling U1/U2 link states, and return immediately
if there is an issue.

Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/core/hub.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index d534d7b4606c..fe8271f46c35 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -4292,7 +4292,7 @@ static bool usb_device_may_initiate_lpm(struct usb_device *udev,
  * driver know about it.  If that call fails, it should be harmless, and just
  * take up more slightly more bus bandwidth for unnecessary U1/U2 exit latency.
  */
-static void usb_enable_link_state(struct usb_hcd *hcd, struct usb_device *udev,
+static int usb_enable_link_state(struct usb_hcd *hcd, struct usb_device *udev,
 		enum usb3_link_state state)
 {
 	int timeout;
@@ -4301,7 +4301,7 @@ static void usb_enable_link_state(struct usb_hcd *hcd, struct usb_device *udev,
 
 	/* Skip if the device BOS descriptor couldn't be read */
 	if (!udev->bos)
-		return;
+		return -EINVAL;
 
 	u1_mel = udev->bos->ss_cap->bU1devExitLat;
 	u2_mel = udev->bos->ss_cap->bU2DevExitLat;
@@ -4312,7 +4312,7 @@ static void usb_enable_link_state(struct usb_hcd *hcd, struct usb_device *udev,
 	 */
 	if ((state == USB3_LPM_U1 && u1_mel == 0) ||
 			(state == USB3_LPM_U2 && u2_mel == 0))
-		return;
+		return -EINVAL;
 
 	/* We allow the host controller to set the U1/U2 timeout internally
 	 * first, so that it can change its schedule to account for the
@@ -4323,13 +4323,13 @@ static void usb_enable_link_state(struct usb_hcd *hcd, struct usb_device *udev,
 
 	/* xHCI host controller doesn't want to enable this LPM state. */
 	if (timeout == 0)
-		return;
+		return -EINVAL;
 
 	if (timeout < 0) {
 		dev_warn(&udev->dev, "Could not enable %s link state, "
 				"xHCI error %i.\n", usb3_lpm_names[state],
 				timeout);
-		return;
+		return timeout;
 	}
 
 	if (usb_set_lpm_timeout(udev, state, timeout)) {
@@ -4338,13 +4338,15 @@ static void usb_enable_link_state(struct usb_hcd *hcd, struct usb_device *udev,
 		 * host know that this link state won't be enabled.
 		 */
 		hcd->driver->disable_usb3_lpm_timeout(hcd, udev, state);
-		return;
+		return -EBUSY;
 	}
 
 	if (state == USB3_LPM_U1)
 		udev->usb3_lpm_u1_enabled = 1;
 	else if (state == USB3_LPM_U2)
 		udev->usb3_lpm_u2_enabled = 1;
+
+	return 0;
 }
 /*
  * Disable the hub-initiated U1/U2 idle timeouts, and disable device-initiated
@@ -4497,10 +4499,12 @@ void usb_enable_lpm(struct usb_device *udev)
 	port_dev = hub->ports[udev->portnum - 1];
 
 	if (port_dev->usb3_lpm_u1_permit)
-		usb_enable_link_state(hcd, udev, USB3_LPM_U1);
+		if (usb_enable_link_state(hcd, udev, USB3_LPM_U1))
+			return;
 
 	if (port_dev->usb3_lpm_u2_permit)
-		usb_enable_link_state(hcd, udev, USB3_LPM_U2);
+		if (usb_enable_link_state(hcd, udev, USB3_LPM_U2))
+			return;
 
 	/*
 	 * Enable device initiated U1/U2 with a SetFeature(U1/U2_ENABLE) request
-- 
2.43.0