On Monday 29 June 2009, Alan Stern wrote: > 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! :-) Yeah, I think they were rather keen on getting an EHCI implementation out there in wide use. Recall this was well before MS-Windows included EHCI support. (They had drivers, but nothing was ready for bundling yet.) > Was any justification provided for the code sequence? It was matching a decision tree in a table, which I think I saw in two versions: ASCII, and later some kind of two PDF formatted appnote (which I may not have cross-checked against the then-current code). That table wasn't what I would call crystal-clear though. > > 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. Yes that's one appropriate case. The original code set status to -EPIPE (stall) in exactly two spots. But your patch removes both of them, reporting -EPROTO in those cases. So I fail to see how this is *NOT* removing all the existing logic that reports STALL ... > > 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. Sounds like it. Such explanations would go well in the patch description ... But that suggests the right fix for the problem is just to look for that particular flavor of STALL reports *first*, rather than remove all the existing logic to do that. - Dave -- 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
