When root_port_reset() in ohci-hub.c polls for the end of the reset,
it puts no limit on the loop and will only exit the loop when either
the RH_PS_PRS bit clears or the register returns all 1's (the latter
condition is a recent addition).

If for some reason the bit never clears, we sit here forever and never
exit the loop.

I actually hit this on one of my machines, and I'm trying to track
down what's happening.

Regardless of why my machine is doing this, there absolutely should be
some upper bound put on the number of times we will run through this
loop, perhaps enough such that up to 5 seconds elapses waiting for the
reset bit to clear.  And if it times out we should print a loud
message onto the console, but still try to continue.

Thanks!
-

[ message continues ]

The all-ones typically indicating something like CardBus eject

Sounds like a hardware issue -- unless something else is trashing
controller state.  We've not observed that specific hardware failure
before, for what it's worth.

Is this SPARC, or is ACPI potentially in play?  PCI, or non-PCI?
Are the other ports still behaving?  Is EHCI maybe trying to switch
ownership of that port?  Is maybe the (newish) autosuspend stuff
kicking in?

The OHCI spec requires the controller to stop the reset itself.

Patches accepted.  :)

Since the PRS bit is specified as "write one", with writing zero
as no-effect (since the rest is hardware-timed), the only recovery
procedure might involve resetting the whole controller.  Messy,
and not something the usb core has historically handled very well.

- Dave

-

[ message continues ]

From: David Brownell <david-b@pacbell.net>



I'm 700 patches deep with an additionally large backlog of
patches to apply for the networking and sparc64 tree.

I don't have the time, which is why I reported the problem
to the OHCI maintainer instead of letting it slip through

At a minimum you should exit the loop and print out a warning
messages and try to continue even without trying to reset the
whole controller if that will take some developer effort.

Anything is better than just hanging there forever.  Not every
user knows to hit ALT-SYSRQ then 'P' to get a register dump to
figure out why their computer stopped booting.

Every register polling loops, without exception, should have a limit
and exit with an error indication when that limit is reached.  It
will never hang someones machine, because although not this time, in
many cases these kinds of hangs are absolutely impossible to debug
(interrupts disabled, critical lock held, etc.)
-

[ message continues ]

From: David Miller <davem@davemloft.net>

To add some more information here, I think the EHCI idea might
hold some water.

What I have here are two NEC OHCI USB interfaces and one NEC EHCI
USB interface on PCI.  Aparently they all go through a shared
USB hub, mapped like this:

HUB Port 1: OHCI #1, EHCI
HUB Port 2: OHCI #2, EHCI
HUB Port 3: OHCI #1, EHCI
HUB Port 4: OHCI #2, EHCI
HUB Port 5: OHCI #1, EHCI

The OHCI ports go out to external USB connectors on the back panel of
the machine, whereas the EHCI is connected up to an internal USB
storage CDROM device and what appears to be another USB hub.

The problem seems to be very strongly tied to timing.  For example
simply adding "ignore_loglevel" to the kernel boot command line can
make the problem go away.

This got me thinking about your EHCI comment.

If these controllers are going through the same HUB, things might go
south if OHCI initialized first, then khubd et al. are asynchronously
accessing the segments behind OHCI at the same time that the EHCI
driver is initializing.  Perhaps, this is the kind of sequence of
events which makes one of the root ports reset in such a way that the
the reset bit never clears.

Given that this machine has 64 cpus, the likelyhood for such parallel
accesses is very likely :-)

Does this make any sense?

Regardless, here is a patch that hardens the OHCI reset handling
loops so that they break out instead of hanging the entire system
should this condition occur.  It's at least better than what the
code does to a user right now which is hang the box completely:

[USB] ohci: Do not hang the system if port reset does not complete.

Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/drivers/usb/host/ohci-hub.c b/drivers/usb/host/ohci-hub.c
index bb9cc59..77ae5b4 100644
--- a/drivers/usb/host/ohci-hub.c
+++ b/drivers/usb/host/ohci-hub.c
@@ -563,14 +563,19 @@ static inline int root_port_reset (struct ohci_hcd *ohci, unsigned port)
 	u32	temp;
 	u16	now = ...
