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.
> @@ -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.
> + !usb_device_supports_ltm(udev))
> + return 0;
> +
> + /* Double check whether the device supports LTM. */
This comment doesn't seem to make sense.
> + 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.
Alan Stern
--
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
