[PATCH] xhci: Check all endpoints for LPM timeout

[PATCH] xhci: Check all endpoints for LPM timeout

[Jan Schmidt]

If an endpoint is encountered that returns USB3_LPM_DEVICE_INITIATED, keep
checking further endpoints, as there might be periodic endpoints later
that return USB3_LPM_DISABLED due to shorter service intervals.

Without this, the code can set too high a maximum-exit-latency and
prevent the use of multiple USB3 cameras that should be able to work.

Signed-off-by: Jan Schmidt <jan@centricular.com>
Tested-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/usb/host/xhci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 03d1e552769b..1986b88661fc 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4673,12 +4673,12 @@ static int xhci_update_timeout_for_endpoint(struct xhci_hcd *xhci,
 	alt_timeout = xhci_call_host_update_timeout_for_endpoint(xhci, udev,
 		desc, state, timeout);
 
-	/* If we found we can't enable hub-initiated LPM, or
+	/* If we found we can't enable hub-initiated LPM, and
 	 * the U1 or U2 exit latency was too high to allow
-	 * device-initiated LPM as well, just stop searching.
+	 * device-initiated LPM as well, then we will disable LPM
+	 * for this device, so stop searching any further.
 	 */
-	if (alt_timeout == USB3_LPM_DISABLED ||
-			alt_timeout == USB3_LPM_DEVICE_INITIATED) {
+	if (alt_timeout == USB3_LPM_DISABLED) {
 		*timeout = alt_timeout;
 		return -E2BIG;
 	}
-- 
2.21.0

Re: [PATCH] xhci: Check all endpoints for LPM timeout

[Philipp Zabel]

On Fri, 2019-09-13 at 00:49 +1000, Jan Schmidt wrote:
> If an endpoint is encountered that returns USB3_LPM_DEVICE_INITIATED, keep
> checking further endpoints, as there might be periodic endpoints later
> that return USB3_LPM_DISABLED due to shorter service intervals.
> 
> Without this, the code can set too high a maximum-exit-latency and
> prevent the use of multiple USB3 cameras that should be able to work.
> 
> Signed-off-by: Jan Schmidt <jan@centricular.com>
> Tested-by: Philipp Zabel <p.zabel@pengutronix.de>

This fixes the capture from multiple Oculus Sensors, as described in the
"Not enough bandwidth with Magewell XI100DUSB-HDMI" thread:

https://lore.kernel.org/linux-usb/CA+gwMce-h9LPCv1hhfEcRz_2w9mEQLOjy42dcjvPxTeawSU5kQ@mail.gmail.com/

regards
Philipp

Re: [PATCH] xhci: Check all endpoints for LPM timeout

[Mathias Nyman]

On 12.9.2019 17.49, Jan Schmidt wrote:
> If an endpoint is encountered that returns USB3_LPM_DEVICE_INITIATED, keep
> checking further endpoints, as there might be periodic endpoints later
> that return USB3_LPM_DISABLED due to shorter service intervals.
> 
> Without this, the code can set too high a maximum-exit-latency and
> prevent the use of multiple USB3 cameras that should be able to work.
> 
> Signed-off-by: Jan Schmidt <jan@centricular.com>
> Tested-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>   drivers/usb/host/xhci.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 03d1e552769b..1986b88661fc 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -4673,12 +4673,12 @@ static int xhci_update_timeout_for_endpoint(struct xhci_hcd *xhci,
>   	alt_timeout = xhci_call_host_update_timeout_for_endpoint(xhci, udev,
>   		desc, state, timeout);
>   
> -	/* If we found we can't enable hub-initiated LPM, or
> +	/* If we found we can't enable hub-initiated LPM, and
>   	 * the U1 or U2 exit latency was too high to allow
> -	 * device-initiated LPM as well, just stop searching.
> +	 * device-initiated LPM as well, then we will disable LPM
> +	 * for this device, so stop searching any further.
>   	 */
> -	if (alt_timeout == USB3_LPM_DISABLED ||
> -			alt_timeout == USB3_LPM_DEVICE_INITIATED) {
> +	if (alt_timeout == USB3_LPM_DISABLED) {
>   		*timeout = alt_timeout;
>   		return -E2BIG;
>   	}
> 

Thanks, nice catch. Adding to queue.

While looking at this I see we have a similar issue if driver has
"disable_hub_initiated_lpm" flag set.

xhci_get_timeout_no_hub_lpm() might return USB3_LPM_DEVICE_INITIATED
before we check if periodic endpoints would require disabling LPM
completely.

xhci_calculate_lpm_timeout()
...
     for (i = 0; i < config->desc.bNumInterfaces; i++) {
         ...
         if (intf->dev.driver) {
             ...
             if (driver && driver->disable_hub_initiated_lpm) {
                 return xhci_get_timeout_no_hub_lpm(udev, state);

I'll write a patch for that

-Mathias

Re: [PATCH] xhci: Check all endpoints for LPM timeout

[Jan Schmidt]

On 13/9/19 10:58 pm, Mathias Nyman wrote:
> On 12.9.2019 17.49, Jan Schmidt wrote:
>> If an endpoint is encountered that returns USB3_LPM_DEVICE_INITIATED,
>> keep
>> checking further endpoints, as there might be periodic endpoints later
>> that return USB3_LPM_DISABLED due to shorter service intervals.
>>
>> Without this, the code can set too high a maximum-exit-latency and
>> prevent the use of multiple USB3 cameras that should be able to work.
>>
>> Signed-off-by: Jan Schmidt <jan@centricular.com>
>> Tested-by: Philipp Zabel <p.zabel@pengutronix.de>
>> ---
>>   drivers/usb/host/xhci.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> index 03d1e552769b..1986b88661fc 100644
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c
>> @@ -4673,12 +4673,12 @@ static int
>> xhci_update_timeout_for_endpoint(struct xhci_hcd *xhci,
>>       alt_timeout = xhci_call_host_update_timeout_for_endpoint(xhci,
>> udev,
>>           desc, state, timeout);
>>   -    /* If we found we can't enable hub-initiated LPM, or
>> +    /* If we found we can't enable hub-initiated LPM, and
>>        * the U1 or U2 exit latency was too high to allow
>> -     * device-initiated LPM as well, just stop searching.
>> +     * device-initiated LPM as well, then we will disable LPM
>> +     * for this device, so stop searching any further.
>>        */
>> -    if (alt_timeout == USB3_LPM_DISABLED ||
>> -            alt_timeout == USB3_LPM_DEVICE_INITIATED) {
>> +    if (alt_timeout == USB3_LPM_DISABLED) {
>>           *timeout = alt_timeout;
>>           return -E2BIG;
>>       }
>>
> 
> Thanks, nice catch. Adding to queue.

Great news for the Oculus Rift support we're working on :)

> While looking at this I see we have a similar issue if driver has
> "disable_hub_initiated_lpm" flag set.
> 
> xhci_get_timeout_no_hub_lpm() might return USB3_LPM_DEVICE_INITIATED
> before we check if periodic endpoints would require disabling LPM
> completely.

Indeed - sorry, I didn't even think to look past the immediate issue.

> xhci_calculate_lpm_timeout()
> ...
>     for (i = 0; i < config->desc.bNumInterfaces; i++) {
>         ...
>         if (intf->dev.driver) {
>             ...
>             if (driver && driver->disable_hub_initiated_lpm) {
>                 return xhci_get_timeout_no_hub_lpm(udev, state);
> 
> I'll write a patch for that

I'll be happy to test by hard-coding the flag.

- Jan.

> 
> -Mathias

-- 
Jan Schmidt, Centricular Ltd - https://centricular.com/

[RFT PATCH] xhci: Prevent device initiated U1/U2 link pm if exit latency is too long

[Mathias Nyman]

If host/hub initiated link pm is prevented by a driver flag we still must
ensure that periodic endpoints have longer service intervals than link pm
exit latency before allowing device initiated link pm.

Fix this by continue walking and checking endpoint service interval if
xhci_get_timeout_no_hub_lpm() returns anything else than USB3_LPM_DISABLED

While at it fix the split line error message

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

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 447c29bbad48..8892dfbb2af7 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4809,10 +4809,12 @@ static u16 xhci_calculate_lpm_timeout(struct usb_hcd *hcd,
 		if (intf->dev.driver) {
 			driver = to_usb_driver(intf->dev.driver);
 			if (driver && driver->disable_hub_initiated_lpm) {
-				dev_dbg(&udev->dev, "Hub-initiated %s disabled "
-						"at request of driver %s\n",
-						state_name, driver->name);
-				return xhci_get_timeout_no_hub_lpm(udev, state);
+				dev_dbg(&udev->dev, "Hub-initiated %s disabled at request of driver %s\n",
+					state_name, driver->name);
+				timeout = xhci_get_timeout_no_hub_lpm(udev,
+								      state);
+				if (timeout == USB3_LPM_DISABLED)
+					return timeout;
 			}
 		}
 
-- 
2.7.4

Re: [RFT PATCH] xhci: Prevent device initiated U1/U2 link pm if exit latency is too long

[Jan Schmidt]

On 18/9/19 12:53 am, Mathias Nyman wrote:
> If host/hub initiated link pm is prevented by a driver flag we still must
> ensure that periodic endpoints have longer service intervals than link pm
> exit latency before allowing device initiated link pm.
> 
> Fix this by continue walking and checking endpoint service interval if
> xhci_get_timeout_no_hub_lpm() returns anything else than USB3_LPM_DISABLED
> 
> While at it fix the split line error message
> 
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>

I tested by forcing the driver->disable_hub_initiated_lpm check and
confirm a) Other USB devices still work as I expect them to b) without
this patch, I'm back to only 1 working Oculus Rift Sensor. With it, I
can capture 3 simultaneously.

Tested-by: Jan Schmidt <jan@centricular.com>

- Jan

> ---
>  drivers/usb/host/xhci.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 447c29bbad48..8892dfbb2af7 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -4809,10 +4809,12 @@ static u16 xhci_calculate_lpm_timeout(struct usb_hcd *hcd,
>  		if (intf->dev.driver) {
>  			driver = to_usb_driver(intf->dev.driver);
>  			if (driver && driver->disable_hub_initiated_lpm) {
> -				dev_dbg(&udev->dev, "Hub-initiated %s disabled "
> -						"at request of driver %s\n",
> -						state_name, driver->name);
> -				return xhci_get_timeout_no_hub_lpm(udev, state);
> +				dev_dbg(&udev->dev, "Hub-initiated %s disabled at request of driver %s\n",
> +					state_name, driver->name);
> +				timeout = xhci_get_timeout_no_hub_lpm(udev,
> +								      state);
> +				if (timeout == USB3_LPM_DISABLED)
> +					return timeout;
>  			}
>  		}
>  
> 

-- 
Jan Schmidt, Centricular Ltd - https://centricular.com/

Re: [RFT PATCH] xhci: Prevent device initiated U1/U2 link pm if exit latency is too long

[Mathias Nyman]

On 17.9.2019 18.56, Jan Schmidt wrote:
> 
> 
> On 18/9/19 12:53 am, Mathias Nyman wrote:
>> If host/hub initiated link pm is prevented by a driver flag we still must
>> ensure that periodic endpoints have longer service intervals than link pm
>> exit latency before allowing device initiated link pm.
>>
>> Fix this by continue walking and checking endpoint service interval if
>> xhci_get_timeout_no_hub_lpm() returns anything else than USB3_LPM_DISABLED
>>
>> While at it fix the split line error message
>>
>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> 
> I tested by forcing the driver->disable_hub_initiated_lpm check and
> confirm a) Other USB devices still work as I expect them to b) without
> this patch, I'm back to only 1 working Oculus Rift Sensor. With it, I
> can capture 3 simultaneously.
> 
> Tested-by: Jan Schmidt <jan@centricular.com>
> 

Great, thanks, I'll queue up that patch as well with your Tested-by tag

-Mathias