> -----Original Message----- > From: Alan Stern [mailto:stern@xxxxxxxxxxxxxxxxxxx] > Sent: Monday, March 14, 2011 11:21 PM > To: David Brownell; Xu, Andiry > Cc: USB list > Subject: RE: Question about HALT bit set of qTD in EHCI driver > > 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? > Yes. The URB is unlinked. > > 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. > That's only my assumption. At this time we're still looking for root cause and can not say it's a HW bug. The HW does fetch a qh overlay with Active and Halt bit set and got confused when trying to process it, we want to figure out why it's exposed to HW. > > 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. > OK, thanks. Thanks, Andiry -- 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
