Half of this loop is code which only executes once to deal with cases
where no TD matches the event and then immediately returns. This code
has no need to be in any kind of loop, so get it out.

Shuffle the remaining conditionals a little to improve readability.

No functional change.

Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
---
 drivers/usb/host/xhci-ring.c | 68 ++++++++++++++++++------------------
 1 file changed, 34 insertions(+), 34 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index af6c4c4cbe1c..9b06a911a16e 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2866,9 +2866,9 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 		/* Is this a TRB in the currently executing TD? */
 		ep_seg = trb_in_td(xhci, td, ep_trb_dma, false);
 
-		if (!ep_seg) {
+		if (ep->skip) {
 
-			if (ep->skip && usb_endpoint_xfer_isoc(&td->urb->ep->desc)) {
+			if (!ep_seg && usb_endpoint_xfer_isoc(&td->urb->ep->desc)) {
 				skip_isoc_td(xhci, td, ep, status);
 				if (!list_empty(&ep_ring->td_list))
 					continue;
@@ -2880,38 +2880,6 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 				goto check_endpoint_halted;
 			}
 
-			/*
-			 * Skip the Force Stopped Event. The 'ep_trb' of FSE is not in the current
-			 * TD pointed by 'ep_ring->dequeue' because that the hardware dequeue
-			 * pointer still at the previous TRB of the current TD. The previous TRB
-			 * maybe a Link TD or the last TRB of the previous TD. The command
-			 * completion handle will take care the rest.
-			 */
-			if (trb_comp_code == COMP_STOPPED ||
-			    trb_comp_code == COMP_STOPPED_LENGTH_INVALID) {
-				return 0;
-			}
-
-			/*
-			 * Some hosts give a spurious success event after a short
-			 * transfer. Ignore it.
-			 */
-			if ((xhci->quirks & XHCI_SPURIOUS_SUCCESS) &&
-			    ep_ring->last_td_was_short) {
-				ep_ring->last_td_was_short = false;
-				return 0;
-			}
-
-			/* HC is busted, give up! */
-			xhci_err(xhci,
-				 "ERROR Transfer event TRB DMA ptr not part of current TD ep_index %d comp_code %u\n",
-				 ep_index, trb_comp_code);
-			trb_in_td(xhci, td, ep_trb_dma, true);
-
-			return -ESHUTDOWN;
-		}
-
-		if (ep->skip) {
 			xhci_dbg(xhci,
 				 "Found td. Clear skip flag for slot %u ep %u.\n",
 				 slot_id, ep_index);
@@ -2926,6 +2894,38 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 	 */
 	} while (ep->skip);
 
+	if (!ep_seg) {
+		/*
+		 * Skip the Force Stopped Event. The 'ep_trb' of FSE is not in the current
+		 * TD pointed by 'ep_ring->dequeue' because that the hardware dequeue
+		 * pointer still at the previous TRB of the current TD. The previous TRB
+		 * maybe a Link TD or the last TRB of the previous TD. The command
+		 * completion handle will take care the rest.
+		 */
+		if (trb_comp_code == COMP_STOPPED ||
+		    trb_comp_code == COMP_STOPPED_LENGTH_INVALID) {
+			return 0;
+		}
+
+		/*
+		 * Some hosts give a spurious success event after a short
+		 * transfer. Ignore it.
+		 */
+		if ((xhci->quirks & XHCI_SPURIOUS_SUCCESS) &&
+		    ep_ring->last_td_was_short) {
+			ep_ring->last_td_was_short = false;
+			return 0;
+		}
+
+		/* HC is busted, give up! */
+		xhci_err(xhci,
+			 "ERROR Transfer event TRB DMA ptr not part of current TD ep_index %d comp_code %u\n",
+			 ep_index, trb_comp_code);
+		trb_in_td(xhci, td, ep_trb_dma, true);
+
+		return -ESHUTDOWN;
+	}
+
 	if (trb_comp_code == COMP_SHORT_PACKET)
 		ep_ring->last_td_was_short = true;
 	else
-- 
2.48.1

On 10/02/2025 9.39, Michal Pecio wrote:
> Half of this loop is code which only executes once to deal with cases
> where no TD matches the event and then immediately returns. This code
> has no need to be in any kind of loop, so get it out.
> 
> Shuffle the remaining conditionals a little to improve readability.
> 
> No functional change.
> 
> Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
> ---
>  
> -		if (!ep_seg) {
> +		if (ep->skip) {
>  
> -			if (ep->skip && usb_endpoint_xfer_isoc(&td->urb->ep->desc)) {
> +			if (!ep_seg && usb_endpoint_xfer_isoc(&td->urb->ep->desc)) {
>  				skip_isoc_td(xhci, td, ep, status);
>  				if (!list_empty(&ep_ring->td_list))
>  					continue;
> @@ -2880,38 +2880,6 @@ static int handle_tx_event(struct xhci_hcd *xhci,
>  				goto check_endpoint_halted;
>  			}
>  
> -			/*
> -			 * Skip the Force Stopped Event. The 'ep_trb' of FSE is not in the current
> -			 * TD pointed by 'ep_ring->dequeue' because that the hardware dequeue
> -			 * pointer still at the previous TRB of the current TD. The previous TRB
> -			 * maybe a Link TD or the last TRB of the previous TD. The command
> -			 * completion handle will take care the rest.
> -			 */
> -			if (trb_comp_code == COMP_STOPPED ||
> -			    trb_comp_code == COMP_STOPPED_LENGTH_INVALID) {
> -				return 0;
> -			}
> -
> -			/*
> -			 * Some hosts give a spurious success event after a short
> -			 * transfer. Ignore it.
> -			 */
> -			if ((xhci->quirks & XHCI_SPURIOUS_SUCCESS) &&
> -			    ep_ring->last_td_was_short) {
> -				ep_ring->last_td_was_short = false;
> -				return 0;
> -			}
> -
> -			/* HC is busted, give up! */
> -			xhci_err(xhci,
> -				 "ERROR Transfer event TRB DMA ptr not part of current TD ep_index %d comp_code %u\n",
> -				 ep_index, trb_comp_code);
> -			trb_in_td(xhci, td, ep_trb_dma, true);
> -
> -			return -ESHUTDOWN;
> -		}
> -
> -		if (ep->skip) {
>  			xhci_dbg(xhci,
>  				 "Found td. Clear skip flag for slot %u ep %u.\n",
>  				 slot_id, ep_index);

This debug message is now misleading, the TD way or may not be found on non-isochronous.

Before:
	if (ep_seg && ep->skip)
		xhci_dbg(xhci, "Found td. ...
After:
	if (ep->skip && (ep_seg || !isoc))
		xhci_dbg(xhci, "Found td. ...


With Best Regards,
Niklas

On Sat, 22 Feb 2025 14:37:58 +0200, Neronin, Niklas wrote:
> This debug message is now misleading, the TD way or may not be found
> on non-isochronous.
> 
> Before:
> 	if (ep_seg && ep->skip)
> 		xhci_dbg(xhci, "Found td. ...
> After:
> 	if (ep->skip && (ep_seg || !isoc))
> 		xhci_dbg(xhci, "Found td. ...

Hmm, you're right, the whole block will now execute in this
pathological edge case and we will clear the flag too.

It can be fixed quite easily, but I think I may actually drop this
patch altogether. It will make the next patch slightly more verbose
(that's why I included this one), but it will also make it possible
to backport any of those patches to 6.12-lts if a need arises.

I also realized that one more skipping pathology is a recent (6.11)
regression and perhaps it too could be fixed without major rework,
basically by going back to something similar to pre-6.11 behavior.

I should have v3 ready in a day or a few.

Regards,
Michal