Re: [PATCH] usb: Don't disable Latency tolerance Messaging (LTM) before port reset

[Mathias Nyman]

On 01.03.2018 20:07, Alan Stern wrote:

On Thu, 1 Mar 2018, Mathias Nyman wrote:


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.


That definitely sounds like the right approach.


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 


Acked-by: Alan Stern 


---
  drivers/usb/core/hub.c | 12 +++-
  1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index c5c1f6c..ac7bab7 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -5505,21 +5505,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.
 */


In theory, since you're changing this comment anyway, it could be
reformatted into the more commonly accepted form:

/*
 * blah, blah, blah
 * blah, blah, blah
 */

But it's not a big deal.


Good point.
I'm trying to get rid of LPM disable before reset as well, then we can get of 
the whole comment.
just need to do some LPM testing first.
 
-Mathias

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: Don't disable Latency tolerance Messaging (LTM) before port reset

[Alan Stern]

On Thu, 1 Mar 2018, Mathias Nyman wrote:

> 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.

That definitely sounds like the right approach.

> 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 

Acked-by: Alan Stern 

> ---
>  drivers/usb/core/hub.c | 12 +++-
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index c5c1f6c..ac7bab7 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -5505,21 +5505,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.
>*/

In theory, since you're changing this comment anyway, it could be 
reformatted into the more commonly accepted form:

/*
 * blah, blah, blah
 * blah, blah, blah
 */

But it's not a big deal.

>   ret = usb_unlocked_disable_lpm(udev);
>   if (ret) {
>   dev_err(>dev, "%s Failed to disable LPM\n", __func__);
>   goto re_enumerate_no_bos;
>   }
> - ret = usb_disable_ltm(udev);
> - if (ret) {
> - dev_err(>dev, "%s Failed to disable LTM\n", __func__);
> - goto re_enumerate_no_bos;
> - }
>  
>   bos = udev->bos;
>   udev->bos = NULL;

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: Don't disable Latency tolerance Messaging (LTM) before port reset

[Mathias Nyman]

On 01.03.2018 13:56, Greg KH wrote:

On Thu, Mar 01, 2018 at 01:15:28PM +0200, Mathias Nyman wrote:

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 
---
  drivers/usb/core/hub.c | 12 +++-
  1 file changed, 3 insertions(+), 9 deletions(-)


Is this causing problems now such that we should get this to older
kernels as well?



Could be, it can reduce extra logical disconnects on reset on resume.
But this didn't resolve some some real life bug, it was one thing Alan 
commented on
while debugging a failing USB3 device reset.

-Mathias
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: Don't disable Latency tolerance Messaging (LTM) before port reset

[Greg KH]

On Thu, Mar 01, 2018 at 01:15:28PM +0200, Mathias Nyman wrote:
> 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 
> ---
>  drivers/usb/core/hub.c | 12 +++-
>  1 file changed, 3 insertions(+), 9 deletions(-)

Is this causing problems now such that we should get this to older
kernels as well?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: Don't disable Latency tolerance Messaging (LTM) before port reset

[Sergei Shtylyov]

Hello!

On 03/01/2018 02:15 PM, Mathias Nyman wrote:

> 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.

   Often.

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

   Especially.

> 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 
[...]

MBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html