On Tue, Jun 26, 2012 at 10:17:31AM -0400, Alan Stern wrote:
> On Mon, 25 Jun 2012, Sarah Sharp wrote:
>
> > USB 3.0 devices may optionally support a new feature called Latency
> > Tolerance Messaging. If both the xHCI host controller and the device
> > support LTM, it should be turned on in order to give the system hardware
> > a better clue about the latency tolerance values of its PCI devices.
> >
> > LTM is disabled by default on enumeration or after device reset. Once a
> > Set Feature request to enable LTM is received, the USB 3.0 device will
> > begin to send LTM updates as its buffers fill or empty, and it can
> > tolerate more or less latency.
> >
> > Make the USB core enable LTM after the device is reset. The USB 3.0
> > spec, section C.4.2 says that LTM should be disabled just before the
> > device is placed into suspend. Then the device will send an updated LTM
> > notification, so that the system doesn't think it should remain in an
> > active state in order to satisfy the latency requirements of the
> > suspended device.
>
> This patch could be improved a bit...
>
> > index 25a7422..b64d11a 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -2607,6 +2607,9 @@ static int check_port_resume_type(struct usb_device *udev,
> > return status;
> > }
> >
> > +static int usb_disable_ltm(struct usb_device *udev);
> > +static void usb_enable_ltm(struct usb_device *udev);
>
> If the function definitions were brought forward, these declarations
> wouldn't be needed.
Ok. I'll have to add a couple of #ifdef CONFIG_PM, since the USB core
suspend, LPM, and LTM functionality is all turned off if CONFIG_PM is
off. It's the same amount of lines either way, but if you just dislike
forward declarations, I can change it.
> > @@ -3461,6 +3471,43 @@ void usb_unlocked_enable_lpm(struct usb_device *udev)
> > }
> > EXPORT_SYMBOL_GPL(usb_unlocked_enable_lpm);
> >
> > +static bool usb_device_supports_ltm(struct usb_device *udev)
> > +{
> > + if (udev->speed != USB_SPEED_SUPER || !udev->bos || !udev->bos->ss_cap)
> > + return false;
> > + return udev->bos->ss_cap->bmAttributes & USB_LTM_SUPPORT;
> > +}
> > +
> > +static int usb_disable_ltm(struct usb_device *udev)
> > +{
> > + struct usb_hcd *hcd = bus_to_hcd(udev->bus);
> > +
> > + /* Check if the roothub and device supports LTM. */
> > + if (!hcd || !usb_device_supports_ltm(hcd->self.root_hub) ||
>
> I'm pretty sure that hcd can never be NULL. If it is, it's a BUG so
> you may as well not bother to test it.
Meh, ok. I figure it's always better to check arguments and return an
error than cause a kernel oops, but I'm cool either way.
> > + !usb_device_supports_ltm(udev))
> > + return 0;
> > +
> > + /* Double check whether the device supports LTM. */
>
> This comment doesn't seem to make sense.
Ah, right, that was from a previous revision. I'll remove it.
> > + return usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
> > + USB_REQ_CLEAR_FEATURE, USB_RECIP_DEVICE,
> > + USB_DEVICE_LTM_ENABLE, 0, NULL, 0,
> > + USB_CTRL_SET_TIMEOUT);
> > +}
> > +
> > +static void usb_enable_ltm(struct usb_device *udev)
> > +{
> > + struct usb_hcd *hcd = bus_to_hcd(udev->bus);
> > +
> > + /* Check if the roothub and device supports LTM. */
> > + if (!hcd || !usb_device_supports_ltm(hcd->self.root_hub) ||
>
> Don't need to check !hcd.
>
> > + !usb_device_supports_ltm(udev))
> > + return;
> > +
> > + usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
> > + USB_REQ_SET_FEATURE, USB_RECIP_DEVICE,
> > + USB_DEVICE_LTM_ENABLE, 0, NULL, 0,
> > + USB_CTRL_SET_TIMEOUT);
> > +}
> >
> > #else /* CONFIG_PM */
> >
> > @@ -3485,6 +3532,13 @@ EXPORT_SYMBOL_GPL(usb_unlocked_disable_lpm);
> >
> > void usb_unlocked_enable_lpm(struct usb_device *udev) { }
> > EXPORT_SYMBOL_GPL(usb_unlocked_enable_lpm);
> > +
> > +static int usb_disable_ltm(struct usb_device *udev)
> > +{
> > + return 0;
> > +}
> > +
> > +static void usb_enable_ltm(struct usb_device *udev) { }
> > #endif
> >
> >
> > @@ -4705,6 +4759,8 @@ static int usb_reset_and_verify_device(struct usb_device *udev)
> > if (ret) {
> > dev_err(&udev->dev, "%s Failed to disable LPM\n.", __func__);
> > mutex_unlock(hcd->bandwidth_mutex);
> > + /* Re-enable LTM, since the default is disabled after reset. */
> > + usb_enable_ltm(udev);
>
> These enable calls don't belong all over the place in this function.
> They should occur in exactly one place, in hub_port_reset(), since they
> are needed after each reset.
Ok, I'll move it.
Sarah Sharp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
