[PATCH 0/2] xhci: Fix the NEC stop bug workaround

[PATCH 0/2] xhci: Fix the NEC stop bug workaround

[Michal Pecio]

Hi,


This is the promised v2 of this bugfix. It took longer than expected
because I got sidetracked by two (related) issues:

1. looking for similar bugs in other chips
2. simplifying this to avoid adding the STOP_CMD_REDUNDANT flag

Changes in v2:

1. Dropped the warning patch, because dealing with other chips is a
   separate issue from fixing the bug in existing code.
2. Added CC:stable to make the patch bot happy.
3. Some comment updates/clarifications to address questions asked by
   reviewers. Comments made vendor-agnostic, no longer mention NEC in
   preparation for other buggy vendors.
4. Added an RFC patch to simplify things and avoid queuing redundant
   commands instead of trying to handle them. Still a little dodgy in
   one place, problem described in a C99 comment. But it works for me.

The simplification is a separate patch because that's how the code
evolved and because it could enable the more straightforward and lower
risk patch 1/2 to be used in stable without patch 2/2, if desired.

Or otherwise, I could squash the patches together, of course.


Regarding other chips, the following was found:
1. NEC discovered this bug and fixed it in a silicon or FW revision.
   Some chips have the bug, but I have one which doesn't.
2. I couldn't reproduce this bug on VIA VL805 and Etron EJ168A.
3. Two ASMedia chips tested, both have the bug. ASM3142 aka ASM2142
   is quite subtle, because it only seems to happen when multiple EPs
   are used at the same time. I suspect it's a matter of the command
   ring fetching commands asynchronously before we ring the command
   doorbell, or simply increased xHC load triggers some internal bug.

ASMedia presents an additional challange because it sometimes gets
stuck: Stop Endpoint fails in Stopped state even though our ep_state
says it should be running, and it never starts. I need to investigate
what exactly goes wrong and if our ep_state is bad or the chip.

This is dangerous, because the naive workaround would simply retry
the command forever. I suppose it may be a very good idea to add some
timeout. Say, if 100ms passes and the commands are still failing, just
assume that it is stopped for good and go ahead.


Regards,
Michal

[PATCH v2 1/2] usb: xhci: Fix the NEC stop bug workaround

[Michal Pecio]

The NEC uPD720200 has a bug which prevents reliably stopping
an endpoint shortly after it has been restarted. This usually
happens when a driver kills many URBs in quick succession and
it results in concurrent execution and cancellation of TDs.

This is handled by stopping the endpoint again if in doubt.

This "doubt" turns out to be a problem, because Stop Endpoint
may be queued when the EP is already Stopped (for Set TR Deq
execution, for example) or becomes Stopped concurrently (by
Reset Endpoint, for example). If the EP is truly Stopped, the
command fails and further retries just keep failing forever.

This is easily triggered by modifying uvcvideo to unlink its
isochronous URBs in 100us intervals instead of poisoning them.
Any driver that unlinks URBs asynchronously may trigger this,
and any URB unlink during ongoing halt recovery also can.

Fix the problem by flagging redundant Stop Endpoint commands
which are sure to fail, and by not retrying them. It's easy,
because xhci_urb_dequeue() is the only user ever queuing the
command with the default handler which can't guarantee that
the endpoint is Running and will not Halt before it Stops.
For this case, we assume that an endpoint with pending URBs
is always Running, unless certain operations are pending on
it which indicate known exceptions.

Also detect if a Halted EP was reset concurrently after our
command failed. As command handlers run in order, EP_HALTED
would still be set in this case, so simply test for it.

It's possible that other HCs have similar bugs (see also the
related "Running" case below), but the workaround is limited
to NEC chips, where it is known to give good results.

