Re: USB OOPS 2.6.25-rc2-git1

[David Brownell <david-b@pacbell.net>]

On Wednesday 05 March 2008, Alan Stern wrote:
> On Tue, 4 Mar 2008, David Brownell wrote:
> 
> > How does the appended patch look?
> 
> It looks very good.  Do you think there should be an "else" clause for 
> the "if ((status & STS_IAA) || !(cmd & CMD_IAAD))" test?  That's the 
> pathway one would observe with a controller that implements IAA very 
> slowly or not at all.  There doesn't seem to be anything more the HCD 
> can do about it, but you could print a log message.

It's already chatty enough, IMO.  :)


> 
> > > Given sufficiently bizarre hardware we can't be 
> > > certain that things won't still go wrong on occasion, but this is the
> > > best we can do for now -- weird hardware can be handled as it arises.
> > 
> > The appended patch does include a bit of paranoia around IAA and IAAD;
> > I figure it can't hurt, although at this point I have no particular
> > reason to believe anyone except VIA has bugs in those areas.
> 
> There's still Bugzilla #8692.  That one appears to be an individual
> hardware failure, though, not a systematic bug.

Maybe; I noticed the "IAAD wasn't clear" message, but that should
actually have been tested earlier (before the completions fired).
So I'm not sure I trust it.


> > Yeah, that seems like a better place to do it.  All the other callers
> > guarantee ehci->reclaim is non-null before calling it.  The fact that
> > it happens in this case suggests IAAD and/or IAAD didn't get cleared
> > properly.
> 
> There is one place where ehci-hcd.c doesn't make that guarantee:

... which is pretty wierd anyway.


> @@ -757,7 +757,7 @@ static void unlink_async (struct ehci_hcd *eh
>  static void unlink_async (struct ehci_hcd *ehci, struct ehci_qh *qh)
>  {
>  	/* failfast */
> -	if (!HC_IS_RUNNING(ehci_to_hcd(ehci)->state))
> +	if (!HC_IS_RUNNING(ehci_to_hcd(ehci)->state) && ehci->reclaim)
>  		end_unlink_async(ehci);
>  
>  	/* if it's not linked then there's nothing to do */
> 
> But if you take out the WARN_ON at the start of end_unlink_async then 
> this isn't needed.

Right, that's gone.

- Dave