Re: Lost IAA and EHCI reclaim problems

On Tuesday 21 August 2007, Alan Stern wrote:

> What I would have done is ...

So might I have done, if I had more time to spend on
such issues back then.  Got patch?  :)


Plus, other complications creep in when you try fancier
"gang unlink" strategies.  I'd hardly deny that code
couldn't be improved, even apart from fixing whatever
bug is lurking there ... but simplicity has many virtues.


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

Given that no URBs should be linked on a suspended bus, the
most you're arguing about there is how obvious that is.  And
once you start bringing in the rest of the driver, it's hard
to say that *anything* is obvious...


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

Because updating a live QH is *VERY* racey and is affected by some
hardware bugs/quirks.  It can be done, but because of the overlay thing
it's not straightforward even without the quirks.  One of the strategies
for minimizing problems there was to minimize the number of places such
updates get done; earlier versions had a few more locations for updates,
and that had proved troublesome.


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

In UHCI the QH is very simple.  No overlay to get out of sync
with the QTD it mirrors, for example, and no quirks where some
silicon doesn't manage that overlay correctly.


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

The basic observation is that "scanning" affects nodes which
are on the schedule, and a QH being unlinked isn't on it.

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

I didn't look for it.  It looked for me and mugged 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.
> 
> 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.

I don't trust the split transaction support as much as I trust
the basic QH manipulation though.  Especially periodic splits;
the async ones are straightforward, at least unless they get an
MMf error.

- 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