Re: Lost IAA and EHCI reclaim problems

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.  From
there qhs go one at a time into QH_STATE_UNLINK.  Not how I would have
done it, but okay.

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?


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

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


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

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

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