RE: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is disconnected

["Erkka Talvitie" <erkka.talvitie@vincit.fi>]

> -----Original Message-----
> From: Erkka Talvitie <erkka.talvitie@vincit.fi>
> Sent: keskiviikko 4. joulukuuta 2019 16.38
> To: 'Alan Stern' <stern@rowland.harvard.edu>
> Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org;
> claus.baumgartner@med.ge.com
> Subject: RE: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is
> disconnected
> 
> 
> 
> > -----Original Message-----
> > From: Alan Stern <stern@rowland.harvard.edu>
> > Sent: keskiviikko 4. joulukuuta 2019 16.24
> > To: Erkka Talvitie <erkka.talvitie@vincit.fi>
> > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org;
> > claus.baumgartner@med.ge.com
> > Subject: RE: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is
> > disconnected
> >
> > On Wed, 4 Dec 2019, Erkka Talvitie wrote:
> >
> > > > > So if CERR == EHCI_TUNE_CERR and the QTD_PID != 1 (not IN) then
> > > > > we should return -EPIPE, as the existing code does.  But if
> > > > > QTD_PID == 1 then the code should continue, as your patch does
> > > > > -- with one
> > > > > difference: Put the additional check for EHCI_TUNE_CERR between
> > > > > the tests for DBE and XACT instead of after XACT (because XACT
> > > > > would decrement CERR whereas DBE wouldn't).
> > > >
> > > > Good point, I agree.
> > > >
> > > > >
> > > > > Can you make that change and test it?
> > > >
> > > > Sure, I have made the change and test it as soon as possible.
> > >
> > > I am not actually totally sure if I understood you correctly, but I
> tested a
> > change where the first stall check is like this (PID_CODE_IN is
> > defined as
> 1):
> > >
> > > -               } else if (QTD_CERR(token)) {
> > > +               } else if (QTD_CERR(token) && (QTD_PID (token) !=
> > > + PID_CODE_IN)) {
> > >                         status = -EPIPE;
> > >
> > > And the second stall check (now between DBE and XACT):
> > > +               } else if (QTD_CERR(token)) {
> > > +                       status = -EPIPE;
> > >
> > > Is this what you meant? Please correct me if I am wrong.
> >
> > Actually, what I meant for the first part was:
> >
> > 		} else if (QTD_CERR(token) &&
> > 				(QTD_CERR(token)
> > < EHCI_TUNE_CERR ||
> > 				 QTD_PID(token) !=
> > PID_CODE_IN)) {
> 
> Ok, now I understand the change. Good.

I tested this change and the issue did not reproduce.

However when I was writing the comments for the patch I started to think
what happens in this following scenario:

The PID Code is IN.

1. First there will be XACT, the CERR is decremented, let's say from 3 to 2
and the host controller executes a retry.
2. On this next try there will happen the condition mentioned in the Table
4-13 of the EHCI specification so that the MMF is set and the queue is
halted (because it is IN).
3. To my understanding now the execution enters to this first stall check
if, as CERR is > 0 and CERR < EHCI_TUNE_CERR.
4. The -EPIPE (stall) is returned when actually the queue was halted due to
MMF - not stall - and the -EPROTO should be returned.

Is my logic correct or am I missing something?

If you agree with me then I would like to present you a bit more bold (in a
sense of amount of refactoring) RFC. In high level this another RFC
separates 1. error check and 2. stall check. For me this approach is a bit
easier to understand from the code. Or then please  propose another
solution.

If you do not agree with this scenario then never mind the above suggestion
about RFC doing more refactoring.

> 
> >
> > And of course there should be a comment before this line, explaining
> > what
> it
> > does.  By the way, the accepted format for multi-line comments
> > is:
> >
> > 		/*
> > 		 * Blah blah blah
> > 		 * Blah blah blah
> > 		 */
> >
> 
> Thanks for the information. I noticed that my comments were different than
> the existing ones in the file and I was already about to change my
comments
> to match the existing style.
> 
> > The second part of the patch looks okay (but again, with a comment).
> 
> Yes, I will add the comment here also.
> 
> >
> > > Anyways with these changes the issue does not reproduce.
> >
> > Very good.
> 
> I will do the changes and re-test.
> 
> >
> > Alan Stern
> 
> Erkka Talvitie

Erkka Talvitie