Fixes: fd9d55d190c0 ("xhci: retry Stop Endpoint on buggy NEC controllers")
CC: stable@vger.kernel.org
Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
---
 drivers/usb/host/xhci-ring.c | 45 +++++++++++++++++++++++++++++++++---
 drivers/usb/host/xhci.h      |  2 ++
 2 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index b6eb928e260f..1441c196a5c0 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1149,15 +1149,37 @@ static void xhci_handle_cmd_stop_ep(struct xhci_hcd *xhci, int slot_id,
 				break;
 			ep->ep_state &= ~EP_STOP_CMD_PENDING;
 			return;
+
 		case EP_STATE_STOPPED:
 			/*
-			 * NEC uPD720200 sometimes sets this state and fails with
-			 * Context Error while continuing to process TRBs.
-			 * Be conservative and trust EP_CTX_STATE on other chips.
+			 * Per xHCI 4.6.9, Stop Endpoint command on a Stopped
+			 * EP is a Context State Error, and EP stays Stopped.
+			 * This outcome is valid if the command was redundant.
+			 */
+			if (ep->ep_state & EP_STOP_CMD_REDUNDANT)
+				break;
+			/*
+			 * Or it really failed on Halted, but somebody ran Reset
+			 * Endpoint later. EP state is now Stopped and EP_HALTED
+			 * still set because Reset EP handler will run after us.
+			 */
+			if (ep->ep_state & EP_HALTED)
+				break;
+			/*
+			 * On some HCs EP state remains Stopped for some tens of
+			 * us to a few ms or more after a doorbell ring, and any
+			 * new Stop Endpoint fails without aborting the restart.
+			 * This handler may run quickly enough to still see this
+			 * Stopped state, but it will soon change to Running.
+			 *
+			 * Assume this bug on unexpected Stop Endpoint failures.
+			 * Keep retrying until the EP starts and stops again, on
+			 * chips known to have the bug and to react positively.
 			 */
 			if (!(xhci->quirks & XHCI_NEC_HOST))
 				break;
 			fallthrough;
+
 		case EP_STATE_RUNNING:
 			/* Race, HW handled stop ep cmd before ep was running */
 			xhci_dbg(xhci, "Stop ep completion ctx error, ep is running\n");
@@ -4375,11 +4397,28 @@ int xhci_queue_evaluate_context(struct xhci_hcd *xhci, struct xhci_command *cmd,
 int xhci_queue_stop_endpoint(struct xhci_hcd *xhci, struct xhci_command *cmd,
 			     int slot_id, unsigned int ep_index, int suspend)
 {
+	struct xhci_virt_ep *ep;
 	u32 trb_slot_id = SLOT_ID_FOR_TRB(slot_id);
 	u32 trb_ep_index = EP_INDEX_FOR_TRB(ep_index);
 	u32 type = TRB_TYPE(TRB_STOP_RING);
 	u32 trb_suspend = SUSPEND_PORT_FOR_TRB(suspend);
 
+	/*
+	 * Any of that means the EP is or will be Stopped by the time this
+	 * command runs. Queue it anyway for its side effects like calling
+	 * our default handler or complete(). But our handler must know if
+	 * the command is redundant, so check it now. The handler can't do
+	 * it later because those operations may finish before it runs.
+	 */
+	ep = xhci_get_virt_ep(xhci, slot_id, ep_index);
+	if (ep) {
+		if (ep->ep_state & (SET_DEQ_PENDING | EP_HALTED | EP_CLEARING_TT))
+			ep->ep_state |= EP_STOP_CMD_REDUNDANT;
+		else
+			ep->ep_state &= ~EP_STOP_CMD_REDUNDANT;
+	}
+	/* else: don't care, the handler will do nothing anyway */
+
 	return queue_command(xhci, cmd, 0, 0, 0,
 			trb_slot_id | trb_ep_index | type | trb_suspend, false);
 }
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index f0fb696d5619..4db2cb843a8b 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -670,6 +670,8 @@ struct xhci_virt_ep {
 #define EP_SOFT_CLEAR_TOGGLE	(1 << 7)
 /* usb_hub_clear_tt_buffer is in progress */
 #define EP_CLEARING_TT		(1 << 8)
+/* queued Stop Endpoint is expected to fail */
+#define EP_STOP_CMD_REDUNDANT	(1 << 9)
 	/* ----  Related to URB cancellation ---- */
 	struct list_head	cancelled_td_list;
 	struct xhci_hcd		*xhci;
-- 
2.43.0

[PATCH v2 2/2 RFC] usb: xhci: Don't queue redundant Stop Endpoint commands

[Michal Pecio]

Sometimes we can tell right away that the command is pointless because
the endpoint is or will soon be stopped anyway. Detect such cases and
process cancelled TDs without queuing redundunt commands, by leaving
this work to already pending command handlers or by doing it instantly.

This saves some time and CPU cycles and simplifies dealing with those
nasty start/stop reordering bugs present in certain host controllers.

There are four endpoint state flags which imply a stopped endpoint:

1. In case of EP_STOP_CMD_PENDING, simply do nothing as before.

2. In case of EP_HALTED, do nothing and let the Reset Endpoint handler
   clean up cancelled TDs, because it already does that.

3. In case of EP_CLEARING_TT but none of the others, the endpoint has
   been reset already, it is stopped now and no commands are pending.
   Call a new xhci_process_cancelled_tds() function immediately, which
   directly activates xhci_ring's cancellation machinery. If necessary,
   a new Set TR Deq command will be queued.

4. In case of SET_DEQ_PENDING, the handler doesn't clean up cancelled
   TDs. Adapt xhci_invalidate_cancelled_tds() to handle being called
   under SET_DEQ_PENDING and run xhci_process_cancelled_tds() as in 3.

If none of these flags are set, an endpoint with pending URBs is always
supposed to be running, so queue a Stop Endpoint then.

Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
---
 drivers/usb/host/xhci-ring.c | 73 ++++++++++++++++++++----------------
 drivers/usb/host/xhci.c      | 24 ++++++++++--
 drivers/usb/host/xhci.h      |  3 +-
 3 files changed, 62 insertions(+), 38 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 1441c196a5c0..ab97f4595e1b 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -954,9 +954,7 @@ static int xhci_handle_halted_endpoint(struct xhci_hcd *xhci,
 
 /*
  * Fix up the ep ring first, so HW stops executing cancelled TDs.
- * We have the xHCI lock, so nothing can modify this list until we drop it.
- * We're also in the event handler, so we can't get re-interrupted if another
- * Stop Endpoint command completes.
+ * Call this under xhci->lock, so nothing can modify TD lists or interrupt us.
  *
  * only call this when ring is not in a running state
  */
@@ -971,6 +969,7 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep)
 	u64			hw_deq;
 	unsigned int		slot_id = ep->vdev->slot_id;
 	int			err;
+	bool			pending_td = false;	/* cached_td is already pending */
 
 	xhci = ep->xhci;
 
@@ -1000,7 +999,18 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep)
 		if (td->cancel_status == TD_HALTED || trb_in_td(xhci, td, hw_deq, false)) {
 			switch (td->cancel_status) {
 			case TD_CLEARED: /* TD is already no-op */
+				break;
 			case TD_CLEARING_CACHE: /* set TR deq command already queued */
+				xhci_dbg(xhci, "Found cached TD in URB %p pending\n", td->urb);
+				if (cached_td && !pending_td)
+					/* not supposed to happen... */
+					xhci_warn(xhci, "Cached TDs cleared out of order: URB %p pending but URB %p not done yet\n",
+							td->urb, cached_td->urb);
+					// WTF, no easy way to handle this. We may have already
+					// picked up to two TDs for clearing and can't undo it
+					// without adding more complexity to this function.
+				cached_td = td;
+				pending_td = true;
 				break;
 			case TD_DIRTY: /* TD is cached, clear it */
 			case TD_HALTED:
@@ -1020,12 +1030,14 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep)
 						  "Found multiple active URBs %p and %p in stream %u?\n",
 						  td->urb, cached_td->urb,
 						  td->urb->stream_id);
-					td_to_noop(xhci, ring, cached_td, false);
-					cached_td->cancel_status = TD_CLEARED;
+					td_to_noop(xhci, ring, td, false);
+					td->cancel_status = TD_CLEARED;
+					break;
 				}
 				td_to_noop(xhci, ring, td, false);
 				td->cancel_status = TD_CLEARING_CACHE;
 				cached_td = td;
+				xhci_dbg(xhci, "Pick cached TD in URB %p for clearing\n", td->urb);
 				break;
 			}
 		} else {
@@ -1034,8 +1046,11 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep)
 		}
 	}
 
