[RFC PATCH 0/2] fix xhci endpoint restart at EPROTO

[RFC PATCH 0/2] fix xhci endpoint restart at EPROTO

[Mathias Nyman]

Hi

These patches are related to the 'correctly handling EPROTO' discussion,
and aim to fix the xhci endpoint automatic restart issue.

It also makes sure that multi TD URBs are given back with proper
EPROTO/EPIPE status if a stall/transaction error is triggered on
any TD of the URB, not just the last one.

endpoints will still be restarted when a new URB is queued, this does
not fix the issue with BH URBs completion, but will print a debug message
if a new URB is queued before previous 'tainted' pending URBs are cancelled.

I haven't been able to test the actual codepaths in these patches yet.
Can't easily queue several URBs and somehow trigger stall or transaction
error on early URBs, but would like to get feeback on this solution early.

Thanks
Mathias

Mathias Nyman (2):
  xhci: prevent automatic endpoint restart after stall or error
  xhci: Ensure URB is given back when endpoint halts on a multi-TD URB

 drivers/usb/host/xhci-ring.c | 50 +++++++++++++++++++++++++++++++-----
 drivers/usb/host/xhci.c      |  7 +++++
 drivers/usb/host/xhci.h      |  4 ++-
 3 files changed, 53 insertions(+), 8 deletions(-)

-- 
2.43.0

[RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Mathias Nyman]

Avoid automatically restarting bulk or interrupt transfers after a
URB is given back due to stall or error.

Introduce a 'TD_TAINTED' state for pending TDs queued on a endpoint when
it halted. The actual TD the endpoint halted on is marked TD_HALTED,
and its URB is given back with proper EPROTO or EPIPE error code.

Don't automatically restart an endpoint if the next queued TD after
the TD_HALTED one is marked tainted.

class driver is expected to cancel all those pending URBs before
submitting more URBs, or before clearing halt, but is not forced to.
Always start the endpoint when a new URB is queued, but print a debug
message if there are still tainted URBs left uncancelled.

The same code is used for clearing up cached TDs from xHC in both
the URB stall/error case as when a class driver cancels URBs.
In the cancel case endpoint is restarted after removing the cancelled
URB.

Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-ring.c | 48 ++++++++++++++++++++++++++++++------
 drivers/usb/host/xhci.c      |  7 ++++++
 drivers/usb/host/xhci.h      |  4 ++-
 3 files changed, 51 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 1cbefee3c4ca..f01f343a7e37 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -981,6 +981,31 @@ static int xhci_reset_halted_ep(struct xhci_hcd *xhci, unsigned int slot_id,
 	return ret;
 }
 
+/*
+ * Mark the TD the endpoint halted on as TD_HALTED and add it to the cancelled
+ * td list, ensuring invalidate_cancelled_tds() clears the TD from xHC cache.
+ * Mark the other TDs on that bulk or interrupt endpoint as TD_TAINTED to
+ * prevent the ring from being restarted too early.
+ */
+
+static void xhci_cancel_halted_tds(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
+				   struct xhci_td *td)
+{
+	struct xhci_ring *ring;
+	struct xhci_td *tdi;
+
+	ring = xhci_urb_to_transfer_ring(xhci, td->urb);
+
+	td->cancel_status = TD_HALTED;
+	if (list_empty(&td->cancelled_td_list))
+		list_add_tail(&td->cancelled_td_list, &ep->cancelled_td_list);
+
+	list_for_each_entry(tdi, &ring->td_list, td_list) {
+		if (!tdi->cancel_status && ring->type != TYPE_CTRL)
+			tdi->cancel_status = TD_TAINTED;
+	}
+}
+
 static int xhci_handle_halted_endpoint(struct xhci_hcd *xhci,
 				struct xhci_virt_ep *ep,
 				struct xhci_td *td,
@@ -999,10 +1024,8 @@ static int xhci_handle_halted_endpoint(struct xhci_hcd *xhci,
 	/* add td to cancelled list and let reset ep handler take care of it */
 	if (reset_type == EP_HARD_RESET) {
 		ep->ep_state |= EP_HARD_CLEAR_TOGGLE;
-		if (td && list_empty(&td->cancelled_td_list)) {
-			list_add_tail(&td->cancelled_td_list, &ep->cancelled_td_list);
-			td->cancel_status = TD_HALTED;
-		}
+		if (td)
+			xhci_cancel_halted_tds(xhci, ep, td);
 	}
 
 	if (ep->ep_state & EP_HALTED) {
@@ -1079,6 +1102,7 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep)
 			case TD_CLEARED: /* TD is already no-op */
 			case TD_CLEARING_CACHE: /* set TR deq command already queued */
 				break;
+			case TD_TAINTED:
 			case TD_DIRTY: /* TD is cached, clear it */
 			case TD_HALTED:
 			case TD_CLEARING_CACHE_DEFERRED:
@@ -1406,6 +1430,14 @@ void xhci_hc_died(struct xhci_hcd *xhci)
 		usb_hc_died(xhci_to_hcd(xhci));
 }
 
+bool next_td_is_tainted(struct xhci_ring *ring)
+{
+	struct xhci_td *td;
+
+	td = list_first_entry_or_null(&ring->td_list, struct xhci_td, td_list);
+	return td && td->cancel_status == TD_TAINTED;
+}
+
 /*
  * When we get a completion for a Set Transfer Ring Dequeue Pointer command,
  * we need to clear the set deq pending flag in the endpoint ring state, so that
@@ -1543,13 +1575,15 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id,
 			 __func__);
 		xhci_invalidate_cancelled_tds(ep);
 		/* Try to restart the endpoint if all is done */
-		ring_doorbell_for_active_rings(xhci, slot_id, ep_index);
+		if (!next_td_is_tainted(ep_ring))
+			ring_doorbell_for_active_rings(xhci, slot_id, ep_index);
 		/* Start giving back any TDs invalidated above */
 		xhci_giveback_invalidated_tds(ep);
 	} else {
 		/* Restart any rings with pending URBs */
-		xhci_dbg(ep->xhci, "%s: All TDs cleared, ring doorbell\n", __func__);
-		ring_doorbell_for_active_rings(xhci, slot_id, ep_index);
+		xhci_dbg(ep->xhci, "%s: All cancelled TDs cleared\n", __func__);
+		if (!next_td_is_tainted(ep_ring))
+			ring_doorbell_for_active_rings(xhci, slot_id, ep_index);
 	}
 }
 
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index ef6d8662adec..8f417836ae1f 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1625,6 +1625,7 @@ static int xhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 	unsigned long flags;
 	int ret = 0;
 	unsigned int slot_id, ep_index;
+	struct xhci_ring *ring;
 	unsigned int *ep_state;
 	struct urb_priv	*urb_priv;
 	int num_tds;
@@ -1661,6 +1662,7 @@ static int xhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 	}
 
 	slot_id = urb->dev->slot_id;
+	ring = xhci_urb_to_transfer_ring(xhci, urb);
 
 	if (!HCD_HW_ACCESSIBLE(hcd)) {
 		ret = -ESHUTDOWN;
@@ -1694,6 +1696,10 @@ static int xhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 		goto free_priv;
 	}
 
+	ring = xhci_urb_to_transfer_ring(xhci, urb);
+	if (ring && next_td_is_tainted(ring))
+		xhci_dbg(xhci, "WARN, enqueue URB without canceling old tainted URBs\n");
+
 	switch (usb_endpoint_type(&urb->ep->desc)) {
 
 	case USB_ENDPOINT_XFER_CONTROL:
@@ -3354,6 +3360,7 @@ static void xhci_endpoint_reset(struct usb_hcd *hcd,
 
 	/* Bail out if toggle is already being cleared by a endpoint reset */
 	spin_lock_irqsave(&xhci->lock, flags);
+
 	if (ep->ep_state & EP_HARD_CLEAR_TOGGLE) {
 		ep->ep_state &= ~EP_HARD_CLEAR_TOGGLE;
 		spin_unlock_irqrestore(&xhci->lock, flags);
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 2b0796f6d00e..0be040f87abf 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1292,7 +1292,8 @@ struct xhci_segment {
 };
 
 enum xhci_cancelled_td_status {
-	TD_DIRTY = 0,
+	TD_TAINTED = 1,
+	TD_DIRTY,
 	TD_HALTED,
 	TD_CLEARING_CACHE,
 	TD_CLEARING_CACHE_DEFERRED,
@@ -1951,6 +1952,7 @@ void xhci_ring_doorbell_for_active_rings(struct xhci_hcd *xhci,
 void xhci_cleanup_command_queue(struct xhci_hcd *xhci);
 void inc_deq(struct xhci_hcd *xhci, struct xhci_ring *ring);
 unsigned int count_trbs(u64 addr, u64 len);
+bool next_td_is_tainted(struct xhci_ring *ring);
 int xhci_stop_endpoint_sync(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 			    int suspend, gfp_t gfp_flags);
 void xhci_process_cancelled_tds(struct xhci_virt_ep *ep);
-- 
2.43.0

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Thinh Nguyen]

On Mon, Mar 23, 2026, Mathias Nyman wrote:
> Avoid automatically restarting bulk or interrupt transfers after a
> URB is given back due to stall or error.
> 
> Introduce a 'TD_TAINTED' state for pending TDs queued on a endpoint when
> it halted. The actual TD the endpoint halted on is marked TD_HALTED,
> and its URB is given back with proper EPROTO or EPIPE error code.
> 
> Don't automatically restart an endpoint if the next queued TD after
> the TD_HALTED one is marked tainted.

Sounds good for -EPROTO, but will a clear-halt ring the corresponding
the endpoint's doorbell for STALL endpoint?

BR,
Thinh

> 
> class driver is expected to cancel all those pending URBs before
> submitting more URBs, or before clearing halt, but is not forced to.
> Always start the endpoint when a new URB is queued, but print a debug
> message if there are still tainted URBs left uncancelled.
> 
> The same code is used for clearing up cached TDs from xHC in both
> the URB stall/error case as when a class driver cancels URBs.
> In the cancel case endpoint is restarted after removing the cancelled
> URB.
> 
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---
>  drivers/usb/host/xhci-ring.c | 48 ++++++++++++++++++++++++++++++------
>  drivers/usb/host/xhci.c      |  7 ++++++
>  drivers/usb/host/xhci.h      |  4 ++-
>  3 files changed, 51 insertions(+), 8 deletions(-)
> 

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Mathias Nyman]

On 3/25/26 03:52, Thinh Nguyen wrote:
> On Mon, Mar 23, 2026, Mathias Nyman wrote:
>> Avoid automatically restarting bulk or interrupt transfers after a
>> URB is given back due to stall or error.
>>
>> Introduce a 'TD_TAINTED' state for pending TDs queued on a endpoint when
>> it halted. The actual TD the endpoint halted on is marked TD_HALTED,
>> and its URB is given back with proper EPROTO or EPIPE error code.
>>
>> Don't automatically restart an endpoint if the next queued TD after
>> the TD_HALTED one is marked tainted.
> 
> Sounds good for -EPROTO, but will a clear-halt ring the corresponding
> the endpoint's doorbell for STALL endpoint?
> 

With this change xhci would not restart the stalled endpoint after a clear-halt
request. The first usb_enqueue() call after clear-halt would start it.

Could make sense to restart the endpoint after a clear-halt, and just add a small
debug message if the next queued URB is marked 'tainted'.

Thanks for the feedback
Mathias

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Thinh Nguyen]

On Wed, Mar 25, 2026, Mathias Nyman wrote:
> On 3/25/26 03:52, Thinh Nguyen wrote:
> > On Mon, Mar 23, 2026, Mathias Nyman wrote:
> > > Avoid automatically restarting bulk or interrupt transfers after a
> > > URB is given back due to stall or error.
> > > 
> > > Introduce a 'TD_TAINTED' state for pending TDs queued on a endpoint when
> > > it halted. The actual TD the endpoint halted on is marked TD_HALTED,
> > > and its URB is given back with proper EPROTO or EPIPE error code.
> > > 
> > > Don't automatically restart an endpoint if the next queued TD after
> > > the TD_HALTED one is marked tainted.
> > 
> > Sounds good for -EPROTO, but will a clear-halt ring the corresponding
> > the endpoint's doorbell for STALL endpoint?
> > 
> 
> With this change xhci would not restart the stalled endpoint after a clear-halt
> request. The first usb_enqueue() call after clear-halt would start it.
> 
> Could make sense to restart the endpoint after a clear-halt, and just add a small
> debug message if the next queued URB is marked 'tainted'.
> 

The -EPROTO should be handled differently than -EPIPE. A STALL endpoint
is part of a normal usb flow. Should the class driver submit a new URB
while the endpoint is STALL, we would always expect a STALL error
response after the endpoint is restarted. That's not the case with
-EPROTO where the data may be corrupted and/or the host and device are
out of sync. We should not continue until the class driver do some
recovery. IMHO, we can keep the handling of -EPIPE how it was before.
Let the xHC tell whether the STALL error still persists instead of
managing it by the xhci driver.

Thanks,
Thinh

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Mathias Nyman]

On 3/26/26 03:19, Thinh Nguyen wrote:
> On Wed, Mar 25, 2026, Mathias Nyman wrote:
>> On 3/25/26 03:52, Thinh Nguyen wrote:
>>> On Mon, Mar 23, 2026, Mathias Nyman wrote:
>>>> Avoid automatically restarting bulk or interrupt transfers after a
>>>> URB is given back due to stall or error.
>>>>
>>>> Introduce a 'TD_TAINTED' state for pending TDs queued on a endpoint when
>>>> it halted. The actual TD the endpoint halted on is marked TD_HALTED,
>>>> and its URB is given back with proper EPROTO or EPIPE error code.
>>>>
>>>> Don't automatically restart an endpoint if the next queued TD after
>>>> the TD_HALTED one is marked tainted.
>>>
>>> Sounds good for -EPROTO, but will a clear-halt ring the corresponding
>>> the endpoint's doorbell for STALL endpoint?
>>>
>>
>> With this change xhci would not restart the stalled endpoint after a clear-halt
>> request. The first usb_enqueue() call after clear-halt would start it.
>>
>> Could make sense to restart the endpoint after a clear-halt, and just add a small
>> debug message if the next queued URB is marked 'tainted'.
>>
> 
> The -EPROTO should be handled differently than -EPIPE. A STALL endpoint
> is part of a normal usb flow. Should the class driver submit a new URB
> while the endpoint is STALL, we would always expect a STALL error
> response after the endpoint is restarted. That's not the case with
> -EPROTO where the data may be corrupted and/or the host and device are
> out of sync. We should not continue until the class driver do some
> recovery. IMHO, we can keep the handling of -EPIPE how it was before.
> Let the xHC tell whether the STALL error still persists instead of
> managing it by the xhci driver.
> 
I agree that that we should restart the endpoint if class/core enqueues a new
URB _after_ xhci gave back an URB with EPIPE after endpoint STALL.

But I don't think we should restart the ring to continue processing URBs that
were queued before the endpoint stalled. This would prevent the class/core
from even attempting to retire the pending URBs, something USB2.0 spec,
'5.8.5 Bulk Transfer Data Sequences' requires:

"If a halt condition is detected on a bulk pipe due to transmission errors or
  a STALL handshake being returned from the endpoint, all pending IRPs are
  retired.  Removal of the halt condition is achieved via software intervention
  through a separate control pipe."

Thanks
Mathias

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Thinh Nguyen]

On Thu, Mar 26, 2026, Mathias Nyman wrote:
> On 3/26/26 03:19, Thinh Nguyen wrote:
> > On Wed, Mar 25, 2026, Mathias Nyman wrote:
> > > On 3/25/26 03:52, Thinh Nguyen wrote:
> > > > On Mon, Mar 23, 2026, Mathias Nyman wrote:
> > > > > Avoid automatically restarting bulk or interrupt transfers after a
> > > > > URB is given back due to stall or error.
> > > > > 
> > > > > Introduce a 'TD_TAINTED' state for pending TDs queued on a endpoint when
> > > > > it halted. The actual TD the endpoint halted on is marked TD_HALTED,
> > > > > and its URB is given back with proper EPROTO or EPIPE error code.
> > > > > 
> > > > > Don't automatically restart an endpoint if the next queued TD after
> > > > > the TD_HALTED one is marked tainted.
> > > > 
> > > > Sounds good for -EPROTO, but will a clear-halt ring the corresponding
> > > > the endpoint's doorbell for STALL endpoint?
> > > > 
> > > 
> > > With this change xhci would not restart the stalled endpoint after a clear-halt
> > > request. The first usb_enqueue() call after clear-halt would start it.
> > > 
> > > Could make sense to restart the endpoint after a clear-halt, and just add a small
> > > debug message if the next queued URB is marked 'tainted'.
> > > 
> > 
> > The -EPROTO should be handled differently than -EPIPE. A STALL endpoint
> > is part of a normal usb flow. Should the class driver submit a new URB
> > while the endpoint is STALL, we would always expect a STALL error
> > response after the endpoint is restarted. That's not the case with
> > -EPROTO where the data may be corrupted and/or the host and device are
> > out of sync. We should not continue until the class driver do some
> > recovery. IMHO, we can keep the handling of -EPIPE how it was before.
> > Let the xHC tell whether the STALL error still persists instead of
> > managing it by the xhci driver.
> > 
> I agree that that we should restart the endpoint if class/core enqueues a new
> URB _after_ xhci gave back an URB with EPIPE after endpoint STALL.
> 
> But I don't think we should restart the ring to continue processing URBs that
> were queued before the endpoint stalled. This would prevent the class/core
> from even attempting to retire the pending URBs, something USB2.0 spec,
> '5.8.5 Bulk Transfer Data Sequences' requires:
> 
> "If a halt condition is detected on a bulk pipe due to transmission errors or
>  a STALL handshake being returned from the endpoint, all pending IRPs are
>  retired.  Removal of the halt condition is achieved via software intervention
>  through a separate control pipe."
> 

Fair point. Then the core will need to track the endpoint's STALL state
and parse the clear-halt request to know which endpoint and when to
clear the STALL before it can accept new URB. So the first usb_enqueue()
call after clear-halt can start the endpoint again. The xhci will also
need to have access to this state.

Currently you have the xhci driver to "retire" the halted URBs. However,
you also noted that class/core may attempt to retire the pending URBs.
Who's expected to handle the retirement here?

On a separate note, will you plan to implement the clear-halt for EPROTO
in xhci?

Thanks!
Thinh

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Mathias Nyman]

On 3/27/26 01:24, Thinh Nguyen wrote:
> On Thu, Mar 26, 2026, Mathias Nyman wrote:
>> On 3/26/26 03:19, Thinh Nguyen wrote:
>>> On Wed, Mar 25, 2026, Mathias Nyman wrote:
>>>> On 3/25/26 03:52, Thinh Nguyen wrote:
>>>>> On Mon, Mar 23, 2026, Mathias Nyman wrote:
>>>>>> Avoid automatically restarting bulk or interrupt transfers after a
>>>>>> URB is given back due to stall or error.
>>>>>>
>>>>>> Introduce a 'TD_TAINTED' state for pending TDs queued on a endpoint when
>>>>>> it halted. The actual TD the endpoint halted on is marked TD_HALTED,
>>>>>> and its URB is given back with proper EPROTO or EPIPE error code.
>>>>>>
>>>>>> Don't automatically restart an endpoint if the next queued TD after
>>>>>> the TD_HALTED one is marked tainted.
>>>>>
>>>>> Sounds good for -EPROTO, but will a clear-halt ring the corresponding
>>>>> the endpoint's doorbell for STALL endpoint?
>>>>>
>>>>
>>>> With this change xhci would not restart the stalled endpoint after a clear-halt
>>>> request. The first usb_enqueue() call after clear-halt would start it.
>>>>
>>>> Could make sense to restart the endpoint after a clear-halt, and just add a small
>>>> debug message if the next queued URB is marked 'tainted'.
>>>>
>>>
>>> The -EPROTO should be handled differently than -EPIPE. A STALL endpoint
>>> is part of a normal usb flow. Should the class driver submit a new URB
>>> while the endpoint is STALL, we would always expect a STALL error
>>> response after the endpoint is restarted. That's not the case with
>>> -EPROTO where the data may be corrupted and/or the host and device are
>>> out of sync. We should not continue until the class driver do some
>>> recovery. IMHO, we can keep the handling of -EPIPE how it was before.
>>> Let the xHC tell whether the STALL error still persists instead of
>>> managing it by the xhci driver.
>>>
>> I agree that that we should restart the endpoint if class/core enqueues a new
>> URB _after_ xhci gave back an URB with EPIPE after endpoint STALL.
>>
>> But I don't think we should restart the ring to continue processing URBs that
>> were queued before the endpoint stalled. This would prevent the class/core
>> from even attempting to retire the pending URBs, something USB2.0 spec,
>> '5.8.5 Bulk Transfer Data Sequences' requires:
>>
>> "If a halt condition is detected on a bulk pipe due to transmission errors or
>>   a STALL handshake being returned from the endpoint, all pending IRPs are
>>   retired.  Removal of the halt condition is achieved via software intervention
>>   through a separate control pipe."
>>
> 
> Fair point. Then the core will need to track the endpoint's STALL state
> and parse the clear-halt request to know which endpoint and when to
> clear the STALL before it can accept new URB. So the first usb_enqueue()
> call after clear-halt can start the endpoint again. The xhci will also
> need to have access to this state.

Ideally xhci driver would return the URB with EPIPE after STALL, and not continue
processing URBs before a clear halt is sent, or a new URB is enqueued.
USB core would hold off submitting URBs to xhci, buffering URBs enqueued for this
STALLED endpoint until class driver urb->complete() finishes for that EPIPE URB.

Usb core could flag this endpoint as "halt_pending" before adding the EPIPE URB to
the bh workqueue. Then after urb->complete() work is called and core is sure class
driver  is aware of the EPIPE, then core would clear the flag and flush the buffered
URBs to the host controller drivers, restarting the ring

Class driver urb->complete() would most likely retire/cancel the pending URBs, both the
earlier queued 'tainted' URBs, and the most recent 'buffered' URBs in usb core.
Class driver should clear the halt, but is free to do whatever it wants.
It could choose to send a new URB without clearing the halt,
have it processed, trigger STALL again, and get URB returned with EPIPE status.

I don't think most class/usb device drivers really handle stall that well.
Above might be wishful thinking.

> 
> Currently you have the xhci driver to "retire" the halted URBs. However,
> you also noted that class/core may attempt to retire the pending URBs.
> Who's expected to handle the retirement here?

Maybe we should let core retire the pending URBS, and only fix the xhci driver
'automatic endpoint restart after stall' part after that core change is done.

Should cause less regression.

> 
> On a separate note, will you plan to implement the clear-halt for EPROTO
> in xhci?

I don't think this should be part of xhci driver. Decision to send control requests
to the device should be done by core or class drivers.

Thanks
Mathias

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[stern]

On Mon, Mar 30, 2026 at 03:51:46PM +0300, Mathias Nyman wrote:
> Ideally xhci driver would return the URB with EPIPE after STALL, and not continue
> processing URBs before a clear halt is sent, or a new URB is enqueued.
> USB core would hold off submitting URBs to xhci, buffering URBs enqueued for this
> STALLED endpoint until class driver urb->complete() finishes for that EPIPE URB.
> 
> Usb core could flag this endpoint as "halt_pending" before adding the EPIPE URB to
> the bh workqueue. Then after urb->complete() work is called and core is sure class
> driver  is aware of the EPIPE, then core would clear the flag and flush the buffered
> URBs to the host controller drivers, restarting the ring

This is not practical; it would result in a big slowdown for large bulk 
transfers.  Doing this would mean the core could not send an URB to the 
HCD until all the previous URBs for that endpoint had completed and the 
interrupt handler had run, which would add significant latency to 
transfers.

Exactly what should happen to URBs coming after one that completes with 
-EPIPE is not immediately obvious.  If the HCD did try to send them to 
the device right away then they would also be stalled, which is 
obviously non-productive.  The only guarantee the kernel makes 
about this situation is that the endpoint queue won't restart 
until all completed or unlinked URBs have been given back (likewise for 
-EPROTO errors).

The only safe course available to class drivers is to unlink all the 
pending URBs.  In theory the core could do this for all drivers 
automatically, but at present it doesn't.

> Class driver urb->complete() would most likely retire/cancel the pending URBs, both the
> earlier queued 'tainted' URBs, and the most recent 'buffered' URBs in usb core.
> Class driver should clear the halt, but is free to do whatever it wants.
> It could choose to send a new URB without clearing the halt,
> have it processed, trigger STALL again, and get URB returned with EPIPE status.
> 
> I don't think most class/usb device drivers really handle stall that well.
> Above might be wishful thinking.

Indeed.  We can make life a little easier for drivers, but clearing the 
endpoint halt is up to them.

> > Currently you have the xhci driver to "retire" the halted URBs. However,
> > you also noted that class/core may attempt to retire the pending URBs.
> > Who's expected to handle the retirement here?
> 
> Maybe we should let core retire the pending URBS, and only fix the xhci driver
> 'automatic endpoint restart after stall' part after that core change is done.
> 
> Should cause less regression.
> 
> > 
> > On a separate note, will you plan to implement the clear-halt for EPROTO
> > in xhci?
> 
> I don't think this should be part of xhci driver. Decision to send control requests
> to the device should be done by core or class drivers.

Here's a troubling consequence for people to consider: Suppose an 
endpoint queue stops with -EPROTO or -EPIPE, and before the class driver 
gets around to calling usb_clear_halt(), it is unbound.  What happens 
the next time a driver binds to the device and tries to use the 
endpoint?

In particular: What does xhci-hcd do if an URB is submitted for an 
endpoint whose queue is currently stopped?  Does it reject the 
submission with some sort of error code, or does it go ahead and add the 
URB to the end of the non-advancing queue?  If the latter is true, how 
will a newly bound driver ever discover that the queue is stopped?

Alan Stern

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Mathias Nyman]

On 3/30/26 17:17, stern@rowland.harvard.edu wrote:
> On Mon, Mar 30, 2026 at 03:51:46PM +0300, Mathias Nyman wrote:
>> Ideally xhci driver would return the URB with EPIPE after STALL, and not continue
>> processing URBs before a clear halt is sent, or a new URB is enqueued.
>> USB core would hold off submitting URBs to xhci, buffering URBs enqueued for this
>> STALLED endpoint until class driver urb->complete() finishes for that EPIPE URB.
>>
>> Usb core could flag this endpoint as "halt_pending" before adding the EPIPE URB to
>> the bh workqueue. Then after urb->complete() work is called and core is sure class
>> driver  is aware of the EPIPE, then core would clear the flag and flush the buffered
>> URBs to the host controller drivers, restarting the ring
> 
> This is not practical; it would result in a big slowdown for large bulk
> transfers.  Doing this would mean the core could not send an URB to the
> HCD until all the previous URBs for that endpoint had completed and the
> interrupt handler had run, which would add significant latency to
> transfers.

The URB submit buffering in core would only take place if endpoint is halted
with EPIPE/EPROTO.

Additional latency should be limited to the time between interrupt handler
returns a URB with EPIPE/EPROTO, and the bh workqueue finishing urb->complete()
for that URB

URBs would normally be sent to HCD directly.

Yes, this might end up being quite complex, need to make sure it solves more
issues than it creates.

Usb core is aware of the halted endpoint before the class driver due to the bh
workqueue giveback. To me it would make sense to make core responsible for babysitting
the class driver urb submission for the time it withholds this information.

Whole reason for this is to prevent new URB submission from HCD restarting an endpoint
after HCD gave back a halted URB, and HCD assuming core/class drivers are aware of
the halt when the new URB is submitted.

Other option is that usb core would just refuse class driver from submitting URBs
during this time. usb_submit_urb() would return with an error, but I think this might
impact existing class drivers more.

> 
> Exactly what should happen to URBs coming after one that completes with
> -EPIPE is not immediately obvious.  If the HCD did try to send them to
> the device right away then they would also be stalled, which is
> obviously non-productive.  The only guarantee the kernel makes
> about this situation is that the endpoint queue won't restart
> until all completed or unlinked URBs have been given back (likewise for
> -EPROTO errors).

My take is that endpoint should stop processing URBs, existing pending URBs
should be retired by class/core, new URBs should restart the endpoint but new URBs
are only permitted _after_ submitter is aware of the halt.

A class driver testing USB specification should be able to resubmit a stalled URB
(EPIPE) and expect it to complete with EPIPE again until it clears the halt.

> 
> The only safe course available to class drivers is to unlink all the
> pending URBs.  In theory the core could do this for all drivers
> automatically, but at present it doesn't.
> 
>> Class driver urb->complete() would most likely retire/cancel the pending URBs, both the
>> earlier queued 'tainted' URBs, and the most recent 'buffered' URBs in usb core.
>> Class driver should clear the halt, but is free to do whatever it wants.
>> It could choose to send a new URB without clearing the halt,
>> have it processed, trigger STALL again, and get URB returned with EPIPE status.
>>
>> I don't think most class/usb device drivers really handle stall that well.
>> Above might be wishful thinking.
> 
> Indeed.  We can make life a little easier for drivers, but clearing the
> endpoint halt is up to them.
> 
>>> Currently you have the xhci driver to "retire" the halted URBs. However,
>>> you also noted that class/core may attempt to retire the pending URBs.
>>> Who's expected to handle the retirement here?
>>
>> Maybe we should let core retire the pending URBS, and only fix the xhci driver
>> 'automatic endpoint restart after stall' part after that core change is done.
>>
>> Should cause less regression.
>>
>>>
>>> On a separate note, will you plan to implement the clear-halt for EPROTO
>>> in xhci?
>>
>> I don't think this should be part of xhci driver. Decision to send control requests
>> to the device should be done by core or class drivers.
> 
> Here's a troubling consequence for people to consider: Suppose an
> endpoint queue stops with -EPROTO or -EPIPE, and before the class driver
> gets around to calling usb_clear_halt(), it is unbound.  What happens
> the next time a driver binds to the device and tries to use the
> endpoint?

The disable/enable interface and set config calls during unbind/bind should,
if I remember correctly flush pending URBs and drop and re-add the endpoint,
clearing the xhci side halt and reset toggle.

> 
> In particular: What does xhci-hcd do if an URB is submitted for an
> endpoint whose queue is currently stopped?  Does it reject the
> submission with some sort of error code, or does it go ahead and add the
> URB to the end of the non-advancing queue?  If the latter is true, how
> will a newly bound driver ever discover that the queue is stopped?

xhci-hcd will queue the new URB  and restart the ring, if device side endpoint
remains halted due to uncleared halt then transfer stalls and URB is returned
with EPIPE.

Thanks
Mathias

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[stern]

On Tue, Mar 31, 2026 at 12:34:54PM +0300, Mathias Nyman wrote:
> On 3/30/26 17:17, stern@rowland.harvard.edu wrote:
> > On Mon, Mar 30, 2026 at 03:51:46PM +0300, Mathias Nyman wrote:
> > > Ideally xhci driver would return the URB with EPIPE after STALL, and not continue
> > > processing URBs before a clear halt is sent, or a new URB is enqueued.
> > > USB core would hold off submitting URBs to xhci, buffering URBs enqueued for this
> > > STALLED endpoint until class driver urb->complete() finishes for that EPIPE URB.
> > > 
> > > Usb core could flag this endpoint as "halt_pending" before adding the EPIPE URB to
> > > the bh workqueue. Then after urb->complete() work is called and core is sure class
> > > driver  is aware of the EPIPE, then core would clear the flag and flush the buffered
> > > URBs to the host controller drivers, restarting the ring
> > 
> > This is not practical; it would result in a big slowdown for large bulk
> > transfers.  Doing this would mean the core could not send an URB to the
> > HCD until all the previous URBs for that endpoint had completed and the
> > interrupt handler had run, which would add significant latency to
> > transfers.
> 
> The URB submit buffering in core would only take place if endpoint is halted
> with EPIPE/EPROTO.
> 
> Additional latency should be limited to the time between interrupt handler
> returns a URB with EPIPE/EPROTO, and the bh workqueue finishing urb->complete()
> for that URB
> 
> URBs would normally be sent to HCD directly.
> 
> Yes, this might end up being quite complex, need to make sure it solves more
> issues than it creates.

Can that be done precisely?  I doubt it -- there are races between the 
core and the HCD and between the HCD and the hardware.

As for doing it imprecisely...  I'm not sure the advantages outweigh the 
complexity.  Anyway, if the class driver is well behaved, this buffering 
won't be needed.

> Usb core is aware of the halted endpoint before the class driver due to the bh
> workqueue giveback. To me it would make sense to make core responsible for babysitting
> the class driver urb submission for the time it withholds this information.
> 
> Whole reason for this is to prevent new URB submission from HCD restarting an endpoint
> after HCD gave back a halted URB, and HCD assuming core/class drivers are aware of
> the halt when the new URB is submitted.
> 
> Other option is that usb core would just refuse class driver from submitting URBs
> during this time. usb_submit_urb() would return with an error, but I think this might
> impact existing class drivers more.

How about this instead?  We add a "halted" flag to the usb_host_endpoint 
structure, and the core will set this flag whenever a bulk or interrupt 
URB gets a status other than 0 (before putting the URB on the bh list).  
If an URB has one of these statuses, when its completion handler returns 
the core will unlink all the URBs queued to the same endpoint.  Finally, 
the "halted" flag should be cleared after a completion handler returns 
if there are no more unlinked URBs still in the queue or URBs waiting on 
the bh list to be given back.

The result of this is that any URB remaining in the queue when the flag 
is cleared must have been submitted by the class driver _after_ the 
failing URB's completion handler has run.  We can assume the class 
driver knows what it's doing in this case.

The endpoint queue shouldn't be restarted until the "halted" flag is 
cleared.  Either right away, if there are any URBs in the queue, or not 
until the next URB is submitted.  Doing this might require a new HCD 
callback.  (It would also mean the kerneldoc for usb_unlink_urb() would 
need to be updated, because the endpoint might restart before all the 
completion handlers for the unlinked URBs have run.)

What I'm trying to do here is come up with a single, consistent proposal 
for exactly when halted endpoint queues should restart.  Maybe someone 
else has a better suggestion.

We also should add a special case for control endpoints because their 
halt conditions can get cleared automatically when the device receives a 
SETUP packet.

> > Exactly what should happen to URBs coming after one that completes with
> > -EPIPE is not immediately obvious.  If the HCD did try to send them to
> > the device right away then they would also be stalled, which is
> > obviously non-productive.  The only guarantee the kernel makes
> > about this situation is that the endpoint queue won't restart
> > until all completed or unlinked URBs have been given back (likewise for
> > -EPROTO errors).
> 
> My take is that endpoint should stop processing URBs, existing pending URBs
> should be retired by class/core, new URBs should restart the endpoint but new URBs
> are only permitted _after_ submitter is aware of the halt.

Yes, that's more or less what my proposal attempts to accomplish.

> A class driver testing USB specification should be able to resubmit a stalled URB
> (EPIPE) and expect it to complete with EPIPE again until it clears the halt.

Agreed.  

> > Here's a troubling consequence for people to consider: Suppose an
> > endpoint queue stops with -EPROTO or -EPIPE, and before the class driver
> > gets around to calling usb_clear_halt(), it is unbound.  What happens
> > the next time a driver binds to the device and tries to use the
> > endpoint?
> 
> The disable/enable interface and set config calls during unbind/bind should,
> if I remember correctly flush pending URBs and drop and re-add the endpoint,
> clearing the xhci side halt and reset toggle.

usb_probe_interface() doesn't do any of that stuff, other than a 
deferred switch to altsetting 0 if needed.

usb_unbind_interface() does call usb_enable_interface() if the interface 
is already in altsetting 0, but the reset_ep argument is false so the 
endpoint state doesn't get affected.  Should that be changed?

Alan Stern

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Mathias Nyman]

On 3/31/26 18:31, stern@rowland.harvard.edu wrote:
> 
> How about this instead?  We add a "halted" flag to the usb_host_endpoint
> structure, and the core will set this flag whenever a bulk or interrupt
> URB gets a status other than 0 (before putting the URB on the bh list).
> If an URB has one of these statuses, when its completion handler returns
> the core will unlink all the URBs queued to the same endpoint.  Finally,
> the "halted" flag should be cleared after a completion handler returns
> if there are no more unlinked URBs still in the queue or URBs waiting on
> the bh list to be given back.
> 
> The result of this is that any URB remaining in the queue when the flag
> is cleared must have been submitted by the class driver _after_ the
> failing URB's completion handler has run.  We can assume the class
> driver knows what it's doing in this case.
> 
> The endpoint queue shouldn't be restarted until the "halted" flag is
> cleared.  Either right away, if there are any URBs in the queue, or not
> until the next URB is submitted.  Doing this might require a new HCD
> callback.  (It would also mean the kerneldoc for usb_unlink_urb() would
> need to be updated, because the endpoint might restart before all the
> completion handlers for the unlinked URBs have run.)
> 
> What I'm trying to do here is come up with a single, consistent proposal
> for exactly when halted endpoint queues should restart.  Maybe someone
> else has a better suggestion.

Sounds like a possible solution to me.

Just to clarify, core should unlink the remaining URBs queued to that endpoint
after setting the "halted" flag, but before URB completion is called.
"Halted" flag should be cleared after URB completion returns, and endpoint
should be restarted if there are any pending URBs.

This allows the class driver URB completion handler to re-queue the halted URB
without core unlinking it.

> 
>>> Here's a troubling consequence for people to consider: Suppose an
>>> endpoint queue stops with -EPROTO or -EPIPE, and before the class driver
>>> gets around to calling usb_clear_halt(), it is unbound.  What happens
>>> the next time a driver binds to the device and tries to use the
>>> endpoint?
>>
>> The disable/enable interface and set config calls during unbind/bind should,
>> if I remember correctly flush pending URBs and drop and re-add the endpoint,
>> clearing the xhci side halt and reset toggle.
> 
> usb_probe_interface() doesn't do any of that stuff, other than a
> deferred switch to altsetting 0 if needed.
> 
> usb_unbind_interface() does call usb_enable_interface() if the interface
> is already in altsetting 0, but the reset_ep argument is false so the
> endpoint state doesn't get affected.  Should that be changed?

Looks like this needs more attention. Interface driver unbind/bind with
halted endpoint could be an issue. I don't have an answer right now.

Thanks
Mathias

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[stern]

On Thu, Apr 02, 2026 at 01:08:31AM +0300, Mathias Nyman wrote:
> On 3/31/26 18:31, stern@rowland.harvard.edu wrote:
> > 
> > How about this instead?  We add a "halted" flag to the usb_host_endpoint
> > structure, and the core will set this flag whenever a bulk or interrupt
> > URB gets a status other than 0 (before putting the URB on the bh list).
> > If an URB has one of these statuses, when its completion handler returns
> > the core will unlink all the URBs queued to the same endpoint.  Finally,
> > the "halted" flag should be cleared after a completion handler returns
> > if there are no more unlinked URBs still in the queue or URBs waiting on
> > the bh list to be given back.
> > 
> > The result of this is that any URB remaining in the queue when the flag
> > is cleared must have been submitted by the class driver _after_ the
> > failing URB's completion handler has run.  We can assume the class
> > driver knows what it's doing in this case.
> > 
> > The endpoint queue shouldn't be restarted until the "halted" flag is
> > cleared.  Either right away, if there are any URBs in the queue, or not
> > until the next URB is submitted.  Doing this might require a new HCD
> > callback.  (It would also mean the kerneldoc for usb_unlink_urb() would
> > need to be updated, because the endpoint might restart before all the
> > completion handlers for the unlinked URBs have run.)
> > 
> > What I'm trying to do here is come up with a single, consistent proposal
> > for exactly when halted endpoint queues should restart.  Maybe someone
> > else has a better suggestion.
> 
> Sounds like a possible solution to me.
> 
> Just to clarify, core should unlink the remaining URBs queued to that endpoint
> after setting the "halted" flag, but before URB completion is called.

Above I wrote that the remaining URBs should be unlinked _after_ the 
completion handler is called.  If we did the unlinks before then the 
class driver might submit a new URB after the unlinks were finished and 
before the completion handler learned about the transaction error, and 
this new URB then wouldn't get unlinked.

It's a race between completion of one URB and submission of another.

> "Halted" flag should be cleared after URB completion returns, and endpoint
> should be restarted if there are any pending URBs.

To be clear, the flag should be cleared after the completion handlers 
for _all_ the unlinked URBs (as well as the URB getting the original 
error) have returned.

> This allows the class driver URB completion handler to re-queue the halted URB
> without core unlinking it.

The completion handler shouldn't do this, because it would mean 
resubmitting without doing a clear-halt first.  (Completion handlers 
can't do clear-halts because they run in atomic context.)  If it does 
try to do this anyway, I see nothing wrong with the core unlinking the 
resubmitted URB.

(Are you going to ask about verification tests that set the endpoint's 
Halt feature, submit an URB, wait for it to fail with -EPIPE, and then 
submit another URB from within the completion handler?  :-) )

> > > > Here's a troubling consequence for people to consider: Suppose an
> > > > endpoint queue stops with -EPROTO or -EPIPE, and before the class driver
> > > > gets around to calling usb_clear_halt(), it is unbound.  What happens
> > > > the next time a driver binds to the device and tries to use the
> > > > endpoint?
> > > 
> > > The disable/enable interface and set config calls during unbind/bind should,
> > > if I remember correctly flush pending URBs and drop and re-add the endpoint,
> > > clearing the xhci side halt and reset toggle.
> > 
> > usb_probe_interface() doesn't do any of that stuff, other than a
> > deferred switch to altsetting 0 if needed.
> > 
> > usb_unbind_interface() does call usb_enable_interface() if the interface
> > is already in altsetting 0, but the reset_ep argument is false so the
> > endpoint state doesn't get affected.  Should that be changed?
> 
> Looks like this needs more attention. Interface driver unbind/bind with
> halted endpoint could be an issue. I don't have an answer right now.

