| Message ID | 20210715131544.1984726-2-mathias.nyman@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] usb: hub: Fix link power management max exit latency (MEL) calculations | expand |
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
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
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.
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(-)