-	/* If there's no need to move the dequeue pointer then we're done */
-	if (!cached_td)
+	/*
+	 * We are done if there's no need to move the dequeue pointer or if the
+	 * command is already pending. Completion handler will call us again.
+	 */
+	if (!cached_td || ep->ep_state & SET_DEQ_PENDING)
 		return 0;
 
 	err = xhci_move_dequeue_past_td(xhci, slot_id, ep->ep_index,
@@ -1061,6 +1076,19 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep)
 	return 0;
 }
 
+/*
+ * Erase queued TDs from transfer ring(s) and give back those the xHC didn't
+ * stop on. If necessary, queue commands to move the xHC off cancelled TDs it
+ * stopped on. Those will be given back later when the commands complete.
+ *
+ * Call under xhci->lock on a stopped/halted endpoint.
+ */
+void xhci_process_cancelled_tds(struct xhci_virt_ep *ep)
+{
+	xhci_invalidate_cancelled_tds(ep);
+	xhci_giveback_invalidated_tds(ep);
+}
+
 /*
  * Returns the TD the endpoint ring halted on.
  * Only call for non-running rings without streams.
@@ -1154,12 +1182,9 @@ static void xhci_handle_cmd_stop_ep(struct xhci_hcd *xhci, int slot_id,
 			/*
 			 * Per xHCI 4.6.9, Stop Endpoint command on a Stopped
 			 * EP is a Context State Error, and EP stays Stopped.
-			 * This outcome is valid if the command was redundant.
-			 */
-			if (ep->ep_state & EP_STOP_CMD_REDUNDANT)
-				break;
-			/*
-			 * Or it really failed on Halted, but somebody ran Reset
+			 * This is avoided by never queuing redundant commands.
+			 *
+			 * But maybe it failed on Halted, and somebody ran Reset
 			 * Endpoint later. EP state is now Stopped and EP_HALTED
 			 * still set because Reset EP handler will run after us.
 			 */
@@ -1199,11 +1224,10 @@ static void xhci_handle_cmd_stop_ep(struct xhci_hcd *xhci, int slot_id,
 	}
 
 	/* will queue a set TR deq if stopped on a cancelled, uncleared TD */
-	xhci_invalidate_cancelled_tds(ep);
-	ep->ep_state &= ~EP_STOP_CMD_PENDING;
+	xhci_process_cancelled_tds(ep);
 
 	/* Otherwise ring the doorbell(s) to restart queued transfers */
-	xhci_giveback_invalidated_tds(ep);
+	ep->ep_state &= ~EP_STOP_CMD_PENDING;
 	ring_doorbell_for_active_rings(xhci, slot_id, ep_index);
 }
 
@@ -4397,28 +4421,11 @@ int xhci_queue_evaluate_context(struct xhci_hcd *xhci, struct xhci_command *cmd,
 int xhci_queue_stop_endpoint(struct xhci_hcd *xhci, struct xhci_command *cmd,
 			     int slot_id, unsigned int ep_index, int suspend)
 {
-	struct xhci_virt_ep *ep;
 	u32 trb_slot_id = SLOT_ID_FOR_TRB(slot_id);
 	u32 trb_ep_index = EP_INDEX_FOR_TRB(ep_index);
 	u32 type = TRB_TYPE(TRB_STOP_RING);
 	u32 trb_suspend = SUSPEND_PORT_FOR_TRB(suspend);
 
-	/*
-	 * Any of that means the EP is or will be Stopped by the time this
-	 * command runs. Queue it anyway for its side effects like calling
-	 * our default handler or complete(). But our handler must know if
-	 * the command is redundant, so check it now. The handler can't do
-	 * it later because those operations may finish before it runs.
-	 */
-	ep = xhci_get_virt_ep(xhci, slot_id, ep_index);
-	if (ep) {
-		if (ep->ep_state & (SET_DEQ_PENDING | EP_HALTED | EP_CLEARING_TT))
-			ep->ep_state |= EP_STOP_CMD_REDUNDANT;
-		else
-			ep->ep_state &= ~EP_STOP_CMD_REDUNDANT;
-	}
-	/* else: don't care, the handler will do nothing anyway */
-
 	return queue_command(xhci, cmd, 0, 0, 0,
 			trb_slot_id | trb_ep_index | type | trb_suspend, false);
 }
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 899c0effb5d3..f57127358bb4 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1768,10 +1768,28 @@ static int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 		}
 	}
 
