[PATCH] usb: hub: Use correct reset for wedged USB3 devices that are NOTATTACHED

[PATCH] usb: hub: Use correct reset for wedged USB3 devices that are NOTATTACHED

[Julius Werner]

This patch adds a check for USB_STATE_NOTATTACHED to the
hub_port_warm_reset_required() workaround for ports that end up in
Compliance Mode in hub_events() when trying to decide which reset
function to use. Trying to call usb_reset_device() with a NOTATTACHED
device will just fail and leave the port broken.

Also bumped the messages about this kind of reset failure from dev_dbg()
to dev_warn() to make it easier to notice, since calling that function
with a NOTATTACHED device would almost always be a bug

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

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index e6b682c..0188056 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -4799,8 +4799,9 @@ static void hub_events(void)
 					hub->ports[i - 1]->child;
 
 				dev_dbg(hub_dev, "warm reset port %d\n", i);
-				if (!udev || !(portstatus &
-						USB_PORT_STAT_CONNECTION)) {
+				if (!udev ||
+				    !(portstatus & USB_PORT_STAT_CONNECTION) ||
+				    udev->state == USB_STATE_NOTATTACHED) {
 					status = hub_port_reset(hub, i,
 							NULL, HUB_BH_RESET_TIME,
 							true);
@@ -5074,7 +5075,7 @@ static int usb_reset_and_verify_device(struct usb_device *udev)
 
 	if (udev->state == USB_STATE_NOTATTACHED ||
 			udev->state == USB_STATE_SUSPENDED) {
-		dev_dbg(&udev->dev, "device reset not allowed in state %d\n",
+		dev_warn(&udev->dev, "device reset not allowed in state %d\n",
 				udev->state);
 		return -EINVAL;
 	}
@@ -5237,7 +5238,7 @@ int usb_reset_device(struct usb_device *udev)
 
 	if (udev->state == USB_STATE_NOTATTACHED ||
 			udev->state == USB_STATE_SUSPENDED) {
-		dev_dbg(&udev->dev, "device reset not allowed in state %d\n",
+		dev_warn(&udev->dev, "device reset not allowed in state %d\n",
 				udev->state);
 		return -EINVAL;
 	}
-- 
1.8.4.1

Re: [PATCH] usb: hub: Use correct reset for wedged USB3 devices that are NOTATTACHED

[Greg Kroah-Hartman]

On Wed, Nov 06, 2013 at 12:27:21PM -0800, Julius Werner wrote:
> This patch adds a check for USB_STATE_NOTATTACHED to the
> hub_port_warm_reset_required() workaround for ports that end up in
> Compliance Mode in hub_events() when trying to decide which reset
> function to use. Trying to call usb_reset_device() with a NOTATTACHED
> device will just fail and leave the port broken.

Who makes those calls, drivers?  Any specific ones that you know need to
be fixed?

> Also bumped the messages about this kind of reset failure from dev_dbg()
> to dev_warn() to make it easier to notice, since calling that function
> with a NOTATTACHED device would almost always be a bug

But what can a user do if those messages show up?

thanks,

greg k-h

Re: [PATCH] usb: hub: Use correct reset for wedged USB3 devices that are NOTATTACHED

[Alan Stern]

On Wed, 6 Nov 2013, Julius Werner wrote:

> This patch adds a check for USB_STATE_NOTATTACHED to the
> hub_port_warm_reset_required() workaround for ports that end up in
> Compliance Mode in hub_events() when trying to decide which reset
> function to use. Trying to call usb_reset_device() with a NOTATTACHED
> device will just fail and leave the port broken.

What if the device is in USB_STATE_SUSPENDED?

> 
> Also bumped the messages about this kind of reset failure from dev_dbg()
> to dev_warn() to make it easier to notice, since calling that function
> with a NOTATTACHED device would almost always be a bug

Not at all.  If a device is unplugged, its state changes to NOTATTACHED 
before the driver is unbound.  During that time, the driver will see 
all its URBs failing, so it may very well try to reset the device.  
(For example, usbhid behaves like this.)  That isn't a bug.

Alan Stern

Re: [PATCH] usb: hub: Use correct reset for wedged USB3 devices that are NOTATTACHED

[Julius Werner]

> Who makes those calls, drivers?  Any specific ones that you know need to
be fixed?

Well, the one I'm worried about is the one this patch is fixing, in
hub_events(). I have seen this happen when having certain USB3 devices
plugged into a host controller that always looses power on
suspend-to-ram (dwc3-exynos). Since the host controller resets itself
the port no longer has PORT_STAT_ENABLE set and that causes
hub_activate() to mark the device as USB_STATE_NOTATTACHED. The next
time khubd runs hub_events(), the port may be in Compliance Mode
(because the unexpected HC reset can confuse the link state machines
on both sides) and the kernel correctly tries to reset it, but doesn't
take the fact into account that usb_reset_device() doesn't work on
NOTATTACHED devices.

> But what can a user do if those messages show up?

Nothing. I was just thinking this might help developers follow the
kernel decisions better and understand a potential problem faster
(e.g. if the user posted his log in a bug report somewhere).

> What if the device is in USB_STATE_SUSPENDED?

I'm not sure that is possible at that point in hub_events(), I don't
know of a way that could lead to this situation. I could still add the
check just to be sure if you want it, though.

> Not at all.  If a device is unplugged, its state changes to NOTATTACHED
> before the driver is unbound.  During that time, the driver will see
> all its URBs failing, so it may very well try to reset the device.
> (For example, usbhid behaves like this.)  That isn't a bug.

Oh, okay, I wasn't quite sure how that plays together. Would you think
it's still valuable to print it out (maybe as dev_info() instead of
dev_warn()) instead of just silently ignoring the reset request? It
would have certainly been useful for me to find this problem faster,
but I can take it out again if you think it would result in too much
noise.

Re: [PATCH] usb: hub: Use correct reset for wedged USB3 devices that are NOTATTACHED

[Alan Stern]

On Wed, 6 Nov 2013, Julius Werner wrote:

> > What if the device is in USB_STATE_SUSPENDED?
> 
> I'm not sure that is possible at that point in hub_events(), I don't
> know of a way that could lead to this situation. I could still add the
> check just to be sure if you want it, though.

I don't know either.  But Sarah has said that ports can spontaneously
go into Compliance Mode for no apparent reason.  If that can happen,
maybe it can happen while the port is in U3 and the device is
suspended.  In such cases, though, you'd need to do a reset-resume
rather than a simple reset.

> > Not at all.  If a device is unplugged, its state changes to NOTATTACHED
> > before the driver is unbound.  During that time, the driver will see
> > all its URBs failing, so it may very well try to reset the device.
> > (For example, usbhid behaves like this.)  That isn't a bug.
> 
> Oh, okay, I wasn't quite sure how that plays together. Would you think
> it's still valuable to print it out (maybe as dev_info() instead of
> dev_warn()) instead of just silently ignoring the reset request? It
> would have certainly been useful for me to find this problem faster,
> but I can take it out again if you think it would result in too much
> noise.

I think keeping dev_dbg() is best.  If you're searching for the
solution to a problem, you should have debugging enabled and so you
ought to see the message.

Alan Stern

Re: [PATCH] usb: hub: Use correct reset for wedged USB3 devices that are NOTATTACHED

[Julius Werner]

> I don't know either.  But Sarah has said that ports can spontaneously
> go into Compliance Mode for no apparent reason.  If that can happen,
> maybe it can happen while the port is in U3 and the device is
> suspended.  In such cases, though, you'd need to do a reset-resume
> rather than a simple reset.

Okay. Looks like this is a more complicated question so let's keep it
out of this patch. We could always add another check to handle
USB_STATE_SUSPENDED later.

> I think keeping dev_dbg() is best.  If you're searching for the
> solution to a problem, you should have debugging enabled and so you
> ought to see the message.

Sure, I'll submit a second version in a moment.

[PATCH v2] usb: hub: Use correct reset for wedged USB3 devices that are NOTATTACHED

[Julius Werner]

This patch adds a check for USB_STATE_NOTATTACHED to the
hub_port_warm_reset_required() workaround for ports that end up in
Compliance Mode in hub_events() when trying to decide which reset
function to use. Trying to call usb_reset_device() with a NOTATTACHED
device will just fail and leave the port broken.

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

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index e6b682c..0188056 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -4799,8 +4799,9 @@ static void hub_events(void)
 					hub->ports[i - 1]->child;
 
 				dev_dbg(hub_dev, "warm reset port %d\n", i);
-				if (!udev || !(portstatus &
-						USB_PORT_STAT_CONNECTION)) {
+				if (!udev ||
+				    !(portstatus & USB_PORT_STAT_CONNECTION) ||
+				    udev->state == USB_STATE_NOTATTACHED) {
 					status = hub_port_reset(hub, i,
 							NULL, HUB_BH_RESET_TIME,
 							true);
-- 
1.8.4.1

Re: [PATCH v2] usb: hub: Use correct reset for wedged USB3 devices that are NOTATTACHED

[Alan Stern]

On Thu, 7 Nov 2013, Julius Werner wrote:

> This patch adds a check for USB_STATE_NOTATTACHED to the
> hub_port_warm_reset_required() workaround for ports that end up in
> Compliance Mode in hub_events() when trying to decide which reset
> function to use. Trying to call usb_reset_device() with a NOTATTACHED
> device will just fail and leave the port broken.
> 
> Signed-off-by: Julius Werner <jwerner@chromium.org>
> ---
>  drivers/usb/core/hub.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index e6b682c..0188056 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -4799,8 +4799,9 @@ static void hub_events(void)
>  					hub->ports[i - 1]->child;
>  
>  				dev_dbg(hub_dev, "warm reset port %d\n", i);
> -				if (!udev || !(portstatus &
> -						USB_PORT_STAT_CONNECTION)) {
> +				if (!udev ||
> +				    !(portstatus & USB_PORT_STAT_CONNECTION) ||
> +				    udev->state == USB_STATE_NOTATTACHED) {
>  					status = hub_port_reset(hub, i,
>  							NULL, HUB_BH_RESET_TIME,
>  							true);

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

Re: [PATCH] usb: hub: Use correct reset for wedged USB3 devices that are NOTATTACHED

[Sarah Sharp]

On Thu, Nov 07, 2013 at 10:32:33AM -0500, Alan Stern wrote:
> On Wed, 6 Nov 2013, Julius Werner wrote:
> 
> > > What if the device is in USB_STATE_SUSPENDED?
> > 
> > I'm not sure that is possible at that point in hub_events(), I don't
> > know of a way that could lead to this situation. I could still add the
> > check just to be sure if you want it, though.
> 
> I don't know either.  But Sarah has said that ports can spontaneously
> go into Compliance Mode for no apparent reason.  If that can happen,
> maybe it can happen while the port is in U3 and the device is
> suspended.  In such cases, though, you'd need to do a reset-resume
> rather than a simple reset.

Looking at commits c3897aa5386faba77e5bbdf94902a1658d3a5b11 and
71c731a296f1b08a3724bd1b514b64f1bda87a23, it seems that the TI host
controllers' ports can go into compliance mode only when a device is
inserted.  Once the device is link trained by the redriver, the port
shouldn't go into compliance mode.  So we should never see compliance
mode on a port with an attached USB device in suspend.

Alex, can you confirm that the TI host's port won't go into compliance
mode while a connected device is suspended?

> > > Not at all.  If a device is unplugged, its state changes to NOTATTACHED
> > > before the driver is unbound.  During that time, the driver will see
> > > all its URBs failing, so it may very well try to reset the device.
> > > (For example, usbhid behaves like this.)  That isn't a bug.
> > 
> > Oh, okay, I wasn't quite sure how that plays together. Would you think
> > it's still valuable to print it out (maybe as dev_info() instead of
> > dev_warn()) instead of just silently ignoring the reset request? It
> > would have certainly been useful for me to find this problem faster,
> > but I can take it out again if you think it would result in too much
> > noise.
> 
> I think keeping dev_dbg() is best.  If you're searching for the
> solution to a problem, you should have debugging enabled and so you
> ought to see the message.
> 
> Alan Stern
> 

Re: [PATCH] usb: hub: Use correct reset for wedged USB3 devices that are NOTATTACHED

[Julius Werner]

I'm not sure if it's worth it further looking into this for the
SUSPENDED state (Sarah's post sounds like that shouldn't happen)...
but at any rate, that would be orthogonal to my patch, right? I'm
really only interested in the NOTATTACHED case, which solves a real
issue on my system. Do you still have questions about that before
you'd consider picking it up, Greg?

RE: [PATCH] usb: hub: Use correct reset for wedged USB3 devices that are NOTATTACHED

[Cortes, Alexis]

Hi Sarah,

Sorry for my delayed response, I just saw your e-mail (it got filtered somehow). About your question: actually I'm not sure, I'll have to check that to confirm it. I'll get back to you with an answer as soon as I have it.

Best Regards,
Alexis Cortes.

-----Original Message-----
From: Sarah Sharp [mailto:sarah.a.sharp@linux.intel.com] 
Sent: Thursday, November 14, 2013 5:31 PM
To: Alan Stern
Cc: Julius Werner; Greg Kroah-Hartman; LKML; linux-usb@vger.kernel.org; Benson Leung; Vincent Palatin; Cortes, Alexis
Subject: Re: [PATCH] usb: hub: Use correct reset for wedged USB3 devices that are NOTATTACHED

On Thu, Nov 07, 2013 at 10:32:33AM -0500, Alan Stern wrote:
> On Wed, 6 Nov 2013, Julius Werner wrote:
> 
> > > What if the device is in USB_STATE_SUSPENDED?
> > 
> > I'm not sure that is possible at that point in hub_events(), I don't 
> > know of a way that could lead to this situation. I could still add 
> > the check just to be sure if you want it, though.
> 
> I don't know either.  But Sarah has said that ports can spontaneously 
> go into Compliance Mode for no apparent reason.  If that can happen, 
> maybe it can happen while the port is in U3 and the device is 
> suspended.  In such cases, though, you'd need to do a reset-resume 
> rather than a simple reset.

Looking at commits c3897aa5386faba77e5bbdf94902a1658d3a5b11 and 71c731a296f1b08a3724bd1b514b64f1bda87a23, it seems that the TI host controllers' ports can go into compliance mode only when a device is inserted.  Once the device is link trained by the redriver, the port shouldn't go into compliance mode.  So we should never see compliance mode on a port with an attached USB device in suspend.

Alex, can you confirm that the TI host's port won't go into compliance mode while a connected device is suspended?

> > > Not at all.  If a device is unplugged, its state changes to 
> > > NOTATTACHED before the driver is unbound.  During that time, the 
> > > driver will see all its URBs failing, so it may very well try to reset the device.
> > > (For example, usbhid behaves like this.)  That isn't a bug.
> > 
> > Oh, okay, I wasn't quite sure how that plays together. Would you 
> > think it's still valuable to print it out (maybe as dev_info() 
> > instead of
> > dev_warn()) instead of just silently ignoring the reset request? It 
> > would have certainly been useful for me to find this problem faster, 
> > but I can take it out again if you think it would result in too much 
> > noise.
> 
> I think keeping dev_dbg() is best.  If you're searching for the 
> solution to a problem, you should have debugging enabled and so you 
> ought to see the message.
> 
> Alan Stern
> 

Re: [PATCH] usb: hub: Use correct reset for wedged USB3 devices that are NOTATTACHED

[Sarah Sharp]

On Tue, Nov 19, 2013 at 02:53:22PM +0000, Cortes, Alexis wrote:
> Hi Sarah,
> 
> Sorry for my delayed response, I just saw your e-mail (it got filtered somehow). About your question: actually I'm not sure, I'll have to check that to confirm it. I'll get back to you with an answer as soon as I have it.

Ping, Alexis: any info on this question?

Sarah Sharp

> -----Original Message-----
> From: Sarah Sharp [mailto:sarah.a.sharp@linux.intel.com] 
> Sent: Thursday, November 14, 2013 5:31 PM
> To: Alan Stern
> Cc: Julius Werner; Greg Kroah-Hartman; LKML; linux-usb@vger.kernel.org; Benson Leung; Vincent Palatin; Cortes, Alexis
> Subject: Re: [PATCH] usb: hub: Use correct reset for wedged USB3 devices that are NOTATTACHED
> 
> On Thu, Nov 07, 2013 at 10:32:33AM -0500, Alan Stern wrote:
> > On Wed, 6 Nov 2013, Julius Werner wrote:
> > 
> > > > What if the device is in USB_STATE_SUSPENDED?
> > > 
> > > I'm not sure that is possible at that point in hub_events(), I don't 
> > > know of a way that could lead to this situation. I could still add 
> > > the check just to be sure if you want it, though.
> > 
> > I don't know either.  But Sarah has said that ports can spontaneously 
> > go into Compliance Mode for no apparent reason.  If that can happen, 
> > maybe it can happen while the port is in U3 and the device is 
> > suspended.  In such cases, though, you'd need to do a reset-resume 
> > rather than a simple reset.
> 
> Looking at commits c3897aa5386faba77e5bbdf94902a1658d3a5b11 and 71c731a296f1b08a3724bd1b514b64f1bda87a23, it seems that the TI host controllers' ports can go into compliance mode only when a device is inserted.  Once the device is link trained by the redriver, the port shouldn't go into compliance mode.  So we should never see compliance mode on a port with an attached USB device in suspend.
> 
> Alex, can you confirm that the TI host's port won't go into compliance mode while a connected device is suspended?
> 
> > > > Not at all.  If a device is unplugged, its state changes to 
> > > > NOTATTACHED before the driver is unbound.  During that time, the 
> > > > driver will see all its URBs failing, so it may very well try to reset the device.
> > > > (For example, usbhid behaves like this.)  That isn't a bug.
> > > 
> > > Oh, okay, I wasn't quite sure how that plays together. Would you 
> > > think it's still valuable to print it out (maybe as dev_info() 
> > > instead of
> > > dev_warn()) instead of just silently ignoring the reset request? It 
> > > would have certainly been useful for me to find this problem faster, 
> > > but I can take it out again if you think it would result in too much 
> > > noise.
> > 
> > I think keeping dev_dbg() is best.  If you're searching for the 
> > solution to a problem, you should have debugging enabled and so you 
> > ought to see the message.
> > 
> > Alan Stern
> > 

RE: [PATCH] usb: hub: Use correct reset for wedged USB3 devices that are NOTATTACHED

[Cortes, Alexis]

Hi Sarah,

Sorry for my delayed response, I was on vacation. Although I wasn't able to verify this on an actual system that is susceptible to the compliance mode issue, after reviewing the USB spec, I don't think it is possible that a port can enter compliance mode while a connected device is suspended. Per the spec, the only way to enter to Compliance Mode state is through the Polling state (after the 'first LFPS timeout'). Does this make sense? 

Best Regards,
Alexis Cortes.

-----Original Message-----
From: Sarah Sharp [mailto:sarah.a.sharp@linux.intel.com] 
Sent: Wednesday, December 04, 2013 6:04 PM
To: Cortes, Alexis
Cc: Alan Stern; Julius Werner; Greg Kroah-Hartman; LKML; linux-usb@vger.kernel.org; Benson Leung; Vincent Palatin
Subject: Re: [PATCH] usb: hub: Use correct reset for wedged USB3 devices that are NOTATTACHED


On Tue, Nov 19, 2013 at 02:53:22PM +0000, Cortes, Alexis wrote:
> Hi Sarah,
> 
> Sorry for my delayed response, I just saw your e-mail (it got filtered somehow). About your question: actually I'm not sure, I'll have to check that to confirm it. I'll get back to you with an answer as soon as I have it.

Ping, Alexis: any info on this question?

Sarah Sharp

> -----Original Message-----
> From: Sarah Sharp [mailto:sarah.a.sharp@linux.intel.com]
> Sent: Thursday, November 14, 2013 5:31 PM
> To: Alan Stern
> Cc: Julius Werner; Greg Kroah-Hartman; LKML; 
> linux-usb@vger.kernel.org; Benson Leung; Vincent Palatin; Cortes, 
> Alexis
> Subject: Re: [PATCH] usb: hub: Use correct reset for wedged USB3 
> devices that are NOTATTACHED
> 
> On Thu, Nov 07, 2013 at 10:32:33AM -0500, Alan Stern wrote:
> > On Wed, 6 Nov 2013, Julius Werner wrote:
> > 
> > > > What if the device is in USB_STATE_SUSPENDED?
> > > 
> > > I'm not sure that is possible at that point in hub_events(), I 
> > > don't know of a way that could lead to this situation. I could 
> > > still add the check just to be sure if you want it, though.
> > 
> > I don't know either.  But Sarah has said that ports can 
> > spontaneously go into Compliance Mode for no apparent reason.  If 
> > that can happen, maybe it can happen while the port is in U3 and the 
> > device is suspended.  In such cases, though, you'd need to do a 
> > reset-resume rather than a simple reset.
> 
> Looking at commits c3897aa5386faba77e5bbdf94902a1658d3a5b11 and 71c731a296f1b08a3724bd1b514b64f1bda87a23, it seems that the TI host controllers' ports can go into compliance mode only when a device is inserted.  Once the device is link trained by the redriver, the port shouldn't go into compliance mode.  So we should never see compliance mode on a port with an attached USB device in suspend.
> 
> Alex, can you confirm that the TI host's port won't go into compliance mode while a connected device is suspended?
> 
> > > > Not at all.  If a device is unplugged, its state changes to 
> > > > NOTATTACHED before the driver is unbound.  During that time, the 
> > > > driver will see all its URBs failing, so it may very well try to reset the device.
> > > > (For example, usbhid behaves like this.)  That isn't a bug.
> > > 
> > > Oh, okay, I wasn't quite sure how that plays together. Would you 
> > > think it's still valuable to print it out (maybe as dev_info() 
> > > instead of
> > > dev_warn()) instead of just silently ignoring the reset request? 
> > > It would have certainly been useful for me to find this problem 
> > > faster, but I can take it out again if you think it would result 
> > > in too much noise.
> > 
> > I think keeping dev_dbg() is best.  If you're searching for the 
> > solution to a problem, you should have debugging enabled and so you 
> > ought to see the message.
> > 
> > Alan Stern
> >