All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alan Stern <stern@rowland.harvard.edu>
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
Date: Wed, 4 Dec 2019 09:24:12 -0500 (EST)	[thread overview]
Message-ID: <Pine.LNX.4.44L0.1912040917500.3941-100000@netrider.rowland.org> (raw)
In-Reply-To: <020901d5aaa5$415457f0$c3fd07d0$@vincit.fi>

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)) {

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
		 */

The second part of the patch looks okay (but again, with a comment).

> Anyways with these changes the issue does not reproduce.

Very good.

Alan Stern


  reply	other threads:[~2019-12-04 14:24 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-29 14:08 [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is disconnected Erkka Talvitie
2019-12-02 19:43 ` Alan Stern
2019-12-03  9:38   ` Erkka Talvitie
2019-12-03 10:54     ` Erkka Talvitie
2019-12-03 13:35       ` Erkka Talvitie
2019-12-03 19:01     ` Alan Stern
2019-12-04  8:55       ` Erkka Talvitie
2019-12-04 13:18         ` Erkka Talvitie
2019-12-04 14:24           ` Alan Stern [this message]
2019-12-04 14:37             ` Erkka Talvitie
2019-12-05 10:35               ` Erkka Talvitie
2019-12-05 14:37                 ` Alan Stern
2019-12-05 15:00                   ` Erkka Talvitie
2019-12-09  9:57                     ` Erkka Talvitie
2019-12-09 15:24                       ` Alan Stern
2019-12-10  6:31                         ` Erkka Talvitie

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Pine.LNX.4.44L0.1912040917500.3941-100000@netrider.rowland.org \
    --to=stern@rowland.harvard.edu \
    --cc=claus.baumgartner@med.ge.com \
    --cc=erkka.talvitie@vincit.fi \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-usb@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.