Re: Lost IAA and EHCI reclaim problems

On Mon, 20 Aug 2007, David Brownell wrote:

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

What I would have done is update the hardware DMA pointers right away
in unlink_async(), and keep a pointer to the first entry on the reclaim
list added after IAAD was set.  Then when IAA arrives, you know that
every entry before that pointer has been fully unlinked and can be
safely removed.  If there are entries still remaining then set the
pointer to the qh currently at the end of the list and issue another
IAAD.

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

What about the fact that the !HCD_IS_RUNNING path in unlink_async() 
never updates the list and DMA pointers?  I suppose this hasn't caused 
any problems because every URB gets unlinked before the bus is 
suspended.  Even so, I wouldn't call it "obviously correct".

> > 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?  :)

To somebody with a sufficiently twisted sense of humor, it could
indicate almost anything...

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

That's a good idea; I'll add it in.


> > 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 true for QHs which were unlinked at some point.  But the case I
mentioned was a QH which stops because of an error.  You (and the
code!) seem to be saying that after qh_completions() gives back the URB
with the error, the QH will be unlinked and eventually
end_unlink_async() will relink it.  Instead why not simply restart the
QH at the end of qh_completions(), with no extra 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.

Okay, if you say so.  The corresponding code in uhci-q.c isn't very
complex, and most of it is there to handle issues of toggle updates and
FSBR -- neither of which matters for EHCI.

It's worth pointing out that the iaa_watchdog() -> end_unlink_async()  
-> qh_completions() path manages to ignore ehci->scanning, as does the
ehci_irq() -> IAA -> end_unlink_async() path.  That's another "not
obviously correct" thing to worry about.

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

It wouldn't be surprising to find the same thing happening with 
uhci-hcd.  I've never looked for it, though.

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

IMO the hardest part has to have been dealing with split transactions.  
Look how long section 4.12 is!  At 25 pages, it's easily the most
complex part of the spec.

Alan Stern


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