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
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
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
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/
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
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/
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