[SRU,Bionic/OEM-B,1/1] usb: Don't disable Latency tolerance Messaging (LTM) before port reset
diff mbox series

Message ID 20181017114508.4483-2-hui.wang@canonical.com
State Accepted
Headers show
Series
  • USB cardreader (0bda:0328) make the system can't enter s3 or hang
Related show

Commit Message

Hui Wang Oct. 17, 2018, 11:45 a.m. UTC
From: Mathias Nyman <mathias.nyman@linux.intel.com>

BugLink: https://bugs.launchpad.net/bugs/1798328

Disabing Latency Tolerance Messaging before port reset is unnecessary.
LTM is automatically disabled at port reset.

If host can't communicate with the device the LTM message will fail, and
the hub driver will unnecessarily do a logical disconnect.
Broken communication is ofter the reason for a reset in the first place.

Additionally we can't guarantee device is in a configured state,
epecially in reset-resume case when root hub lost power.
LTM can't be modified unless device is in a configured state.

Just remove LTM disabling before port reset.

Details about LTM and port reset in USB 3 specification:

USB 3 spec section 9.4.5
"The LTM Enable field can be modified by the SetFeature() and
ClearFeature() requests using the LTM_ENABLE feature selector.
This field is reset to zero when the device is reset."

USB 3 spec section 9.4.1
"The device shall process a Clear Feature (U1_Enable or U2_Enable or
LTM_Enable) only if the device is in the configured state."

Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
Acked-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
(backported from commit 57edd462270bf2c50049b73774cb5915e2f12aa8)
Signed-off-by: Hui Wang <hui.wang@canonical.com>
---
 drivers/usb/core/hub.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

Comments

Kleber Souza Oct. 17, 2018, 11:49 a.m. UTC | #1
On 10/17/18 13:45, Hui Wang wrote:
> From: Mathias Nyman <mathias.nyman@linux.intel.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1798328
> 
> Disabing Latency Tolerance Messaging before port reset is unnecessary.
> LTM is automatically disabled at port reset.
> 
> If host can't communicate with the device the LTM message will fail, and
> the hub driver will unnecessarily do a logical disconnect.
> Broken communication is ofter the reason for a reset in the first place.
> 
> Additionally we can't guarantee device is in a configured state,
> epecially in reset-resume case when root hub lost power.
> LTM can't be modified unless device is in a configured state.
> 
> Just remove LTM disabling before port reset.
> 
> Details about LTM and port reset in USB 3 specification:
> 
> USB 3 spec section 9.4.5
> "The LTM Enable field can be modified by the SetFeature() and
> ClearFeature() requests using the LTM_ENABLE feature selector.
> This field is reset to zero when the device is reset."
> 
> USB 3 spec section 9.4.1
> "The device shall process a Clear Feature (U1_Enable or U2_Enable or
> LTM_Enable) only if the device is in the configured state."
> 
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> Acked-by: Alan Stern <stern@rowland.harvard.edu>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> (backported from commit 57edd462270bf2c50049b73774cb5915e2f12aa8)
> Signed-off-by: Hui Wang <hui.wang@canonical.com>

Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>

> ---
>  drivers/usb/core/hub.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 2e7ba720d465..1bcd1755f97d 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -5485,22 +5485,15 @@ static int usb_reset_and_verify_device(struct usb_device *udev)
>  	if (udev->usb2_hw_lpm_enabled == 1)
>  		usb_set_usb2_hardware_lpm(udev, 0);
>  
> -	/* Disable LPM and LTM while we reset the device and reinstall the alt
> -	 * settings.  Device-initiated LPM settings, and system exit latency
> -	 * settings are cleared when the device is reset, so we have to set
> -	 * them up again.
> +	/* Disable LPM while we reset the device and reinstall the alt settings.
> +	 * Device-initiated LPM, and system exit latency settings are cleared
> +	 * when the device is reset, so we have to set them up again.
>  	 */
>  	ret = usb_unlocked_disable_lpm(udev);
>  	if (ret) {
>  		dev_err(&udev->dev, "%s Failed to disable LPM\n.", __func__);
>  		goto re_enumerate_no_bos;
>  	}
> -	ret = usb_disable_ltm(udev);
> -	if (ret) {
> -		dev_err(&udev->dev, "%s Failed to disable LTM\n.",
> -				__func__);
> -		goto re_enumerate_no_bos;
> -	}
>  
>  	bos = udev->bos;
>  	udev->bos = NULL;
>
Aaron Ma Oct. 17, 2018, 1:20 p.m. UTC | #2
Good backporting with positive test results.

Acked-by: Aaron Ma <aaron.ma@canonical.com>

Patch
diff mbox series

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 2e7ba720d465..1bcd1755f97d 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -5485,22 +5485,15 @@  static int usb_reset_and_verify_device(struct usb_device *udev)
 	if (udev->usb2_hw_lpm_enabled == 1)
 		usb_set_usb2_hardware_lpm(udev, 0);
 
-	/* Disable LPM and LTM while we reset the device and reinstall the alt
-	 * settings.  Device-initiated LPM settings, and system exit latency
-	 * settings are cleared when the device is reset, so we have to set
-	 * them up again.
+	/* Disable LPM while we reset the device and reinstall the alt settings.
+	 * Device-initiated LPM, and system exit latency settings are cleared
+	 * when the device is reset, so we have to set them up again.
 	 */
 	ret = usb_unlocked_disable_lpm(udev);
 	if (ret) {
 		dev_err(&udev->dev, "%s Failed to disable LPM\n.", __func__);
 		goto re_enumerate_no_bos;
 	}
-	ret = usb_disable_ltm(udev);
-	if (ret) {
-		dev_err(&udev->dev, "%s Failed to disable LTM\n.",
-				__func__);
-		goto re_enumerate_no_bos;
-	}
 
 	bos = udev->bos;
 	udev->bos = NULL;