> -----Original Message----- > From: Erkka Talvitie <erkka.talvitie@xxxxxxxxx> > Sent: tiistai 3. joulukuuta 2019 12.54 > To: 'Alan Stern' <stern@xxxxxxxxxxxxxxxxxxx> > Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; > claus.baumgartner@xxxxxxxxxx > Subject: RE: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is > disconnected > > > -----Original Message----- > > From: Erkka Talvitie <erkka.talvitie@xxxxxxxxx> > > Sent: tiistai 3. joulukuuta 2019 11.39 > > To: 'Alan Stern' <stern@xxxxxxxxxxxxxxxxxxx> > > Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; > > claus.baumgartner@xxxxxxxxxx > > Subject: RE: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is > > disconnected > > > > Thank you for the comments. > > > > > -----Original Message----- > > > From: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> > > > Sent: maanantai 2. joulukuuta 2019 21.44 > > > To: Erkka Talvitie <erkka.talvitie@xxxxxxxxx> > > > Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; > > > claus.baumgartner@xxxxxxxxxx > > > Subject: Re: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is > > > disconnected > > > > > > On Fri, 29 Nov 2019, Erkka Talvitie wrote: > > > > > > > When disconnecting a USB hub that has some child device(s) > > > > connected to it (such as a USB mouse), then the stack tries to > > > > clear halt and reset device(s) which are _already_ physically > disconnected. > > > > > > That behavior is understandable. The kernel doesn't know that the > > > device has been disconnected until it can process the notification > > > from an > > upstream > > > hub, and it can't process that notification while it's trying to > > > reset the > > device. > > > > > > > Ok. I was thinking that in this use case , it should not be trying to > clear the halt > > (and reset the device when the clear halt fails). And this behavior > > was > altered > > by this RFC. > > > > > > The issue has been reproduced with: > > > > > > > > CPU: IMX6D5EYM10AD or MCIMX6D5EYM10AE. > > > > SW: U-Boot 2019.07 and kernel 4.19.40. > > > > > > > > In this situation there will be error bit for MMF active yet the > > > > CERR equals EHCI_TUNE_CERR + halt. > > > > > > Why? In general, setting the MMF bit does not cause the halt bit to > > > be > > set > > > (set Table 4-13 in the EHCI spec). In fact, MMF refers to errors > > > that > > occur on > > > the host, not bus errors caused by a disconnected device. > > > > I do not know for sure why that happens. I was suspecting that there > > has been MMF error and a stall at the same time. And in this RFC it > > was > assumed > > that MMF is with greater priority than stall. > > The disconnecting of a hub with attached devices cause the MMF error > > bit set even though it is a host side error. > > > > > > > > > Existing implementation > > > > interprets this as a stall [1] (chapter 8.4.5). > > > > > > That is the correct thing to do. When a transaction error occurs > > > during a Complete-Split transaction, the host controller is supposed > > > to decrement > > the > > > CERR value, set the XACT bit, and retry the transaction unless the > > > CERR > > value > > > is 0 or there isn't enough time left in the microframe. > > > > > > The fact that you saw CERR equal to EHCI_TUNE_CERR and XACT clear > > > probably means that your EHCI hardware is not behaving properly. > > > > If you refer to the XactErr bit (Table 4-13 [2] )with the "XACT clear" > then > > unfortunately I did not check it's state ,so I am not sure if it was > clear. > > In this patch, like also in the existing implementation, the MMF bit > > is > checked > > first and since it is active in this situation the XactErr is not checked. > > > > I will check this. > > > > But as in this use case the CERR has not been decreased yet there is > > error > bit > > active (MMF) do you see it is still correct to interpret it as a stall > (even when > > the halt bit is set)? > > > > I have tried to find out more details about our EHCI controller > > version, > but I > > have only found out those CPU versions. It might help in a search > > whether this could be a HW issue. > > > > > > > > > Fix for the issue is at first to check for a stall that comes > > > > after an error (the CERR has been decreased). > > > > > > > > Then after that, check for other errors. > > > > > > > > And at last check for stall without other errors (the CERR equals > > > > EHCI_TUNE_CERR as stall does not decrease the CERR [2] (table 3-16)). > > > > > > > > What happens after the fix is that when disconnecting a hub with > > > > attached device(s) the situation is not interpret as a stall. > > > > > > > > The specification [2] is not clear about error priorities, but > > > > since there is no explicit error bit for the stall, it is assumed > > > > to be lower priority than other errors. > > > > > > On the contrary, the specification is very clear. Since transaction > > errors cause > > > CERR to be decremented until it reaches 0, a nonzero value for CERR > > > means the endpoint was halted for some other reason. And the only > > > other reason is a stall. (Or end of the microframe, but there's no > > > way to tell if that > > > happened.) > > > > I see your point. EHCI specification states that babble is a serious > > error > and it > > will also cause the halt. The babble error bit is checked first. But > > the specification does not say about order of the other errors or > > about what > to > > do if there is an error, no retries executed, yet a halt (stall). For > example > > should the XactErr be checked before the MMF. > > > > >(Or end of the microframe, but there's no way to tell if that > > >happened.) > > > > I was not able to locate this from the specification. Could you please > point > > out where this statement is from? > > Could the way to tell if "end of microframe" happened, be what is done > here > > - check for MMF error bit and if CERR has not been decreased? > > > > > > > > > [1] https://www.usb.org/document-library/usb-20-specification, > > > > usb_20.pdf [2] > > > > > > > > > > https://www.intel.com/content/dam/www/public/us/en/documents/techn > > > ical > > > > -specifications/ehci-specification-for-usb.pdf > > > > > > > > Signed-off-by: Erkka Talvitie <erkka.talvitie@xxxxxxxxx> > > > > > > Can you duplicate this behavior on a standard PC, say with an Intel > > > EHCI controller? > > > > We tested with native Linux PC and the error did not reproduce. > > However I am not sure about the used host controller in that PC. > > I will check that or try to get a setup with Intel EHCI. > > The PC where the issue did not reproduce was ThinkPad T480 with: > > 3c:00.0 USB controller: Intel Corporation Device 15c1 (rev 01) > > [ 4.229310] xhci_hcd 0000:3c:00.0: xHCI Host Controller > [ 4.238578] ehci_hcd: USB 2.0 'Enhanced' Host Controller (EHCI) Driver > [ 4.239754] ohci_hcd: USB 1.1 'Open' Host Controller (OHCI) Driver > [ 4.240857] ehci-pci: EHCI PCI platform driver > [ 4.241437] ohci-pci: OHCI PCI platform driver > [ 4.243080] uhci_hcd: USB Universal Host Controller Interface driver > Another test, HP Proliant Microserver Gen8 Linux version 4.2.3-300.fc23.x86_64 ehci_hcd: USB 2.0 'Enhanced' Host Controller (EHCI) Driver ehci-pci: EHCI PCI platform driver With this setup the behavior reproduces. kernel: usb 3-1.2: clear tt 1 (0080) error -71 kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: Cannot enable. Maybe the USB cable is bad? kernel: usb 3-1.2-port1: cannot disable (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: Cannot enable. Maybe the USB cable is bad? kernel: usb 3-1.2-port1: cannot disable (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: Cannot enable. Maybe the USB cable is bad? kernel: usb 3-1.2-port1: cannot disable (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: Cannot enable. Maybe the USB cable is bad? kernel: usb 3-1.2-port1: cannot disable (err = -71) kernel: usb 3-1.2-port1: cannot disable (err = -71) kernel: hub 3-1.2:1.0: hub_port_status failed (err = -71) > > > > > > > > > drivers/usb/host/ehci-q.c | 9 +++++++-- > > > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c > > > > index 3276304..da7fd12 100644 > > > > --- a/drivers/usb/host/ehci-q.c > > > > +++ b/drivers/usb/host/ehci-q.c > > > > @@ -206,8 +206,9 @@ static int qtd_copy_status ( > > > > if (token & QTD_STS_BABBLE) { > > > > /* FIXME "must" disable babbling > > > device's port too */ > > > > status = -EOVERFLOW; > > > > - /* CERR nonzero + halt --> stall */ > > > > - } else if (QTD_CERR(token)) { > > > > + /* CERR nonzero and less than > > > EHCI_TUNE_CERR + halt --> stall. > > > > + This handles situation where stall comes after > > > an error. */ > > > > > > This comment doesn't make sense. Who cares whether a stall comes > > > after an error or not? It's still a stall and should be reported. > > > > This was basically a comment trying to answer to this commit: > > ba516de332c0 USB: EHCI: check for STALL before other errors > > > > "The existing code doesn't do this properly, because it tests for MMF > > (Missed MicroFrame) and DBE (Data Buffer Error) before testing the > > retry counter. Thus, if a transaction gets either MMF or DBE the > > corresponding flag is set and the transaction is retried. If the > > second attempt receives a STALL then -EPIPE is the correct return > > value. But the existing code would see the MMF or DBE flag instead > > and return -EPROTO, -ENOSR, or -ECOMM." > > > > The comment tries to explain that it will not revert the fix made in > > the commit ba516de332c0. > > > > > > > > > > > + } else if (QTD_CERR(token) && > > > QTD_CERR(token) < EHCI_TUNE_CERR) { > > > > status = -EPIPE; > > > > > > If an error occurs and the transaction is retried and the retry gets > > > a > > stall, then > > > the final status should be -EPIPE, not something else. > > > > This is how the RFC also works. If the transaction has been retried > > and > gets > > stall then -EPIPE is returned. > > Or if there are no errors but there is a stall then -EPIPE is returned. > > > > The only difference in this patch in comparison to the existing > > implementation is that if there is an error but the transaction has > > not > been > > retried it is not interpret as a stall even if there is a halt. > > > > > > > > > /* In theory, more than one of the following bits > > > can be set @@ > > > > -228,6 +229,10 @@ static int qtd_copy_status ( > > > > > > > usb_pipeendpoint(urb->pipe), > > > > usb_pipein(urb- > > > >pipe) ? "in" : "out"); > > > > status = -EPROTO; > > > > + /* CERR equals EHCI_TUNE_CERR, no other > > > errors + halt --> stall. > > > > + This handles situation where stall comes > > > without error bits set. > > > > +*/ > > > > > > If CERR is equal to EHCI_TUNE_CERR then no other errors could have > > > occurred (since any error will decrement CERR). So why shouldn't > > > this > > case > > > be included with the earlier case? > > > > That is what I also understood from the EHCI specification. If there > > is an > error > > the CERR should decrease. Only babble, data buffer error and stall (or > > no > > error) will not decrement the CERR. > > However in this use case there is an error (MMF) but the CERR still > > equals > to > > the EHCI_TUNE_CERR. > > > > So that's why the RFC separates these. This is the logic in the RFC: > > > > 1. The first if handles the situation where the stall comes after > > there > has > > been an error AND a retry. CERR has been decreased. This is so that > > ba516de332c0 is not reverted. > > 2. The second if handles the situation where the halt has been caused > > by > the > > stall AND there are no other errors. > > 3. If there are errors + halt, but no retries executed (CERR equals > > EHCI_TUNE_CERR) the response here is to return error value according > > to the error bit, not returning EPIPE according to the stall. > > I am using CERR here confusingly. It is not a retry counter, instead it is an > error counter. > > > > > > > > > > + } else if (QTD_CERR(token)) { > > > > + status = -EPIPE; > > > > } else { /* unknown */ > > > > status = -EPROTO; > > > > } > > > > > > Alan Stern > > > > Erkka Talvitie > > > > Erkka Talvitie > Erkka Talvitie