[ message continues ]

Yes it does, I'm seeing reports from some hardware companies of the very
same thing.  If you serialize and load the ehci driver first, and then
the ohci driver, that should fix the problem.

Does that also work for you?  Or are these drivers built into the
kernel?

thanks,

greg k-h
-

[ message continues ]

From: Greg KH <greg@kroah.com>

As coicidence would have it I finally found a recipe for triggering
the issue, and it ties into what you're talking about here.

It happens only if I make sure OHCI gets loaded first and then EHCI
right afterwards.

It seems that indeed it is important for EHCI to get loaded first,
and in-kernel this is ensured by the link ordering.

However, when both OHCI and EHCI are built as modules (or, similarly
I guess, OHCI is built-in and EHCI is modular) there appears to be
nothing in userspace which makes sure EHCI gets loaded first.

When this triggers, in OHCI's root_port_reset(), the port status
register reads 0x111 in that inner-loop and the value never changes.
It stays like this forever.
-

[ message continues ]

The old /etc/hotplug/usb.rc script made sure to load those modules
in the correct order:  EHCI first.

-

[ message continues ]

From: David Brownell <david-b@pacbell.net>

I expected to find something cute attempting to handle this under
/etc/udev, I have failed so far :-)
-

[ message continues ]

No, nothing cute in udev itself, but it seems that all distros that I
know of have a "load these modules now" type setting in their init
scripts that can be used here.

I can't think of a way to enforce this load order on the modules
themselves due to the fact that OHCI might not even be needed for EHCI
devices on UHCI (Intel) based chipsets :(

Can anyone else?

thanks,

greg k-h
-

[ message continues ]

From: Greg KH <greg@kroah.com>

The three modules perhaps should be a bundle of whatever ones you have
enabled, and internally we can dispatch the initialization to occur in
the correct order from a top-level module_init().

If the devices need to be initialized in a certain order in a
situation like this, it really seems like it is the kernel's job to
enforce it.
-

[ message continues ]

Is the problem strictly an ordering problem or just a race ? In the
later case, maybe some better arbitration by the USB core to make
sure things are quiescent or in a known state when letting a new HCD
register might help ?

Ben.


-

[ message continues ]

I agree.

Here's some information from Intel about where they have seen this
happen for UHCI controllers, so it's not just an OHCI issue :(

thanks,

greg k-h

----------------


We had a logic analyzer attached to the bus going to the ESB (ICH) which
has the USB controller in it. In the passing case we would see no
accesses to UHCI IO registers while EHCI initialized and sets its config
flag. The EHCI Port Status & Control registers were then read and then
we see a write to the EHCI Port Status & Control registers port owner
bit for the low speed devices (keyboard & mouse). This turns control
back over to the companion UHCI controller.=20

In our most prevalent failing case (#1 below) we never saw the write to
port owner bit on the ports with the low speed devices. In the passing
case we see the write to the port owner bit.

I do not see how this would have anything to do with flakey hardware
especially since we can reproduce this on all of our systems and the
same device (USB controller) is used on multiple products.=20

I really believe that this has to do with the UHCI and EHCI drivers
running on top of each other. This seems to be happening fairly often on
our systems. If the EHCI driver runs first then we do not see the
failure. If they are running at the same time then we see different
failure symptoms.=20

1) We see that the ports with low speed devices are still in EHCI mode
(port owner bit not written to in EHCI driver). In our analyzer captures
we see the reads from the Port Status & Control register and it is
indicating that there are low speed devices on the ports. Can you tell
us why the driver would not be doing the write to the port owner bit
when it sees that low speed devices are attached to that port? Is there
something specific that it looks for and decides not to do the write?

2) In other cases we see that the ports with the low speed devices are
back in UHCI mode but the ports are disabled. In this case we see from
the analyzer traces that ...
[ message continues ]

It's hard to say without more info about what's going on.  It
might be on a non-enumeraton code path -- where nothing expects
to set the OWNER bit -- or the CONNECT bit might not be set.
See host/ehci-hub.c for details about exactly what the EHCI parts
are doing, and core/hub.c for how it's driven.

(It can be tricky to make sense of all that, easier if debugging
messages are turned on.   But that changes timings.  Better yet
would be being non-intrusive tracing of the actual code paths

If the UHCI driver got disconnect and reconnect notifications,
I expect it would do so.  At least OHCI seems *not* to have gotten
such notifications.  I'm speculating that the "just a switch"
behavior for the CF (and OWNER) bits may not allow notifications
like that to happen when the companion controllers are resetting.

- Dave

-

[ message continues ]

Right.  In general the driver _does_ write to the port owner bit when 
it sees a low-speed device is attached to the port.  If that didn't 
happen in this case, maybe it's because the driver didn't see it.  

It certainly would help us if the tests were made on a kernel with

This implies that the reset sequence finished successfully.  Did that 

But above they said that it _had_ completed!  Did they mean that the 
reset was complete but the driver hadn't yet detected that it was 

That seems likely.  There's no way (as far as I can tell) for a host to 
see a disconnect while a reset is in progress.  Of course, as soon as 
the reset is over then the host should see the disconnect, and it 
should set the corresponding Connect-Status-Changed bit.

Amusing scenario: Suppose that ehci-hcd initializes the EHCI controller 
(taking control of the port), sees that the device isn't high speed, 
and switches port ownership back to the companion controller, all 
during the span of the companion's reset.  The companion would never 
know anything had happened!  But then everything should just work -- 
the port would be enabled and communication should proceed normally.

Alan Stern

-

[ message continues ]

Assuming PCI is present, /sys/bus/pci/devices/*/class can tell
if EHCI is present (0x0c0320) ... if so, load that driver.
Then repeat for OHCI (0x0c0310) and UHCI (0x0c0300).
-

[ message continues ]

From: David Brownell <david-b@pacbell.net>

These are facts all of us know very well, but implementing this in
userspace in a failsafe manner isn't practical.  That's what we're
discussing.

There are things that autoload USB drivers way before udev or similar
even get started.

For example, the first thing some distributions do is try to load the
correct keyboard maps.  Guess what that can do?  It triggers a load of
all of the modular USB host controller drivers in case we have a USB
keyboard.

The only real solution is in the kernel, because it is the only
clean place to trap all of the potential module load events.
-

[ message continues ]

That will not work for all of the non-PCI implementations though.

Ben.


-

[ message continues ]

Oddly enough, that's why I said "assuming PCI is present".  ;)

Anything using the ARC/TDI RTL doesn't have this issue ... it
includes full/low speed support directly, without a companion
controller.  That covers the Freescale silicon and some others.
For a long time I think that was the only generally available
non-PCI implementation.

But you're right that there are (now) a few other cases.
In-tree right now are also the Au1200, ps3, and 440epx.

- Dave

-

[ message continues ]

I agree with Ben; the best way to solve this is in the kernel.  Even if 
you did manage to make sure that modules were loaded in the right order 
initially, that wouldn't help if somebody starts unloading and loading 
modules by hand.

The underlying cause of the problem appears to be related to the fact
that it is impossible in principle to detect a disconnect while a
full/low-speed reset is in progress.  However the hardware shouldn't
rely on this, and it should fail gracefully when an unexpected
disconnect does occur.

For example, what would the controller do if the user unplugged the USB 
cable right in the middle of a reset?

Anyway, it certainly would be easy enough to make port resets mutually
exclusive with EHCI initialization.  That would work on any platform,
PCI or not.

Alan Stern

-

[ message continues ]

That's a good idea, any thoughts as to how to do this?  I think I can
find some Intel and Dell people who would be glad to test this out if
you have a proposed patch.

thanks,

greg k-h
-

[ message continues ]

Here is a proposed patch.  I haven't tried running it, but it compiles 
okay.

Alan Stern



Index: usb-2.6/drivers/usb/core/hcd.h
===================================================================
--- usb-2.6.orig/drivers/usb/core/hcd.h
+++ usb-2.6/drivers/usb/core/hcd.h
@@ -19,6 +19,8 @@
 
 #ifdef __KERNEL__
 
+#include <linux/rwsem.h>
+
 /* This file contains declarations of usbcore internals that are mostly
  * used or exposed by Host Controller Drivers.
  */
@@ -470,5 +472,9 @@ static inline void usbmon_urb_complete(s
 		: (in_interrupt () ? "in_interrupt" : "can sleep"))
 
 
-#endif /* __KERNEL__ */
+/* Mutual exclusion for EHCI CF initialization.  This interferes with
+ * port reset on some companion controllers.
+ */
+extern struct rw_semaphore ehci_cf_port_reset_rwsem;
 
+#endif /* __KERNEL__ */
Index: usb-2.6/drivers/usb/core/hub.c
===================================================================
--- usb-2.6.orig/drivers/usb/core/hub.c
+++ usb-2.6/drivers/usb/core/hub.c
@@ -125,6 +125,12 @@ MODULE_PARM_DESC(use_both_schemes,
 		"try the other device initialization scheme if the "
 		"first one fails");
 
+/* Mutual exclusion for EHCI CF initialization.  This interferes with
+ * port reset on some companion controllers.
+ */
+DECLARE_RWSEM(ehci_cf_port_reset_rwsem);
+EXPORT_SYMBOL_GPL(ehci_cf_port_reset_rwsem);
+
 
 static inline char *portspeed(int portstatus)
 {
@@ -1579,6 +1585,8 @@ static int hub_port_reset(struct usb_hub
 {
 	int i, status;
 
+	down_read(&ehci_cf_port_reset_rwsem);
+
 	/* Reset the port */
 	for (i = 0; i < PORT_RESET_TRIES; i++) {
 		status = set_port_feature(hub->hdev,
@@ -1610,7 +1618,7 @@ static int hub_port_reset(struct usb_hub
 			usb_set_device_state(udev, status
 					? USB_STATE_NOTATTACHED
 					: USB_STATE_DEFAULT);
-			return status;
+			goto done;
 		}
 
 		dev_dbg (hub->intfdev,
@@ -1623,6 +1631,8 @@ static int hub_port_reset(struct usb_hub
 		"Cannot enable port %i.  Maybe the USB cable is ...
[ message continues ]

You have two copies of that comment; I'd drop this one, and add a
better one around CF initialization.

What I'd have here is a comment that this symbol is exported ONLY

That deserves a comment about how CF and companion port resets
don't mix well.

... assuming this does check out as the problem, which I expect
it will.

-

[ message continues ]

From: Alan Stern <stern@rowland.harvard.edu>

Are we sure that this is the only event that causes the conflicts between
the EHCI and it's companion virtual OHCI/UHCI host(s)?

Is there a chip reset done to the EHCI controller early during device
probe that could cause problems too?

I know it's grotty, but I definitely feel more comfortable with the
fix involving making sure EHCI loads and fully probes first.
-

[ message continues ]

It's the event which can cause trouble when EHCI starts up
after OHCI/UHCI (rather than before it).

The other event is setting the OWNER bit, which clearly
has not caused trouble.

Does it resolve the problem you observed?

- Dave

-

[ message continues ]

From: David Brownell <david-b@pacbell.net>

I will definitely give the patch a try and report back.
-

[ message continues ]

From: David Miller <davem@davemloft.net>

My tests look really good with Alan Stern's patch.

With a kernel is hacked to load OHCI before EHCI, and
that always hangs, the patch makes the hang go away
reliably.
-

[ message continues ]

From: David Miller <davem@davemloft.net>

Bad news, even with the rwsem after a lot more testing I can still
trigger the hang in ohci_hub_control() :-(

I think we need to go back to considering the total serialization
approach to this problem.
-

[ message continues ]

We shouldn't need that.  What happens if you add an msleep(5)
before ehci-hcd::ehci_run() drops ehci_cf_port_reset_rwsem?

The theory there being that the switch triggered by setting CF
doesn't take effect instantaneously, contrary to the effective
assumption of that code.  A delay of 5 msec seems like it should
be more than enough, but that's kind of a guess ... it's good to
keep that low, since unfortunately that's in the critical path
for OLPC "resume from idle".

- Dave

-

[ message continues ]

From: David Brownell <david-b@pacbell.net>


I want to help with this, but if I even breath on the kernel the bug
goes away.  The race just gets harder to trigger, and if we just keep
adding things it'll make the problem go away but for the absolutely
wrong reasons.

The only way we will provably fix this is to make sure EHCI initialize
fully, first, regardless of kernel config or what userland does.

Also, David, you haven't done anything with the feedback I gave to the
most recent revision of the OHCI hub reset anti-wedge patch.  You
removed the debug logging when the outer-loop timeout expires, and I
asked that you put that back so that if it happens there is some
chance to know that this is what happened.  If it's not supposed to
happen, there is no harm in putting the debugging log message there
so that if the impossible does happen we find out about it.

I really don't think it's appropriate for that bug fix to sit yet
another week.
-

[ message continues ]

Unfortunately that simply isn't possible.  No matter what you do, the 
user can always unload ehci-hcd and then load it back in again.

Do you have any idea _where_ in ohci_hub_control the hang still occurs?  
Is it the same unbounded reset loop?

Does the patch below satisfy both Davids?

Alan Stern



Index: usb-2.6/drivers/usb/host/ohci-hub.c
===================================================================
--- usb-2.6.orig/drivers/usb/host/ohci-hub.c
+++ usb-2.6/drivers/usb/host/ohci-hub.c
@@ -560,10 +560,10 @@ static void start_hnp(struct ohci_hcd *o
 /* called from some task, normally khubd */
 static inline int root_port_reset (struct ohci_hcd *ohci, unsigned port)
 {
-	__hc32 __iomem *portstat = &ohci->regs->roothub.portstatus [port];
-	u32	temp;
-	u16	now = ohci_readl(ohci, &ohci->regs->fmnumber);
-	u16	reset_done = now + PORT_RESET_MSEC;
+	__hc32 __iomem	*portstat = &ohci->regs->roothub.portstatus[port];
+	u32		temp;
+	unsigned long	reset_done = jiffies +
+				msecs_to_jiffies(PORT_RESET_MSEC);
 
 	/* build a "continuous enough" reset signal, with up to
 	 * 3msec gap between pulses.  scheduler HZ==100 must work;
@@ -578,6 +578,8 @@ static inline int root_port_reset (struc
 				return -ESHUTDOWN;
 			if (!(temp & RH_PS_PRS))
 				break;
+			if (time_after(jiffies, reset_done))
+				break;
 			udelay (500);
 		}
 
@@ -589,8 +591,7 @@ static inline int root_port_reset (struc
 		/* start the next reset, sleep till it's probably done */
 		ohci_writel (ohci, RH_PS_PRS, portstat);
 		msleep(PORT_RESET_HW_MSEC);
-		now = ohci_readl(ohci, &ohci->regs->fmnumber);
-	} while (tick_before(now, reset_done));
+	} while (time_before_eq(jiffies, reset_done));
 	/* caller synchronizes using PRSC */
 
 	return 0;

-

[ message continues ]

From: Alan Stern <stern@rowland.harvard.edu>

Yes we can, by making OHCI and EHCI one module with a top-level
dispatch.  If you enable both OHCI and EHCI, the top-level
module will dispatch the host initializations in the correct order.

This is what I've suggested from the beginning.
-

[ message continues ]

Wait, you can have hardware with both EHCI and UHCI too.  Does that mean
we should merge all three together?  I don't think so :)

But perhaps we can order the hardware init stuff from all three together
like this into a separate module they all depend on.  In a way, that's
what the lock tried to do, right?  Are we just not catching all places
we could have hardware being talked to by two modules at the same time?

thanks,

greg k-h
-

[ message continues ]

No, we do catch the one place where it happens.  The problem seems to
be that the hardware update takes some time.  That is, on one side we
take the write lock, talk to the EHCI hardware, and drop the write
lock.  On the other side we take the read lock, talk to the OHCI 
hardware, and drop the read lock.  Nevertheless, the interference 
occurs.  David B.'s interpretation is that the hardware's change of 
state takes more time than the CPU uses in manipulating locks and 
switching tasks.  Hence his suggestion for adding a delay.

Alan Stern

-

[ message continues ]

From: Alan Stern <stern@rowland.harvard.edu>


No, there are no warning messages that trigger

My last patch was just fine, it timed out appropriately and warned the
user telling them exactly what event timed out.

I'm reposting it here for reference, this is getting silly:

Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/drivers/usb/host/ohci-hub.c b/drivers/usb/host/ohci-hub.c
index bb9cc59..9149593 100644
--- a/drivers/usb/host/ohci-hub.c
+++ b/drivers/usb/host/ohci-hub.c
@@ -563,14 +563,19 @@ static inline int root_port_reset (struct ohci_hcd *ohci, unsigned port)
 	u32	temp;
 	u16	now = ohci_readl(ohci, &ohci->regs->fmnumber);
 	u16	reset_done = now + PORT_RESET_MSEC;
+	int	limit_1;
 
 	/* build a "continuous enough" reset signal, with up to
 	 * 3msec gap between pulses.  scheduler HZ==100 must work;
 	 * this might need to be deadline-scheduled.
 	 */
-	do {
+	limit_1 = 100;
+	while (--limit_1 >= 0) {
+		int limit_2;
+
 		/* spin until any current reset finishes */
-		for (;;) {
+		limit_2 = PORT_RESET_HW_MSEC * 2;
+		while (--limit_2 >= 0) {
 			temp = ohci_readl (ohci, portstat);
 			/* handle e.g. CardBus eject */
 			if (temp == ~(u32)0)
@@ -579,6 +584,10 @@ static inline int root_port_reset (struct ohci_hcd *ohci, unsigned port)
 				break;
 			udelay (500);
 		}
+		if (limit_2 < 0) {
+			ohci_warn(ohci, "Root port inner-loop reset timeout, "
+				  "portstat[%08x]\n", temp);
+		}
 
 		if (!(temp & RH_PS_CCS))
 			break;
@@ -589,7 +598,14 @@ static inline int root_port_reset (struct ohci_hcd *ohci, unsigned port)
 		ohci_writel (ohci, RH_PS_PRS, portstat);
 		msleep(PORT_RESET_HW_MSEC);
 		now = ohci_readl(ohci, &ohci->regs->fmnumber);
-	} while (tick_before(now, reset_done));
+		if (!tick_before(now, reset_done))
+			break;
+	}
+	if (limit_1 < 0) {
+		ohci_warn(ohci, "Root port outer-loop reset timeout, "
+			  "now[%04x] reset_done[%04x]\n",
+			  now, reset_done);
+	}
 	/* caller synchronizes using PRSC */
 
 ...
[ message continues ]

Where did 100 come from?  Why not rely on the reset_done criterion,

This also is a somewhat arbitrary limit.  There's no obvious reason why
PORT_RESET_HW_MSEC should be both the limit for this loop and the

I realize that you are copying existing code here, but this makes the 
same kind of mistake you are trying to fix.  If a broken controller 
fails to increment its framenumber register, this termination condition 

What reason is there for having two warning messages?  One ought to be 
enough.

Alan Stern



Index: usb-2.6/drivers/usb/host/ohci-hub.c
===================================================================
--- usb-2.6.orig/drivers/usb/host/ohci-hub.c
+++ usb-2.6/drivers/usb/host/ohci-hub.c
@@ -560,10 +560,10 @@ static void start_hnp(struct ohci_hcd *o
 /* called from some task, normally khubd */
 static inline int root_port_reset (struct ohci_hcd *ohci, unsigned port)
 {
-	__hc32 __iomem *portstat = &ohci->regs->roothub.portstatus [port];
-	u32	temp;
-	u16	now = ohci_readl(ohci, &ohci->regs->fmnumber);
-	u16	reset_done = now + PORT_RESET_MSEC;
+	__hc32 __iomem	*portstat = &ohci->regs->roothub.portstatus[port];
+	u32		temp;
+	unsigned long	reset_done = jiffies +
+				msecs_to_jiffies(PORT_RESET_MSEC);
 
 	/* build a "continuous enough" reset signal, with up to
 	 * 3msec gap between pulses.  scheduler HZ==100 must work;
@@ -578,6 +578,12 @@ static inline int root_port_reset (struc
 				return -ESHUTDOWN;
 			if (!(temp & RH_PS_PRS))
 				break;
+			if (time_after(jiffies, reset_done)) {
+				ohci_warn(ohci, "Port %d reset timeout, "
+						"status %x\n",
+						port + 1, temp);
+				break;
+			}
 			udelay (500);
 		}
 
@@ -589,8 +595,7 @@ static inline int root_port_reset (struc
 		/* start the next reset, sleep till it's probably done */
 		ohci_writel (ohci, RH_PS_PRS, portstat);
 		msleep(PORT_RESET_HW_MSEC);
-		now = ohci_readl(ohci, &ohci->regs->fmnumber);
-	} while (tick_before(now, reset_done));
+	} while (time_before_eq(jiffies, ...
[ message continues ]

From: Alan Stern <stern@rowland.harvard.edu>

In my patch it was possible for the inner loop one to succeed, but the
outer one to not do so.

In your's this is not the case so I guess it's OK.

I wonder if it's so wise trying to do two things at once.  Here we are
adding the loop timeouts, and also changing to using jiffies based
timeouts rather than a chip timer register based one.

I preferred my patches because it solved one single problem, the lack
of loop limits.  The timeout mechanism could have been changed in
another followon patch.
-

[ message continues ]

At this point I'd be perfectly happy to sit back and let David B.
choose, among all the possibilities that have been posted, the one he
thinks is best.

Alan Stern

-

[ message continues ]

Not if what I suggested is happening is really what's happening.
(Quoted next.)

It's got to be just a *simple* hardware race, and the msleep would
reliably prevent it since the switch takes a finite amount of time
to do its job.  I've had to struggle with real heisenbugs, and this
doesn't have enough conflicting behaviors inside the silicon (or

So, you're unwilling to explore whether that suggestion addresses

As Alan noted: no can do, in general.  That's why I've not griped
harder at the distro vendors who are ignoring the fairly simple
recommendation that's been around for six years now:  load EHCI
before other USB controller drivers.  

Admittedly, until you turned up this glitch there was no downside


It will exit by the inner loop (with diagnostic) before it exits
from the outer one.  Then the hub logic and other code will give

The version I sent should just merge.

- Dave

-

[ message continues ]

There's actually no such thing as an "EHCI port" or an "OHCI port".
Instead, there's a set of ports, each of which can be switched so
the USB differential data signals go up to either controller.

When EHCI starts, that switch points to EHCI so that devices can try
enumerating with high speed signaling.  When a device doesn't respond
to that "chirp", the EHCI root hub driver switches the port to the

Yes, that's why I asked about EHCI.  My speculation would be that
OHCI starts the reset, and EHCI claims the port before it completes;
or contrariwise OHCI starts the reset right after EHCI claims it.

And there's some point in that process where a hardware race makes
the trouble you've observed.  I believe there are plenty of other
places where it's perfectly fine if EHCI grabs the port, or this
little race would have shown up many times before.

- Dave


-

[ message continues ]

Since we can't know which O/UHCI is paired with which EHCI, we can't
really have generic code to deal with that race, but maybe we can be
smart and basically mutex khubd activity such as port reset vs.
registration of any new HCD ?

I'm not even sure module load order is 100% fault proof here since khubd
spawns as a thread...
 
Ben.


-

[ message continues ]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>

I'm concerned about that as well, thanks for bringing it up.

My understanding, however, is that the critical thing is that the EHCI
device reset being done by the EHCI driver probe occurs and completes
first.  If that is true, then just making sure EHCI loads initially is
a sufficient constraint to fix this problem.
-

[ message continues ]

Yup, that would be, though I hate that sort of load order
dependencies...

Ben.


-

[ message continues ]

Don't need this "limit_1" timeout; "reset_done" handles all
the timeout needed there.  The regs->fmnumber is essentially

This is the loop that didn't terminate for you, right?
PORT_RESET_HW_MSEC is the ceiling you should use here,

What values do you see for "portstat"?

I suspect there will be some flag set which would allow a more
immediate exit from that loop.  RH_PS_CCS might clear, for example.

And in any case, if that fails I don't see any reason not to just
-

[ message continues ]

From: David Brownell <david-b@pacbell.net>

If the hardware hangs and the register stops incrementing,
the entire kernel will hang.  That is unacceptable.




Absolutely nothing clears in the register from it's initial value.

Here is the patch with the limit_2 initial value fixed.

I kept loop_1 in there, it is necessary.  No kernel code should
hang in an endless loop because of malfunctioning hardware.

Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/drivers/usb/host/ohci-hub.c b/drivers/usb/host/ohci-hub.c
index bb9cc59..9149593 100644
--- a/drivers/usb/host/ohci-hub.c
+++ b/drivers/usb/host/ohci-hub.c
@@ -563,14 +563,19 @@ static inline int root_port_reset (struct ohci_hcd *ohci, unsigned port)
 	u32	temp;
 	u16	now = ohci_readl(ohci, &ohci->regs->fmnumber);
 	u16	reset_done = now + PORT_RESET_MSEC;
+	int	limit_1;
 
 	/* build a "continuous enough" reset signal, with up to
 	 * 3msec gap between pulses.  scheduler HZ==100 must work;
 	 * this might need to be deadline-scheduled.
 	 */
-	do {
+	limit_1 = 100;
+	while (--limit_1 >= 0) {
+		int limit_2;
+
 		/* spin until any current reset finishes */
-		for (;;) {
+		limit_2 = PORT_RESET_HW_MSEC * 2;
+		while (--limit_2 >= 0) {
 			temp = ohci_readl (ohci, portstat);
 			/* handle e.g. CardBus eject */
 			if (temp == ~(u32)0)
@@ -579,6 +584,10 @@ static inline int root_port_reset (struct ohci_hcd *ohci, unsigned port)
 				break;
 			udelay (500);
 		}
+		if (limit_2 < 0) {
+			ohci_warn(ohci, "Root port inner-loop reset timeout, "
+				  "portstat[%08x]\n", temp);
+		}
 
 		if (!(temp & RH_PS_CCS))
 			break;
@@ -589,7 +598,14 @@ static inline int root_port_reset (struct ohci_hcd *ohci, unsigned port)
 		ohci_writel (ohci, RH_PS_PRS, portstat);
 		msleep(PORT_RESET_HW_MSEC);
 		now = ohci_readl(ohci, &ohci->regs->fmnumber);
-	} while (tick_before(now, reset_done));
+		if (!tick_before(now, reset_done))
+			break;
+	}
+	if (limit_1 < 0) {
+		ohci_warn(ohci, "Root port ...
[ message continues ]

That's unfortunately useless ... PPS, PRS, CCS (meaning powered,
resetting, connected).  In short, there is *no* indication from
the OHCI hardware that a disconnect happened.

-

[ message continues ]

From: David Brownell <david-b@pacbell.net>

But it's an excellent example of why my patch is mandatory.

When the device gets into a stuck state, this code does the wrong
thing.

Instead of trying to deal with bad situations, it hangs the system
instead.  That sucks for anyone trying to debug something like this.
This has caused weeks of debugging and grief for people, and it
could have all been eliminated if this looping code were more
robust to hardware failures.

If you can't see why this is bad programming practice in a hardware
driver, I will try to get my patch to someone who does.

Even though I'm severly overloaded, you asked me for a patch, I gave
you one.  And now you want to push it back at me because it isn't
clear to you yet that the code there right now is a huge problem.

And all of this is independant of making sure EHCI loads first,
we need to fix that too.
-

[ message continues ]

I agree.  David Brownell, unless you strongly object, I'll take David
Miller's patch and add it to my tree.

And if you do object, can you respond to the objection with a version of
the patch that you would accept?  :)

thanks,

greg k-h
-

[ message continues ]

I'll send some version with a signoff.

- Dave

-

[ message continues ]

And I got some questions as I reviewed it.  That's far
from un-heard-of.  One particular question related to
one potential simplification ... which unfortunately

You're reading things into what I wrote which were not there.
Calm down.

- Dave

-

[ message continues ]