Re: Lost IAA and EHCI reclaim problems

On Monday 20 August 2007, Alan Stern wrote:
> On Sun, 19 Aug 2007, David Brownell wrote:
> 
> > On Sunday 19 August 2007, Alan Stern wrote:
> > > Dave:
> > > 
> > > I just read through your old iaa_watchdog patch, and there are several 
> > > things about it (maybe about the pre-patch code too) that seem strange.
> > > 
> > > Management of the reclaim list is wrong.  Every qh that was added 
> > > before the IAA doorbell bit was set should get unlinked when 
> > > end_unlink_async() runs, whereas qhs added after the IAAD bit should 
> > > cause another IAA cycle.  However end_unlink_async() never changes
> > > more than one qh's state to QH_STATE_IDLE.
> > 
> > The pessimistic assumption is that only one QH will be removed per
> > doorbell.  I don't see how that would be "wrong"...
> 
> I see.  You've got this funny QH_STATE_UNLINK_WAIT, meaning that
> someone wants to unlink the qh but the IAAD is already in use.  

Sort of.  It's not "funny" since it's inherent in the problem;
there's at least a race between arrival of IAA and the actual
dequeue.  No matter how many you unlink at once, you can never
guarantee that you dequeue *before* IAA arrives ... so there
will always be two types of pending unink state:  the kind where
IAA completes the unlink, and the kind where it doesn't.


> From 
> there qhs go one at a time into QH_STATE_UNLINK.  Not how I would have
> done it, but okay.

"Obviously correct" has some advantages, and speed of unlinking
has never been been observed as a problem.  :)


> BTW, what's the funny test for qh == ehci->async doing there in 
> start_unlink_async()?  Why would you ever want to unlink the dummy qh 
> pointed to by ehci->async?

>From reading the code, you saw that unlinking that ehci->async
is how the whole async schedule gets stopped.  What else should
unlinking the very last QH indicate?  :)


> > > The iaa_watchdog routine seems to assume that STS_IAA gets set properly
> > > and the interrupt is never delivered.
> > 
> > That is, assumes that the hardware behaves correctly.
> 
> Apart from not delivering the interrupt.  But it seems strange for a 
> watchdog routine to assume that the hardware behaves correctly; after 
> all, the whole point of that code is to recover from hardware bugs!

Recover from *known* bugs.  If we start guessing about random
ways the hardware could flake, there would be no end to the odd
failure modes we'd have to defend against.

The *known* issue is IAA getting lost -- sometimes -- on VIA.

 
> > > Isn't it likely that some controllers may fail to set STS_IAA at all?
> > 
> > I wouldn't think so; but of course if you *knew* of such a controller
> > it wouldn't be likely ... it'd be a certainty!
> 
> The attachment in comment #12 of Bugzilla #8692 shows IAAD set, IAA 
> clear, and lots of qhs in the reclaim list.  QED  :-)

Sounds plausible ... Another theory might be that IAAD needs to
be cleared before it gets set again.
 

> > > Here's a somewhat unrelated question that came up earlier:  When an
> > > async transfer stops because of an error the endpoint queue has to be
> > > restarted, presumably by setting qh->hw_current to point to the first
> > > qtd in the next URB.  But the driver never does this.  How does a
> > > stopped queue get restarted?  Does it go through an unlink cycle first?
> > 
> > It's been a long time since I touched that code, so details are fuzzy...
> > 
> > But the basic notion is that there's only one routine scanning a QH and
> > removing TDs from it -- qh_completions() is its name.  This ensures that
> > there are no little races of the type ISTR the original OHCI code suffered
> > from, with one routine fighting another.
> > 
> > A comment at the top of qh_completions() says that it leaves HALT (error)
> > status set until the queue is completely scrubbed.  A QH that's halted like
> > that, or else is in QH_STATE_IDLE, can safely be scanned and modified even
> > after the hardware's view of the queue head.  Otherwise the TD queue only
> > get modified by removing completed TDs (before the head) or by adding new
> > ones (after the tail).
> > 
> > A QH that's getting that "scrub everything" treatment should always get
> > reactivated using qh_refresh(), and thence qh_update().  And if it's
> > in that mode, the qh_completions() loop doesn't stop when it gets to
> > an active TD ... it keeps scanning to the end of the list, processing
> > pending unlinks (in queue order, once).
> > 
> > Now, I seem to recall that there were two refresh modes.  One was where
> > the QH overlay was partially completed ... e.g. TD at list head was
> > for 8 packets, and 3 of them were processed already, but an URB after
> > this one got unlinked.  In that case the important part is clearing the
> > overlay bits that said "be a NOP for now".  The other mode was where
> > that overlay was no longer valid, in which case it invalidates the
> > overlay so that the next QTD is read.  Both require careful attention
> > to the text in 4.10 regarding operation of queue heads.
> > 
> > Now, in the error case you asked about, the overlay became invalid.
> > 
> > So 4.10.2 needs to come into play ... it ignores hw_current when both
> > ACTIVE and HALT bits are cleared.  That's ensured by qh_update(); at
> > the very end it scrubs those bits.  Which makes the HC fetch from
> > the qh->hw_next field, and then normally process that QTD and update
> > the overlay.  Tadaa!
> 
> But exactly where is the relevant call to qh_refresh()?  There's a 
> comment near the end of qh_completions() which says:
> 
> 	/* restore original state; caller must unlink or relink */
> 
> If stopped != 0 and state == QH_STATE_LINKED, as they would be in this 
> case, the code goes on to call unlink_async().  And there's nothing in 
> scan_async() -- the caller -- to relink qh.

Notice how end_unlink_async() handles the case of completing an
unlink for a QH with a non-empty queue ... it relinks.  And
that's where this QH will get refreshed.

That's actually a moderately common non-fault scenario for code
that's using the synchronous calls, e.g. for control messages
the queue empties, but a new control message gets issued pretty
quickly at least during enumeration.  Sometimes before the QH
starts to unlink; sometimes not.

The fun bit:  after an empty QH starts to unlink, some URBs can
be submitted to it ... that happens surprisingly often.  The
real surprise is how frequently some of those new TDs complete
before the unlink!

 
> Doesn't this mean that qh really does end up going through an
> unnecessary unlink cycle?

The alternative would be to have a special/racy/error-prone code
path just for that uncommon cleanup-after-fault case.  The way it
is now, that's morphed into more common code paths which get used
quite often, and hence are more solid.  That seems safest to me.

The lowlevel QH manipulation was tricky to get right.  I think
it's basically correct now, but part of getting that far was to
minimize the number of code paths which are racing with the HC,
and which break given silicon bugs with respect to QH updating.

- Dave

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel