Dave:
Can you help with this issue? Do you remember why the code below in
qh_completions() sets the HALT bit in the QH overlay region? The
comment doesn't seem to be relevant any more.
/* force halt for unlinked or blocked qh, so we'll
* patch the qh later and so that completions can't
* activate it while we "know" it's stopped.
*/
if ((halt & hw->hw_token) == 0) {
halt:
hw->hw_token |= halt;
wmb ();
}
Can we eliminate these statements? The fact that HALT is set in
hw_token won't prevent the driver from doing anything: Contrary to
what the comment says, whether or not the QH gets "patched" or
reactivated has nothing to do with the HALT bit.
In fact, there are circumstances in which the QH gets reactivated
_without_ being updated (i.e., when an URB is unlinked before the
hardware starts executing it. After the URB is given back the queue
picks up right where it left off, so qh_refresh() doesn't call
qh_update()). In this case setting the HALT bit would be a definite
mistake!
In fact, maybe that mistake is the reason for Andiry's problem...
On Mon, 14 Mar 2011, Xu, Andiry wrote:
> > Furthermore, these statements get executed in only three ways:
> >
> > The QH is unlinked. In this case the hardware should never
> > see the QH so it shouldn't be confused by it.
> >
> > The QH is halted because an earlier qTD terminated with an
> > error. In this case the Active bit should already be turned
> > off, so turning on the Halt bit shouldn't hurt.
> >
> > A short read occurred. Is this the case that's bothering you?
> >
> > Now, I have to admit that I don't remember why this code sets the Halt
> > bit. Maybe it doesn't have to be done at all; I'm not sure.
> The qh state is QH_STATE_IDLE when the driver set the Halt bit. No error
> or short read occurs. In this case HW should not see this qh, right?
Right. QH_STATE_IDLE means the QH is no longer on the async list _and_
an IAA interrupt has occurred since it was removed.
If there was no error and no short read, then this statement must have
been executed because an URB was unlinked. Is that what happened?
> I've verified clear either the Active bit or the Halt bit in this case
> can workaround this issue.
>
> I've found that when disable async qh/qtd cache (a HW feature on our
> platform), the issue disappears. Seems the HW caches the idle qh and be
> able to see it.
That really sounds like a bug in the hardware design. The HW should
_never_ cache QHs that have been fully removed from the async list.
In fact, that's exactly what the IAA interrupt is supposed to mean --
it means that any QHs removed from the async list before the IAAD bit
was set are no longer present in the HW cache. See section 4.8.2 in
the EHCI spec.
> Can I propose a workaround, do not set halt bit if it's an affected
> platform and Active bit is set in this case?
We may be able to avoid setting the halt bit entirely. Let's see what
Dave says.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
