[PATCH 1/2] usb: hub: Fix link power management max exit latency (MEL) calculations

[PATCH 1/2] usb: hub: Fix link power management max exit latency (MEL) calculations

[Mathias Nyman]

Maximum Exit Latency (MEL) value is used by host to know how much in
advance it needs to start waking up a U1/U2 suspended link in order to
service a periodic transfer in time.

Current MEL calculation only includes the time to wake up the path from
U1/U2 to U0. This is called tMEL1 in USB 3.1 section C 1.5.2

Total MEL = tMEL1 + tMEL2 +tMEL3 + tMEL4 which should additinally include:
- tMEL2 which is the time it takes for PING message to reach device
- tMEL3 time for device to process the PING and submit a PING_RESPONSE
- tMEL4 time for PING_RESPONSE to traverse back upstream to host.

Add the missing tMEL2, tMEL3 and tMEL4 to MEL calculation.

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

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index d1efc7141333..a35d0bedafa3 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -48,6 +48,7 @@
 
 #define USB_TP_TRANSMISSION_DELAY	40	/* ns */
 #define USB_TP_TRANSMISSION_DELAY_MAX	65535	/* ns */
+#define USB_PING_RESPONSE_TIME		400	/* ns */
 
 /* Protect struct usb_device->state and ->children members
  * Note: Both are also protected by ->dev.sem, except that ->state can
@@ -182,8 +183,9 @@ int usb_device_supports_lpm(struct usb_device *udev)
 }
 
 /*
- * Set the Maximum Exit Latency (MEL) for the host to initiate a transition from
- * either U1 or U2.
+ * Set the Maximum Exit Latency (MEL) for the host to wakup up the path from
+ * U1/U2, send a PING to the device and receive a PING_RESPONSE.
+ * See USB 3.1 section C.1.5.2
  */
 static void usb_set_lpm_mel(struct usb_device *udev,
 		struct usb3_lpm_parameters *udev_lpm_params,
@@ -193,35 +195,37 @@ static void usb_set_lpm_mel(struct usb_device *udev,
 		unsigned int hub_exit_latency)
 {
 	unsigned int total_mel;
-	unsigned int device_mel;
-	unsigned int hub_mel;
 
 	/*
-	 * Calculate the time it takes to transition all links from the roothub
-	 * to the parent hub into U0.  The parent hub must then decode the
-	 * packet (hub header decode latency) to figure out which port it was
-	 * bound for.
-	 *
-	 * The Hub Header decode latency is expressed in 0.1us intervals (0x1
-	 * means 0.1us).  Multiply that by 100 to get nanoseconds.
+	 * tMEL1. time to transition path from host to device into U0.
+	 * MEL for parent already contains the delay up to parent, so only add
+	 * the exit latency for the last link (pick the slower exit latency),
+	 * and the hub header decode latency. See USB 3.1 section C 2.2.1
+	 * Store MEL in nanoseconds
 	 */
 	total_mel = hub_lpm_params->mel +
-		(hub->descriptor->u.ss.bHubHdrDecLat * 100);
+		max(udev_exit_latency, hub_exit_latency) * 1000 +
+		hub->descriptor->u.ss.bHubHdrDecLat * 100;
 
 	/*
-	 * How long will it take to transition the downstream hub's port into
-	 * U0?  The greater of either the hub exit latency or the device exit
-	 * latency.
-	 *
-	 * The BOS U1/U2 exit latencies are expressed in 1us intervals.
-	 * Multiply that by 1000 to get nanoseconds.
+	 * tMEL2. Time to submit PING packet. Sum of tTPTransmissionDelay for
+	 * each link + wHubDelay for each hub. Add only for last link.
+	 * tMEL4, the time for PING_RESPONSE to traverse upstream is similar.
+	 * Multiply by 2 to include it as well.
 	 */
-	device_mel = udev_exit_latency * 1000;
-	hub_mel = hub_exit_latency * 1000;
-	if (device_mel > hub_mel)
-		total_mel += device_mel;
-	else
-		total_mel += hub_mel;
+	total_mel += (__le16_to_cpu(hub->descriptor->u.ss.wHubDelay) +
+		      USB_TP_TRANSMISSION_DELAY) * 2;
+
+	/*
+	 * tMEL3, tPingResponse. Time taken by device to generate PING_RESPONSE
+	 * after receiving PING. Also add 2100ns as stated in USB 3.1 C 1.5.2.4
+	 * to cover the delay if the PING_RESPONSE is queued behind a Max Packet
+	 * Size DP.
+	 * Note these delays should be added only once for the entire path, so
+	 * add them to the MEL of the device connected to the roothub.
+	 */
+	if (!hub->hdev->parent)
+		total_mel += USB_PING_RESPONSE_TIME + 2100;
 
 	udev_lpm_params->mel = total_mel;
 }
-- 
2.25.1

[PATCH 2/2] usb: hub: Disable USB 3 device initiated lpm if exit latency is too high

[Mathias Nyman]

The device initiated link power management U1/U2 states should not be
enabled in case the system exit latency plus one bus interval (125us) is
greater than the shortest service interval of any periodic endpoint.

This is the case for both U1 and U2 sytstem exit latencies and link states.

See USB 3.2 section 9.4.9 "Set Feature" for more details

If host initiated lpm is enabled but device initiated is not due to exit
latency limitations then still set the udev->usb3_lpm_ux_enabled flag so
that sysfs users can see the link may go to U1/U2.

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

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index a35d0bedafa3..63e150982da9 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -4116,6 +4116,47 @@ static int usb_set_lpm_timeout(struct usb_device *udev,
 	return 0;
 }
 
+/*
+ * 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
+ */
+static bool usb_device_may_initiate_lpm(struct usb_device *udev,
+					enum usb3_link_state state)
+{
+	unsigned long long sel;		/* us */
+	int i, j;
+
+	if (state == USB3_LPM_U1)
+		sel = DIV_ROUND_UP(udev->u1_params.sel, 1000);
+	else if (state == USB3_LPM_U2)
+		sel = DIV_ROUND_UP(udev->u2_params.sel, 1000);
+	else
+		return false;
+
+	for (i = 0; i < udev->actconfig->desc.bNumInterfaces; i++) {
+		struct usb_interface *intf;
+		struct usb_endpoint_descriptor *desc;
+		unsigned int interval;
+
+		intf = udev->actconfig->interface[i];
+		if (!intf)
+			continue;
+
+		for (j = 0; j < intf->cur_altsetting->desc.bNumEndpoints; j++) {
+			desc = &intf->cur_altsetting->endpoint[j].desc;
+
+			if (usb_endpoint_xfer_int(desc) ||
+			    usb_endpoint_xfer_isoc(desc)) {
+				interval = (1 << (desc->bInterval - 1)) * 125;
+				if (sel + 125 > interval)
+					return false;
+			}
+		}
+	}
+	return true;
+}
+
 /*
  * Enable the hub-initiated U1/U2 idle timeouts, and enable device-initiated
  * U1/U2 entry.
@@ -4188,20 +4229,23 @@ static void usb_enable_link_state(struct usb_hcd *hcd, struct usb_device *udev,
 	 * U1/U2_ENABLE
 	 */
 	if (udev->actconfig &&
-	    usb_set_device_initiated_lpm(udev, state, true) == 0) {
-		if (state == USB3_LPM_U1)
-			udev->usb3_lpm_u1_enabled = 1;
-		else if (state == USB3_LPM_U2)
-			udev->usb3_lpm_u2_enabled = 1;
-	} else {
-		/* Don't request U1/U2 entry if the device
-		 * cannot transition to U1/U2.
-		 */
-		usb_set_lpm_timeout(udev, state, 0);
-		hcd->driver->disable_usb3_lpm_timeout(hcd, udev, state);
+	    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 (state == USB3_LPM_U1)
+		udev->usb3_lpm_u1_enabled = 1;
+	else if (state == USB3_LPM_U2)
+		udev->usb3_lpm_u2_enabled = 1;
+}
 /*
  * Disable the hub-initiated U1/U2 idle timeouts, and disable device-initiated
  * U1/U2 entry.
-- 
2.25.1

Re: [PATCH 2/2] usb: hub: Disable USB 3 device initiated lpm if exit latency is too high

[Greg KH]

On Thu, Jul 15, 2021 at 04:15:44PM +0300, Mathias Nyman wrote:
> The device initiated link power management U1/U2 states should not be
> enabled in case the system exit latency plus one bus interval (125us) is
> greater than the shortest service interval of any periodic endpoint.
> 
> This is the case for both U1 and U2 sytstem exit latencies and link states.
> 
> See USB 3.2 section 9.4.9 "Set Feature" for more details
> 
> If host initiated lpm is enabled but device initiated is not due to exit
> latency limitations then still set the udev->usb3_lpm_ux_enabled flag so
> that sysfs users can see the link may go to U1/U2.
> 
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---
>  drivers/usb/core/hub.c | 68 ++++++++++++++++++++++++++++++++++--------
>  1 file changed, 56 insertions(+), 12 deletions(-)

Do either of these need to go to older kernels?

> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index a35d0bedafa3..63e150982da9 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -4116,6 +4116,47 @@ static int usb_set_lpm_timeout(struct usb_device *udev,
>  	return 0;
>  }
>  
> +/*
> + * 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
> + */
> +static bool usb_device_may_initiate_lpm(struct usb_device *udev,
> +					enum usb3_link_state state)
> +{
> +	unsigned long long sel;		/* us */

Do you mean u64 here?  If so, you might want to use that :)

thanks,

greg k-h

Re: [PATCH 2/2] usb: hub: Disable USB 3 device initiated lpm if exit latency is too high

[Mathias Nyman]

On 15.7.2021 16.18, Greg KH wrote:
> On Thu, Jul 15, 2021 at 04:15:44PM +0300, Mathias Nyman wrote:
>> The device initiated link power management U1/U2 states should not be
>> enabled in case the system exit latency plus one bus interval (125us) is
>> greater than the shortest service interval of any periodic endpoint.
>>
>> This is the case for both U1 and U2 sytstem exit latencies and link states.
>>
>> See USB 3.2 section 9.4.9 "Set Feature" for more details
>>
>> If host initiated lpm is enabled but device initiated is not due to exit
>> latency limitations then still set the udev->usb3_lpm_ux_enabled flag so
>> that sysfs users can see the link may go to U1/U2.
>>
>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>> ---
>>  drivers/usb/core/hub.c | 68 ++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 56 insertions(+), 12 deletions(-)
> 
> Do either of these need to go to older kernels?

Right, first patch should go, I'll figure out to what version.

Second one not so sure. It does a minor change in lpm logic.

Before second patch enabling lpm meant enabling both host and device initiated
lpm transitions. After this patch we might enable host initated lpm without device
lpm.

That could be stated clearer in the description as well..  

> 
>>
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index a35d0bedafa3..63e150982da9 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -4116,6 +4116,47 @@ static int usb_set_lpm_timeout(struct usb_device *udev,
>>  	return 0;
>>  }
>>  
>> +/*
>> + * 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
>> + */
>> +static bool usb_device_may_initiate_lpm(struct usb_device *udev,
>> +					enum usb3_link_state state)
>> +{
>> +	unsigned long long sel;		/* us */
> 
> Do you mean u64 here?  If so, you might want to use that :)

Ah, my mistake, no need for 64 bits.
Original sel value we store here is an unsigned int.
sel = DIV_ROUND_UP(udev->u1_params.sel, 1000);

Must have copied that from usb_req_set_sel() that uses unsigned long long for sel and pel
values for some odd reason.
I'll change it.

-Mathias