[PATCH] usb: hub: Clear Port Reset Change during init/resume

[PATCH] usb: hub: Clear Port Reset Change during init/resume

[Julius Werner]

This patch adds the Port Reset Change flag to the set of bits that are
preemptively cleared on init/resume of a hub. In theory this bit should
never be set unexpectedly... in practice it can still happen if BIOS,
SMM or ACPI code plays around with USB devices without cleaning up
correctly. This is especially dangerous for XHCI root hubs, which don't
generate any more Port Status Change Events until all change bits are
cleared, so this is a good precaution to have (similar to how it's
already done for the Warm Port Reset Change flag).

Signed-off-by: Julius Werner <jwerner@chromium.org>
---
 drivers/usb/core/hub.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index e6b682c..c9ef5b8 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -1130,6 +1130,11 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
 			usb_clear_port_feature(hub->hdev, port1,
 					USB_PORT_FEAT_C_ENABLE);
 		}
+		if ((portchange & USB_PORT_STAT_C_RESET)) {
+			need_debounce_delay = true;
+			usb_clear_port_feature(hub->hdev, port1,
+					USB_PORT_FEAT_C_RESET);
+		}
 		if ((portchange & USB_PORT_STAT_C_BH_RESET) &&
 				hub_is_superspeed(hub->hdev)) {
 			need_debounce_delay = true;
-- 
1.7.12.4

Re: [PATCH] usb: hub: Clear Port Reset Change during init/resume

[Sergei Shtylyov]

Hello.

On 10/16/2013 01:23 AM, Julius Werner wrote:

> This patch adds the Port Reset Change flag to the set of bits that are
> preemptively cleared on init/resume of a hub. In theory this bit should
> never be set unexpectedly... in practice it can still happen if BIOS,
> SMM or ACPI code plays around with USB devices without cleaning up
> correctly. This is especially dangerous for XHCI root hubs, which don't
> generate any more Port Status Change Events until all change bits are
> cleared, so this is a good precaution to have (similar to how it's
> already done for the Warm Port Reset Change flag).

> Signed-off-by: Julius Werner <jwerner@chromium.org>
> ---
>   drivers/usb/core/hub.c | 5 +++++
>   1 file changed, 5 insertions(+)

> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index e6b682c..c9ef5b8 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -1130,6 +1130,11 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
>   			usb_clear_port_feature(hub->hdev, port1,
>   					USB_PORT_FEAT_C_ENABLE);
>   		}
> +		if ((portchange & USB_PORT_STAT_C_RESET)) {

    Hm, why these double parens?

WBR, Sergei

Re: [PATCH] usb: hub: Clear Port Reset Change during init/resume

[Julius Werner]

>> +               if ((portchange & USB_PORT_STAT_C_RESET)) {
>
>
>    Hm, why these double parens?

Oh... good question. I copied the entry below it, remove the && and
must have overlooked those. Sorry, v2 incoming...

[PATCH v2] usb: hub: Clear Port Reset Change during init/resume

[Julius Werner]

This patch adds the Port Reset Change flag to the set of bits that are
preemptively cleared on init/resume of a hub. In theory this bit should
never be set unexpectedly... in practice it can still happen if BIOS,
SMM or ACPI code plays around with USB devices without cleaning up
correctly. This is especially dangerous for XHCI root hubs, which don't
generate any more Port Status Change Events until all change bits are
cleared, so this is a good precaution to have (similar to how it's
already done for the Warm Port Reset Change flag).

Signed-off-by: Julius Werner <jwerner@chromium.org>
---
 drivers/usb/core/hub.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index e6b682c..c3dd64c 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -1130,6 +1130,11 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
 			usb_clear_port_feature(hub->hdev, port1,
 					USB_PORT_FEAT_C_ENABLE);
 		}
+		if (portchange & USB_PORT_STAT_C_RESET) {
+			need_debounce_delay = true;
+			usb_clear_port_feature(hub->hdev, port1,
+					USB_PORT_FEAT_C_RESET);
+		}
 		if ((portchange & USB_PORT_STAT_C_BH_RESET) &&
 				hub_is_superspeed(hub->hdev)) {
 			need_debounce_delay = true;
-- 
1.7.12.4

Re: [PATCH v2] usb: hub: Clear Port Reset Change during init/resume

[Alan Stern]

On Tue, 15 Oct 2013, Julius Werner wrote:

> This patch adds the Port Reset Change flag to the set of bits that are
> preemptively cleared on init/resume of a hub. In theory this bit should
> never be set unexpectedly... in practice it can still happen if BIOS,
> SMM or ACPI code plays around with USB devices without cleaning up
> correctly. This is especially dangerous for XHCI root hubs, which don't
> generate any more Port Status Change Events until all change bits are
> cleared, so this is a good precaution to have (similar to how it's
> already done for the Warm Port Reset Change flag).
> 
> Signed-off-by: Julius Werner <jwerner@chromium.org>
> ---
>  drivers/usb/core/hub.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index e6b682c..c3dd64c 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -1130,6 +1130,11 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
>  			usb_clear_port_feature(hub->hdev, port1,
>  					USB_PORT_FEAT_C_ENABLE);
>  		}
> +		if (portchange & USB_PORT_STAT_C_RESET) {
> +			need_debounce_delay = true;
> +			usb_clear_port_feature(hub->hdev, port1,
> +					USB_PORT_FEAT_C_RESET);
> +		}
>  		if ((portchange & USB_PORT_STAT_C_BH_RESET) &&
>  				hub_is_superspeed(hub->hdev)) {
>  			need_debounce_delay = true;

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Re: [PATCH v2] usb: hub: Clear Port Reset Change during init/resume

[Sarah Sharp]

On Tue, Oct 15, 2013 at 05:45:00PM -0700, Julius Werner wrote:
> This patch adds the Port Reset Change flag to the set of bits that are
> preemptively cleared on init/resume of a hub. In theory this bit should
> never be set unexpectedly... in practice it can still happen if BIOS,
> SMM or ACPI code plays around with USB devices without cleaning up
> correctly. This is especially dangerous for XHCI root hubs, which don't
> generate any more Port Status Change Events until all change bits are
> cleared, so this is a good precaution to have (similar to how it's
> already done for the Warm Port Reset Change flag).

Did you run into an issue where port status change events weren't being
generated because the Port Reset flag was set?  I'm trying to figure out
if this addresses a real issue you hit (and thus should be queued for
stable), or if this is just a precaution.

I do agree this is a good fix.  ISTR that with some xHCI vendors, when
the host is reset, the hardware will drive a port reset down all ports.
That may cause the port reset and even warm port reset bits to be set.
I have a vague recollection that some implementations might not even
wait for the port reset to complete before saying the reset is done.  We
can't tell which hosts will and won't drive a reset, so we rely on the
USB core to clear those bits if they are set.

So perhaps instead of the BIOS/SMM/ACPI code leaving the ports in an
unclean state, the root cause of your issue is actually the call to
xhci_reset?  You can check the port status before that call to find out.

> Signed-off-by: Julius Werner <jwerner@chromium.org>
> ---
>  drivers/usb/core/hub.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index e6b682c..c3dd64c 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -1130,6 +1130,11 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
>  			usb_clear_port_feature(hub->hdev, port1,
>  					USB_PORT_FEAT_C_ENABLE);
>  		}
> +		if (portchange & USB_PORT_STAT_C_RESET) {
> +			need_debounce_delay = true;
> +			usb_clear_port_feature(hub->hdev, port1,
> +					USB_PORT_FEAT_C_RESET);
> +		}
>  		if ((portchange & USB_PORT_STAT_C_BH_RESET) &&
>  				hub_is_superspeed(hub->hdev)) {
>  			need_debounce_delay = true;
> -- 
> 1.7.12.4
> 

Re: [PATCH v2] usb: hub: Clear Port Reset Change during init/resume

[Benson Leung]

On Wed, Oct 16, 2013 at 4:57 PM, Sarah Sharp
<sarah.a.sharp@linux.intel.com> wrote:
>
> Did you run into an issue where port status change events weren't being
> generated because the Port Reset flag was set?  I'm trying to figure out
> if this addresses a real issue you hit (and thus should be queued for
> stable), or if this is just a precaution.

We ran into this on HP Chromebook 14 (Falco). The port reset flag
would be set after a suspend/resume cycle with nothing attached.


-- 
Benson Leung
Software Engineer, Chrom* OS
bleung@chromium.org

Re: [PATCH v2] usb: hub: Clear Port Reset Change during init/resume

[Julius Werner]

> Did you run into an issue where port status change events weren't being
> generated because the Port Reset flag was set?  I'm trying to figure out
> if this addresses a real issue you hit (and thus should be queued for
> stable), or if this is just a precaution.

As Benson said, we're seeing this on the HP Chromebook 14 (Intel
LynxPoint xHC). It only happens after system suspend/resume, so I'm
not sure if there even is an xHC reset involved (not by the kernel
anyway, but with all that other stuff it's hard to say). Note that we
build our own firmware (including SMM/ACPI handlers), so this does not
directly translate to LynxPoint PCs, but I think we based some of our
code off Intel reference code and guides which may have already
included the problem. I've found port reset code in both our firmware
resume path and ACPI _PS0 handler, but they both seem to clear all
Port Change bits correctly, so we are not sure about our true root
cause yet.