We can ponder it...

Alan Stern

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Thinh Nguyen]

On Wed, Apr 01, 2026, stern@rowland.harvard.edu wrote:
> On Thu, Apr 02, 2026 at 01:08:31AM +0300, Mathias Nyman wrote:
> > On 3/31/26 18:31, stern@rowland.harvard.edu wrote:
> > > 
> > > How about this instead?  We add a "halted" flag to the usb_host_endpoint
> > > structure, and the core will set this flag whenever a bulk or interrupt
> > > URB gets a status other than 0 (before putting the URB on the bh list).
> > > If an URB has one of these statuses, when its completion handler returns
> > > the core will unlink all the URBs queued to the same endpoint.  Finally,
> > > the "halted" flag should be cleared after a completion handler returns
> > > if there are no more unlinked URBs still in the queue or URBs waiting on
> > > the bh list to be given back.
> > > 
> > > The result of this is that any URB remaining in the queue when the flag
> > > is cleared must have been submitted by the class driver _after_ the
> > > failing URB's completion handler has run.  We can assume the class
> > > driver knows what it's doing in this case.
> > > 
> > > The endpoint queue shouldn't be restarted until the "halted" flag is
> > > cleared.  Either right away, if there are any URBs in the queue, or not
> > > until the next URB is submitted.  Doing this might require a new HCD
> > > callback.  (It would also mean the kerneldoc for usb_unlink_urb() would
> > > need to be updated, because the endpoint might restart before all the
> > > completion handlers for the unlinked URBs have run.)
> > > 
> > > What I'm trying to do here is come up with a single, consistent proposal
> > > for exactly when halted endpoint queues should restart.  Maybe someone
> > > else has a better suggestion.
> > 
> > Sounds like a possible solution to me.
> > 
> > Just to clarify, core should unlink the remaining URBs queued to that endpoint
> > after setting the "halted" flag, but before URB completion is called.
> 
> Above I wrote that the remaining URBs should be unlinked _after_ the 
> completion handler is called.  If we did the unlinks before then the 
> class driver might submit a new URB after the unlinks were finished and 
> before the completion handler learned about the transaction error, and 
> this new URB then wouldn't get unlinked.
> 
> It's a race between completion of one URB and submission of another.
> 
> > "Halted" flag should be cleared after URB completion returns, and endpoint
> > should be restarted if there are any pending URBs.
> 
> To be clear, the flag should be cleared after the completion handlers 
> for _all_ the unlinked URBs (as well as the URB getting the original 
> error) have returned.
> 
> > This allows the class driver URB completion handler to re-queue the halted URB
> > without core unlinking it.
> 
> The completion handler shouldn't do this, because it would mean 
> resubmitting without doing a clear-halt first.  (Completion handlers 
> can't do clear-halts because they run in atomic context.)  If it does 
> try to do this anyway, I see nothing wrong with the core unlinking the 
> resubmitted URB.
> 
> (Are you going to ask about verification tests that set the endpoint's 
> Halt feature, submit an URB, wait for it to fail with -EPIPE, and then 
> submit another URB from within the completion handler?  :-) )
> 

How about this:

Introduce a halted flag the following conditions:

* Introduce the halted flag in usb_host_endpoint
* The halted flag must be implemented as a bit in a unsigned long so
  we can use atomic bit operation
* Only the HCD may set the halted flag, and only upon checking the
  first URB completing with a halted status
* Only the USB core may clear the halted flag, and only after
  usb_reset_endpoint returns (this makes sure the HCD drained and reset
  the endpoint before the flag is cleared and new URBs are accepted)
* The usb_reset_endpoint must be called after clear-halt, SetInterface,
  and SetConfiguration.
* 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
* The class driver is responsible to check the halted flag to
  determine whether to initiate error recovery via usb_clear_halt

I'm trying to keep a clear separation of responsibilities between HCD
and the USB core. Also, I try to keep the halted flag more closely match
the state of the endpoint. 

Let me know what you think?

BR,
Thinh

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[stern]

On Fri, Apr 03, 2026 at 01:59:58AM +0000, Thinh Nguyen wrote:
> On Wed, Apr 01, 2026, stern@rowland.harvard.edu wrote:
> > On Thu, Apr 02, 2026 at 01:08:31AM +0300, Mathias Nyman wrote:
> > > On 3/31/26 18:31, stern@rowland.harvard.edu wrote:
> > > > 
> > > > How about this instead?  We add a "halted" flag to the usb_host_endpoint
> > > > structure, and the core will set this flag whenever a bulk or interrupt
> > > > URB gets a status other than 0 (before putting the URB on the bh list).
> > > > If an URB has one of these statuses, when its completion handler returns
> > > > the core will unlink all the URBs queued to the same endpoint.  Finally,
> > > > the "halted" flag should be cleared after a completion handler returns
> > > > if there are no more unlinked URBs still in the queue or URBs waiting on
> > > > the bh list to be given back.
> > > > 
> > > > The result of this is that any URB remaining in the queue when the flag
> > > > is cleared must have been submitted by the class driver _after_ the
> > > > failing URB's completion handler has run.  We can assume the class
> > > > driver knows what it's doing in this case.
> > > > 
> > > > The endpoint queue shouldn't be restarted until the "halted" flag is
> > > > cleared.  Either right away, if there are any URBs in the queue, or not
> > > > until the next URB is submitted.  Doing this might require a new HCD
> > > > callback.  (It would also mean the kerneldoc for usb_unlink_urb() would
> > > > need to be updated, because the endpoint might restart before all the
> > > > completion handlers for the unlinked URBs have run.)
> > > > 
> > > > What I'm trying to do here is come up with a single, consistent proposal
> > > > for exactly when halted endpoint queues should restart.  Maybe someone
> > > > else has a better suggestion.

> How about this:
> 
> Introduce a halted flag the following conditions:
> 
> * Introduce the halted flag in usb_host_endpoint
> * The halted flag must be implemented as a bit in a unsigned long so
>   we can use atomic bit operation

I don't see why it needs to use atomic operations.  Unless you're 
thinking that we might want to add other bitflags into the same unsigned 
long?

> * Only the HCD may set the halted flag, and only upon checking the
>   first URB completing with a halted status

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.

Why do you want the HCD to set the flag instead of 
usb_hcd_giveback_urb()?  Just one central place is simpler than in every 
HCD.

> * Only the USB core may clear the halted flag, and only after
>   usb_reset_endpoint returns (this makes sure the HCD drained and reset
>   the endpoint before the flag is cleared and new URBs are accepted)
> * The usb_reset_endpoint must be called after clear-halt, SetInterface,
>   and SetConfiguration.

That's easy to do just by adding it into the appropriate core routines.

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

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 -- it 
would mean adding error handling to the submission code as well as to 
the completion handler code.

(Actually, that wouldn't matter unless the driver was queuing up 
multiple URBs, in which case it should be prepared to handle submission 
errors in the middle.)

> * The class driver is responsible to check the halted flag to
>   determine whether to initiate error recovery via usb_clear_halt
> 
> I'm trying to keep a clear separation of responsibilities between HCD
> and the USB core. Also, I try to keep the halted flag more closely match
> the state of the endpoint. 
> 
> Let me know what you think?

Making the flag match the endpoint state is a good idea.  But there is 
some ambiguity: Do you mean the state in the device, or the state in the 
HC hardware, or the state in the HCD?  After all, these aren't always 
the same.  For instance, unlinking an URB won't affect the device's 
state but it will affect the state on the host side.

Alan Stern

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Michal Pecio]

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

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[stern]

On Fri, Apr 03, 2026 at 10:51:45AM +0200, Michal Pecio wrote:
> 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.

Truly?  What happens if usb-storage is doing a large bulk-IN transfer 
comprising multiple data URBs (with URB_SHORT_NOT_OK set in all but the 
last one), and the device indicates an early end-of-data by sending a 
short packet in the middle?  Will xhci-hcd store the information in the 
following bulk-IN status transaction to the transfer buffer for the next 
URB in the queue of data URBs?

(That's a misleading question.  The BOT class specification requires the 
device to stall the bulk-IN endpoint following a short data transfer, so 
this scenario wouldn't arise -- although not all devices follow the spec 
in this regard.  And it might arise in other protocols.  Regardless, 
it's hard for me to believe there's no way to tell an xHCI controller to 
stop the endpoint on a short transfer.  EHCI, OHCI, and UHCI all have 
ways to do it.)

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

We have to decide on a single approach that can work for everybody.  If 
it means that some class drivers will have to be changed, so be it.

A lazy driver that just ignores errors will currently not work very well 
if at all following an error on an xHCI host controller.  But yes, it 
will work with a USB-2 controller.

If we want to minimize the changes needed in class drivers then we will 
have to make the core and HCDs recover automatically from transaction 
errors.  Non-lazy class drivers still have a chance to do their own 
recovery when the completion handler for the bad URB runs.  To make 
their lives easier, we could add a usb_purge_endpoint_queue() routine to 
the core; this seems a lot safer than summarily unlinking all the URBs 
all the time.

Is that what we should do?

> > 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.

They might do usb_clear_halt() if the completed URB had -EPIPE status.  
But they won't do it when usb_submit_urb() returns -EPIPE (or any other 
error, for that matter).

> 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?

Good point.

> 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.

Think of it this way: If any URBs remain in the queue when the 
completion handler returns, we can assume the class driver doesn't want 
them to be unlinked.  When that happens, the best thing the core/HCD can 
do is issue a clear-halt and then restart the endpoint, so the queued 
URBs will run.  Except for the -EPIPE case, where the device halted its 
side of the endpoint and sent a STALL packet -- it's reasonable to 
require class drivers to handle that case themselves.

On the other hand, if the queue is empty when the completion handler 
returns (or if all the URBs still in the queue have already been 
unlinked but not yet given back), is there any harm in the core issuing 
a clear-halt and restarting the endpoint (with the same exception as 
above)?  This doesn't allow the class driver to do any further error 
recovery of its own, but when you get down to it, what else is there for 
it to do?

Also, this way we don't have to reject any URBs submitted while endpoint 
is stopped.  They simply get added to the end of the queue, so they will 
run when the endpoint is restarted.

Alan Stern

Re: xhci-hcd and URB_SHORT_NOT_OK

[Michal Pecio]

On Fri, 3 Apr 2026 10:55:58 -0400, stern@rowland.harvard.edu wrote:
> > 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
> > 2. All others log a dbg() message. Supposedly, core is
> >    responsible for updating urb->status then.  
> 
> Truly?  What happens if usb-storage is doing a large bulk-IN transfer
> comprising multiple data URBs (with URB_SHORT_NOT_OK set in all but
> the last one), and the device indicates an early end-of-data by
> sending a short packet in the middle?  Will xhci-hcd store the
> information in the following bulk-IN status transaction to the
> transfer buffer for the next URB in the queue of data URBs?

Not sure if I understand the question, but if you mean that the driver
submits multiple "data" URBs followed by a "status" URB to the same
pipe and expects it to stop when any data URB completes short, so that
all others can be unlinked before the status transfer, then nope, not
gonna happen. It will be racing with status being written to the data
URBs and likely lose. The status URB may never complete.

I really don't recall any way of stopping on short transfers and the
relevant xHCI 4.10.1.1 section isn't helpful. It's possible to stop
unconditionally and decide to restart or not, at some performance cost.

But short transfer always terminates the current URB, so natural way is
to coalesce all data into a big SG URB. Perhaps xhci-hcd could do it
transparently, though that would get awkward if a driver doesn't unlink
the remaining SHORT_NOT_OK URBs - we would need to "rewind".

On the upside, I don't see many users of SHORT_NOT_OK URB. This string
appears in Documentation, include, drivers/usb/host, files ending wih
hcd.c, devio/usbip, and usb_sg_init(). The latter is probably what you
were referring to, and it seems to have a shortcut for SG-capable HCs,
which hopefully doesn't require SHORT_NOT_OK to be functional?

Regards,
Michal

Re: xhci-hcd and URB_SHORT_NOT_OK

[stern]

On Fri, Apr 03, 2026 at 09:13:37PM +0200, Michal Pecio wrote:
> On Fri, 3 Apr 2026 10:55:58 -0400, stern@rowland.harvard.edu wrote:
> > > 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
> > > 2. All others log a dbg() message. Supposedly, core is
> > >    responsible for updating urb->status then.  
> > 
> > Truly?  What happens if usb-storage is doing a large bulk-IN transfer
> > comprising multiple data URBs (with URB_SHORT_NOT_OK set in all but
> > the last one), and the device indicates an early end-of-data by
> > sending a short packet in the middle?  Will xhci-hcd store the
> > information in the following bulk-IN status transaction to the
> > transfer buffer for the next URB in the queue of data URBs?
> 
> Not sure if I understand the question, but if you mean that the driver
> submits multiple "data" URBs followed by a "status" URB to the same
> pipe and expects it to stop when any data URB completes short, so that
> all others can be unlinked before the status transfer, then nope, not
> gonna happen. It will be racing with status being written to the data
> URBs and likely lose. The status URB may never complete.
> 
> I really don't recall any way of stopping on short transfers and the
> relevant xHCI 4.10.1.1 section isn't helpful. It's possible to stop
> unconditionally and decide to restart or not, at some performance cost.
> 
> But short transfer always terminates the current URB, so natural way is
> to coalesce all data into a big SG URB. Perhaps xhci-hcd could do it
> transparently, though that would get awkward if a driver doesn't unlink
> the remaining SHORT_NOT_OK URBs - we would need to "rewind".
> 
> On the upside, I don't see many users of SHORT_NOT_OK URB. This string
> appears in Documentation, include, drivers/usb/host, files ending wih
> hcd.c, devio/usbip, and usb_sg_init(). The latter is probably what you
> were referring to, and it seems to have a shortcut for SG-capable HCs,
> which hopefully doesn't require SHORT_NOT_OK to be functional?

usb_sg_init() _is_ what I was thinking of, and I had forgotten about the 
single-URB shortcut for SG-capable HCs.

Clearly we're also going to have to update the kerneldoc, not just the 
code.  If an HCD doesn't support SG then it needs to support stopping an 
endpoint on short transfers.

Alan Stern

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Thinh Nguyen]

On Thu, Apr 02, 2026, stern@rowland.harvard.edu wrote:
> On Fri, Apr 03, 2026 at 01:59:58AM +0000, Thinh Nguyen wrote:
> > On Wed, Apr 01, 2026, stern@rowland.harvard.edu wrote:
> > > On Thu, Apr 02, 2026 at 01:08:31AM +0300, Mathias Nyman wrote:
> > > > On 3/31/26 18:31, stern@rowland.harvard.edu wrote:
> > > > > 
> > > > > How about this instead?  We add a "halted" flag to the usb_host_endpoint
> > > > > structure, and the core will set this flag whenever a bulk or interrupt
> > > > > URB gets a status other than 0 (before putting the URB on the bh list).
> > > > > If an URB has one of these statuses, when its completion handler returns
> > > > > the core will unlink all the URBs queued to the same endpoint.  Finally,
> > > > > the "halted" flag should be cleared after a completion handler returns
> > > > > if there are no more unlinked URBs still in the queue or URBs waiting on
> > > > > the bh list to be given back.
> > > > > 
> > > > > The result of this is that any URB remaining in the queue when the flag
> > > > > is cleared must have been submitted by the class driver _after_ the
> > > > > failing URB's completion handler has run.  We can assume the class
> > > > > driver knows what it's doing in this case.
> > > > > 
> > > > > The endpoint queue shouldn't be restarted until the "halted" flag is
> > > > > cleared.  Either right away, if there are any URBs in the queue, or not
> > > > > until the next URB is submitted.  Doing this might require a new HCD
> > > > > callback.  (It would also mean the kerneldoc for usb_unlink_urb() would
> > > > > need to be updated, because the endpoint might restart before all the
> > > > > completion handlers for the unlinked URBs have run.)
> > > > > 
> > > > > What I'm trying to do here is come up with a single, consistent proposal
> > > > > for exactly when halted endpoint queues should restart.  Maybe someone
> > > > > else has a better suggestion.
> 
> > How about this:
> > 
> > Introduce a halted flag the following conditions:
> > 
> > * Introduce the halted flag in usb_host_endpoint
> > * The halted flag must be implemented as a bit in a unsigned long so
> >   we can use atomic bit operation
> 
> I don't see why it needs to use atomic operations.  Unless you're 
> thinking that we might want to add other bitflags into the same unsigned 
> long?

Now that I think about it again, it's not needed if we have the
requirement for clearing the flag only after usb_reset_endpoint.

> 
> > * Only the HCD may set the halted flag, and only upon checking the
> >   first URB completing with a halted status
> 
> 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.

It seems we're using the "halted" flag for different things. The halted
flag to me is the endpoint's state and rather than the endpoint queue
state. That is, if the endpoint is halted, there's an error that was
reported on the pipe. So, an -ECONNRESET would not cause a halted
endpoint.

> 
> Why do you want the HCD to set the flag instead of 
> usb_hcd_giveback_urb()?  Just one central place is simpler than in every 
> HCD.

I'm just thinking in term of whose role to return the pending URBs.
Typically the HCD handles when to give back URBs. If the HCD were to
handle giving back pending URBs due to halted endpoint, then it should
be the one to set the halted flag. However, if the core tracks and does
both setting and clearing of the halted flag, then the core should
handle canceling the URBs. As you said, if we can shift the burden to
the core, that would help keep the flow consistent and centralized.

> 
> > * Only the USB core may clear the halted flag, and only after
> >   usb_reset_endpoint returns (this makes sure the HCD drained and reset
> >   the endpoint before the flag is cleared and new URBs are accepted)
> > * The usb_reset_endpoint must be called after clear-halt, SetInterface,
> >   and SetConfiguration.
> 
> That's easy to do just by adding it into the appropriate core routines.

Yes.

> 
> > * 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.
> 
> 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 -- it 
> would mean adding error handling to the submission code as well as to 
> the completion handler code.
> 
> (Actually, that wouldn't matter unless the driver was queuing up 
> multiple URBs, in which case it should be prepared to handle submission 
> errors in the middle.)

I didn't think about failing immediately on submission because we would
need to audit every class driver for that. Perhaps the core can
intercept the URBs and immediately schedule a completion handler with
the updated URB's status?

> 
> > * The class driver is responsible to check the halted flag to
> >   determine whether to initiate error recovery via usb_clear_halt
> > 
> > I'm trying to keep a clear separation of responsibilities between HCD
> > and the USB core. Also, I try to keep the halted flag more closely match
> > the state of the endpoint. 
> > 
> > Let me know what you think?
> 
> Making the flag match the endpoint state is a good idea.  But there is 
> some ambiguity: Do you mean the state in the device, or the state in the 
> HC hardware, or the state in the HCD?  After all, these aren't always 
> the same.  For instance, unlinking an URB won't affect the device's 
> state but it will affect the state on the host side.
> 

The HCD state. If we can let the core manage the halted state, then here
are the points where the halted state is updated (hopefully I didn't
miss any other ones):

Endpoint is halted (reported by HCD):
* URB returned with -EPROTO or -EPIPE

Endpoint is active (updated by the core):
* SetConfiguration, SetInterface, clear-halt

Thanks,
Thinh

ps. I'll be out of town next week. Apologies if my response will be
delayed...

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[stern]

On Sat, Apr 04, 2026 at 01:15:52AM +0000, Thinh Nguyen wrote:
> > > How about this:
> > > 
> > > Introduce a halted flag the following conditions:
> > > 
> > > * Introduce the halted flag in usb_host_endpoint
> > > * The halted flag must be implemented as a bit in a unsigned long so
> > >   we can use atomic bit operation
> > 
> > I don't see why it needs to use atomic operations.  Unless you're 
> > thinking that we might want to add other bitflags into the same unsigned 
> > long?
> 
> Now that I think about it again, it's not needed if we have the
> requirement for clearing the flag only after usb_reset_endpoint.
> 
> > 
> > > * Only the HCD may set the halted flag, and only upon checking the
> > >   first URB completing with a halted status
> > 
> > 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.
> 
> It seems we're using the "halted" flag for different things. The halted
> flag to me is the endpoint's state and rather than the endpoint queue
> state. That is, if the endpoint is halted, there's an error that was
> reported on the pipe. So, an -ECONNRESET would not cause a halted
> endpoint.
> 
> > 
> > Why do you want the HCD to set the flag instead of 
> > usb_hcd_giveback_urb()?  Just one central place is simpler than in every 
> > HCD.
> 
> I'm just thinking in term of whose role to return the pending URBs.
> Typically the HCD handles when to give back URBs. If the HCD were to
> handle giving back pending URBs due to halted endpoint, then it should
> be the one to set the halted flag. However, if the core tracks and does
> both setting and clearing of the halted flag, then the core should
> handle canceling the URBs. As you said, if we can shift the burden to
> the core, that would help keep the flow consistent and centralized.
> 
> > 
> > > * Only the USB core may clear the halted flag, and only after
> > >   usb_reset_endpoint returns (this makes sure the HCD drained and reset
> > >   the endpoint before the flag is cleared and new URBs are accepted)
> > > * The usb_reset_endpoint must be called after clear-halt, SetInterface,
> > >   and SetConfiguration.
> > 
> > That's easy to do just by adding it into the appropriate core routines.
> 
> Yes.
> 
> > 
> > > * 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.
> > 
> > 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 -- it 
> > would mean adding error handling to the submission code as well as to 
> > the completion handler code.
> > 
> > (Actually, that wouldn't matter unless the driver was queuing up 
> > multiple URBs, in which case it should be prepared to handle submission 
> > errors in the middle.)
> 
> I didn't think about failing immediately on submission because we would
> need to audit every class driver for that. Perhaps the core can
> intercept the URBs and immediately schedule a completion handler with
> the updated URB's status?
> 
> > 
> > > * The class driver is responsible to check the halted flag to
> > >   determine whether to initiate error recovery via usb_clear_halt
> > > 
> > > I'm trying to keep a clear separation of responsibilities between HCD
> > > and the USB core. Also, I try to keep the halted flag more closely match
> > > the state of the endpoint. 
> > > 
> > > Let me know what you think?
> > 
> > Making the flag match the endpoint state is a good idea.  But there is 
> > some ambiguity: Do you mean the state in the device, or the state in the 
> > HC hardware, or the state in the HCD?  After all, these aren't always 
> > the same.  For instance, unlinking an URB won't affect the device's 
> > state but it will affect the state on the host side.
> > 
> 
> The HCD state. If we can let the core manage the halted state, then here
> are the points where the halted state is updated (hopefully I didn't
> miss any other ones):
> 
> Endpoint is halted (reported by HCD):
> * URB returned with -EPROTO or -EPIPE
> 
> Endpoint is active (updated by the core):
> * SetConfiguration, SetInterface, clear-halt

Hmmm.  What did you think of my recent proposal in a message to Michal?  
Summarizing:

If the class driver wants to unlink queued URBs when a transaction error 
occurs, it has to do so itself in the failed URB's completion handler.  
We can make this easier by adding a usb_flush_endpoint_queue() routine 
to the core.  In the meantime, the HCD ensures that the queue is stopped 
before giving back the URB.  (Note: -EPIPE, -ENOENT, -ECONNRESET, and 
-EREMOTEIO aren't considered to be transaction errors.)

When the completion handler returns, the core will automatically call 
usb_clear_halt(), which will also reset the endpoint on the host side 
and will restart the queue.  This also happens after SetConfiguration 
and SetInterface, either explicitly or implicitly.

For -EPIPE (device sent a STALL token), the class driver has to clear 
the halt itself.  This is because stalls aren't errors; they are an 
intentional part of the USB protocol.

-ENOENT and -ECONNRESET (URB was unlinked) and -EREMOTEIO (short packet 
received with URB_SHORT_NOT_OK set) are a little trickier.  The HCD may 
or may not need to stop the queue for an unlink, possibly depending on 
whether the URB being unlinked is the active one.  When a short packet 
is received, the HC hardware may or may not stop its queue.  Either way, 
the class driver shouldn't need to take any special recovery action; any 
necessary actions should be taken automatically by the HCD and the core.

All of this applies only to bulk and interrupt endpoints.  Control 
endpoints generally need error recovery only on the host side, because 
the device resets automatically when it gets a new SETUP packet, and so 
the HCD should handle whatever is needed.  Isochronous endpoints don't 
ever halt and they shouldn't need to be reset when an error occurs.

Overall, this seems simpler than anything else we have discussed.

Alan Stern

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Thinh Nguyen]

On Fri, Apr 03, 2026, stern@rowland.harvard.edu wrote:
> On Sat, Apr 04, 2026 at 01:15:52AM +0000, Thinh Nguyen wrote:
> > > > How about this:
> > > > 
> > > > Introduce a halted flag the following conditions:
> > > > 
> > > > * Introduce the halted flag in usb_host_endpoint
> > > > * The halted flag must be implemented as a bit in a unsigned long so
> > > >   we can use atomic bit operation
> > > 
> > > I don't see why it needs to use atomic operations.  Unless you're 
> > > thinking that we might want to add other bitflags into the same unsigned 
> > > long?
> > 
> > Now that I think about it again, it's not needed if we have the
> > requirement for clearing the flag only after usb_reset_endpoint.
> > 
> > > 
> > > > * Only the HCD may set the halted flag, and only upon checking the
> > > >   first URB completing with a halted status
> > > 
> > > 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.
> > 
> > It seems we're using the "halted" flag for different things. The halted
> > flag to me is the endpoint's state and rather than the endpoint queue
> > state. That is, if the endpoint is halted, there's an error that was
> > reported on the pipe. So, an -ECONNRESET would not cause a halted
> > endpoint.
> > 
> > > 
> > > Why do you want the HCD to set the flag instead of 
> > > usb_hcd_giveback_urb()?  Just one central place is simpler than in every 
> > > HCD.
> > 
> > I'm just thinking in term of whose role to return the pending URBs.
> > Typically the HCD handles when to give back URBs. If the HCD were to
> > handle giving back pending URBs due to halted endpoint, then it should
> > be the one to set the halted flag. However, if the core tracks and does
> > both setting and clearing of the halted flag, then the core should
> > handle canceling the URBs. As you said, if we can shift the burden to
> > the core, that would help keep the flow consistent and centralized.
> > 
> > > 
> > > > * Only the USB core may clear the halted flag, and only after
> > > >   usb_reset_endpoint returns (this makes sure the HCD drained and reset
> > > >   the endpoint before the flag is cleared and new URBs are accepted)
> > > > * The usb_reset_endpoint must be called after clear-halt, SetInterface,
> > > >   and SetConfiguration.
> > > 
> > > That's easy to do just by adding it into the appropriate core routines.
> > 
> > Yes.
> > 
> > > 
> > > > * 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.
> > > 
> > > 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 -- it 
> > > would mean adding error handling to the submission code as well as to 
> > > the completion handler code.
> > > 
> > > (Actually, that wouldn't matter unless the driver was queuing up 
> > > multiple URBs, in which case it should be prepared to handle submission 
> > > errors in the middle.)
> > 
> > I didn't think about failing immediately on submission because we would
> > need to audit every class driver for that. Perhaps the core can
> > intercept the URBs and immediately schedule a completion handler with
> > the updated URB's status?
> > 
> > > 
> > > > * The class driver is responsible to check the halted flag to
> > > >   determine whether to initiate error recovery via usb_clear_halt
> > > > 
> > > > I'm trying to keep a clear separation of responsibilities between HCD
> > > > and the USB core. Also, I try to keep the halted flag more closely match
> > > > the state of the endpoint. 
> > > > 
> > > > Let me know what you think?
> > > 
> > > Making the flag match the endpoint state is a good idea.  But there is 
> > > some ambiguity: Do you mean the state in the device, or the state in the 
> > > HC hardware, or the state in the HCD?  After all, these aren't always 
> > > the same.  For instance, unlinking an URB won't affect the device's 
> > > state but it will affect the state on the host side.
> > > 
> > 
> > The HCD state. If we can let the core manage the halted state, then here
> > are the points where the halted state is updated (hopefully I didn't
> > miss any other ones):
> > 
> > Endpoint is halted (reported by HCD):
> > * URB returned with -EPROTO or -EPIPE
> > 
> > Endpoint is active (updated by the core):
> > * SetConfiguration, SetInterface, clear-halt
> 
> Hmmm.  What did you think of my recent proposal in a message to Michal?  
> Summarizing:
> 
> If the class driver wants to unlink queued URBs when a transaction error 
> occurs, it has to do so itself in the failed URB's completion handler.  
> We can make this easier by adding a usb_flush_endpoint_queue() routine 
> to the core.  In the meantime, the HCD ensures that the queue is stopped 
> before giving back the URB.  (Note: -EPIPE, -ENOENT, -ECONNRESET, and 
> -EREMOTEIO aren't considered to be transaction errors.)
> 
> When the completion handler returns, the core will automatically call 
> usb_clear_halt(), which will also reset the endpoint on the host side 
> and will restart the queue.  This also happens after SetConfiguration 
> and SetInterface, either explicitly or implicitly.

I like that the core will handle this automatically. But one concern:
How will the class driver know when the clear-halt complete so it can
perform the recovery? (ie. it shouldn't perform recovery immediately
after seeing -EPROTO)

> 
> For -EPIPE (device sent a STALL token), the class driver has to clear 
> the halt itself.  This is because stalls aren't errors; they are an 
> intentional part of the USB protocol.
> 
> -ENOENT and -ECONNRESET (URB was unlinked) and -EREMOTEIO (short packet 
> received with URB_SHORT_NOT_OK set) are a little trickier.  The HCD may 
> or may not need to stop the queue for an unlink, possibly depending on 
> whether the URB being unlinked is the active one.  When a short packet 
> is received, the HC hardware may or may not stop its queue.  Either way, 
> the class driver shouldn't need to take any special recovery action; any 
> necessary actions should be taken automatically by the HCD and the core.
> 
> All of this applies only to bulk and interrupt endpoints.  Control 
> endpoints generally need error recovery only on the host side, because 
> the device resets automatically when it gets a new SETUP packet, and so 
> the HCD should handle whatever is needed.  Isochronous endpoints don't 
> ever halt and they shouldn't need to be reset when an error occurs.
> 
> Overall, this seems simpler than anything else we have discussed.
> 

The rest sounds good to me!

Thanks Alan,
Thinh

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Alan Stern]

On Sat, Apr 04, 2026 at 08:41:36PM +0000, Thinh Nguyen wrote:
> On Fri, Apr 03, 2026, stern@rowland.harvard.edu wrote:
> > Summarizing:
> > 
> > If the class driver wants to unlink queued URBs when a transaction error 
> > occurs, it has to do so itself in the failed URB's completion handler.  
> > We can make this easier by adding a usb_flush_endpoint_queue() routine 
> > to the core.  In the meantime, the HCD ensures that the queue is stopped 
> > before giving back the URB.  (Note: -EPIPE, -ENOENT, -ECONNRESET, and 
> > -EREMOTEIO aren't considered to be transaction errors.)
> > 
> > When the completion handler returns, the core will automatically call 
> > usb_clear_halt(), which will also reset the endpoint on the host side 
> > and will restart the queue.  This also happens after SetConfiguration 
> > and SetInterface, either explicitly or implicitly.
> 
> I like that the core will handle this automatically. But one concern:
> How will the class driver know when the clear-halt complete so it can
> perform the recovery? (ie. it shouldn't perform recovery immediately
> after seeing -EPROTO)

It doesn't know, and it doesn't need to know.  Any recovery URBs the 
class driver wants to send can be submitted as usual, and they will be 
added onto the queue.  When the core resets the endpoint, the queue will 
start going again and the URBs will run.

If the class driver wants to take some other action (like submitting 
URBs to a different endpoint) before using the endpoint that stopped, 
it's free to do so.  It only has to make sure that it doesn't submit any 
URBs to the stopped endpoint until after the other action is finished -- 
which is what it would do anyway.  (And maybe it has to unlink any URBs 
that are already queued, which can be done with a simple function call.)

> > For -EPIPE (device sent a STALL token), the class driver has to clear 
> > the halt itself.  This is because stalls aren't errors; they are an 
> > intentional part of the USB protocol.
> > 
> > -ENOENT and -ECONNRESET (URB was unlinked) and -EREMOTEIO (short packet 
> > received with URB_SHORT_NOT_OK set) are a little trickier.  The HCD may 
> > or may not need to stop the queue for an unlink, possibly depending on 
> > whether the URB being unlinked is the active one.  When a short packet 
> > is received, the HC hardware may or may not stop its queue.  Either way, 
> > the class driver shouldn't need to take any special recovery action; any 
> > necessary actions should be taken automatically by the HCD and the core.
> > 
> > All of this applies only to bulk and interrupt endpoints.  Control 
> > endpoints generally need error recovery only on the host side, because 
> > the device resets automatically when it gets a new SETUP packet, and so 
> > the HCD should handle whatever is needed.  Isochronous endpoints don't 
> > ever halt and they shouldn't need to be reset when an error occurs.
> > 
> > Overall, this seems simpler than anything else we have discussed.
> > 
> 
> The rest sounds good to me!

Good!  Let's wait to hear from Michal, Mathias, and Oliver.

Alan

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Thinh Nguyen]

On Sat, Apr 04, 2026, Alan Stern wrote:
> On Sat, Apr 04, 2026 at 08:41:36PM +0000, Thinh Nguyen wrote:
> > On Fri, Apr 03, 2026, stern@rowland.harvard.edu wrote:
> > > Summarizing:
> > > 
> > > If the class driver wants to unlink queued URBs when a transaction error 
> > > occurs, it has to do so itself in the failed URB's completion handler.  
> > > We can make this easier by adding a usb_flush_endpoint_queue() routine 
> > > to the core.  In the meantime, the HCD ensures that the queue is stopped 
> > > before giving back the URB.  (Note: -EPIPE, -ENOENT, -ECONNRESET, and 
> > > -EREMOTEIO aren't considered to be transaction errors.)
> > > 
> > > When the completion handler returns, the core will automatically call 
> > > usb_clear_halt(), which will also reset the endpoint on the host side 
> > > and will restart the queue.  This also happens after SetConfiguration 
> > > and SetInterface, either explicitly or implicitly.
> > 
> > I like that the core will handle this automatically. But one concern:
> > How will the class driver know when the clear-halt complete so it can
> > perform the recovery? (ie. it shouldn't perform recovery immediately
> > after seeing -EPROTO)
> 
> It doesn't know, and it doesn't need to know.  Any recovery URBs the 
> class driver wants to send can be submitted as usual, and they will be 
> added onto the queue.  When the core resets the endpoint, the queue will 
> start going again and the URBs will run.
> 
> If the class driver wants to take some other action (like submitting 
> URBs to a different endpoint) before using the endpoint that stopped, 
> it's free to do so.  It only has to make sure that it doesn't submit any 
> URBs to the stopped endpoint until after the other action is finished -- 
> which is what it would do anyway.  (And maybe it has to unlink any URBs 
> that are already queued, which can be done with a simple function call.)
> 

Then the xhci must make sure that it should not ring the doorbell to
restart the endpoint when giving back the canceled URBs. It should only
do so on newly submitted URBs.

We can add a requirement such that if the class driver submitted the
recovery URBs prior to completing the usb_reset_endpoint (which should
be done after clear-halt), then the HCD may keep those URBs on a queue
and only process those URBs and restart the endpoint afterward.

BR,
Thinh

> > > For -EPIPE (device sent a STALL token), the class driver has to clear 
> > > the halt itself.  This is because stalls aren't errors; they are an 
> > > intentional part of the USB protocol.
> > > 
> > > -ENOENT and -ECONNRESET (URB was unlinked) and -EREMOTEIO (short packet 
> > > received with URB_SHORT_NOT_OK set) are a little trickier.  The HCD may 
> > > or may not need to stop the queue for an unlink, possibly depending on 
> > > whether the URB being unlinked is the active one.  When a short packet 
> > > is received, the HC hardware may or may not stop its queue.  Either way, 
> > > the class driver shouldn't need to take any special recovery action; any 
> > > necessary actions should be taken automatically by the HCD and the core.
> > > 
> > > All of this applies only to bulk and interrupt endpoints.  Control 
> > > endpoints generally need error recovery only on the host side, because 
> > > the device resets automatically when it gets a new SETUP packet, and so 
> > > the HCD should handle whatever is needed.  Isochronous endpoints don't 
> > > ever halt and they shouldn't need to be reset when an error occurs.
> > > 
> > > Overall, this seems simpler than anything else we have discussed.
> > > 
> > 
> > The rest sounds good to me!
> 
> Good!  Let's wait to hear from Michal, Mathias, and Oliver.
> 
> Alan

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Thinh Nguyen]

On Sat, Apr 04, 2026, Thinh Nguyen wrote:
> On Sat, Apr 04, 2026, Alan Stern wrote:
> > On Sat, Apr 04, 2026 at 08:41:36PM +0000, Thinh Nguyen wrote:
> > > On Fri, Apr 03, 2026, stern@rowland.harvard.edu wrote:
> > > > Summarizing:
> > > > 
> > > > If the class driver wants to unlink queued URBs when a transaction error 
> > > > occurs, it has to do so itself in the failed URB's completion handler.  
> > > > We can make this easier by adding a usb_flush_endpoint_queue() routine 
> > > > to the core.  In the meantime, the HCD ensures that the queue is stopped 
> > > > before giving back the URB.  (Note: -EPIPE, -ENOENT, -ECONNRESET, and 
> > > > -EREMOTEIO aren't considered to be transaction errors.)
> > > > 
> > > > When the completion handler returns, the core will automatically call 
> > > > usb_clear_halt(), which will also reset the endpoint on the host side 
> > > > and will restart the queue.  This also happens after SetConfiguration 
> > > > and SetInterface, either explicitly or implicitly.
> > > 
> > > I like that the core will handle this automatically. But one concern:
> > > How will the class driver know when the clear-halt complete so it can
> > > perform the recovery? (ie. it shouldn't perform recovery immediately
> > > after seeing -EPROTO)
> > 
> > It doesn't know, and it doesn't need to know.  Any recovery URBs the 
> > class driver wants to send can be submitted as usual, and they will be 
> > added onto the queue.  When the core resets the endpoint, the queue will 
> > start going again and the URBs will run.
> > 
> > If the class driver wants to take some other action (like submitting 
> > URBs to a different endpoint) before using the endpoint that stopped, 
> > it's free to do so.  It only has to make sure that it doesn't submit any 
> > URBs to the stopped endpoint until after the other action is finished -- 
> > which is what it would do anyway.  (And maybe it has to unlink any URBs 
> > that are already queued, which can be done with a simple function call.)
> > 
> 
> Then the xhci must make sure that it should not ring the doorbell to
> restart the endpoint when giving back the canceled URBs. It should only
> do so on newly submitted URBs.

Ignore this comment, it's not restarting the endpoint in the case of
unlinking.

> 
> We can add a requirement such that if the class driver submitted the
> recovery URBs prior to completing the usb_reset_endpoint (which should
> be done after clear-halt), then the HCD may keep those URBs on a queue
> and only process those URBs and restart the endpoint afterward.
> 

Actually, adding this new requirement would be tricky because we don't
know whether it's recovery URBs or not.

BR,
Thinh

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Alan Stern]

On Sat, Apr 04, 2026 at 10:28:24PM +0000, Thinh Nguyen wrote:
> On Sat, Apr 04, 2026, Thinh Nguyen wrote:
> > On Sat, Apr 04, 2026, Alan Stern wrote:
> > > If the class driver wants to take some other action (like submitting 
> > > URBs to a different endpoint) before using the endpoint that stopped, 
> > > it's free to do so.  It only has to make sure that it doesn't submit any 
> > > URBs to the stopped endpoint until after the other action is finished -- 
> > > which is what it would do anyway.  (And maybe it has to unlink any URBs 
> > > that are already queued, which can be done with a simple function call.)
> > > 
> > 
> > Then the xhci must make sure that it should not ring the doorbell to
> > restart the endpoint when giving back the canceled URBs. It should only
> > do so on newly submitted URBs.
> 
> Ignore this comment, it's not restarting the endpoint in the case of
> unlinking.

I was going to say that xhci-hcd shouldn't restart the endpoint until 
the usb_reset_endpoint() call is made.  Whether or not it rings the 
doorbell at that time may depend on whether there are any URBs on the 
queue; that's a relatively unimportant implementation detail.

> > We can add a requirement such that if the class driver submitted the
> > recovery URBs prior to completing the usb_reset_endpoint (which should
> > be done after clear-halt), then the HCD may keep those URBs on a queue
> > and only process those URBs and restart the endpoint afterward.
> > 
> 
> Actually, adding this new requirement would be tricky because we don't
> know whether it's recovery URBs or not.

The purpose of the submitted URBs doesn't matter.  The HCD shouldn't 
restart the endpoint until the usb_reset_endpoint() call occurs.

Also, I should point out that usbcore will call usb_clear_halt() and 
usb_reset_endpoint(), presumably using a work queue for the calls.  The 
class driver doesn't need to do it -- in fact, doing those things could 
lead to errors because the endpoint may already be running (the core may 
already have made the calls).

Alan Stern

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Thinh Nguyen]

On Sat, Apr 04, 2026, Alan Stern wrote:
> On Sat, Apr 04, 2026 at 10:28:24PM +0000, Thinh Nguyen wrote:
> > On Sat, Apr 04, 2026, Thinh Nguyen wrote:
> > > On Sat, Apr 04, 2026, Alan Stern wrote:
> > > > If the class driver wants to take some other action (like submitting 
> > > > URBs to a different endpoint) before using the endpoint that stopped, 
> > > > it's free to do so.  It only has to make sure that it doesn't submit any 
> > > > URBs to the stopped endpoint until after the other action is finished -- 
> > > > which is what it would do anyway.  (And maybe it has to unlink any URBs 
> > > > that are already queued, which can be done with a simple function call.)
> > > > 
> > > 
> > > Then the xhci must make sure that it should not ring the doorbell to
> > > restart the endpoint when giving back the canceled URBs. It should only
> > > do so on newly submitted URBs.
> > 
> > Ignore this comment, it's not restarting the endpoint in the case of
> > unlinking.
> 
> I was going to say that xhci-hcd shouldn't restart the endpoint until 
> the usb_reset_endpoint() call is made.  Whether or not it rings the 
> doorbell at that time may depend on whether there are any URBs on the 
> queue; that's a relatively unimportant implementation detail.
> 
> > > We can add a requirement such that if the class driver submitted the
> > > recovery URBs prior to completing the usb_reset_endpoint (which should
> > > be done after clear-halt), then the HCD may keep those URBs on a queue
> > > and only process those URBs and restart the endpoint afterward.
> > > 
> > 
> > Actually, adding this new requirement would be tricky because we don't
> > know whether it's recovery URBs or not.
> 
> The purpose of the submitted URBs doesn't matter.  The HCD shouldn't 
> restart the endpoint until the usb_reset_endpoint() call occurs.
> 
> Also, I should point out that usbcore will call usb_clear_halt() and 
> usb_reset_endpoint(), presumably using a work queue for the calls.  The 
> class driver doesn't need to do it -- in fact, doing those things could 
> lead to errors because the endpoint may already be running (the core may 
> already have made the calls).
> 

That's good. This is what I wanted to confirm.

May need to update how xhci handles usb_reset_endpoint() because I
believe it's expected the endpoint transfer ring to be drained prior (by
the class driver unlinking URBs).

Thanks for the clarifications,
Thinh

ps. Have a good weekend! I'll be back a week after.

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Alan Stern]

It's been a while now, and nobody has objected to the proposed plan for 
handling this issue, so I'm going to assume that everyone is on board 
with the idea.

There is a loose end still to be straightened out.  It concerns handling 
of -EREMOTEIO errors (short packet received with URB_SHORT_NOT_OK set).  
While some HCDs -- especially those supporting SG -- may not stop the 
endpoint queue when this error occurs, other HCDs will do so.  The 
question is how the core should tell them to start it up again.  This 
shouldn't happen until after the completion handler returns.

Short packets don't cause any loss of synchronization between the 
endpoint state on the host and on the device, so -EREMOTEIO doesn't 
require usb_clear_halt() or usb_reset_endpoint() for recovery.  This 
means we need to find some other way to tell the HCD when the queue can 
restart.  Should we create a new hc_driver callback specifically for 
this purpose?

Related question: Although URB_SHORT_NOT_OK is allowed for all types of 
IN URB other than isochronous, does its queue-stopping property make 
sense for control or interrupt URBs?  As far as I know, no kernel code 
uses it for them, although some userspace code might (via usbfs).

Alan Stern

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Mathias Nyman]

On 4/7/26 18:23, Alan Stern wrote:
> It's been a while now, and nobody has objected to the proposed plan for
> handling this issue, so I'm going to assume that everyone is on board
> with the idea.

Yes, I support this

So basically usb core will call usb_clear_halt() after EPROTO URB completion
handler finishes, and xhci-hcd needs to prevent bulk/interrupt endpoint
from restarting after returning a EPROTO URB up until usb_reset_endpoint()
is called

I also support adding usb_purge_endpoint_queue(), but it doesn't have to
be done at the same time as the EPROTO changes.

> 
> There is a loose end still to be straightened out.  It concerns handling
> of -EREMOTEIO errors (short packet received with URB_SHORT_NOT_OK set).
> While some HCDs -- especially those supporting SG -- may not stop the
> endpoint queue when this error occurs, other HCDs will do so.  The
> question is how the core should tell them to start it up again.  This
> shouldn't happen until after the completion handler returns.
> 
> Short packets don't cause any loss of synchronization between the
> endpoint state on the host and on the device, so -EREMOTEIO doesn't
> require usb_clear_halt() or usb_reset_endpoint() for recovery.  This
> means we need to find some other way to tell the HCD when the queue can
> restart.  Should we create a new hc_driver callback specifically for
> this purpose?

For xHC the issue is the other way around as Michal pointed out.

Can't find a way to stop the endpoint on short packets.
Can only manually stop the endpoint when xhci-hcd detects the short transfer
event, when xHC likely already processing the next URB.

Thanks
Mathias

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Alan Stern]

On Tue, Apr 07, 2026 at 11:24:01PM +0300, Mathias Nyman wrote:
> On 4/7/26 18:23, Alan Stern wrote:
> > It's been a while now, and nobody has objected to the proposed plan for
> > handling this issue, so I'm going to assume that everyone is on board
> > with the idea.
> 
> Yes, I support this
> 
> So basically usb core will call usb_clear_halt() after EPROTO URB completion
> handler finishes, and xhci-hcd needs to prevent bulk/interrupt endpoint
> from restarting after returning a EPROTO URB up until usb_reset_endpoint()
> is called
> 
> I also support adding usb_purge_endpoint_queue(), but it doesn't have to
> be done at the same time as the EPROTO changes.

I've started working on an implementation of this, and there are a few 
edge cases.  Can anyone answer these questions?

The behavior for control endpoints isn't clear.  When a transaction 
error occurs in a control transfer, does xHCI stop the endpoint queue?  
Presumably usb_clear_halt() isn't needed, but what about 
usb_reset_endpoint()?

I will assume that all stalls for control endpoints are protocol stalls 
and therefore do not need to be cleared (the device will automatically 
clear the halt condition when it receives the next SETUP packet for the 
endpoint).  But does xhci-hcd still require a usb_reset_endpoint() 
after a control stall?

Recovery from a transaction error on a bulk or interrupt endpoint 
involves sending a Clear-Halt request to the device.  But if the error 
was caused by some sort of transient interference that hasn't ended yet, 
the Clear-Halt might itself fail with the same error.  To handle this, 
we should retry the Clear-Halt at increasing time intervals.  At what 
point should the core give up?

When the error recovery mechanism is in place and working, should 
usb_bulk_msg() and friends automatically retry when a transaction error 
occurs?  If they do, how many times?

Alan Stern

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Michal Pecio]

On Fri, 17 Apr 2026 13:38:46 -0400, Alan Stern wrote:
> The behavior for control endpoints isn't clear.  When a transaction 
> error occurs in a control transfer, does xHCI stop the endpoint
> queue? Presumably usb_clear_halt() isn't needed, but what about 
> usb_reset_endpoint()?
> 
> I will assume that all stalls for control endpoints are protocol
> stalls and therefore do not need to be cleared (the device will
> automatically clear the halt condition when it receives the next
> SETUP packet for the endpoint).  But does xhci-hcd still require a
> usb_reset_endpoint() after a control stall?

Nope.

The host endpoint halts on every STALL handshake or non-isoc protocol
error, but the driver immediately resets every halted endpoint because
it's a precondition to even remove the failed URB from the HW queue
before giving it back, which looks like a prudent thing to do.

Currently, by the time the URB is given back, its endpoint is already
in a "stopped but runnable" state and its sequence state is zeroed.
And it may have already been restarted if there are more pending URBs.

This seems to need no change for control endpoints?

For bulk/interrupt it also means that if we delay the restart until
endpoint_reset() then endpoint_reset() has nothing to do besides
restarting the endpoint, which is a good thing because this method
under normal conditions requires no URBs to be present.

> Recovery from a transaction error on a bulk or interrupt endpoint 
> involves sending a Clear-Halt request to the device.  But if the
> error was caused by some sort of transient interference that hasn't
> ended yet, the Clear-Halt might itself fail with the same error. To
> handle this, we should retry the Clear-Halt at increasing time
> intervals.  At what point should the core give up?

Good question, I don't know. One thing I noticed is that Windows does
tend to lose patience with completely unresponsive devices and kicks
them out, but I don't know the exact criteria.

And if core is to clear halt autonomously, things may get interesting
if a driver tries to do the same, or something else like removing the
endpoint with usb_set_interface().

A related issue is clearing TT buffers. AFAIK this has no retries, it
fails silently and leaves the endpoint potentially broken, and it is
waited for to complete in case of usb_set_interface().

Regards,
Michal

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Alan Stern]

On Fri, Apr 17, 2026 at 11:48:46PM +0200, Michal Pecio wrote:
> On Fri, 17 Apr 2026 13:38:46 -0400, Alan Stern wrote:
> > The behavior for control endpoints isn't clear.  When a transaction 
> > error occurs in a control transfer, does xHCI stop the endpoint
> > queue? Presumably usb_clear_halt() isn't needed, but what about 
> > usb_reset_endpoint()?
> > 
> > I will assume that all stalls for control endpoints are protocol
> > stalls and therefore do not need to be cleared (the device will
> > automatically clear the halt condition when it receives the next
> > SETUP packet for the endpoint).  But does xhci-hcd still require a
> > usb_reset_endpoint() after a control stall?
> 
> Nope.
> 
> The host endpoint halts on every STALL handshake or non-isoc protocol
> error, but the driver immediately resets every halted endpoint because
> it's a precondition to even remove the failed URB from the HW queue
> before giving it back, which looks like a prudent thing to do.

Okay, good, we'll require all HCDs to reset control endpoints 
automatically after every error and stall.

> Currently, by the time the URB is given back, its endpoint is already
> in a "stopped but runnable" state and its sequence state is zeroed.
> And it may have already been restarted if there are more pending URBs.

Ah, I was going to ask about that.  This will be different from the way 
bulk and interrupt endpoints will behave, but I think it is acceptable.  
Control endpoints aren't used for anything that requires high 
throughput; if a driver wants an error to prevent later transfers from 
starting right away then it can simply avoid submitting those later 
transfers until the earlier ones have completed.

> This seems to need no change for control endpoints?

No change needed.

> For bulk/interrupt it also means that if we delay the restart until
> endpoint_reset() then endpoint_reset() has nothing to do besides
> restarting the endpoint, which is a good thing because this method
> under normal conditions requires no URBs to be present.

That could be an issue, because there may well be some URBs that were on 
the queue when the error occurred and were not unlinked, or were added 
afterward but before usb_endpoint_reset() could be called.

> > Recovery from a transaction error on a bulk or interrupt endpoint 
> > involves sending a Clear-Halt request to the device.  But if the
> > error was caused by some sort of transient interference that hasn't
> > ended yet, the Clear-Halt might itself fail with the same error. To
> > handle this, we should retry the Clear-Halt at increasing time
> > intervals.  At what point should the core give up?
> 
> Good question, I don't know. One thing I noticed is that Windows does
> tend to lose patience with completely unresponsive devices and kicks
> them out, but I don't know the exact criteria.

Two reasonable possibilities are 250 ms (because that's about how long 
an intermediate hub might take to notify the core about a disconnect) or 
5 seconds (the normal timeout for control transfers).  Of course, 5 
seconds is an awfully long time to wait for a mouse or keyboard to 
recover, so maybe something in between would be best.

> And if core is to clear halt autonomously, things may get interesting
> if a driver tries to do the same, or something else like removing the
> endpoint with usb_set_interface().

I will set things up so that an extraneous clear-halt (such as one 
submitted by the driver) will prevent the core from doing its own.  This 
leaves the possibility of the core clearing the halt and restarting the 
endpoint and then the driver doing it again, while the endpoint is 
running and the queue is nonempty.  Hopefully drivers avoid doing this.  
But if it helps, I could print a warning if usb_clear_halt() is called 
for an endpoint that isn't stopped and has a nonempty queue.

Also I will be careful to make the core's actions mutually exclusive 
with: suspend/resume, reset, disconnect, set-config, and set-interface.
Should anything else be added to this list?

> A related issue is clearing TT buffers. AFAIK this has no retries, it
> fails silently and leaves the endpoint potentially broken, and it is
> waited for to complete in case of usb_set_interface().

Is there anything we can do besides calling usb_clear_halt() and 
usb_reset_endpoint()?  If not, and data loss is unavoidable, then so be 
it.

Alan Stern

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Michal Pecio]

On Fri, 17 Apr 2026 22:34:58 -0400, Alan Stern wrote:
> Okay, good, we'll require all HCDs to reset control endpoints 
> automatically after every error and stall.

Are they not doing it?

Say that something like lsusb encounters protocol stall while another
URB from the class driver is pending, will the other URB time out just
because host endpoint halted on an earlier one?

> > Currently, by the time the URB is given back, its endpoint is
> > already in a "stopped but runnable" state and its sequence state is
> > zeroed. And it may have already been restarted if there are more
> > pending URBs.  
> 
> Ah, I was going to ask about that.  This will be different from the
> way bulk and interrupt endpoints will behave, but I think it is
> acceptable. Control endpoints aren't used for anything that requires
> high throughput; if a driver wants an error to prevent later
> transfers from starting right away then it can simply avoid
> submitting those later transfers until the earlier ones have
> completed.

Or it could unlink if the async giveback race is fixed with a new
callback separate from endpoint_reset(), but IDK if any demand exists.

Same thing with "chain unlinking" - unlink one URB and expect others
not to execute so that unlink completion can unlink them later. Looks
odd, but it's guaranteed by kerneldocs. And currently broken.

> > > Recovery from a transaction error on a bulk or interrupt endpoint 
> > > involves sending a Clear-Halt request to the device.  But if the
> > > error was caused by some sort of transient interference that
> > > hasn't ended yet, the Clear-Halt might itself fail with the same
> > > error. To handle this, we should retry the Clear-Halt at
> > > increasing time intervals. At what point should the core give up?
> > 
> > Good question, I don't know. One thing I noticed is that Windows
> > does tend to lose patience with completely unresponsive devices and
> > kicks them out, but I don't know the exact criteria.  
> 
> Two reasonable possibilities are 250 ms (because that's about how
> long an intermediate hub might take to notify the core about a
> disconnect) or 5 seconds (the normal timeout for control transfers).
> Of course, 5 seconds is an awfully long time to wait for a mouse or
> keyboard to recover, so maybe something in between would be best.

What happens after giving up? If control requests don't work, most
likely nothing works anyway. Reset may work, or not if it's bad cable.

Retrying too long may cause class drivers to time out on pending URBs,
not sure if it matters. Drivers may have no way to distinguish this
from any other timeout, not sure if this matters either.

> I will set things up so that an extraneous clear-halt (such as one 
> submitted by the driver) will prevent the core from doing its own.
> This leaves the possibility of the core clearing the halt and
> restarting the endpoint and then the driver doing it again, while the
> endpoint is running and the queue is nonempty.  Hopefully drivers
> avoid doing this.

Yes, that's just dodgy, what would such driver even expect to happen?
An URB may be in progress and then what? On xHCI we would need to throw
out this URB, so it simply isn't supported.

> But if it helps, I could print a warning if usb_clear_halt() is
> called for an endpoint that isn't stopped and has a nonempty queue.

Not sure what core considers a "stopped" endpoint. FYI, xhci-hcd
logs dev_err() when reset is attempted while URBs are running.

> > A related issue is clearing TT buffers. AFAIK this has no retries,
> > it fails silently and leaves the endpoint potentially broken, and
> > it is waited for to complete in case of usb_set_interface().  
> 
> Is there anything we can do besides calling usb_clear_halt() and 
> usb_reset_endpoint()?  If not, and data loss is unavoidable, then so
> be it.

If this "clear-halt by usbcore" materializes and survives confrotation
with the real world, it could make sense to look into combining TT
clearing with it. It's a similar thing, but tracked separately now.

One thing that could reduce data loss is never giving up on those
control requests, or resetting/disconnecting the device if giving up.
It's a general problem that control requests can fail and nobody has
much idea what to do then. Some drivers ignore errors. If the device
returns to operation, it may end up running in an unknown state.

This is apparently rare enough that nobody complains, though on
low- and full-speed it's relatively easy to produce artificially with
a particularly defective cable.

Regards,
Michal

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Alan Stern]

On Sat, Apr 18, 2026 at 11:21:46AM +0200, Michal Pecio wrote:
> On Fri, 17 Apr 2026 22:34:58 -0400, Alan Stern wrote:
> > Okay, good, we'll require all HCDs to reset control endpoints 
> > automatically after every error and stall.
> 
> Are they not doing it?

As far as I know, they are.  But it's not currently a requirement.  In 
fact, I don't know of any place in the source code where all the special 
requirements for HCDs are listed.  Probably the best locations are the 
kerneldoc for usb_submit_urb() and usb_unlink_urb().

> Say that something like lsusb encounters protocol stall while another
> URB from the class driver is pending, will the other URB time out just
> because host endpoint halted on an earlier one?

Even if that were true now (although I don't think it is), it won't be 
true once this new mechanism is added.

However, I did change my mind about one thing.  It would be a good idea 
for a control endpoint's queue to stop when a transaction error (not a 
stall) occurs, until usb_endpoint_reset() is called.  Or maybe 
usb_restart_endpoint(), a new callback that I will add.  (It's needed 
for HCDs that stop the queue when there's a short packet and 
URB_SHORT_NOT_OK is set; in this situation the endpoint doesn't need to 
be reset, because the host side is still in sync with the device side.  
The queue merely has to be restarted.)

This will allow the core's centralized backoff-and-retry approach to 
prevent ep0 resubmissions and failures from getting into a tight loop 
when a device disconnects.

> > > Currently, by the time the URB is given back, its endpoint is
> > > already in a "stopped but runnable" state and its sequence state is
> > > zeroed. And it may have already been restarted if there are more
> > > pending URBs.  
> > 
> > Ah, I was going to ask about that.  This will be different from the
> > way bulk and interrupt endpoints will behave, but I think it is
> > acceptable. Control endpoints aren't used for anything that requires
> > high throughput; if a driver wants an error to prevent later
> > transfers from starting right away then it can simply avoid
> > submitting those later transfers until the earlier ones have
> > completed.

So this would have to be changed.  It will be okay for the control 
endpoint to be in a "stopped but runnable" state, but not okay for it to 
be restarted.  Not until the core explicitly restarts it.

> Or it could unlink if the async giveback race is fixed with a new
> callback separate from endpoint_reset(), but IDK if any demand exists.
> 
> Same thing with "chain unlinking" - unlink one URB and expect others
> not to execute so that unlink completion can unlink them later. Looks
> odd, but it's guaranteed by kerneldocs. And currently broken.

I'm not sure what async giveback race you're talking about.  In any 
case, the proper way to unlink a sequence of URBs is in reverse order of 
submission.  That way there is no possibility for the HCD to skip some 
URBs and then execute the later ones before they can be unlinked.  This 
is how usb_flush_endpoint() will work.

And I intend to change the kerneldoc.

> > > > Recovery from a transaction error on a bulk or interrupt endpoint 
> > > > involves sending a Clear-Halt request to the device.  But if the
> > > > error was caused by some sort of transient interference that
> > > > hasn't ended yet, the Clear-Halt might itself fail with the same
> > > > error. To handle this, we should retry the Clear-Halt at
> > > > increasing time intervals. At what point should the core give up?
> > > 
> > > Good question, I don't know. One thing I noticed is that Windows
> > > does tend to lose patience with completely unresponsive devices and
> > > kicks them out, but I don't know the exact criteria.  
> > 
> > Two reasonable possibilities are 250 ms (because that's about how
> > long an intermediate hub might take to notify the core about a
> > disconnect) or 5 seconds (the normal timeout for control transfers).
> > Of course, 5 seconds is an awfully long time to wait for a mouse or
> > keyboard to recover, so maybe something in between would be best.
> 
> What happens after giving up? If control requests don't work, most
> likely nothing works anyway. Reset may work, or not if it's bad cable.

As I see it, there are two possibilities: (1) Try doing a reset.  (2) Do 
nothing at all, leaving the device in a non-working state until the user 
unplugs it.

Remember that the vast majority of transaction errors that will occur 
will be the result of a device being unplugged while it is in use.  What 
we do in this case doesn't matter much.

> Retrying too long may cause class drivers to time out on pending URBs,
> not sure if it matters. Drivers may have no way to distinguish this
> from any other timeout, not sure if this matters either.

I suspect it doesn't.

> > I will set things up so that an extraneous clear-halt (such as one 
> > submitted by the driver) will prevent the core from doing its own.
> > This leaves the possibility of the core clearing the halt and
> > restarting the endpoint and then the driver doing it again, while the
> > endpoint is running and the queue is nonempty.  Hopefully drivers
> > avoid doing this.
> 
> Yes, that's just dodgy, what would such driver even expect to happen?
> An URB may be in progress and then what? On xHCI we would need to throw
> out this URB, so it simply isn't supported.
> 
> > But if it helps, I could print a warning if usb_clear_halt() is
> > called for an endpoint that isn't stopped and has a nonempty queue.
> 
> Not sure what core considers a "stopped" endpoint. FYI, xhci-hcd
> logs dev_err() when reset is attempted while URBs are running.

Similar things can be done in the other HCDs.  Good enough.

> > > A related issue is clearing TT buffers. AFAIK this has no retries,
> > > it fails silently and leaves the endpoint potentially broken, and
> > > it is waited for to complete in case of usb_set_interface().  
> > 
> > Is there anything we can do besides calling usb_clear_halt() and 
> > usb_reset_endpoint()?  If not, and data loss is unavoidable, then so
> > be it.
> 
> If this "clear-halt by usbcore" materializes and survives confrotation
> with the real world, it could make sense to look into combining TT
> clearing with it. It's a similar thing, but tracked separately now.

That hadn't occurred to me, but sure.

> One thing that could reduce data loss is never giving up on those
> control requests, or resetting/disconnecting the device if giving up.
> It's a general problem that control requests can fail and nobody has
> much idea what to do then. Some drivers ignore errors. If the device
> returns to operation, it may end up running in an unknown state.
> 
> This is apparently rare enough that nobody complains, though on
> low- and full-speed it's relatively easy to produce artificially with
> a particularly defective cable.

Yes.  It seems that there isn't much the kernel can do; user 
intervention will be needed.

And come to think of it, usb_reset_and_verify_device() does a logical 
disconnect if the reset fails.  So if (1) above doesn't work, there's 
nothing left to do.  In any case, we can settle this issue later on.

Alan Stern

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Alan Stern]

On Tue, Apr 07, 2026 at 11:24:01PM +0300, Mathias Nyman wrote:
> On 4/7/26 18:23, Alan Stern wrote:
> > It's been a while now, and nobody has objected to the proposed plan for
> > handling this issue, so I'm going to assume that everyone is on board
> > with the idea.
> 
> Yes, I support this
> 
> So basically usb core will call usb_clear_halt() after EPROTO URB completion
> handler finishes, and xhci-hcd needs to prevent bulk/interrupt endpoint
> from restarting after returning a EPROTO URB up until usb_reset_endpoint()
> is called

Can you confirm that it's also possible for xhci-hcd to prevent control 
endpoints from restarting when a transaction error (-EPROTO) occurs?  Up 
until usb_reset_endpoint() or a new callback?

I've been thinking about how to implement all this, and some issues have 
to be solved.  In particular, suppose we have sent a Clear-Halt request 
for an endpoint that has gotten an error, and either the request times 
out or the device replies with a stall.  What can we do then?

My feeling is that either of these events would mean that the device is 
so far out of whack that the only thing to do is try resetting it.  Any 
proposals for something a little less drastic?

Also, it seems reasonable to devote only a single thread to endpoint 
error recovery.  Another possibility would be to have one thread for 
each device having problems, but I think the likelihood of this 
happening to multiple devices at once is pretty small unless the problem 
affects a hub upstream from all of them -- in which case having multiple 
threads wouldn't really help much.  Other opinions?

Alan Stern

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Michal Pecio]

On Tue, 21 Apr 2026 22:11:41 -0400, Alan Stern wrote:
> On Tue, Apr 07, 2026 at 11:24:01PM +0300, Mathias Nyman wrote:
> > On 4/7/26 18:23, Alan Stern wrote:  
> > > It's been a while now, and nobody has objected to the proposed
> > > plan for handling this issue, so I'm going to assume that
> > > everyone is on board with the idea.  
> > 
> > Yes, I support this
> > 
> > So basically usb core will call usb_clear_halt() after EPROTO URB
> > completion handler finishes, and xhci-hcd needs to prevent
> > bulk/interrupt endpoint from restarting after returning a EPROTO
> > URB up until usb_reset_endpoint() is called  
> 
> Can you confirm that it's also possible for xhci-hcd to prevent
> control endpoints from restarting when a transaction error (-EPROTO)
> occurs?  Up until usb_reset_endpoint() or a new callback?

Doable. They halt like any other and it's SW's choice how to restart.

BTW, what about EOVERFLOW? It's treated similarly by xhci-hcd.

> I've been thinking about how to implement all this, and some issues
> have to be solved.  In particular, suppose we have sent a Clear-Halt
> request for an endpoint that has gotten an error, and either the
> request times out or the device replies with a stall.

(... or the TT replies with STALL due to downstream bus EPROTO).

> My feeling is that either of these events would mean that the device
> is so far out of whack that the only thing to do is try resetting it.
> Any proposals for something a little less drastic?

Let's look at possible causes:

1. disconnected device
Doesn't matter what happens.

2. completely brain dead host controller
Ditto. Just be sure not to lock up so xhci-hcd can be reloaded.

3. temporary EMI or low link quality
This should clear itself after a few retries.

4. broken D+/D- wire in a LS/FS cable
Issues can last arbitrarily long and yet still clear.
Least disruptive solution: wait forever with sporadic retries.
Acceptable alternative: request user attention, i.e. disconnect.

Note: we would disconnect instantly if the opposite wire broke.

5. crashed device firmware
In this case a reset seems more productive than retrying forever.

A compromise betwen 4 and 5 could be to retry for some time, then
reset a few times, then disconnect.

6. device doesn't support clear-halt, stalls or does something odd

Nightmare fuel.

> Also, it seems reasonable to devote only a single thread to endpoint 
> error recovery.  Another possibility would be to have one thread for 
> each device having problems, but I think the likelihood of this 
> happening to multiple devices at once is pretty small unless the
> problem affects a hub upstream from all of them -- in which case
> having multiple threads wouldn't really help much.  Other opinions?

Well, another option is asynchronous URBs and "callback hell". For
instance, besides hcd methods, all xhci-hcd endpoint management is
asynchronous, tracks current state with bit flags and defers actions
blocked by flags until the flags are cleared. This includes waiting
for Reset Endpoint commands, TT clearing, ongoing unlinks, etc.

One practical complication is that hcd->endpoint_reset() may sleep.
But it will only extremely rarely take 5 seconds and time out.

Regards,
Michal

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Mathias Nyman]

On 4/22/26 08:30, Michal Pecio wrote:
> On Tue, 21 Apr 2026 22:11:41 -0400, Alan Stern wrote:
>> On Tue, Apr 07, 2026 at 11:24:01PM +0300, Mathias Nyman wrote:
>>> On 4/7/26 18:23, Alan Stern wrote:
>>>> It's been a while now, and nobody has objected to the proposed
>>>> plan for handling this issue, so I'm going to assume that
>>>> everyone is on board with the idea.
>>>
>>> Yes, I support this
>>>
>>> So basically usb core will call usb_clear_halt() after EPROTO URB
>>> completion handler finishes, and xhci-hcd needs to prevent
>>> bulk/interrupt endpoint from restarting after returning a EPROTO
>>> URB up until usb_reset_endpoint() is called
>>
>> Can you confirm that it's also possible for xhci-hcd to prevent
>> control endpoints from restarting when a transaction error (-EPROTO)
>> occurs?  Up until usb_reset_endpoint() or a new callback?
> 
> Doable. They halt like any other and it's SW's choice how to restart.

Yes, doable.

Would this be used in cases where all hope is lost and we want to reset the
device, canceling all pending control URBs before restarting the ring,
thus avoiding sending any extra URBs to the device just to wait for
them to fail or timeout?

In most cases I can think of it would make sense to keep the control endpoint
running. Just let the hcd move to the SETUP stage of the next control transfer
URB and continue.

For example EMF causing transaction error (-EPROTO) on active IN and OUT bulk
transfers. Two clear-halt requests are queued, one for each endpoint.
If first clear-halt request fails with -EPROTO  we still want to continue with
the next request.
Shouldn't be any toggle/seq-nr issues here on the control endpoint.

Most control transfer STALL (EPIPE) cases are protocol stalls, and we should
just continue running. Exception here might be a STALL response
to a clear-halt request. I assume (didn't check) device must support those.
So in that case we may want to reset the device.

There's also a risk that the control endpoint isn't started when it should.
For example class driver could potentially call usb_clear_halt() on the control
endpoint, synchronously waiting  for the clear-halt request to complete before
calling usb_reset_endpoint(). If endpoint restarts at usb_reset_endpoint() then
the clear-halt control transfer would time out.

Thanks
Mathias

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Alan Stern]

On Wed, Apr 22, 2026 at 04:51:30PM +0300, Mathias Nyman wrote:
> On 4/22/26 08:30, Michal Pecio wrote:
> > On Tue, 21 Apr 2026 22:11:41 -0400, Alan Stern wrote:
> > > Can you confirm that it's also possible for xhci-hcd to prevent
> > > control endpoints from restarting when a transaction error (-EPROTO)
> > > occurs?  Up until usb_reset_endpoint() or a new callback?
> > 
> > Doable. They halt like any other and it's SW's choice how to restart.
> 
> Yes, doable.
> 
> Would this be used in cases where all hope is lost and we want to reset the
> device, canceling all pending control URBs before restarting the ring,
> thus avoiding sending any extra URBs to the device just to wait for
> them to fail or timeout?

That wasn't my intention.  The purpose was to avoid getting into a tight 
loop such as in fuzzing tests with dummy-hcd.  When a device is 
disconnected, if the class driver's completion handler immediately 
resubmits after a -EPROTO error, and the resubmitted URB immediately 
fails with -EPROTO status, the resulting loop can use enough of a CPU's 
time to cause a "CPU hung" error.

The same sort of thing can happen with real host controllers, although 
the loop won't be quite so tight.  But if we prevent resubmitted URBs 
from failing until the endpoint is reset or restarted, that will slow 
down the loop a lot.  Especially with a series of increasing delays 
between the restarts.

> In most cases I can think of it would make sense to keep the control endpoint
> running. Just let the hcd move to the SETUP stage of the next control transfer
> URB and continue.

I'd be in favor of that except for this tight-loop problem.

> For example EMF causing transaction error (-EPROTO) on active IN and OUT bulk
> transfers. Two clear-halt requests are queued, one for each endpoint.
> If first clear-halt request fails with -EPROTO  we still want to continue with
> the next request.

Actually no, we don't.  If the first clear-halt request fails, it means 
the EMF is still present.  We want to wait for a short time before 
sending the next request.  If we don't, and the next request gets sent 
right away, it will probably fail also.

> Shouldn't be any toggle/seq-nr issues here on the control endpoint.

Indeed.

> Most control transfer STALL (EPIPE) cases are protocol stalls, and we should
> just continue running. Exception here might be a STALL response
> to a clear-halt request. I assume (didn't check) device must support those.
> So in that case we may want to reset the device.

As I said to Michal, we can treat this the same as a transaction error: 
delay and retry, then eventually reset if needed.

In theory there's a risk of racing with a Set-Interface or Set-Config 
request, so that the clear-halt refers to an endpoint that doesn't exist 
any more.  This race can be prevented by making usb_set_interface() and 
usb_set_configuration() wait until all pending error recovery is done.

> There's also a risk that the control endpoint isn't started when it should.
> For example class driver could potentially call usb_clear_halt() on the control
> endpoint, synchronously waiting  for the clear-halt request to complete before
> calling usb_reset_endpoint(). If endpoint restarts at usb_reset_endpoint() then
> the clear-halt control transfer would time out.

You're suggesting that the clear-halt request might not be sent to the 
device (and would therefore time out) because the endpoint queue isn't 
running, and it won't start running until usb_reset_endpoint() is 
called?  That objection applies only to ep0.  Other control endpoints 
don't have this problem because the clear-halt request is always sent to 
ep0.

ep0 is a special case.  If it ever does halt in such an extreme fashion 
that it can't even accept SETUP packets then there's no way to 
communicate with it at all.  The USB 2.0 spec mentions this possibility 
in section 5.5.5; it says the only way to recover is then to reset the 
device.  There should never be any reason to send a clear-halt request 
for ep0.

Alan Stern

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Alan Stern]

On Wed, Apr 22, 2026 at 11:35:42AM -0400, Alan Stern wrote:
> On Wed, Apr 22, 2026 at 04:51:30PM +0300, Mathias Nyman wrote:
> > On 4/22/26 08:30, Michal Pecio wrote:
> > > On Tue, 21 Apr 2026 22:11:41 -0400, Alan Stern wrote:
> > > > Can you confirm that it's also possible for xhci-hcd to prevent
> > > > control endpoints from restarting when a transaction error (-EPROTO)
> > > > occurs?  Up until usb_reset_endpoint() or a new callback?
> > > 
> > > Doable. They halt like any other and it's SW's choice how to restart.
> > 
> > Yes, doable.
> > 
> > Would this be used in cases where all hope is lost and we want to reset the
> > device, canceling all pending control URBs before restarting the ring,
> > thus avoiding sending any extra URBs to the device just to wait for
> > them to fail or timeout?
> 
> That wasn't my intention.  The purpose was to avoid getting into a tight 
> loop such as in fuzzing tests with dummy-hcd.  When a device is 
> disconnected, if the class driver's completion handler immediately 
> resubmits after a -EPROTO error, and the resubmitted URB immediately 
> fails with -EPROTO status, the resulting loop can use enough of a CPU's 
> time to cause a "CPU hung" error.
> 
> The same sort of thing can happen with real host controllers, although 
> the loop won't be quite so tight.  But if we prevent resubmitted URBs 
> from failing until the endpoint is reset or restarted, that will slow 
> down the loop a lot.  Especially with a series of increasing delays 
> between the restarts.

You know, on further thought I'm not sure the benefit is worth the 
effort.  I'll see if the tight-loop problem in dummy-hcd can be fixed by 
changing dummy-hcd.

Error recovery for control endpoints would be tricky when an error 
occurred during new device initialization and enumeration.  Avoiding it 
seems better.

Alan Stern

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Mathias Nyman]

On 4/23/26 17:12, Alan Stern wrote:
> On Wed, Apr 22, 2026 at 11:35:42AM -0400, Alan Stern wrote:
>> On Wed, Apr 22, 2026 at 04:51:30PM +0300, Mathias Nyman wrote:
>>> On 4/22/26 08:30, Michal Pecio wrote:
>>>> On Tue, 21 Apr 2026 22:11:41 -0400, Alan Stern wrote:
>>>>> Can you confirm that it's also possible for xhci-hcd to prevent
>>>>> control endpoints from restarting when a transaction error (-EPROTO)
>>>>> occurs?  Up until usb_reset_endpoint() or a new callback?
>>>>
>>>> Doable. They halt like any other and it's SW's choice how to restart.
>>>
>>> Yes, doable.
>>>
>>> Would this be used in cases where all hope is lost and we want to reset the
>>> device, canceling all pending control URBs before restarting the ring,
>>> thus avoiding sending any extra URBs to the device just to wait for
>>> them to fail or timeout?
>>
>> That wasn't my intention.  The purpose was to avoid getting into a tight
>> loop such as in fuzzing tests with dummy-hcd.  When a device is
>> disconnected, if the class driver's completion handler immediately
>> resubmits after a -EPROTO error, and the resubmitted URB immediately
>> fails with -EPROTO status, the resulting loop can use enough of a CPU's
>> time to cause a "CPU hung" error.
>>
>> The same sort of thing can happen with real host controllers, although
>> the loop won't be quite so tight.  But if we prevent resubmitted URBs
>> from failing until the endpoint is reset or restarted, that will slow
>> down the loop a lot.  Especially with a series of increasing delays
>> between the restarts.
> 
> You know, on further thought I'm not sure the benefit is worth the
> effort.  I'll see if the tight-loop problem in dummy-hcd can be fixed by
> changing dummy-hcd.
> 

The tight loop -EPROTO return case is probably not a big issue for xhci
anyway  as both xhci driver and hardware cause some delay before giving back
an URB with EPROTO status.

xHC halts the host side of the control endpoints on transaction errors, and
driver will reset the internal endpoint and move the ring dequeue pointer past
the URB before giving the URB back with -EPROTO status.

-Mathias

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Alan Stern]

On Wed, Apr 22, 2026 at 07:30:54AM +0200, Michal Pecio wrote:
> On Tue, 21 Apr 2026 22:11:41 -0400, Alan Stern wrote:
> > Can you confirm that it's also possible for xhci-hcd to prevent
> > control endpoints from restarting when a transaction error (-EPROTO)
> > occurs?  Up until usb_reset_endpoint() or a new callback?
> 
> Doable. They halt like any other and it's SW's choice how to restart.
> 
> BTW, what about EOVERFLOW? It's treated similarly by xhci-hcd.

That also counts as a transaction error.  So do EILSEQ, ETIME, ECOMM, 
and ENOSR (even if xhci-hcd never generates them).

> > I've been thinking about how to implement all this, and some issues
> > have to be solved.  In particular, suppose we have sent a Clear-Halt
> > request for an endpoint that has gotten an error, and either the
> > request times out or the device replies with a stall.
> 
> (... or the TT replies with STALL due to downstream bus EPROTO).
> 
> > My feeling is that either of these events would mean that the device
> > is so far out of whack that the only thing to do is try resetting it.
> > Any proposals for something a little less drastic?
> 
> Let's look at possible causes:
> 
> 1. disconnected device
> Doesn't matter what happens.
> 
> 2. completely brain dead host controller
> Ditto. Just be sure not to lock up so xhci-hcd can be reloaded.
> 
> 3. temporary EMI or low link quality
> This should clear itself after a few retries.

In this case we would get an error, not a timeout or stall.  Unless the 
problem is on the downstream side of a TT, in which case we would get a 
stall.  (Oddly, this case seems to be missing from the state diagrams of 
Figures 11-55 and 11-59 in the USB 2.0 spec.)

I guess to be safe, we should treat stall the same as an error.

> 4. broken D+/D- wire in a LS/FS cable
> Issues can last arbitrarily long and yet still clear.
> Least disruptive solution: wait forever with sporadic retries.
> Acceptable alternative: request user attention, i.e. disconnect.
> 
> Note: we would disconnect instantly if the opposite wire broke.

Same as above, except for the "temporary" part.

> 5. crashed device firmware
> In this case a reset seems more productive than retrying forever.
> 
> A compromise betwen 4 and 5 could be to retry for some time, then
> reset a few times, then disconnect.

My plan is to retry up until about 1 second and then do a reset.  If the 
reset fails, the device is logically disconnected.

> 6. device doesn't support clear-halt, stalls or does something odd
> 
> Nightmare fuel.

Devices are required to support clear-halt for bulk and interrupt 
endpoints.  And the only reason for stalling a clear-halt request, as 
far as I know, is if the request is invalid in some way (such as 
specifying an endpoint that doesn't exist).

Either way, I can't think of anything else to do besides retries and 
then reset.

> > Also, it seems reasonable to devote only a single thread to endpoint 
> > error recovery.  Another possibility would be to have one thread for 
> > each device having problems, but I think the likelihood of this 
> > happening to multiple devices at once is pretty small unless the
> > problem affects a hub upstream from all of them -- in which case
> > having multiple threads wouldn't really help much.  Other opinions?
> 
> Well, another option is asynchronous URBs and "callback hell". For
> instance, besides hcd methods, all xhci-hcd endpoint management is
> asynchronous, tracks current state with bit flags and defers actions
> blocked by flags until the flags are cleared. This includes waiting
> for Reset Endpoint commands, TT clearing, ongoing unlinks, etc.
> 
> One practical complication is that hcd->endpoint_reset() may sleep.
> But it will only extremely rarely take 5 seconds and time out.

Even so, that means the recovery has to be done by a thread.

Alan Stern

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Thinh Nguyen]

On Mon, Mar 30, 2026, Mathias Nyman wrote:
> 
> > 
> > On a separate note, will you plan to implement the clear-halt for EPROTO
> > in xhci?
> 
> I don't think this should be part of xhci driver. Decision to send control requests
> to the device should be done by core or class drivers.
> 

This not like STALL where it's standardized for the core or class driver
to know how to handle. The programming sequence for the errors that
resulted in EPROTO from xhci is specific to xhci. That is, the xhci
reset endpoint command will reset the bulk sequence, it's specific to
xhci. The xhci spec recommends to send a clear-halt for this scenario,
not the USB spec or any other class specific spec. So we should not
delegate this to the core or class driver to handle.

BR,
Thinh

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Mathias Nyman]

On 4/2/26 01:08, Thinh Nguyen wrote:
> On Mon, Mar 30, 2026, Mathias Nyman wrote:
>>
>>>
>>> On a separate note, will you plan to implement the clear-halt for EPROTO
>>> in xhci?
>>
>> I don't think this should be part of xhci driver. Decision to send control requests
>> to the device should be done by core or class drivers.
>>
> 
> This not like STALL where it's standardized for the core or class driver
> to know how to handle. The programming sequence for the errors that
> resulted in EPROTO from xhci is specific to xhci. That is, the xhci
> reset endpoint command will reset the bulk sequence, it's specific to
> xhci. The xhci spec recommends to send a clear-halt for this scenario,
> not the USB spec or any other class specific spec. So we should not
> delegate this to the core or class driver to handle.
> 
USB 2 Specification does mention handling halt conditions due to  transmission
erros _OR_ STALL handshake with clearing halt and resetting both host and device
endpoint toggle.

See USB 2

5.7.5 Interrupt Transfer Data Sequences

"If a halt condition is detected on an interrupt pipe due to transmission errors or
  a STALL handshake being returned from the endpoint, all pending IRPs are retired.
  Removal of the halt condition is achieved via software intervention through a
  separate control pipe. This recovery will reset the data toggle bit to DATA0 for
  the endpoint on both the host and the device"

5.8.5 Bulk Transfer Data Sequences

"If a halt condition is detected on a bulk pipe due to transmission errors or a
  STALL handshake being returned from the endpoint, all pending IRPs are retired.
  Removal of the halt condition is achieved via software intervention through a
  separate control pipe. This recovery will reset the data toggle bit to DATA0
  for the endpoint on both the host and the device"

Thanks
Mathias

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Thinh Nguyen]

On Thu, Apr 02, 2026, Mathias Nyman wrote:
> On 4/2/26 01:08, Thinh Nguyen wrote:
> > On Mon, Mar 30, 2026, Mathias Nyman wrote:
> > > 
> > > > 
> > > > On a separate note, will you plan to implement the clear-halt for EPROTO
> > > > in xhci?
> > > 
> > > I don't think this should be part of xhci driver. Decision to send control requests
> > > to the device should be done by core or class drivers.
> > > 
> > 
> > This not like STALL where it's standardized for the core or class driver
> > to know how to handle. The programming sequence for the errors that
> > resulted in EPROTO from xhci is specific to xhci. That is, the xhci
> > reset endpoint command will reset the bulk sequence, it's specific to
> > xhci. The xhci spec recommends to send a clear-halt for this scenario,
> > not the USB spec or any other class specific spec. So we should not
> > delegate this to the core or class driver to handle.
> > 
> USB 2 Specification does mention handling halt conditions due to  transmission
> erros _OR_ STALL handshake with clearing halt and resetting both host and device
> endpoint toggle.
> 
> See USB 2
> 
> 5.7.5 Interrupt Transfer Data Sequences
> 
> "If a halt condition is detected on an interrupt pipe due to transmission errors or
>  a STALL handshake being returned from the endpoint, all pending IRPs are retired.
>  Removal of the halt condition is achieved via software intervention through a
>  separate control pipe. This recovery will reset the data toggle bit to DATA0 for
>  the endpoint on both the host and the device"
> 
> 5.8.5 Bulk Transfer Data Sequences
> 
> "If a halt condition is detected on a bulk pipe due to transmission errors or a
>  STALL handshake being returned from the endpoint, all pending IRPs are retired.
>  Removal of the halt condition is achieved via software intervention through a
>  separate control pipe. This recovery will reset the data toggle bit to DATA0
>  for the endpoint on both the host and the device"
> 

Ah~ I stand corrected. My memory served me wrong. Thanks for pointing
this out.

BR,
Thinh

[RFC PATCH 2/2] xhci: Ensure URB is given back when endpoint halts on a multi-TD URB

[Mathias Nyman]

Make sure URB is given back by xhci driver after a stall (EPIPE)
or transaction error (EPROTO) on one of the TDs in the URB.

Most bulk, interrupt and control transfer URBs consist of one TD,
so adding that TD to the cancel list was enough to give back the
entire URB when invalidating and giving back cancelled TDs

Some bulk URBs that need an additional zero length packet consist
of two TDs. Make sure the second TD is added to the cancel list so
that URB is given back when clearing the internal endpoint halt.

Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-ring.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index f01f343a7e37..5305ad7be025 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -982,8 +982,9 @@ static int xhci_reset_halted_ep(struct xhci_hcd *xhci, unsigned int slot_id,
 }
 
 /*
- * Mark the TD the endpoint halted on as TD_HALTED and add it to the cancelled
- * td list, ensuring invalidate_cancelled_tds() clears the TD from xHC cache.
+ * Mark the TD the endpoint halted on as TD_HALTED, add all TDs of that URB
+ * to the cancelled td list, ensuring invalidate_cancelled_tds() clears the
+ * TD from xHC cache and URB is given back.
  * Mark the other TDs on that bulk or interrupt endpoint as TD_TAINTED to
  * prevent the ring from being restarted too early.
  */
@@ -997,10 +998,11 @@ static void xhci_cancel_halted_tds(struct xhci_hcd *xhci, struct xhci_virt_ep *e
 	ring = xhci_urb_to_transfer_ring(xhci, td->urb);
 
 	td->cancel_status = TD_HALTED;
-	if (list_empty(&td->cancelled_td_list))
-		list_add_tail(&td->cancelled_td_list, &ep->cancelled_td_list);
 
 	list_for_each_entry(tdi, &ring->td_list, td_list) {
+		if (tdi->urb == td->urb && list_empty(&tdi->cancelled_td_list))
+			list_add_tail(&tdi->cancelled_td_list, &ep->cancelled_td_list);
+
 		if (!tdi->cancel_status && ring->type != TYPE_CTRL)
 			tdi->cancel_status = TD_TAINTED;
 	}
-- 
2.43.0

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[kernel test robot]

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20260323122512.2019893-2-mathias.nyman@linux.intel.com>
References: <20260323122512.2019893-2-mathias.nyman@linux.intel.com>
TO: Mathias Nyman <mathias.nyman@linux.intel.com>

Hi Mathias,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on usb/usb-next usb/usb-linus westeri-thunderbolt/next linus/master v7.0-rc5 next-20260320]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mathias-Nyman/xhci-prevent-automatic-endpoint-restart-after-stall-or-error/20260323-224110
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link:    https://lore.kernel.org/r/20260323122512.2019893-2-mathias.nyman%40linux.intel.com
patch subject: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error
:::::: branch date: 3 hours ago
:::::: commit date: 3 hours ago
config: x86_64-randconfig-161-20260323 (https://download.01.org/0day-ci/archive/20260324/202603240138.ZNq5FcFq-lkp@intel.com/config)
compiler: gcc-13 (Debian 13.3.0-16) 13.3.0
smatch: v0.5.0-9004-gb810ac53

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202603240138.ZNq5FcFq-lkp@intel.com/

smatch warnings:
drivers/usb/host/xhci-ring.c:1578 xhci_handle_cmd_set_deq() error: we previously assumed 'ep_ring' could be null (see line 1467)

vim +/ep_ring +1578 drivers/usb/host/xhci-ring.c

02324cb6fadb62 Mathias Nyman    2026-03-23  1440  
ae636747146ea9 Sarah Sharp      2009-04-29  1441  /*
ae636747146ea9 Sarah Sharp      2009-04-29  1442   * When we get a completion for a Set Transfer Ring Dequeue Pointer command,
ae636747146ea9 Sarah Sharp      2009-04-29  1443   * we need to clear the set deq pending flag in the endpoint ring state, so that
ae636747146ea9 Sarah Sharp      2009-04-29  1444   * the TD queueing code can ring the doorbell again.  We also need to ring the
ae636747146ea9 Sarah Sharp      2009-04-29  1445   * endpoint doorbell to restart the ring, but only if there aren't more
ae636747146ea9 Sarah Sharp      2009-04-29  1446   * cancellations pending.
ae636747146ea9 Sarah Sharp      2009-04-29  1447   */
b8200c9479b804 Xenia Ragiadakou 2013-09-09  1448  static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id,
c69a059783b241 Xenia Ragiadakou 2013-09-09  1449  		union xhci_trb *trb, u32 cmd_comp_code)
ae636747146ea9 Sarah Sharp      2009-04-29  1450  {
ae636747146ea9 Sarah Sharp      2009-04-29  1451  	unsigned int ep_index;
e9df17eb1408cf Sarah Sharp      2010-04-02  1452  	unsigned int stream_id;
ae636747146ea9 Sarah Sharp      2009-04-29  1453  	struct xhci_ring *ep_ring;
9aad95e292f58d Hans de Goede    2013-10-04  1454  	struct xhci_virt_ep *ep;
d115b04818e57b John Youn        2009-07-27  1455  	struct xhci_ep_ctx *ep_ctx;
d115b04818e57b John Youn        2009-07-27  1456  	struct xhci_slot_ctx *slot_ctx;
4aa2e16e052b43 Mathias Nyman    2024-11-06  1457  	struct xhci_stream_ctx *stream_ctx;
674f8438c12125 Mathias Nyman    2021-01-29  1458  	struct xhci_td *td, *tmp_td;
ae636747146ea9 Sarah Sharp      2009-04-29  1459  
28ccd2962c6655 Matt Evans       2011-03-29  1460  	ep_index = TRB_TO_EP_INDEX(le32_to_cpu(trb->generic.field[3]));
28ccd2962c6655 Matt Evans       2011-03-29  1461  	stream_id = TRB_TO_STREAM_ID(le32_to_cpu(trb->generic.field[2]));
b1adc42d440df3 Mathias Nyman    2021-01-29  1462  	ep = xhci_get_virt_ep(xhci, slot_id, ep_index);
b1adc42d440df3 Mathias Nyman    2021-01-29  1463  	if (!ep)
b1adc42d440df3 Mathias Nyman    2021-01-29  1464  		return;
e9df17eb1408cf Sarah Sharp      2010-04-02  1465  
42f2890aa998d1 Mathias Nyman    2021-01-29  1466  	ep_ring = xhci_virt_ep_to_ring(xhci, ep, stream_id);
e9df17eb1408cf Sarah Sharp      2010-04-02 @1467  	if (!ep_ring) {
e587b8b270d370 Oliver Neukum    2014-01-08  1468  		xhci_warn(xhci, "WARN Set TR deq ptr command for freed stream ID %u\n",
e9df17eb1408cf Sarah Sharp      2010-04-02  1469  				stream_id);
e9df17eb1408cf Sarah Sharp      2010-04-02  1470  		/* XXX: Harmless??? */
0d4976ec8ec17f Hans de Goede    2014-08-20  1471  		goto cleanup;
e9df17eb1408cf Sarah Sharp      2010-04-02  1472  	}
e9df17eb1408cf Sarah Sharp      2010-04-02  1473  
b1adc42d440df3 Mathias Nyman    2021-01-29  1474  	ep_ctx = xhci_get_ep_ctx(xhci, ep->vdev->out_ctx, ep_index);
b1adc42d440df3 Mathias Nyman    2021-01-29  1475  	slot_ctx = xhci_get_slot_ctx(xhci, ep->vdev->out_ctx);
19a7d0d65c4a81 Felipe Balbi     2017-04-07  1476  	trace_xhci_handle_cmd_set_deq(slot_ctx);
19a7d0d65c4a81 Felipe Balbi     2017-04-07  1477  	trace_xhci_handle_cmd_set_deq_ep(ep_ctx);
ae636747146ea9 Sarah Sharp      2009-04-29  1478  
4aa2e16e052b43 Mathias Nyman    2024-11-06  1479  	if (ep->ep_state & EP_HAS_STREAMS) {
4aa2e16e052b43 Mathias Nyman    2024-11-06  1480  		stream_ctx = &ep->stream_info->stream_ctx_array[stream_id];
4aa2e16e052b43 Mathias Nyman    2024-11-06  1481  		trace_xhci_handle_cmd_set_deq_stream(ep->stream_info, stream_id);
4aa2e16e052b43 Mathias Nyman    2024-11-06  1482  	}
4aa2e16e052b43 Mathias Nyman    2024-11-06  1483  
c69a059783b241 Xenia Ragiadakou 2013-09-09  1484  	if (cmd_comp_code != COMP_SUCCESS) {
ae636747146ea9 Sarah Sharp      2009-04-29  1485  		unsigned int ep_state;
ae636747146ea9 Sarah Sharp      2009-04-29  1486  		unsigned int slot_state;
ae636747146ea9 Sarah Sharp      2009-04-29  1487  
c69a059783b241 Xenia Ragiadakou 2013-09-09  1488  		switch (cmd_comp_code) {
0b7c105a04ca79 Felipe Balbi     2017-01-23  1489  		case COMP_TRB_ERROR:
e587b8b270d370 Oliver Neukum    2014-01-08  1490  			xhci_warn(xhci, "WARN Set TR Deq Ptr cmd invalid because of stream ID configuration\n");
ae636747146ea9 Sarah Sharp      2009-04-29  1491  			break;
0b7c105a04ca79 Felipe Balbi     2017-01-23  1492  		case COMP_CONTEXT_STATE_ERROR:
e587b8b270d370 Oliver Neukum    2014-01-08  1493  			xhci_warn(xhci, "WARN Set TR Deq Ptr cmd failed due to incorrect slot or ep state.\n");
5071e6b279178f Mathias Nyman    2016-11-11  1494  			ep_state = GET_EP_CTX_STATE(ep_ctx);
28ccd2962c6655 Matt Evans       2011-03-29  1495  			slot_state = le32_to_cpu(slot_ctx->dev_state);
ae636747146ea9 Sarah Sharp      2009-04-29  1496  			slot_state = GET_SLOT_STATE(slot_state);
aa50b29061d3df Xenia Ragiadakou 2013-08-14  1497  			xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
aa50b29061d3df Xenia Ragiadakou 2013-08-14  1498  					"Slot state = %u, EP state = %u",
ae636747146ea9 Sarah Sharp      2009-04-29  1499  					slot_state, ep_state);
ae636747146ea9 Sarah Sharp      2009-04-29  1500  			break;
0b7c105a04ca79 Felipe Balbi     2017-01-23  1501  		case COMP_SLOT_NOT_ENABLED_ERROR:
e587b8b270d370 Oliver Neukum    2014-01-08  1502  			xhci_warn(xhci, "WARN Set TR Deq Ptr cmd failed because slot %u was not enabled.\n",
e587b8b270d370 Oliver Neukum    2014-01-08  1503  					slot_id);
ae636747146ea9 Sarah Sharp      2009-04-29  1504  			break;
ae636747146ea9 Sarah Sharp      2009-04-29  1505  		default:
e587b8b270d370 Oliver Neukum    2014-01-08  1506  			xhci_warn(xhci, "WARN Set TR Deq Ptr cmd with unknown completion code of %u.\n",
c69a059783b241 Xenia Ragiadakou 2013-09-09  1507  					cmd_comp_code);
ae636747146ea9 Sarah Sharp      2009-04-29  1508  			break;
ae636747146ea9 Sarah Sharp      2009-04-29  1509  		}
ae636747146ea9 Sarah Sharp      2009-04-29  1510  		/* OK what do we do now?  The endpoint state is hosed, and we
ae636747146ea9 Sarah Sharp      2009-04-29  1511  		 * should never get to this point if the synchronization between
ae636747146ea9 Sarah Sharp      2009-04-29  1512  		 * queueing, and endpoint state are correct.  This might happen
ae636747146ea9 Sarah Sharp      2009-04-29  1513  		 * if the device gets disconnected after we've finished
ae636747146ea9 Sarah Sharp      2009-04-29  1514  		 * cancelling URBs, which might not be an error...
ae636747146ea9 Sarah Sharp      2009-04-29  1515  		 */
ae636747146ea9 Sarah Sharp      2009-04-29  1516  	} else {
9aad95e292f58d Hans de Goede    2013-10-04  1517  		u64 deq;
9aad95e292f58d Hans de Goede    2013-10-04  1518  		/* 4.6.10 deq ptr is written to the stream ctx for streams */
9aad95e292f58d Hans de Goede    2013-10-04  1519  		if (ep->ep_state & EP_HAS_STREAMS) {
931e468764b22a Niklas Neronin   2025-09-18  1520  			deq = le64_to_cpu(stream_ctx->stream_ring) & TR_DEQ_PTR_MASK;
e5fa8db0be3e87 Pawel Laszczak   2024-09-05  1521  
e5fa8db0be3e87 Pawel Laszczak   2024-09-05  1522  			/*
e5fa8db0be3e87 Pawel Laszczak   2024-09-05  1523  			 * Cadence xHCI controllers store some endpoint state
e5fa8db0be3e87 Pawel Laszczak   2024-09-05  1524  			 * information within Rsvd0 fields of Stream Endpoint
e5fa8db0be3e87 Pawel Laszczak   2024-09-05  1525  			 * context. This field is not cleared during Set TR
e5fa8db0be3e87 Pawel Laszczak   2024-09-05  1526  			 * Dequeue Pointer command which causes XDMA to skip
e5fa8db0be3e87 Pawel Laszczak   2024-09-05  1527  			 * over transfer ring and leads to data loss on stream
e5fa8db0be3e87 Pawel Laszczak   2024-09-05  1528  			 * pipe.
e5fa8db0be3e87 Pawel Laszczak   2024-09-05  1529  			 * To fix this issue driver must clear Rsvd0 field.
e5fa8db0be3e87 Pawel Laszczak   2024-09-05  1530  			 */
e5fa8db0be3e87 Pawel Laszczak   2024-09-05  1531  			if (xhci->quirks & XHCI_CDNS_SCTX_QUIRK) {
4aa2e16e052b43 Mathias Nyman    2024-11-06  1532  				stream_ctx->reserved[0] = 0;
4aa2e16e052b43 Mathias Nyman    2024-11-06  1533  				stream_ctx->reserved[1] = 0;
e5fa8db0be3e87 Pawel Laszczak   2024-09-05  1534  			}
9aad95e292f58d Hans de Goede    2013-10-04  1535  		} else {
931e468764b22a Niklas Neronin   2025-09-18  1536  			deq = le64_to_cpu(ep_ctx->deq) & TR_DEQ_PTR_MASK;
9aad95e292f58d Hans de Goede    2013-10-04  1537  		}
aa50b29061d3df Xenia Ragiadakou 2013-08-14  1538  		xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
9aad95e292f58d Hans de Goede    2013-10-04  1539  			"Successful Set TR Deq Ptr cmd, deq = @%08llx", deq);
9aad95e292f58d Hans de Goede    2013-10-04  1540  		if (xhci_trb_virt_to_dma(ep->queued_deq_seg,
9aad95e292f58d Hans de Goede    2013-10-04  1541  					 ep->queued_deq_ptr) == deq) {
bf161e85fb153c Sarah Sharp      2011-02-23  1542  			/* Update the ring's dequeue segment and dequeue pointer
bf161e85fb153c Sarah Sharp      2011-02-23  1543  			 * to reflect the new position.
bf161e85fb153c Sarah Sharp      2011-02-23  1544  			 */
856563be98b2e9 Niklas Neronin   2025-03-06  1545  			ep_ring->deq_seg = ep->queued_deq_seg;
856563be98b2e9 Niklas Neronin   2025-03-06  1546  			ep_ring->dequeue = ep->queued_deq_ptr;
bf161e85fb153c Sarah Sharp      2011-02-23  1547  		} else {
e587b8b270d370 Oliver Neukum    2014-01-08  1548  			xhci_warn(xhci, "Mismatch between completed Set TR Deq Ptr command & xHCI internal state.\n");
bf161e85fb153c Sarah Sharp      2011-02-23  1549  			xhci_warn(xhci, "ep deq seg = %p, deq ptr = %p\n",
9aad95e292f58d Hans de Goede    2013-10-04  1550  				  ep->queued_deq_seg, ep->queued_deq_ptr);
bf161e85fb153c Sarah Sharp      2011-02-23  1551  		}
ae636747146ea9 Sarah Sharp      2009-04-29  1552  	}
674f8438c12125 Mathias Nyman    2021-01-29  1553  	/* HW cached TDs cleared from cache, give them back */
674f8438c12125 Mathias Nyman    2021-01-29  1554  	list_for_each_entry_safe(td, tmp_td, &ep->cancelled_td_list,
674f8438c12125 Mathias Nyman    2021-01-29  1555  				 cancelled_td_list) {
674f8438c12125 Mathias Nyman    2021-01-29  1556  		ep_ring = xhci_urb_to_transfer_ring(ep->xhci, td->urb);
674f8438c12125 Mathias Nyman    2021-01-29  1557  		if (td->cancel_status == TD_CLEARING_CACHE) {
674f8438c12125 Mathias Nyman    2021-01-29  1558  			td->cancel_status = TD_CLEARED;
0d9b9f533bf1aa Mathias Nyman    2021-08-20  1559  			xhci_dbg(ep->xhci, "%s: Giveback cancelled URB %p TD\n",
0d9b9f533bf1aa Mathias Nyman    2021-08-20  1560  				 __func__, td->urb);
674f8438c12125 Mathias Nyman    2021-01-29  1561  			xhci_td_cleanup(ep->xhci, td, ep_ring, td->status);
0d9b9f533bf1aa Mathias Nyman    2021-08-20  1562  		} else {
0d9b9f533bf1aa Mathias Nyman    2021-08-20  1563  			xhci_dbg(ep->xhci, "%s: Keep cancelled URB %p TD as cancel_status is %d\n",
0d9b9f533bf1aa Mathias Nyman    2021-08-20  1564  				 __func__, td->urb, td->cancel_status);
674f8438c12125 Mathias Nyman    2021-01-29  1565  		}
674f8438c12125 Mathias Nyman    2021-01-29  1566  	}
0d4976ec8ec17f Hans de Goede    2014-08-20  1567  cleanup:
b1adc42d440df3 Mathias Nyman    2021-01-29  1568  	ep->ep_state &= ~SET_DEQ_PENDING;
b1adc42d440df3 Mathias Nyman    2021-01-29  1569  	ep->queued_deq_seg = NULL;
b1adc42d440df3 Mathias Nyman    2021-01-29  1570  	ep->queued_deq_ptr = NULL;
5ceac4402f5d97 Hector Martin    2024-06-11  1571  
484c3bab2d5dfa Michal Pecio     2024-11-06  1572  	/* Check for deferred or newly cancelled TDs */
484c3bab2d5dfa Michal Pecio     2024-11-06  1573  	if (!list_empty(&ep->cancelled_td_list)) {
5ceac4402f5d97 Hector Martin    2024-06-11  1574  		xhci_dbg(ep->xhci, "%s: Pending TDs to clear, continuing with invalidation\n",
5ceac4402f5d97 Hector Martin    2024-06-11  1575  			 __func__);
5ceac4402f5d97 Hector Martin    2024-06-11  1576  		xhci_invalidate_cancelled_tds(ep);
484c3bab2d5dfa Michal Pecio     2024-11-06  1577  		/* Try to restart the endpoint if all is done */
02324cb6fadb62 Mathias Nyman    2026-03-23 @1578  		if (!next_td_is_tainted(ep_ring))
484c3bab2d5dfa Michal Pecio     2024-11-06  1579  			ring_doorbell_for_active_rings(xhci, slot_id, ep_index);
484c3bab2d5dfa Michal Pecio     2024-11-06  1580  		/* Start giving back any TDs invalidated above */
484c3bab2d5dfa Michal Pecio     2024-11-06  1581  		xhci_giveback_invalidated_tds(ep);
5ceac4402f5d97 Hector Martin    2024-06-11  1582  	} else {
e9df17eb1408cf Sarah Sharp      2010-04-02  1583  		/* Restart any rings with pending URBs */
02324cb6fadb62 Mathias Nyman    2026-03-23  1584  		xhci_dbg(ep->xhci, "%s: All cancelled TDs cleared\n", __func__);
02324cb6fadb62 Mathias Nyman    2026-03-23  1585  		if (!next_td_is_tainted(ep_ring))
e9df17eb1408cf Sarah Sharp      2010-04-02  1586  			ring_doorbell_for_active_rings(xhci, slot_id, ep_index);
ae636747146ea9 Sarah Sharp      2009-04-29  1587  	}
5ceac4402f5d97 Hector Martin    2024-06-11  1588  }
ae636747146ea9 Sarah Sharp      2009-04-29  1589  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error

[Dan Carpenter]

Hi Mathias,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mathias-Nyman/xhci-prevent-automatic-endpoint-restart-after-stall-or-error/20260323-224110
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link:    https://lore.kernel.org/r/20260323122512.2019893-2-mathias.nyman%40linux.intel.com
patch subject: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error
config: x86_64-randconfig-161-20260323 (https://download.01.org/0day-ci/archive/20260324/202603240138.ZNq5FcFq-lkp@intel.com/config)
compiler: gcc-13 (Debian 13.3.0-16) 13.3.0
smatch: v0.5.0-9004-gb810ac53

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202603240138.ZNq5FcFq-lkp@intel.com/

smatch warnings:
drivers/usb/host/xhci-ring.c:1578 xhci_handle_cmd_set_deq() error: we previously assumed 'ep_ring' could be null (see line 1467)

vim +/ep_ring +1578 drivers/usb/host/xhci-ring.c

b8200c9479b804 Xenia Ragiadakou 2013-09-09  1448  static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id,
c69a059783b241 Xenia Ragiadakou 2013-09-09  1449  		union xhci_trb *trb, u32 cmd_comp_code)
ae636747146ea9 Sarah Sharp      2009-04-29  1450  {
ae636747146ea9 Sarah Sharp      2009-04-29  1451  	unsigned int ep_index;
e9df17eb1408cf Sarah Sharp      2010-04-02  1452  	unsigned int stream_id;
ae636747146ea9 Sarah Sharp      2009-04-29  1453  	struct xhci_ring *ep_ring;
9aad95e292f58d Hans de Goede    2013-10-04  1454  	struct xhci_virt_ep *ep;
d115b04818e57b John Youn        2009-07-27  1455  	struct xhci_ep_ctx *ep_ctx;
d115b04818e57b John Youn        2009-07-27  1456  	struct xhci_slot_ctx *slot_ctx;
4aa2e16e052b43 Mathias Nyman    2024-11-06  1457  	struct xhci_stream_ctx *stream_ctx;
674f8438c12125 Mathias Nyman    2021-01-29  1458  	struct xhci_td *td, *tmp_td;
ae636747146ea9 Sarah Sharp      2009-04-29  1459  
28ccd2962c6655 Matt Evans       2011-03-29  1460  	ep_index = TRB_TO_EP_INDEX(le32_to_cpu(trb->generic.field[3]));
28ccd2962c6655 Matt Evans       2011-03-29  1461  	stream_id = TRB_TO_STREAM_ID(le32_to_cpu(trb->generic.field[2]));
b1adc42d440df3 Mathias Nyman    2021-01-29  1462  	ep = xhci_get_virt_ep(xhci, slot_id, ep_index);
b1adc42d440df3 Mathias Nyman    2021-01-29  1463  	if (!ep)
b1adc42d440df3 Mathias Nyman    2021-01-29  1464  		return;
e9df17eb1408cf Sarah Sharp      2010-04-02  1465  
42f2890aa998d1 Mathias Nyman    2021-01-29  1466  	ep_ring = xhci_virt_ep_to_ring(xhci, ep, stream_id);
e9df17eb1408cf Sarah Sharp      2010-04-02 @1467  	if (!ep_ring) {
                                                             ^^^^^^^

e587b8b270d370 Oliver Neukum    2014-01-08  1468  		xhci_warn(xhci, "WARN Set TR deq ptr command for freed stream ID %u\n",
e9df17eb1408cf Sarah Sharp      2010-04-02  1469  				stream_id);
e9df17eb1408cf Sarah Sharp      2010-04-02  1470  		/* XXX: Harmless??? */
0d4976ec8ec17f Hans de Goede    2014-08-20  1471  		goto cleanup;
                                                                ^^^^^^^^^^^^
ep_ring is NULL

e9df17eb1408cf Sarah Sharp      2010-04-02  1472  	}
e9df17eb1408cf Sarah Sharp      2010-04-02  1473  
b1adc42d440df3 Mathias Nyman    2021-01-29  1474  	ep_ctx = xhci_get_ep_ctx(xhci, ep->vdev->out_ctx, ep_index);
b1adc42d440df3 Mathias Nyman    2021-01-29  1475  	slot_ctx = xhci_get_slot_ctx(xhci, ep->vdev->out_ctx);
19a7d0d65c4a81 Felipe Balbi     2017-04-07  1476  	trace_xhci_handle_cmd_set_deq(slot_ctx);
19a7d0d65c4a81 Felipe Balbi     2017-04-07  1477  	trace_xhci_handle_cmd_set_deq_ep(ep_ctx);
ae636747146ea9 Sarah Sharp      2009-04-29  1478  
4aa2e16e052b43 Mathias Nyman    2024-11-06  1479  	if (ep->ep_state & EP_HAS_STREAMS) {
4aa2e16e052b43 Mathias Nyman    2024-11-06  1480  		stream_ctx = &ep->stream_info->stream_ctx_array[stream_id];
4aa2e16e052b43 Mathias Nyman    2024-11-06  1481  		trace_xhci_handle_cmd_set_deq_stream(ep->stream_info, stream_id);
4aa2e16e052b43 Mathias Nyman    2024-11-06  1482  	}
4aa2e16e052b43 Mathias Nyman    2024-11-06  1483  
c69a059783b241 Xenia Ragiadakou 2013-09-09  1484  	if (cmd_comp_code != COMP_SUCCESS) {
ae636747146ea9 Sarah Sharp      2009-04-29  1485  		unsigned int ep_state;
ae636747146ea9 Sarah Sharp      2009-04-29  1486  		unsigned int slot_state;
ae636747146ea9 Sarah Sharp      2009-04-29  1487  
c69a059783b241 Xenia Ragiadakou 2013-09-09  1488  		switch (cmd_comp_code) {
0b7c105a04ca79 Felipe Balbi     2017-01-23  1489  		case COMP_TRB_ERROR:
e587b8b270d370 Oliver Neukum    2014-01-08  1490  			xhci_warn(xhci, "WARN Set TR Deq Ptr cmd invalid because of stream ID configuration\n");
ae636747146ea9 Sarah Sharp      2009-04-29  1491  			break;
0b7c105a04ca79 Felipe Balbi     2017-01-23  1492  		case COMP_CONTEXT_STATE_ERROR:
e587b8b270d370 Oliver Neukum    2014-01-08  1493  			xhci_warn(xhci, "WARN Set TR Deq Ptr cmd failed due to incorrect slot or ep state.\n");
5071e6b279178f Mathias Nyman    2016-11-11  1494  			ep_state = GET_EP_CTX_STATE(ep_ctx);
28ccd2962c6655 Matt Evans       2011-03-29  1495  			slot_state = le32_to_cpu(slot_ctx->dev_state);
ae636747146ea9 Sarah Sharp      2009-04-29  1496  			slot_state = GET_SLOT_STATE(slot_state);
aa50b29061d3df Xenia Ragiadakou 2013-08-14  1497  			xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
aa50b29061d3df Xenia Ragiadakou 2013-08-14  1498  					"Slot state = %u, EP state = %u",
ae636747146ea9 Sarah Sharp      2009-04-29  1499  					slot_state, ep_state);
ae636747146ea9 Sarah Sharp      2009-04-29  1500  			break;
0b7c105a04ca79 Felipe Balbi     2017-01-23  1501  		case COMP_SLOT_NOT_ENABLED_ERROR:
e587b8b270d370 Oliver Neukum    2014-01-08  1502  			xhci_warn(xhci, "WARN Set TR Deq Ptr cmd failed because slot %u was not enabled.\n",
e587b8b270d370 Oliver Neukum    2014-01-08  1503  					slot_id);
ae636747146ea9 Sarah Sharp      2009-04-29  1504  			break;
ae636747146ea9 Sarah Sharp      2009-04-29  1505  		default:
e587b8b270d370 Oliver Neukum    2014-01-08  1506  			xhci_warn(xhci, "WARN Set TR Deq Ptr cmd with unknown completion code of %u.\n",
c69a059783b241 Xenia Ragiadakou 2013-09-09  1507  					cmd_comp_code);
ae636747146ea9 Sarah Sharp      2009-04-29  1508  			break;
ae636747146ea9 Sarah Sharp      2009-04-29  1509  		}
ae636747146ea9 Sarah Sharp      2009-04-29  1510  		/* OK what do we do now?  The endpoint state is hosed, and we
ae636747146ea9 Sarah Sharp      2009-04-29  1511  		 * should never get to this point if the synchronization between
ae636747146ea9 Sarah Sharp      2009-04-29  1512  		 * queueing, and endpoint state are correct.  This might happen
ae636747146ea9 Sarah Sharp      2009-04-29  1513  		 * if the device gets disconnected after we've finished
ae636747146ea9 Sarah Sharp      2009-04-29  1514  		 * cancelling URBs, which might not be an error...
ae636747146ea9 Sarah Sharp      2009-04-29  1515  		 */
ae636747146ea9 Sarah Sharp      2009-04-29  1516  	} else {
9aad95e292f58d Hans de Goede    2013-10-04  1517  		u64 deq;
9aad95e292f58d Hans de Goede    2013-10-04  1518  		/* 4.6.10 deq ptr is written to the stream ctx for streams */
9aad95e292f58d Hans de Goede    2013-10-04  1519  		if (ep->ep_state & EP_HAS_STREAMS) {
931e468764b22a Niklas Neronin   2025-09-18  1520  			deq = le64_to_cpu(stream_ctx->stream_ring) & TR_DEQ_PTR_MASK;
e5fa8db0be3e87 Pawel Laszczak   2024-09-05  1521  
e5fa8db0be3e87 Pawel Laszczak   2024-09-05  1522  			/*
e5fa8db0be3e87 Pawel Laszczak   2024-09-05  1523  			 * Cadence xHCI controllers store some endpoint state
e5fa8db0be3e87 Pawel Laszczak   2024-09-05  1524  			 * information within Rsvd0 fields of Stream Endpoint
e5fa8db0be3e87 Pawel Laszczak   2024-09-05  1525  			 * context. This field is not cleared during Set TR
e5fa8db0be3e87 Pawel Laszczak   2024-09-05  1526  			 * Dequeue Pointer command which causes XDMA to skip
e5fa8db0be3e87 Pawel Laszczak   2024-09-05  1527  			 * over transfer ring and leads to data loss on stream
e5fa8db0be3e87 Pawel Laszczak   2024-09-05  1528  			 * pipe.
e5fa8db0be3e87 Pawel Laszczak   2024-09-05  1529  			 * To fix this issue driver must clear Rsvd0 field.
e5fa8db0be3e87 Pawel Laszczak   2024-09-05  1530  			 */
e5fa8db0be3e87 Pawel Laszczak   2024-09-05  1531  			if (xhci->quirks & XHCI_CDNS_SCTX_QUIRK) {
4aa2e16e052b43 Mathias Nyman    2024-11-06  1532  				stream_ctx->reserved[0] = 0;
4aa2e16e052b43 Mathias Nyman    2024-11-06  1533  				stream_ctx->reserved[1] = 0;
e5fa8db0be3e87 Pawel Laszczak   2024-09-05  1534  			}
9aad95e292f58d Hans de Goede    2013-10-04  1535  		} else {
931e468764b22a Niklas Neronin   2025-09-18  1536  			deq = le64_to_cpu(ep_ctx->deq) & TR_DEQ_PTR_MASK;
9aad95e292f58d Hans de Goede    2013-10-04  1537  		}
aa50b29061d3df Xenia Ragiadakou 2013-08-14  1538  		xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
9aad95e292f58d Hans de Goede    2013-10-04  1539  			"Successful Set TR Deq Ptr cmd, deq = @%08llx", deq);
9aad95e292f58d Hans de Goede    2013-10-04  1540  		if (xhci_trb_virt_to_dma(ep->queued_deq_seg,
9aad95e292f58d Hans de Goede    2013-10-04  1541  					 ep->queued_deq_ptr) == deq) {
bf161e85fb153c Sarah Sharp      2011-02-23  1542  			/* Update the ring's dequeue segment and dequeue pointer
bf161e85fb153c Sarah Sharp      2011-02-23  1543  			 * to reflect the new position.
bf161e85fb153c Sarah Sharp      2011-02-23  1544  			 */
856563be98b2e9 Niklas Neronin   2025-03-06  1545  			ep_ring->deq_seg = ep->queued_deq_seg;
856563be98b2e9 Niklas Neronin   2025-03-06  1546  			ep_ring->dequeue = ep->queued_deq_ptr;
bf161e85fb153c Sarah Sharp      2011-02-23  1547  		} else {
e587b8b270d370 Oliver Neukum    2014-01-08  1548  			xhci_warn(xhci, "Mismatch between completed Set TR Deq Ptr command & xHCI internal state.\n");
bf161e85fb153c Sarah Sharp      2011-02-23  1549  			xhci_warn(xhci, "ep deq seg = %p, deq ptr = %p\n",
9aad95e292f58d Hans de Goede    2013-10-04  1550  				  ep->queued_deq_seg, ep->queued_deq_ptr);
bf161e85fb153c Sarah Sharp      2011-02-23  1551  		}
ae636747146ea9 Sarah Sharp      2009-04-29  1552  	}
674f8438c12125 Mathias Nyman    2021-01-29  1553  	/* HW cached TDs cleared from cache, give them back */
674f8438c12125 Mathias Nyman    2021-01-29  1554  	list_for_each_entry_safe(td, tmp_td, &ep->cancelled_td_list,
674f8438c12125 Mathias Nyman    2021-01-29  1555  				 cancelled_td_list) {
674f8438c12125 Mathias Nyman    2021-01-29  1556  		ep_ring = xhci_urb_to_transfer_ring(ep->xhci, td->urb);
674f8438c12125 Mathias Nyman    2021-01-29  1557  		if (td->cancel_status == TD_CLEARING_CACHE) {
674f8438c12125 Mathias Nyman    2021-01-29  1558  			td->cancel_status = TD_CLEARED;
0d9b9f533bf1aa Mathias Nyman    2021-08-20  1559  			xhci_dbg(ep->xhci, "%s: Giveback cancelled URB %p TD\n",
0d9b9f533bf1aa Mathias Nyman    2021-08-20  1560  				 __func__, td->urb);
674f8438c12125 Mathias Nyman    2021-01-29  1561  			xhci_td_cleanup(ep->xhci, td, ep_ring, td->status);
0d9b9f533bf1aa Mathias Nyman    2021-08-20  1562  		} else {
0d9b9f533bf1aa Mathias Nyman    2021-08-20  1563  			xhci_dbg(ep->xhci, "%s: Keep cancelled URB %p TD as cancel_status is %d\n",
0d9b9f533bf1aa Mathias Nyman    2021-08-20  1564  				 __func__, td->urb, td->cancel_status);
674f8438c12125 Mathias Nyman    2021-01-29  1565  		}
674f8438c12125 Mathias Nyman    2021-01-29  1566  	}
0d4976ec8ec17f Hans de Goede    2014-08-20  1567  cleanup:
b1adc42d440df3 Mathias Nyman    2021-01-29  1568  	ep->ep_state &= ~SET_DEQ_PENDING;
b1adc42d440df3 Mathias Nyman    2021-01-29  1569  	ep->queued_deq_seg = NULL;
b1adc42d440df3 Mathias Nyman    2021-01-29  1570  	ep->queued_deq_ptr = NULL;
5ceac4402f5d97 Hector Martin    2024-06-11  1571  
484c3bab2d5dfa Michal Pecio     2024-11-06  1572  	/* Check for deferred or newly cancelled TDs */
484c3bab2d5dfa Michal Pecio     2024-11-06  1573  	if (!list_empty(&ep->cancelled_td_list)) {
5ceac4402f5d97 Hector Martin    2024-06-11  1574  		xhci_dbg(ep->xhci, "%s: Pending TDs to clear, continuing with invalidation\n",
5ceac4402f5d97 Hector Martin    2024-06-11  1575  			 __func__);
5ceac4402f5d97 Hector Martin    2024-06-11  1576  		xhci_invalidate_cancelled_tds(ep);
484c3bab2d5dfa Michal Pecio     2024-11-06  1577  		/* Try to restart the endpoint if all is done */
02324cb6fadb62 Mathias Nyman    2026-03-23 @1578  		if (!next_td_is_tainted(ep_ring))
                                                                                        ^^^^^^^

484c3bab2d5dfa Michal Pecio     2024-11-06  1579  			ring_doorbell_for_active_rings(xhci, slot_id, ep_index);
484c3bab2d5dfa Michal Pecio     2024-11-06  1580  		/* Start giving back any TDs invalidated above */
484c3bab2d5dfa Michal Pecio     2024-11-06  1581  		xhci_giveback_invalidated_tds(ep);
5ceac4402f5d97 Hector Martin    2024-06-11  1582  	} else {
e9df17eb1408cf Sarah Sharp      2010-04-02  1583  		/* Restart any rings with pending URBs */
02324cb6fadb62 Mathias Nyman    2026-03-23  1584  		xhci_dbg(ep->xhci, "%s: All cancelled TDs cleared\n", __func__);
02324cb6fadb62 Mathias Nyman    2026-03-23  1585  		if (!next_td_is_tainted(ep_ring))
                                                                                        ^^^^^^^
Dead.

e9df17eb1408cf Sarah Sharp      2010-04-02  1586  			ring_doorbell_for_active_rings(xhci, slot_id, ep_index);
ae636747146ea9 Sarah Sharp      2009-04-29  1587  	}
5ceac4402f5d97 Hector Martin    2024-06-11  1588  }

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki