> -----Original Message----- > From: Erkka Talvitie <erkka.talvitie@xxxxxxxxx> > Sent: keskiviikko 11. joulukuuta 2019 9.04 > To: 'Alan Stern' <stern@xxxxxxxxxxxxxxxxxxx> > Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; > claus.baumgartner@xxxxxxxxxx > Subject: RE: [PATCH] USB: EHCI: Do not return -EPIPE when hub is > disconnected > > 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. Nice tool! I tested it with my old patch and indeed it revealed the issues. Now with the new patch 0 warnings or errors. Thanks for the tip! > > > > > > 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