-	/* Queue a stop endpoint command, but only if this is
-	 * the first cancellation to be handled.
+	/*
+	 * The endpoint needs to be stopped to remove cancelled TDs from xHC
+	 * queues. But don't stop it unnecessarily: firstly - for efficiency,
+	 * secondly - for correctness, because our completion handler assumes
+	 * that any apparent attempt to stop a stopped EP is a hardware bug.
 	 */
-	if (!(ep->ep_state & EP_STOP_CMD_PENDING)) {
+
+	/* These completion handlers will sort out cancelled TDs for us */
+	if (ep->ep_state & (EP_STOP_CMD_PENDING | EP_HALTED)) {
+		xhci_dbg(xhci, "Not queuing Stop Endpoint on slot %d ep %d state 0x%x\n",
+				urb->dev->slot_id, ep_index, ep->ep_state);
+		goto done;
+	}
+
+	/* In these cases the endpoint is stopped already */
+	if (ep->ep_state & (SET_DEQ_PENDING | EP_CLEARING_TT)) {
+		/* and cancelled TDs can be given back right away */
+		xhci_dbg(xhci, "Invalidating TDs instantly on slot %d ep %d state 0x%x\n",
+				urb->dev->slot_id, ep_index, ep->ep_state);
+		xhci_process_cancelled_tds(ep);
+	} else {
+		/* Otherwise, queue a new Stop Endpoint command */
 		command = xhci_alloc_command(xhci, false, GFP_ATOMIC);
 		if (!command) {
 			ret = -ENOMEM;
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 4db2cb843a8b..ba253e8c79b7 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -670,8 +670,6 @@ struct xhci_virt_ep {
 #define EP_SOFT_CLEAR_TOGGLE	(1 << 7)
 /* usb_hub_clear_tt_buffer is in progress */
 #define EP_CLEARING_TT		(1 << 8)
-/* queued Stop Endpoint is expected to fail */
-#define EP_STOP_CMD_REDUNDANT	(1 << 9)
 	/* ----  Related to URB cancellation ---- */
 	struct list_head	cancelled_td_list;
 	struct xhci_hcd		*xhci;
@@ -1915,6 +1913,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);
+void xhci_process_cancelled_tds(struct xhci_virt_ep *ep);
 
 /* xHCI roothub code */
 void xhci_set_link_state(struct xhci_hcd *xhci, struct xhci_port *port,
-- 
2.43.0

Re: [PATCH 0/2] xhci: Fix the NEC stop bug workaround

[Michal Pecio]

Hi Mathias,

I would be grateful if you could take a look at patch 2/2 and tell if
there is anything obviously wrong with this approach. As far as I see,
it should be OK to just call those invalidation and giveback functions
directly from xhci_urb_dequeue(), and it works for me in practice.

Regarding the probem with xhci_invalidate_cancelled_tds() being called
while Set TR Dequeue is already pending, I found that it is much easier
to handle it by looking at SET_DEQ_PENDING and simply setting all TDs
to TD_CLEARING_CACHE_DEFERRED if that's the case. So this is solved.


However, these patches still don't solve the issue of infinite retries
completely. There is one more annoying case caused by halts. It is very
poorly defined what happens when a halted EP is hard-reset. Usually Set
TR Dequeue executes afterwards and it restarts the EP when done. But if
it doesn't, the EP stays stopped until a new URB is submitted, if ever.

There is the EP_HARD_CLEAR_TOGGLE flag which is set until the class
driver calls usb_clear_halt(), but it's neither the case that the EP is
guaranteed to be stopped until usb_clear_halt() is called nor that it
is guaranteed to restart afterwards.

Actually, I think it might be a bug? What if Set TR Dequeue restarts an
EP before the class driver clears the device side of the halt?


I'm starting to think that it may not be realistic to quickly solve all
those (and possibly other not known yet) problems now. Maybe just slap
a 100ms timeout on those retries, add quirks for ASMedia/Intel and call
it a day for now?

Regards,
Michal

Re: [PATCH 0/2] xhci: Fix the NEC stop bug workaround

[Mathias Nyman]

On 28.10.2024 9.33, Michal Pecio wrote:
> Hi Mathias,
> 
> I would be grateful if you could take a look at patch 2/2 and tell if
> there is anything obviously wrong with this approach. As far as I see,
> it should be OK to just call those invalidation and giveback functions
> directly from xhci_urb_dequeue(), and it works for me in practice.

Adding EP_HALTED case to xhci_urb_deqeue() should work fine, we
will both invalidate and give back the invalidated TDs in the completion
handler.

> 
> Regarding the probem with xhci_invalidate_cancelled_tds() being called
> while Set TR Dequeue is already pending, I found that it is much easier
> to handle it by looking at SET_DEQ_PENDING and simply setting all TDs
> to TD_CLEARING_CACHE_DEFERRED if that's the case. So this is solved.
>

The SET_DEQ_PENDING case is trickier. We would read the dequeue pointer
from hardware while we know hardware is processing a command to move the
dequeue pointer. Result may be old dequeue, or new dequeue,
possible unknown.

We are turning an already difficult issue even more complex

> 
> However, these patches still don't solve the issue of infinite retries
> completely. There is one more annoying case caused by halts. It is very
> poorly defined what happens when a halted EP is hard-reset. Usually Set
> TR Dequeue executes afterwards and it restarts the EP when done. But if
> it doesn't, the EP stays stopped until a new URB is submitted, if ever.
> 
> There is the EP_HARD_CLEAR_TOGGLE flag which is set until the class
> driver calls usb_clear_halt(), but it's neither the case that the EP is
> guaranteed to be stopped until usb_clear_halt() is called nor that it
> is guaranteed to restart afterwards.
> 
> Actually, I think it might be a bug? What if Set TR Dequeue restarts an
> EP before the class driver clears the device side of the halt?

Ok, I need to take some time to dig into this.

> 
> 
> I'm starting to think that it may not be realistic to quickly solve all
> those (and possibly other not known yet) problems now. Maybe just slap
> a 100ms timeout on those retries, add quirks for ASMedia/Intel and call
> it a day for now?

There are some mitigations that could be done.

As many of these issues are related to slow enpoint slow start causing
next stop endpoint command to complete with context error as endpoint is
still stopped.

We could ring the doorbell before giving back the invalidated tds in
xhci_handle_cmd_stop_ep(), and possibly xhci_handle_cmd_set_deq().
This gives hardware a bit more time to start the endpoint.

We could also track pending ring starts.
Set a "EP_START_PENDING flag when doorbell is rung on a stopped endpoint.
clear the flag when firt transfer event is received on that endpoint.

If a stop endpoint command fails with context error due to still being
stopped queue a new stop endpoint command, but only if flag was set:

xhci_handle_cmd_stop_ep()
	if (comp_code == COMP_CONTEXT_STATE_ERROR) {
		switch (GET_EP_CTX_STATE(ep_ctx))
		case EP_STATE_STOPPED:
			if (!(ep->ep_state & EP_START_PENDING)
				break;
			ep->ep_state &= ~EP_START_PENDING;
			xhci_queue_stop_endpoint();

Thanks
Mathias

Re: [PATCH 0/2] xhci: Fix the NEC stop bug workaround

[Michał Pecio]

On Mon, 28 Oct 2024 11:54:39 +0200, Mathias Nyman wrote:
> The SET_DEQ_PENDING case is trickier. We would read the dequeue
> pointer from hardware while we know hardware is processing a command
> to move the dequeue pointer. Result may be old dequeue, or new
> dequeue, possible unknown.

Damn, I looked at various things but I haven't thought about it. Yes,
it's dodgy and not really a great idea. Although I suspect it wouldn't
be *very* harmful, because the truly bad case (failing to queue a Set
TR Deq when it's necessary) is triggered by Set TR Deq already pending
on the same stream, so the stream's cache will be cleared anyway.

But it could easily queue a bunch of pointless commands, for example.

By the way, I think this race is already possible today, without my
patches. There is no testing for SET_DEQ_PENDING in xhci_urb_dequeue()
and no testing in handle_cmd_stop_ep(). If this happens in the middle
of a Set TR Deq chain on a streams endpoint, nothing seems to stop the
Stop EP handler from attempting invalidation under SET_DEQ_PENDING.

Maybe invalidate_cancelled_tds() should bail out if SET_DEQ_PENDING
and later Set Deq completion handler should unconditionally call the
invalidate/giveback combo before it exits.

> We could ring the doorbell before giving back the invalidated tds in
> xhci_handle_cmd_stop_ep(), and possibly xhci_handle_cmd_set_deq().
> This gives hardware a bit more time to start the endpoint.

This unfortunately doesn't buy much time, because giveback is a very
cheap operation - it just adds the URBs to a queue and wakes a worker
which runs all those callbacks. It finishes under 1us on my system.

> We could also track pending ring starts.
> Set a "EP_START_PENDING flag when doorbell is rung on a stopped
> endpoint. clear the flag when firt transfer event is received on that
> endpoint.

Yes, that was the second thing I tried. But I abandoned it:

Problem 1: URBs on "idle" devices are cancelled before generating
any event, so we need to clear the flag on Stop EP and Reset EP.
Not all of them use the default completion handler. Maybe it could
be handled reliably by tapping into handle_cmd_completion(). But...

Problem 2: hardware bugs. On ASMedia 3142, I can trigger (from
userspace) a condition when EP 0 doorbell is rung (even twice)
and its state is still Stopped a few seconds later, and remains
so after repeated Stop EP / doorbell rings.

It looks like a timeout is the only way to be really sure.


Regards,
Michal

Re: [PATCH 0/2] xhci: Fix the NEC stop bug workaround

[Mathias Nyman]

On 29.10.2024 10.28, Michał Pecio wrote:
> 
> By the way, I think this race is already possible today, without my
> patches. There is no testing for SET_DEQ_PENDING in xhci_urb_dequeue()
> and no testing in handle_cmd_stop_ep(). If this happens in the middle
> of a Set TR Deq chain on a streams endpoint, nothing seems to stop the
> Stop EP handler from attempting invalidation under SET_DEQ_PENDING.
> 
> Maybe invalidate_cancelled_tds() should bail out if SET_DEQ_PENDING
> and later Set Deq completion handler should unconditionally call the
> invalidate/giveback combo before it exits.
> 

I think you are on to something.
If we add invalidate/givaback to Set TR deq completion handler, allowing
it to possible queue new Set TR Deq commands, then we can bail out in
xhci_urb_dequeue() if SET_DEQ_PENDING is set.

xhci_urb_dequeue() would not queue a extra stop endpoint command, only
set td->cancel_status to TD_DIRTY dirty, and Set TR Deq handler would
not ring the doorbell unnecessary.

Sounds like a plan to ne.

>> We could ring the doorbell before giving back the invalidated tds in
>> xhci_handle_cmd_stop_ep(), and possibly xhci_handle_cmd_set_deq().
>> This gives hardware a bit more time to start the endpoint.
> 
> This unfortunately doesn't buy much time, because giveback is a very
> cheap operation - it just adds the URBs to a queue and wakes a worker
> which runs all those callbacks. It finishes under 1us on my system.

Probably true, but change is simple and free so might be worth it.

Thanks
Mathias

Re: [PATCH 0/2] xhci: Fix the NEC stop bug workaround

[Mathias Nyman]

On 29.10.2024 11.16, Mathias Nyman wrote:
> On 29.10.2024 10.28, Michał Pecio wrote:
>>
>> By the way, I think this race is already possible today, without my
>> patches. There is no testing for SET_DEQ_PENDING in xhci_urb_dequeue()
>> and no testing in handle_cmd_stop_ep(). If this happens in the middle
>> of a Set TR Deq chain on a streams endpoint, nothing seems to stop the
>> Stop EP handler from attempting invalidation under SET_DEQ_PENDING.
>>
>> Maybe invalidate_cancelled_tds() should bail out if SET_DEQ_PENDING
>> and later Set Deq completion handler should unconditionally call the
>> invalidate/giveback combo before it exits.
>>
> 
> I think you are on to something.
> If we add invalidate/givaback to Set TR deq completion handler, allowing
> it to possible queue new Set TR Deq commands, then we can bail out in
> xhci_urb_dequeue() if SET_DEQ_PENDING is set.
> 
> xhci_urb_dequeue() would not queue a extra stop endpoint command, only
> set td->cancel_status to TD_DIRTY dirty, and Set TR Deq handler would
> not ring the doorbell unnecessary.
> 
> Sounds like a plan to ne.

I wrote a testseries for this.

1st patch avoids stopping endpoint in urb cancel if Set TR Deq is pending
2nd patch handles Set TR Deq command ctx error due to running ep.
3rd patch tracks doorbell ring with a flag. It's for now only used to prevent
     infinite stop ep retries. Flag is not properly cleared in other cases.

Series can be found in my tree in a fix_stop_ep_race branch:

https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/log/?h=fix_stop_ep_race
git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git fix_stop_ep_race branch

Do these help in your NEC host case?

I'll see if I can set up some system to trigger this myself

Thanks
Mathias

Re: [PATCH 0/2] xhci: Fix the NEC stop bug workaround

[Michał Pecio]

Hi,

I will send out v3 of my own patches soon.

I also plan to research a radically different solution, which is simply
to prevent failed Stop Endpoint in the first place. General idea:

1. If commands pending, "outsource" the work to their handlers.
2. If EP stopped for other reason, invalidate/giveback immediately.
3. If Context State != Stopped, queue Stop Endpoint.
4. If Context State == Stopped but shouldn't, set up a delayed work.
5. The work looks at Context State and goto 3.
6. The work may choose to give up retrying after some time.
7. The work could even act as watchdog and call hc_died() if Set Deq
   or Reset EP get stuck in retry or abort loops (not seen so far).

On Wed, 30 Oct 2024 10:29:12 +0200, Mathias Nyman wrote:
> 1st patch avoids stopping endpoint in urb cancel if Set TR Deq is
> pending
> 2nd patch handles Set TR Deq command ctx error due to running ep.
> 3rd patch tracks doorbell ring with a flag. It's for now only
> used to prevent infinite stop ep retries. Flag is not properly
> cleared in other cases.

Quick comments:

1. To be specific, my suggestion was to make the function work even
   if called under SET_DEQ_PENDING rather than try to avoid this. It
   ends up simpler IMO and solves any risk of calls from other places.

   Then the change in xhci_urb_dequeue() becomes an optimization only,
   which is not required for correct operation, may be combined with
   other similar optimizations, or even reverted if necessary without
   breaking multiple stream cancellations again.

2. Mixed feelings. Adds complexity, obviously. Shouldn't be necessary
   if the retries prove to work on other chips (I have never had a Set
   TR Deq error on NEC with the workaround). Could help otherwise.

3. No need to pollute handle_tx_event() at all, because the flag only
   needs to be guaranteed false when the EP is Stopped, and this can
   only happen by *successful* execution of Stop EP or Reset EP. Easy
   to detect this in handle_cmd_completion().

   But I'm not sure if a new flag is needed at all. Its value will
   simply be negation of the condition in xhci_ring_ep_doorbell(),
   except for cases when we "forget" to ring EP doorbell, which are
   probably bugs that should be fixed.

Bugs need to be handled some other way, because hardware has them too
and it's impossible to predict when they bite. See below.

> Series can be found in my tree in a fix_stop_ep_race branch:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/log/?h=fix_stop_ep_race
> git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git
> fix_stop_ep_race branch
> 
> Do these help in your NEC host case?

This looks like it should work on semi-well-behaved HC like NEC, but it
doesn't account for hardware not restarting an EP "just because".


while true; do ifconfig eth0 up; ifconfig eth0 down; done

locks up on ASM3142 with AX88179 adapter as expected, and when the NIC
is disconnected I get those 'ep ctx error, ep still running' every few
seconds. It seems lots of network code got locked up and I can't even
ssh into the box anymore to copy exact dmesg output.

> I'll see if I can set up some system to trigger this myself

Good idea, lot's of fun ;)

Michal

Re: [PATCH 0/2] xhci: Fix the NEC stop bug workaround

[Michał Pecio]

On Thu, 31 Oct 2024 09:13:47 +0100, Michał Pecio wrote:
> This looks like it should work on semi-well-behaved HC like NEC, but
> it doesn't account for hardware not restarting an EP "just because".
> 
> 
> while true; do ifconfig eth0 up; ifconfig eth0 down; done
> 
> locks up on ASM3142 with AX88179 adapter as expected, and when the NIC
> is disconnected I get those 'ep ctx error, ep still running' every few
> seconds. It seems lots of network code got locked up and I can't even
> ssh into the box anymore to copy exact dmesg output.

I apologize for rushed testing and providing bad information.

Your patch doesn't trigger infinite retries because it gives up after
just one retry. The infinite retries every few seconds I observed were
all caused by separate cancellations. The class driver timed out on its
control transfer, cancelled one URB, tried another one, timed out, ...

It takes a few minutes before it gives up, and only if I kill the
ifconfig loop, otherwise it seems to be forever.

Your patch prints one dev_dbg() each time, mine spams many of them for
100ms each time. I will remove this one retry limit from your patch to
see if starts spinning infinitely, but I strongly suspect it will.


One retry is not enough. This is what I got on the first try with a
random UVC webcam:

[ 7297.326596] usb 10-2: new high-speed USB device number 2 using xhci_hcd
[ 7297.465252] usb 10-2: New USB device found, idVendor=1e4e, idProduct=0103, bcdDevice= 0.02
[ 7297.465259] usb 10-2: New USB device strings: Mfr=1, Product=2, SerialNumber=0
[ 7297.465261] usb 10-2: Product: USB2.0 Camera
[ 7297.465263] usb 10-2: Manufacturer: Etron Technology, Inc.
[ 7297.468898] usb 10-2: Found UVC 1.00 device USB2.0 Camera (1e4e:0103)
[ 7297.475995] usb 10-2: UVC non compliance - GET_DEF(PROBE) not supported. Enabling workaround.
[ 7297.476928] input: USB2.0 Camera: USB2.0 Camera as /devices/pci0000:00/0000:00:05.0/0000:03:00.0/usb10/10-2/10-2:1.0/input/input25
[ 7297.492153] usb 10-2: Warning! Unlikely big volume range (=6144), cval->res is probably wrong.
[ 7297.492159] usb 10-2: [3] FU [Mic Capture Volume] ch = 1, val = 5120/11264/1
[ 7299.301892] usb 10-2: Device requested 3060 B/frame bandwidth
[ 7299.301907] usb 10-2: Selecting alternate setting 12 (3060 B/frame bandwidth)
[ 7299.304772] usb 10-2: Allocated 5 URB buffers of 32x3060 bytes each
[ 7300.339246] xhci_hcd 0000:03:00.0: Stop ep ctx error, already stopped with pending start
[ 7300.339252] xhci_hcd 0000:03:00.0: Stop ep completion ctx error, ep is running
[ 7300.339283] xhci_hcd 0000:03:00.0: Context Error unhandled, ctx_state 3
[ 7300.339324] xhci_hcd 0000:03:00.0: Stop ep ctx error, already stopped with pending start
[ 7300.339326] xhci_hcd 0000:03:00.0: Stop ep completion ctx error, ep is running
[ 7300.339365] xhci_hcd 0000:03:00.0: Stop ep completion ctx error, ep is running
[ 7300.339492] xhci_hcd 0000:03:00.0: Stop ep ctx error, already stopped with pending start
[ 7300.339494] xhci_hcd 0000:03:00.0: Stop ep completion ctx error, ep is running
[ 7300.339533] xhci_hcd 0000:03:00.0: Context Error unhandled, ctx_state 3
[ 7300.339593] xhci_hcd 0000:03:00.0: Mismatch between completed Set TR Deq Ptr command & xHCI internal state.
[ 7300.339594] xhci_hcd 0000:03:00.0: ep deq seg = ffff88810b006000, deq ptr = ffff88810ae0e780
[ 7300.339634] xhci_hcd 0000:03:00.0: Stop ep completion ctx error, ep is running

Not sure what caused this mismatch, it might be a new bug of yours
because I don't recall seeing such things before.

This was with your three patches on v6.12-rc4 plus the following:

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 7d036fda354c..7325729beac8 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1182,6 +1182,8 @@ static void xhci_handle_cmd_stop_ep(struct xhci_hcd *xhci, int slot_id,
 		default:
 			break;
 		}
+		xhci_err(xhci, "Context Error unhandled, ctx_state %d\n",
+				GET_EP_CTX_STATE(ep_ctx));
 	}
 
 	/* will queue a set TR deq if stopped on a cancelled, uncleared TD */

Re: [PATCH 0/2] xhci: Fix the NEC stop bug workaround

[Michał Pecio]

Update:

> Your patch prints one dev_dbg() each time, mine spams many of them for
> 100ms each time. I will remove this one retry limit from your patch to
> see if starts spinning infinitely, but I strongly suspect it will.

Yes, that's exactly what happens.

This time I have killed the ifconfig loop, unplugged the NIC and
started 'rmmod xhci_pci', which is still hanging 10 minutes later.

So business as usual when these things go wrong.

> One retry is not enough. This is what I got on the first try with a
> random UVC webcam:
> [...]

The Set TR Deq mismatch errors went away when I fixed your patch not
to give up on first try. Maybe I remembered wrong and they have always
been around.

Regards,
Michal

Re: [PATCH 0/2] xhci: Fix the NEC stop bug workaround

[Mathias Nyman]

On 31.10.2024 13.17, Michał Pecio wrote:
> Update:
> 
>> Your patch prints one dev_dbg() each time, mine spams many of them for
>> 100ms each time. I will remove this one retry limit from your patch to
>> see if starts spinning infinitely, but I strongly suspect it will.
> 
> Yes, that's exactly what happens.
> 
> This time I have killed the ifconfig loop, unplugged the NIC and
> started 'rmmod xhci_pci', which is still hanging 10 minutes later.
> 
> So business as usual when these things go wrong.
> 
>> One retry is not enough. This is what I got on the first try with a
>> random UVC webcam:
>> [...]

Ok, good to know, then using flag is not enough.

Using a retry timeout for failed stop endpoint commands also sounds good
to me.
It has a slight downside of a possible 100ms aggressive 'Stop Endpoint'
retry loop in cases where endpoint was stopped earlier for some other reason.

Not sure if that is a problem, if it is then we can add the flag and only
retry for 100ms if flag is set (only clear flag in handle_tx_event())

Another reason for the flag is the additional note in xhci 4.8.3 [1], we might
need to track the state better in software.

[1] xhci 4.8.3 Endpoint Context state

"There are several cases where the EP State field in the Output Endpoint Context
may not reflect the current state of an endpoint. The xHC should attempt to
keep EP State as current as possible, however it may defer these updates to
perform higher priority references to memory, e.g. Isoch data transfers, etc.
Software should maintain an internal variable that tracks the state of an
endpoint and not depend on EP State to represent the instantaneous state of
an endpoint.
For example, when a Command that affects EP State is issued, the value of EP
State may be updated anytime between when software rings the Command
Ring doorbell for a command and when the associated Command Completion
Event is placed on the Event Ring by the xHC. The update of EP State may also
be delayed relative to a Doorbell ring or error condition (e.g. TRB Error, STALL,
or USB Transaction Error) that causes an EP State change not generated by a
command.

Software should maintain an accurate value for EP State, by tracking it with an
internal variable that is driven by Events and Doorbell accesses associated with
an endpoint using the following method:

• When a command is issued to an endpoint that affects its state, software
should use the Command Completion Event to update its image of EP State
to the appropriate state.
• When a Transfer Event reports a TRB Error, software should update its image
of EP State to Error.
• When a Transfer Event reports a Stall Error or USB Transaction Error,
software should update its image of EP State to Halted.
• When software rings the Doorbell of an endpoint to transition it from the
Stopped to Running state, it should update its image of EP State to Running."

Thanks
-Mathias

Re: [PATCH 0/2] xhci: Fix the NEC stop bug workaround

[Michał Pecio]

On Thu, 31 Oct 2024 16:22:14 +0200, Mathias Nyman wrote:
> Ok, good to know, then using flag is not enough.
> 
> Using a retry timeout for failed stop endpoint commands also sounds
> good to me.
> It has a slight downside of a possible 100ms aggressive 'Stop
> Endpoint' retry loop in cases where endpoint was stopped earlier for
> some other reason.

Waiting 100ms is rare. It happens in cases when the original bad
workaround would retry infinitely, and this bug still hasn't been
reported by users for half a year. But I triggered it with patched
drivers or by stopping a device with flaky cable, so it can happen.

EP_RESTARTING flag could be used to avoid wasting 100ms in those
cases, but I found that I can predict its value in advance while
queuing the command, without adding noise to unrelated code. That
was the STOP_CMD_REDUNDANT patch, and now I'm trying to just avoid
queuing such redundant commands at all. And timeout if that fails.

I found this "redundant" prediction very accurate and pointless
commands are practically eliminated. Correctness is guaranteed by
ring_ep_doorbell() always starting an endpoint with pending URBs
if a few ep_state flags aren't set, and always being called when
any of those flags are cleared. Only two exceptions are known:

1. The "transferless tx events", which may trigger a hard reset
   without Set Deq. In this case ring_ep_doorbell() isn't called.
2. The bizarre ASM3142 ifconfig up/down issue, which crashes the
   whole bus and really looks like a hardware bug.

Generally, all cases of failing to restart an active endpoint are
very user-visible and problematic on their own right, so a bit of
extra delay wouldn't be the worst problem at this point, I hope.

> • When software rings the Doorbell of an endpoint to transition it
> from the Stopped to Running state, it should update its image of EP
> State to Running.

Making this assumption is exactly why we have problems, because the
start/stop race is tricky for hardware and some chips clearly don't
behave in such simple manner as suggested.

Regards,
Michal