On Mon, 29 Jun 2009, David Brownell wrote: > On Monday 29 June 2009, Alan Stern wrote: > > This patch (as1257) revises the way ehci-hcd detects STALLs. The > > logic has to be a little peculiar, because there's no hardware status > > bit specifically meant to indicate a STALL. You just have to guess > > that a STALL was received if none of the fatal error bits are set and > > the transfer stopped before all its retries were used up. The > > existing code doesn't quite do this, and its logic is more tortuous > > than necessary. > > The existing code was according to "private communication" > from the EHCI spec author, FWIW ... If only we all had such reliable sources! :-) Was any justification provided for the code sequence? > and this change makes > sure STALL is *NEVER* reported. Which can't be good, and > surely will break something. Not true at all. STALL will be reported whenever: HALT is set and BABBLE isn't, and the error counter hasn't reached 0. These are the appropriate tests. Certainly we want HALT to be set; otherwise the endpoint queue is still running. If BABBLE is set then the device tried to send too much data -- i.e., it didn't set a STALL -- and the controller won't retry the transaction. So again we shouldn't return -EPIPE. All the other types of errors _are_ retried. So if there isn't a STALL but some other error(s), the endpoint won't halt until the error counter reaches 0, in which case we don't return -EPIPE. The only remaining case is that the endpoint is halted and the error counter is still positive. The only way this can happen is if we get BABBLE or STALL. Ergo the new logic is correct. Besides, I have tested the new code. It works. > Is there some specific problem with the existing code? Yes. Suppose the first time the transaction is attempted, the controller gets a Missed MicroFrame (MMF) or Data Buffer Error (DBE). The corresonding flag will be set (and will remain set!) and the transaction will be retried. Suppose the retry encounters a STALL. Then the proper return value is -EPIPE. But the existing code tests MMF and DBE before looking for stalls. Hence it will return -EPROTO, -ENOSR, or -ECOMM instead of -EPIPE. This is a bug. 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
