Thanks for the review. > -----Original Message----- > From: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> > Sent: tiistai 10. joulukuuta 2019 17.12 > To: Erkka Talvitie <erkka.talvitie@xxxxxxxxx> > Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; > claus.baumgartner@xxxxxxxxxx > Subject: Re: [PATCH] USB: EHCI: Do not return -EPIPE when hub is > disconnected > > 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/techn > ical > > -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. Ok, I will do that. > > > 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. Sure, I will fix those. > > > 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 ... > */ I will fix this. > > > + } 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! This is a strange thing since in my editor there is a margin line visible with 80 characters wide. And this line is inside limits, actually my editor states that the line is 77 chars long. That said, I will break the line from the && and fix the if. > > > + 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> Thank you! > > Alan Stern Erkka Talvitie
