On Tue, 10 Dec 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. > > The issue has been reproduced with: > > CPU: IMX6D5EYM10AD or MCIMX6D5EYM10AE. > SW: U-Boot 2019.07 and kernel 4.19.40. > > CPU: HP Proliant Microserver Gen8. > SW: Linux version 4.2.3-300.fc23.x86_64 > > In this situation there will be error bit for MMF active yet the > CERR equals EHCI_TUNE_CERR + halt. Existing implementation > interprets this as a stall [1] (chapter 8.4.5). > > The possible conditions when the MMF will be active + halt > can be found from [2] (Table 4-13). > > Fix for the issue is to check whether MMF is active and PID Code is > IN before checking for the stall. If these conditions are true then > it is not a stall. > > What happens after the fix is that when disconnecting a hub with > attached device(s) the situation is not interpret as a stall. > > [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/technical-specifications/ehci-specification-for-usb.pdf > > Signed-off-by: Erkka Talvitie <erkka.talvitie@xxxxxxxxx> > --- Basically good, but you should always run patches through the scripts/checkpatch.pl script before sending them. There are several places where the formatting needs to be fixed. > drivers/usb/host/ehci-q.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c > index 3276304..285622d 100644 > --- a/drivers/usb/host/ehci-q.c > +++ b/drivers/usb/host/ehci-q.c > @@ -27,6 +27,10 @@ > > /*-------------------------------------------------------------------------*/ > > +/* PID Codes that are used here, from EHCI specification, Table 3-16. */ > +#define PID_CODE_IN 1 > +#define PID_CODE_SETUP 2 > + > /* fill a qtd, returning how much of the buffer we were able to queue up */ > > static int > @@ -190,7 +194,7 @@ static int qtd_copy_status ( > int status = -EINPROGRESS; > > /* count IN/OUT bytes, not SETUP (even short packets) */ > - if (likely (QTD_PID (token) != 2)) > + if (likely (QTD_PID (token) != PID_CODE_SETUP)) This should be "QTD_PID(token)" with no extra space before the left paren, and similarly for "likely(". I realize you just kept the code the way it already was, but we prefer to fix formatting errors like these whenever the line gets changed, even if it's for a different reason. > urb->actual_length += length - QTD_LENGTH (token); > > /* don't modify error codes */ > @@ -206,6 +210,11 @@ static int qtd_copy_status ( > if (token & QTD_STS_BABBLE) { > /* FIXME "must" disable babbling device's port too */ > status = -EOVERFLOW; > + /* When MMF is active and PID Code is IN, queue is halted. > + * EHCI Specification, Table 4-13. > + */ Multi-line comments should be formatted like thus: /* * When MMF... * EHCI ... */ > + } else if((token & QTD_STS_MMF) && (QTD_PID(token) == PID_CODE_IN)) { Try to avoid letting code extend beyond column 80 (for example, you could beak the line following the "&&"). Also, there should be a space between the "if" and the left paren -- the "if" isn't a function call! > + status = -EPROTO; > /* CERR nonzero + halt --> stall */ > } else if (QTD_CERR(token)) { > status = -EPIPE; When you fix up these minor issues and resubmit, you can add: Reviewed-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> Alan Stern
