public inbox for linux-usb@vger.kernel.org
 help / color / mirror / Atom feed
From: Michal Pecio <michal.pecio@gmail.com>
To: "stern@rowland.harvard.edu" <stern@rowland.harvard.edu>
Cc: Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
	Mathias Nyman <mathias.nyman@linux.intel.com>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	"oneukum@suse.com" <oneukum@suse.com>,
	"niklas.neronin@linux.intel.com" <niklas.neronin@linux.intel.com>
Subject: Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error
Date: Fri, 3 Apr 2026 10:51:45 +0200	[thread overview]
Message-ID: <20260403105145.7e74a410.michal.pecio@gmail.com> (raw)
In-Reply-To: <28a00143-85fa-4043-93a0-c2b687ff1bcd@rowland.harvard.edu>

On Thu, 2 Apr 2026 22:42:39 -0400, stern@rowland.harvard.edu wrote:
> Every status other than 0 should mean that the endpoint's queue is 
> halted.  But not all statuses require a clear-halt or reset-endpoint
> to recover.  For instance, a short transfer when the URB_SHORT_NOT_OK
> flag is set.

This doesn't work in xhci-hcd and I'm not sure if it can. I don't recall
any way to make endpoints halt on short transfer at all.

The driver does exactly two things with this flag:
1. Isochronus frames get EREMOTEIO status when it's detected, although
   ISTR that URB_SHORT_NOT_OK isn't defined for isochronous URBs.
2. All others log a dbg() message. Supposedly, core is responsible for
   updating urb->status then.

> > * The USB core will not attempt to unlink pending URBs due to halted
> >   condition
> > * The HCD is responsible for completing or canceling queued URBs
> >   when the halted flag is set. Cancelled and newly submitted URBs
> > will be returned with -EPIPE as long as the halted flag is set  
> 
> Why make the HCD responsible for draining the queue?  It's like
> setting the halted flag; one central place is simpler and less
> error-prone than lots of separate places.

Is emptying the queue really a good idea at all? It obviously breaks
lazy drivers which just ignore errors and continue with the URBs they
have already submitted. Removing the URBs only adds more data loss.

> For newly submitted URBs, should the submission fail with -EPIPE, or 
> should the submission succeed and then the URB complete with -EPIPE?  
> The first is simpler, but drivers probably aren't prepared for it

Are they truly expecting the alternative? I bet most of them would
usb_clear_halt() for each received EPIPE, or not at all.

If completion unlinks remaining URBs before returning, this question
doesn't exist (if we fix restarting before completion). If it doesn't
unlink them, who even knows what the driver wants?

For example, some drivers resubmit the URB while handling EPROTO and
don't unlink anything. To me, it says "screw data loss and continue".

It admittedly doesn't work on SuperSpeed anymore, but not all drivers
need to worry about SuperSpeed. Including some old and lazy ones.

Regards,
Michal

  reply	other threads:[~2026-04-03  8:51 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-03-23 12:25 [RFC PATCH 0/2] fix xhci endpoint restart at EPROTO Mathias Nyman
2026-03-23 12:25 ` [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error Mathias Nyman
2026-03-25  1:52   ` Thinh Nguyen
2026-03-25  9:38     ` Mathias Nyman
2026-03-26  1:19       ` Thinh Nguyen
2026-03-26 11:25         ` Mathias Nyman
2026-03-26 23:24           ` Thinh Nguyen
2026-03-30 12:51             ` Mathias Nyman
2026-03-30 14:17               ` stern
2026-03-31  9:34                 ` Mathias Nyman
2026-03-31 15:31                   ` stern
2026-04-01 22:08                     ` Mathias Nyman
2026-04-02  2:36                       ` stern
2026-04-03  1:59                         ` Thinh Nguyen
2026-04-03  2:42                           ` stern
2026-04-03  8:51                             ` Michal Pecio [this message]
2026-04-03 14:55                               ` stern
2026-04-03 19:13                                 ` xhci-hcd and URB_SHORT_NOT_OK Michal Pecio
2026-04-03 20:17                                   ` stern
2026-04-04  1:15                             ` [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error Thinh Nguyen
2026-04-04  1:54                               ` stern
2026-04-04 20:41                                 ` Thinh Nguyen
2026-04-04 21:54                                   ` Alan Stern
2026-04-04 22:15                                     ` Thinh Nguyen
2026-04-04 22:28                                       ` Thinh Nguyen
2026-04-05  1:30                                         ` Alan Stern
2026-04-05  3:10                                           ` Thinh Nguyen
2026-04-07 15:23                                             ` Alan Stern
2026-04-07 20:24                                               ` Mathias Nyman
2026-04-01 22:08               ` Thinh Nguyen
2026-04-01 22:34                 ` Mathias Nyman
2026-04-01 22:47                   ` Thinh Nguyen
2026-03-23 12:25 ` [RFC PATCH 2/2] xhci: Ensure URB is given back when endpoint halts on a multi-TD URB Mathias Nyman

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=20260403105145.7e74a410.michal.pecio@gmail.com \
    --to=michal.pecio@gmail.com \
    --cc=Thinh.Nguyen@synopsys.com \
    --cc=linux-usb@vger.kernel.org \
    --cc=mathias.nyman@linux.intel.com \
    --cc=niklas.neronin@linux.intel.com \
    --cc=oneukum@suse.com \
    --cc=stern@rowland.harvard.edu \
    /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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox