[PATCH 0/4] usb: xhci: improve Set TR Deq completion error handling

[PATCH 0/4] usb: xhci: improve Set TR Deq completion error handling

[Niklas Neronin]

The Set TR Deq command instructs the hardware to skip Transfer Descriptors
(TDs) from A to B. Once the hardware processes the Set TR Deq command,
it returns a completion event indicating either success or an error.

In the event of success, TDs A to B are released, the dequeue pointer is
updated, and if there are still cancelled TDs, a new Set TR Deq command
is issued. Conversely, if an error occurs, a warning is logged, and TDs A
to B are released. This can lead to a mismatch between the driver
and hardware, eventually resulting in an critical error.

When feasible, attempt to resolve errors. Otherwise, retain unresolved
errors as they are and improve the warning message.

xHCI driver issues Set TR Deq commands in 4 places:
xhci_handle_cmd_stop_ep()
  After a successful Stop Endpoint completion event is received.

xhci_handle_cmd_set_deq()
  After receiving a Set TR Deq completion event and there are new
  cancelled TDs.

xhci_handle_cmd_reset_ep()
  After a Endpoint reset completion, Endpoint state is Stopped.

xhci_urb_dequeue()
  If clearing Endpoints Transaction Translator, Endpoint state is Stopped.

Niklas Neronin (4):
  usb: xhci: handle Set TR Deq TRB Error
  usb: xhci: handle Set TR Deq Slot Not Enabled Error
  usb: xhci: handle Set TR Deq Context State Error due to Slot state
  usb: xhci: handle Set TR Deq Context State Error due to Endpoint state

 drivers/usb/host/xhci-ring.c | 68 ++++++++++++++++++++++++++++--------
 1 file changed, 54 insertions(+), 14 deletions(-)

-- 
2.50.1

[PATCH 1/4] usb: xhci: handle Set TR Deq TRB Error

[Niklas Neronin]

Set TR Deq TRB Error can occur for two reasons:

1. Stream ID is zero or more than the Max Primary Streams.
   This is currently caught by xhci_virt_ep_to_ring(), at the start of the
   function. Thus, this commit does not add handling for this error.

2. Stream ID is non-zero and Stream ID boundary check failed.
   Add a warning detailing the exact reason for this failure and print
   the deq ptr from the Set TR Deq command.
   Macro 'SCTX_DEQ_MASK' is a mask for the TR Dequeue ptr bit field 63:4.

Reuse local variable 'deq'; just change it to 'dma_addr_t', which is what
it should have originally been. (This and 'SCTX_DEQ_MASK' rename is part of
another patch series that I will soon submit)

xHCI specification, rev 1.2:
 Set TR Dequeue Pointer	section 4.6.10
 Stream Array Boundary Checking	4.12.2.1

Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
 drivers/usb/host/xhci-ring.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index ecd757d482c5..7bd559294742 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1420,6 +1420,7 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id,
 	struct xhci_slot_ctx *slot_ctx;
 	struct xhci_stream_ctx *stream_ctx;
 	struct xhci_td *td, *tmp_td;
+	dma_addr_t deq;
 
 	ep_index = TRB_TO_EP_INDEX(le32_to_cpu(trb->generic.field[3]));
 	stream_id = TRB_TO_STREAM_ID(le32_to_cpu(trb->generic.field[2]));
@@ -1451,7 +1452,14 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id,
 
 		switch (cmd_comp_code) {
 		case COMP_TRB_ERROR:
-			xhci_warn(xhci, "WARN Set TR Deq Ptr cmd invalid because of stream ID configuration\n");
+			/*
+			 * Caused by incorrect Stream ID, already checked by xhci_virt_ep_to_ring(),
+			 * or failed boundary check. A failed boundary check means that Set TR Deq
+			 * command references memory that it doesn't have access to.
+			 */
+			deq = le64_to_cpu(trb->event_cmd.cmd_trb) & SCTX_DEQ_MASK;
+			xhci_warn(xhci, "Set TR Deq error stream %u boundary check failed TRB @%pad slot %d ep %u\n",
+				  stream_id, &deq, slot_id, ep_index);
 			break;
 		case COMP_CONTEXT_STATE_ERROR:
 			xhci_warn(xhci, "WARN Set TR Deq Ptr cmd failed due to incorrect slot or ep state.\n");
@@ -1478,7 +1486,6 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id,
 		 * cancelling URBs, which might not be an error...
 		 */
 	} else {
-		u64 deq;
 		/* 4.6.10 deq ptr is written to the stream ctx for streams */
 		if (ep->ep_state & EP_HAS_STREAMS) {
 			deq = le64_to_cpu(stream_ctx->stream_ring) & SCTX_DEQ_MASK;
@@ -1500,7 +1507,7 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id,
 			deq = le64_to_cpu(ep_ctx->deq) & ~EP_CTX_CYCLE_MASK;
 		}
 		xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
-			"Successful Set TR Deq Ptr cmd, deq = @%08llx", deq);
+			"Successful Set TR Deq Ptr cmd, deq = @%pad", &deq);
 		if (xhci_trb_virt_to_dma(ep->queued_deq_seg,
 					 ep->queued_deq_ptr) == deq) {
 			/* Update the ring's dequeue segment and dequeue pointer
-- 
2.50.1

Re: [PATCH 1/4] usb: xhci: handle Set TR Deq TRB Error

[Michał Pecio]

On Mon, 18 Aug 2025 14:57:39 +0200, Niklas Neronin wrote:
> Set TR Deq TRB Error can occur for two reasons:

Surely for other reasons too, like the infamous ASMedia case
I mentioned last time when this patch showed up here.

In general, any illegal value in any TRB field is a TRB Error.

And in general, I think it would be better if those log messages simply
named the error received from HW, without trying to speculate about
possible causes (and getting it wrong). It would be less misleading and
it would instantly give the keywoard to search for in the spec, without
grepping for the error message to find which event code triggers it.

> 1. Stream ID is zero or more than the Max Primary Streams.
>    This is currently caught by xhci_virt_ep_to_ring(), at the start of the
>    function. Thus, this commit does not add handling for this error.

Nit: this is only true if ep->stream_info->num_streams at the time of
handling this event <= MaxPStreams at the time of command execution ;)

So there are two theoretical bugs which could make this check fail.

> 2. Stream ID is non-zero and Stream ID boundary check failed.

Not sure what's the difference? Per spec, boundary check *is* checking
if Stream ID is in range, to prevent the xHC from writing to a Stream
Context outside actual SC Array and corrupting some random memory.

>    Add a warning detailing the exact reason for this failure and print
>    the deq ptr from the Set TR Deq command.
>    Macro 'SCTX_DEQ_MASK' is a mask for the TR Dequeue ptr bit field 63:4.
> 
> Reuse local variable 'deq'; just change it to 'dma_addr_t', which is what
> it should have originally been.

Not sure, this is an always-64bit value read from xHCI data structures.

On a 32bit DMA system it probably won't have the high bits set if you
are reading it from the command (unless there was a bug), but later
other code will read it from HW and then all bets are off. Note that
a 64bit xHC may be installed in a 32bit system. And it may be buggy,
or it may have escaped the transfer ring due to broken Link TRB.

Re: [PATCH 1/4] usb: xhci: handle Set TR Deq TRB Error

[Neronin, Niklas]

On 21/08/2025 12.32, Michał Pecio wrote:
> On Mon, 18 Aug 2025 14:57:39 +0200, Niklas Neronin wrote:
>> Set TR Deq TRB Error can occur for two reasons:
> 
> Surely for other reasons too, like the infamous ASMedia case
> I mentioned last time when this patch showed up here.

I poorly worded it, the reasons are according to Set TR Deq TRB
Error reasons outlined in xHCI specification Section 4.6.10, page 142.
The entire series adds only error handling according to the reasons
in the xHCI specification.

> 
> In general, any illegal value in any TRB field is a TRB Error.
> 
> And in general, I think it would be better if those log messages simply
> named the error received from HW, without trying to speculate about
> possible causes (and getting it wrong). It would be less misleading and
> it would instantly give the keywoard to search for in the spec, without
> grepping for the error message to find which event code triggers it.

Good point, there can still be "out-of-spec" reasons for a TRB Error.
A generic TRB error warning is better.

> 
>> 1. Stream ID is zero or more than the Max Primary Streams.
>>    This is currently caught by xhci_virt_ep_to_ring(), at the start of the
>>    function. Thus, this commit does not add handling for this error.
> 
> Nit: this is only true if ep->stream_info->num_streams at the time of
> handling this event <= MaxPStreams at the time of command execution ;)
> 
> So there are two theoretical bugs which could make this check fail.
> 
>> 2. Stream ID is non-zero and Stream ID boundary check failed.
> 
> Not sure what's the difference? Per spec, boundary check *is* checking
> if Stream ID is in range, to prevent the xHC from writing to a Stream
> Context outside actual SC Array and corrupting some random memory.
> 
>>    Add a warning detailing the exact reason for this failure and print
>>    the deq ptr from the Set TR Deq command.
>>    Macro 'SCTX_DEQ_MASK' is a mask for the TR Dequeue ptr bit field 63:4.
>>
>> Reuse local variable 'deq'; just change it to 'dma_addr_t', which is what
>> it should have originally been.
> 
> Not sure, this is an always-64bit value read from xHCI data structures.
> 
> On a 32bit DMA system it probably won't have the high bits set if you
> are reading it from the command (unless there was a bug), but later
> other code will read it from HW and then all bets are off. Note that
> a 64bit xHC may be installed in a 32bit system. And it may be buggy,
> or it may have escaped the transfer ring due to broken Link TRB.

As I understand it, the actual address values stored in the bitfields
may vary depending on the system architecture, but the bitfield size itself
remains consistent. On a 32-bit system, 'dma_addr_t' is 32 bits, and placing
a 64-bit value into it will result in the upper 32 bits being dropped.

Thank you for the review.

[PATCH 2/4] usb: xhci: handle Set TR Deq Slot Not Enabled Error

[Niklas Neronin]

The Set TR Deq Slot Not Enabled error occurs when the Endpoint State is
disabled. In this state, the slot's Doorbell register is disabled, meaning
it cannot receive or handle Transfer Descriptors (TDs).

Because the slot cannot receive or handle TDs, it should not have any TDs.
Consequently, all cancelled TDs are released.
(The goto 'td_cleanup' is used for other cases in later patches)

Case 'COMP_SLOT_NOT_ENABLED_ERROR' (11) is moved between 'COMP_TRB_ERROR'
(5) and 'COMP_CONTEXT_STATE_ERROR' (19).

xHCI specification, rev 1.2:
 Slot State		section 4.5.3

Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
 drivers/usb/host/xhci-ring.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 7bd559294742..79c15dbae43b 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1461,6 +1461,9 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id,
 			xhci_warn(xhci, "Set TR Deq error stream %u boundary check failed TRB @%pad slot %d ep %u\n",
 				  stream_id, &deq, slot_id, ep_index);
 			break;
+		case COMP_SLOT_NOT_ENABLED_ERROR:
+			xhci_warn(xhci, "Set TR Deq error slot %d is Disabled\n", slot_id);
+			goto td_cleanup;
 		case COMP_CONTEXT_STATE_ERROR:
 			xhci_warn(xhci, "WARN Set TR Deq Ptr cmd failed due to incorrect slot or ep state.\n");
 			ep_state = GET_EP_CTX_STATE(ep_ctx);
@@ -1470,10 +1473,6 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id,
 					"Slot state = %u, EP state = %u",
 					slot_state, ep_state);
 			break;
-		case COMP_SLOT_NOT_ENABLED_ERROR:
-			xhci_warn(xhci, "WARN Set TR Deq Ptr cmd failed because slot %u was not enabled.\n",
-					slot_id);
-			break;
 		default:
 			xhci_warn(xhci, "WARN Set TR Deq Ptr cmd with unknown completion code of %u.\n",
 					cmd_comp_code);
@@ -1554,6 +1553,17 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id,
 		xhci_dbg(ep->xhci, "%s: All TDs cleared, ring doorbell\n", __func__);
 		ring_doorbell_for_active_rings(xhci, slot_id, ep_index);
 	}
+	return;
+td_cleanup:
+	ep->ep_state &= ~SET_DEQ_PENDING;
+	ep->queued_deq_seg = NULL;
+	ep->queued_deq_ptr = NULL;
+
+	list_for_each_entry_safe(td, tmp_td, &ep->cancelled_td_list, cancelled_td_list) {
+		td->cancel_status = TD_CLEARED;
+		ep_ring = xhci_urb_to_transfer_ring(xhci, td->urb);
+		xhci_td_cleanup(xhci, td, ep_ring, td->status);
+	}
 }
 
 static void xhci_handle_cmd_reset_ep(struct xhci_hcd *xhci, int slot_id,
-- 
2.50.1

Re: [PATCH 2/4] usb: xhci: handle Set TR Deq Slot Not Enabled Error

[Michał Pecio]

On Mon, 18 Aug 2025 14:57:40 +0200, Niklas Neronin wrote:
> The Set TR Deq Slot Not Enabled error occurs when the Endpoint State is
> disabled. In this state, the slot's Doorbell register is disabled, meaning
> it cannot receive or handle Transfer Descriptors (TDs).
> 
> Because the slot cannot receive or handle TDs, it should not have any TDs.
> Consequently, all cancelled TDs are released.
> (The goto 'td_cleanup' is used for other cases in later patches)

If there should be no TDs then there should be no need to clean up ;)

Does xhci_hcd really need to bloat itself with workarounds for
hypothetical kernel bugs which are not known to exist and which
should be fixed as soon as they are discovered?

And suppose that somebody does indeed disable a slot without waiting
for pending URBs to finish unlinking, what if he also frees those
virtual endpoints that you would like to manipulate here? And maybe
forgets to clear xhci->devs[x]->eps[y] to NULL?

What if a different device uses the same slot ID now? (That's possibly
a serious problem which perhaps requires a serious solution, btw).

The only thing we know about conditions which would cause this code to
execute is that they are unknown, and the only thing we can expect is
something unexpected, or else we wouldn't be here in the first place.


If there are concerns that slots may be disabled while commands are
pending for them, I think it would make more sense to add sanity checks
right where that happens: ensure that commands cannot be queued for
disabled slots, and that slots cannot be disabled with commands on them.

Then at least you know which function is responsible for the screwup.

In fact, I'm running a local patch which checks for pending URBs when
endpoints or slots are disabled. No violations logged so far.

Re: [PATCH 2/4] usb: xhci: handle Set TR Deq Slot Not Enabled Error

[Michał Pecio]

On Thu, 21 Aug 2025 12:35:21 +0200, Michał Pecio wrote:
> And suppose that somebody does indeed disable a slot without waiting
> for pending URBs to finish unlinking, what if he also frees those
> virtual endpoints that you would like to manipulate here? And maybe
> forgets to clear xhci->devs[x]->eps[y] to NULL?

I dug deeper and realized that this cannot happen, because virtual eps
belong to the same allocation as their parent virtual dev.

What is actually going to happen is that every xhci_disable_slot() is
followed by xhci_free_virt_dev(), so virtual endpoint lookup at the
beginning of xhci_handle_cmd_set_deq() will fail and the function will
bail out silently. This 'td_cleanup' code will get no chance to run.

The silent bailout is obviously wrong, but it can only be improved by
logging the error, and queuing Set TR Deq onto a disabled slot needs to
be prevented from happening in the first place.

As far as I see, it currently is supposed to be prevented by:
1. the core not freeing devices with pending URBs
2. the driver not giving back URBs before Set TR Dequeue completes

One interesting question is what happens if Set TR Dequeue is pending
but the endpoint starts and completes the CLEARING_CACHE TD normally,
I suspect that handle_tx_event() may give it back. Will look into it.

BTW, endpoints are not supposed to start like that after Stop Endpoint
retries have been implemented and I just sent a revert of a dodgy patch
which broke that, but well, in theory it *might* possibly still happen.

> What if a different device uses the same slot ID now? (That's possibly
> a serious problem which perhaps requires a serious solution, btw).

Actually, nothing interesting happens. SLOT_NOT_ENABLED means that the
slot was Disabled. By now, it can at most be Enabled, because any
completion of a later Enable Slot command couldn't have executed yet.
There are no new TDs on the slot and no damage by giving them back.

Also no point trying to give them back ;)

[PATCH 3/4] usb: xhci: handle Set TR Deq Context State Error due to Slot state

[Niklas Neronin]

One of the reasons for a Set TR Deq Context State Error, is that the Slot
state is not Default, Configured, or Addressed. This leaves it 2 options;
Enabled and Disabled. If the slot state was Disabled, HW would have
returned a Slot Not Enabled Error, which only leaves Enabled Slot state.

Devices with a slot state of Enabled must have previously been in a
Disabled slot state. Both Enabled and Disabled slot states have the
Doorbell register disabled, meaning they do not receive or handle TDs.
Consequently, all cancelled TDs are released.

xHCI specification, rev 1.2:
 Slot State		section 4.5.3
 Set TR Dequeue Pointer	section 4.6.10

Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
 drivers/usb/host/xhci-ring.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 79c15dbae43b..f4e4bd8db210 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1465,10 +1465,15 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id,
 			xhci_warn(xhci, "Set TR Deq error slot %d is Disabled\n", slot_id);
 			goto td_cleanup;
 		case COMP_CONTEXT_STATE_ERROR:
-			xhci_warn(xhci, "WARN Set TR Deq Ptr cmd failed due to incorrect slot or ep state.\n");
-			ep_state = GET_EP_CTX_STATE(ep_ctx);
 			slot_state = le32_to_cpu(slot_ctx->dev_state);
 			slot_state = GET_SLOT_STATE(slot_state);
+			if (slot_state == SLOT_STATE_ENABLED) {
+				xhci_warn(xhci, "Set TR Deq error slot %d is Enabled\n", slot_id);
+				goto td_cleanup;
+			}
+
+			xhci_warn(xhci, "WARN Set TR Deq Ptr cmd failed due to incorrect ep state.\n");
+			ep_state = GET_EP_CTX_STATE(ep_ctx);
 			xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
 					"Slot state = %u, EP state = %u",
 					slot_state, ep_state);
-- 
2.50.1

Re: [PATCH 3/4] usb: xhci: handle Set TR Deq Context State Error due to Slot state

[Michał Pecio]

On Mon, 18 Aug 2025 14:57:41 +0200, Niklas Neronin wrote:
> One of the reasons for a Set TR Deq Context State Error, is that the Slot
> state is not Default, Configured, or Addressed. This leaves it 2 options;
> Enabled and Disabled. If the slot state was Disabled, HW would have
> returned a Slot Not Enabled Error, which only leaves Enabled Slot state.
> 
> Devices with a slot state of Enabled must have previously been in a
> Disabled slot state. Both Enabled and Disabled slot states have the
> Doorbell register disabled, meaning they do not receive or handle TDs.
> Consequently, all cancelled TDs are released.

Is there any realistic chance that the virtual endpoint to which those
TDs belonged still exists after a slot disable/enable cycle?

Disabling a slot normally frees the virtual device with all virt eps.
This is true even in exotic cases like using a disable/enable cycle on
Etron HCs as a substitute for Reset Device command.

In more typical cases, the newly enabled slot is a different device.

[PATCH 4/4] usb: xhci: handle Set TR Deq Context State Error due to Endpoint state

[Niklas Neronin]

A Set TR Deq Context State Error occurs when the Slot state is Enabled or
the Endpoint state is Disabled, Running, or Halted. The Slot state is
already handled, leaving the Endpoint state to be addressed.

These are all possible Endpoint state:

 - Disabled: The Endpoint is not operational and cannot process any TDs.
   Release all canceled TDs in the same way as when the Slot state is
   Disabled or Enabled.

 - Running: Reason unknown, print a detailed warning.

 - Halted: Reason unknown, print a detailed warning.

 - Stopped: The state is correct? Something has fixed itself, so it should
   be safe to reissue a new Set TR Deq command. All TDs associated with the
   failed Set TR Deq command are marked as 'TD_DIRTY'. This marking ensures
   that they will be eligible for subsequent Set TR Deq command.

 - Error: The state is correct, similar to the Stopped state, reissue a
   new Set TR Deq command.

xHCI specification, rev 1.2:
 Slot State		section 4.5.3
 Endpoint State		section 4.8.3
 Set TR Dequeue Pointer	section 4.6.10

Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
 drivers/usb/host/xhci-ring.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index f4e4bd8db210..2c0238c37418 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1472,11 +1472,29 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id,
 				goto td_cleanup;
 			}
 
-			xhci_warn(xhci, "WARN Set TR Deq Ptr cmd failed due to incorrect ep state.\n");
 			ep_state = GET_EP_CTX_STATE(ep_ctx);
-			xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
-					"Slot state = %u, EP state = %u",
-					slot_state, ep_state);
+			switch (ep_state) {
+			case EP_STATE_DISABLED:
+				xhci_warn(xhci, "Set TR Deq error slot %d ep %u is Disabled\n",
+					  slot_id, ep_index);
+				goto td_cleanup;
+			case EP_STATE_RUNNING:
+				xhci_warn(xhci, "Set TR Deq error ep Running slot %d ep %u ep flag 0x%x\n",
+					  slot_id, ep_index, ep->ep_state);
+				break;
+			case EP_STATE_HALTED:
+				xhci_warn(xhci, "Set TR Deq error ep Halted slot %d ep %u ep flag 0x%x\n",
+					  slot_id, ep_index, ep->ep_state);
+				break;
+			case EP_STATE_STOPPED:
+			case EP_STATE_ERROR:
+				xhci_warn(xhci, "Set TR Deq error with correct states, reissuing the command\n");
+				list_for_each_entry(td, &ep->cancelled_td_list, cancelled_td_list) {
+					if (td->cancel_status == TD_CLEARING_CACHE)
+						td->cancel_status = TD_DIRTY;
+				}
+				goto cleanup;
+			}
 			break;
 		default:
 			xhci_warn(xhci, "WARN Set TR Deq Ptr cmd with unknown completion code of %u.\n",
-- 
2.50.1

Re: [PATCH 4/4] usb: xhci: handle Set TR Deq Context State Error due to Endpoint state

[Michał Pecio]

On Mon, 18 Aug 2025 14:57:42 +0200, Niklas Neronin wrote:
> A Set TR Deq Context State Error occurs when the Slot state is Enabled or
> the Endpoint state is Disabled, Running, or Halted. The Slot state is
> already handled, leaving the Endpoint state to be addressed.
> 
> These are all possible Endpoint state:
> 
>  - Disabled: The Endpoint is not operational and cannot process any TDs.
>    Release all canceled TDs in the same way as when the Slot state is
>    Disabled or Enabled.

This should really be prevented from happening.
 
>  - Running: Reason unknown, print a detailed warning.
> 
>  - Halted: Reason unknown, print a detailed warning.

Not a bad idea, but no need to duplicate those xhci_warns. There are
helpers in xhci.h to convert comp codes, endpoint states and all sorts
of other magic numbers to human readable string.

>  - Stopped: The state is correct? Something has fixed itself, so it should
>    be safe to reissue a new Set TR Deq command. All TDs associated with the
>    failed Set TR Deq command are marked as 'TD_DIRTY'. This marking ensures
>    that they will be eligible for subsequent Set TR Deq command.

I'm skeptical if this makes sense. What would make an endpoint run with
Set TR Deq pending? What would make it stop? How much confidence there
is that things aren't FUBARed beyond imagination? Was it tested? ;)

Set TR Dequeue code is pretty dumb and it will jump to the first TRB
after the first cancelled TD (or something like that). If the endpoint
may have been running when Set TR Dequeue failed, is it guaranteed that
trying again won't rewind it to some already completed TD?

Is it impossible that the TD we tried to skip has been given back and
the endpoint and/or slot are being disabled right now?

Re: [PATCH 4/4] usb: xhci: handle Set TR Deq Context State Error due to Endpoint state

[Michał Pecio]

On Fri, 22 Aug 2025 10:15:14 +0200, Michał Pecio wrote:
> Set TR Dequeue code is pretty dumb and it will jump to the first TRB
> after the first cancelled TD (or something like that). If the endpoint
> may have been running when Set TR Dequeue failed, is it guaranteed that
> trying again won't rewind it to some already completed TD?

Answering my own question, rewinding back to completed TDs could only
happen if TD cancel status were TD_HALTED, but it's made TD_DIRTY here.
In such case xhci_invalidate_cancelled_tds() will perform trb_in_td()
check and find that another Set TR Deq is not necessary if the HW has
already moved past the TD we tried to get out of. (It may still issue
the command for some other TD, of course.)

BTW, if the HW completed this TD, the TD *should* have already been
given back and this patch should have no effect then. But this is not
guaranteed, because fe49df60cdb7 ("xhci: Mitigate failed set dequeue
pointer commands") turned the TD into No-Ops. (Note that the contrary
isn't guaranteed either, since the xHC is allowed to cache this TD.)

Perhaps fe49df60cdb7 isn't really right? It's not illegal to complete
an unlinked URB due to race with normal execution. Sure, the condition
discussed here is abnormal, but callers won't know the difference.

And in the ASMedia case mentioned by fe49df60cdb7:

1. The unhandled Babble Error makes no real difference. IME, after
   a TRB Error from Set TR Dequeue, the endpoint stays stuck anyway.
   Some other HCs also get stuck similarly, but without TRB Error.

2. Babble Error wasn't handled because the TD had already been given
   back earlier when Set TR Deq failed with TRB Error. Perhaps that's
   the real bug which deserves fixing?

The ASMedia TRB Error case:
https://lore.kernel.org/linux-usb/ZsjgmCjHdzck9UKd@alphanet.ch/
https://lore.kernel.org/linux-usb/20241113002658.0a031681@foxbook/

> Is it impossible that the TD we tried to skip has been given back and
> the endpoint and/or slot are being disabled right now?

It's not impossible, but if the TD is given back and no other TDs
remain then nothing more will be done, so no problem.


In general, this patch looks like an improvement over the status quo -
pretending that the command succeeded and then unconditionally giving
back the TD, even though the HW may still complete it later.

That being said,

1. I'm not aware of any known cases leading to this situation.

2. A loop which finds and updates the TD_CLEARING_CACHE item already
   exists, so I think it would be better to modify this loop instead
   of adding another one. And the loop prints some xhci_dbg(), so it
   would be nice if they showed up in this case as well.


Regards,
Michal

Re: [PATCH 4/4] usb: xhci: handle Set TR Deq Context State Error due to Endpoint state

[Neronin, Niklas]

Thank you for the thorough review and the additional debugging efforts;
they are greatly appreciated. :)


I want to clarify my intentions with this patch set:
1. Add exit points for as many error cases as possible. The function
   xhci_handle_cmd_set_deq() is tightly interwoven, causing any change to
   impact all completion cases. By adding exit points to error cases,
   future modifications can be more precise and targeted.

2. Keep it simple. Handle errors that do not require "fixing" and handle
   them simply. For example, Set TR Deq commands that failed because the
   command is no longer needed. In this case, the logical coding pattern
   is to free the command and return. This approach does not require the
   reader to know that a completion handler for some other command has
   freed the TDs.

3. Assume the cause of the errors are according to the xHCI specification,
   section 4.6.10.

With the above in mind, this is how each error IMO should be handled:

case COMP_TRB_ERROR:
	No known fix, print warning without specifying a reason.
case COMP_SLOT_NOT_ENABLED_ERROR:
	The Set TR Deq command is no longer needed, free and exit.
case COMP_CONTEXT_STATE_ERROR:
	case SLOT_STATE_ENABLED:
		The Set TR Deq command is no longer needed, free and exit.

	case EP_STATE_DISABLED:
		The Set TR Deq command is no longer needed, free and exit.
	case EP_STATE_RUNNING:
	case EP_STATE_HALTED:
		No simple fix, print detailed warning and proceed as usual.
	case EP_STATE_STOPPED:
	case EP_STATE_ERROR:
		IMO it is worthwhile to retry the Set TR Deq command.
		If we don't retry, the driver and hardware will become out
		of sync, which eventually leads to a crash. By retrying,
		we either succeed or still face a potential crash.
		But I'm open to simply printing a warning, as this should
		be an extremely uncommon error case.

> 1. I'm not aware of any known cases leading to this situation.

If that's the case, then we have successfully prevented the error,
which is the best outcome. :)
However, if it happens or a future unrelated change causes it to happen,
we have at least some error handling in place.

> 2. A loop which finds and updates the TD_CLEARING_CACHE item already
>    exists, so I think it would be better to modify this loop instead
>    of adding another one. And the loop prints some xhci_dbg(), so it
>    would be nice if they showed up in this case as well.

To keep things simple, it's better to keep them separate for now. 
This patch set is not the final version of the function; instead,
it's a small step.
However, adding a debug message to the 'td_cleanup' is a good idea.


That being said, I'll do some more testing for the cases you mentioned.
And it might be better as a first step to just print a detailed warning
and promptly exit the function for all error cases. Then in separate
patch sets add handling for specific error cases.

Best Regards
Niklas

Re: [PATCH 4/4] usb: xhci: handle Set TR Deq Context State Error due to Endpoint state

[Michał Pecio]

On Wed, 27 Aug 2025 14:53:14 +0300, Neronin, Niklas wrote:
> Thank you for the thorough review and the additional debugging efforts;
> they are greatly appreciated. :)
> 
> 
> I want to clarify my intentions with this patch set:
> 1. Add exit points for as many error cases as possible. The function
>    xhci_handle_cmd_set_deq() is tightly interwoven, causing any change to
>    impact all completion cases. By adding exit points to error cases,
>    future modifications can be more precise and targeted.

I think the flow can be quite linear and with little duplication:

1. Start with gathering information. Does the endpoint still exist,
   does a TD in TD_CLEARING_CACHE state exist, what's EP Ctx state.

   The TD can be found once and saved on a variable with some clever
   name like 'td' for later use by different switch cases. The loop
   doesn't actually need to give back this TD because later we call
   xhci_giveback_invalidated_tds(). Not sure why I didn't clean it
   up when I added this call, maybe becasue it was a -stable patch.

2. If success, update ep_ring->dequeue and td->cancel_status. The TD
   is supposed to exist, success practically guarantees that endpoint
   is stopped so the TD couldn't complete and nobody else should give
   it back in TD_CLEARING_CACHE state, so !td here would be a bug.

3. If failure, report it with any relevant information from step 1.
   Then decide what to do with the TD, because somebody expected it
   to be given back when this command completes:
   - if !td then nothing left to do, though it's still a bug
   - maybe mark it TD_CLEARED (current behavior, not great)
   - maybe mark it TD_DIRTY for a retry if there is hope it will work
   - maybe try some crazy desperate error recovery means

   - or maybe give it back and never restart the endpoint again?

     It's not a regression if it never happens or the xHC is unhappy
     and the endpoint will not work afterwards anyway. This would
     need some thinking about the exact definition of "never again".

4. Clear SET_DEQ_PENDING and related stuff.

5. Call xhci_process_cancelled_tds() or equivalent. If there are any
   cancelled TDs left at this point, this step is not optional. Anyone
   could add new TDs while the command was pending and this handler is
   responsible for ensuring that they are invalidated and given back.

> 2. Keep it simple. Handle errors that do not require "fixing" and handle
>    them simply. For example, Set TR Deq commands that failed because the
>    command is no longer needed. In this case, the logical coding pattern
>    is to free the command and return. This approach does not require the
>    reader to know that a completion handler for some other command has
>    freed the TDs.

If "no longer needed" means "the TD in TD_CLEARING_CACHE state is gone"
then this case is already covered by existing code. And a new command
may still be needed for other cancelled TDs, so the hanler can't just
do nothing and return. A bigger problem is that in this situation we
most likely got a Context State Error and the endpoint is now running.

It is virtually impossible for other handler to give back a TD waiting
for Set TR Dequeue because handlers use xhci_giveback_invalidated_tds()
which wouldn't touch a TD in this state.

> 
> 3. Assume the cause of the errors are according to the xHCI specification,
>    section 4.6.10.
>
> With the above in mind, this is how each error IMO should be handled:
> 
> case COMP_TRB_ERROR:
> 	No known fix, print warning without specifying a reason.

The ugly question is what to do with the TD. We know that the command
failed, the endpoint is probably stopped and if we restart it, the TD
may execute from xHC cache. But it may also not execute because it has
been turned into No-Ops, or it may execute in a corrupted form (partly
from cache, partly from the ring.

And somebody wanted this TD to be given back ASAP.

> case COMP_SLOT_NOT_ENABLED_ERROR:
> 	The Set TR Deq command is no longer needed, free and exit.
> case COMP_CONTEXT_STATE_ERROR:
> 	case SLOT_STATE_ENABLED:
> 		The Set TR Deq command is no longer needed, free and exit.
> 
> 	case EP_STATE_DISABLED:
> 		The Set TR Deq command is no longer needed, free and exit.

As far as I see, in these cases it is practically guaranteed that the
virtual endpoint no longer exists, so this code wouldn't even execute.

And if the xHCI slot has been disabled, what might be the state of USB
core at this point? Could 'struct usb_device' be freed as well, maybe
the driver unloaded too? Basically somebody leaked an URB which is now
filled with dangling pointers?

> 	case EP_STATE_RUNNING:
> 	case EP_STATE_HALTED:
> 		No simple fix, print detailed warning and proceed as usual.

Halted may be OK to log an error and ignore. We will get a completion
event and things will perhaps get back on track, although it's still
WTF that we got here and unsure if it won't happen again in the future.

Running is problematic, because we may have more cancelled TDs pending
and we clearly lost control of the endpoint for unknown reason.

> 	case EP_STATE_STOPPED:
> 	case EP_STATE_ERROR:
> 		IMO it is worthwhile to retry the Set TR Deq command.
> 		If we don't retry, the driver and hardware will become out
> 		of sync, which eventually leads to a crash. By retrying,
> 		we either succeed or still face a potential crash.
> 		But I'm open to simply printing a warning, as this should
> 		be an extremely uncommon error case.

Possibly makes sense in Stopped. Which is also not likely to happen,
because Stop Endpoint shouldn't be queued with Set TR Dequeue pending.

But maybe we could get there through Reset Endpoint with only one bug:
1. BUG: the endpoint runs while Set TR Dequeue is pending
2. then it halts for some reason before the command executes
3. transfer event handler queues Reset Endpoint
4. the command fails (Halted) and writes a Context State Error event
5. Reset Endpoint completes before this event is handled

We generally try not to queue multiple commands on the same endpoint
to reduce race condition headache, but Reset Endpoint is an exception.
I don't remember why, but I thought this exception is necessary evil.

As for Error, I don't know. The driver makes no effort to recover from
transfer TRB Errors. If it ever happens, handle_tx_event() gives back
the TD normally without attempting to fix the endpoint.

This also means that the TD which xhci_get_hw_deq() currently points to
has either already been given back or will be given back soon.

It would take a whole separate work to enable TRB Error recovery, and
why bother? Apparently nobody is complaining, and if it ever happens,
just fix the real bug which caused it.

Restarting an endpoint after TRB Error risks that we get it again when
the class driver retries the same or similar URB.

> > 1. I'm not aware of any known cases leading to this situation.  
> 
> If that's the case, then we have successfully prevented the error,
> which is the best outcome. :)
> However, if it happens or a future unrelated change causes it to happen,
> we have at least some error handling in place.
> 
> > 2. A loop which finds and updates the TD_CLEARING_CACHE item already
> >    exists, so I think it would be better to modify this loop instead
> >    of adding another one. And the loop prints some xhci_dbg(), so it
> >    would be nice if they showed up in this case as well.  
> 
> To keep things simple, it's better to keep them separate for now. 
> This patch set is not the final version of the function; instead,
> it's a small step.
> However, adding a debug message to the 'td_cleanup' is a good idea.
> 
> 
> That being said, I'll do some more testing for the cases you mentioned.
> And it might be better as a first step to just print a detailed warning
> and promptly exit the function for all error cases. Then in separate
> patch sets add handling for specific error cases.
> 
> Best Regards
> Niklas

Re: [PATCH 4/4] usb: xhci: handle Set TR Deq Context State Error due to Endpoint state

[Michał Pecio]

On Fri, 29 Aug 2025 10:06:49 +0200, Michał Pecio wrote:
> We generally try not to queue multiple commands on the same endpoint
> to reduce race condition headache, but Reset Endpoint is an exception.
> I don't remember why, but I thought this exception is necessary evil.

I think it's because handle_tx_event() knows the completion code, so it
can assign accurate urb->status (EPRORO or EPIPE). I guess it would take
a bit of work to separate updating status (immediately) from queuing
Reset Endpoint (later when pending commands complete), though I haven't
really looked into it.

It could possibly increase error recovery latency in some cases.

For now, we live with the headache of Reset Endpoint executing
concurrently with other commands or their completion handlers.

Re: [PATCH 4/4] usb: xhci: handle Set TR Deq Context State Error due to Endpoint state

[Michał Pecio]

Correction:

I previously missed this line buried in xhci_invalidate_cancelled_tds():

                list_del_init(&td->td_list);

Which means that a TD waiting for Set TR Dequeue cannot be processed by
handle_tx_event() if it completes normally. I don't know whether it's a
good thing or not, but that's what it is today.

Ideally, endpoints just shouldn't run with Set TR Deq pending.