Re: [RFC 2/2] USB: Enable Latency Tolerance Messaging (LTM).

[Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>]

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