[10/12] usb: xhci: adjust empty TD list handling in handle_tx_event()

From: Niklas Neronin <niklas.neronin@linux.intel.com>

Introduce an initial check for an empty list prior to entering the while
loop. Which enables, the implementation of distinct warnings to
differentiate between scenarios where the list is initially empty and
when it has been emptied during processing skipped isoc TDs.

These adjustments not only simplifies the large while loop, but also
facilitates future enhancements to the handle_tx_event() function.

Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-ring.c | 51 +++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 27 deletions(-)

On September 5, 2024 4:32:58 PM GMT+02:00, Mathias Nyman <mathias.nyman@linux.intel.com> wrote:
>From: Niklas Neronin <niklas.neronin@linux.intel.com>
>
>Introduce an initial check for an empty list prior to entering the while
>loop. Which enables, the implementation of distinct warnings to
>differentiate between scenarios where the list is initially empty and
>when it has been emptied during processing skipped isoc TDs.
>
>These adjustments not only simplifies the large while loop, but also
>facilitates future enhancements to the handle_tx_event() function.
>
>Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
>Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>---
> drivers/usb/host/xhci-ring.c | 51 +++++++++++++++++-------------------
> 1 file changed, 24 insertions(+), 27 deletions(-)
>
>diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>index d37eeee74960..a4383735b16c 100644
>--- a/drivers/usb/host/xhci-ring.c
>+++ b/drivers/usb/host/xhci-ring.c
>@@ -2761,35 +2761,25 @@ static int handle_tx_event(struct xhci_hcd *xhci,
> 		return 0;
> 	}
> 
>-	do {
>-		/* This TRB should be in the TD at the head of this ring's
>-		 * TD list.
>+	if (list_empty(&ep_ring->td_list)) {
>+		/*
>+		 * Don't print wanings if ring is empty due to a stopped endpoint generating an

"wanings" should be "warnings", no?


>+		 * extra completion event if the device was suspended. Or, a event for the last TRB
>+		 * of a short TD we already got a short event for. The short TD is already removed
>+		 * from the TD list.
> 		 */
>-		if (list_empty(&ep_ring->td_list)) {
>-			/*
>-			 * Don't print wanings if it's due to a stopped endpoint
>-			 * generating an extra completion event if the device
>-			 * was suspended. Or, a event for the last TRB of a
>-			 * short TD we already got a short event for.
>-			 * The short TD is already removed from the TD list.
>-			 */
>-
>-			if (!(trb_comp_code == COMP_STOPPED ||
>-			      trb_comp_code == COMP_STOPPED_LENGTH_INVALID ||
>-			      ep_ring->last_td_was_short)) {
>-				xhci_warn(xhci, "WARN Event TRB for slot %u ep %d with no TDs queued?\n",
>-					  slot_id, ep_index);
>-			}
>-			if (ep->skip) {
>-				ep->skip = false;
>-				xhci_dbg(xhci, "td_list is empty while skip flag set. Clear skip flag for slot %u ep %u.\n",
>-					 slot_id, ep_index);
>-			}
>-
>-			td = NULL;
>-			goto check_endpoint_halted;
>+		if (trb_comp_code != COMP_STOPPED &&
>+		    trb_comp_code != COMP_STOPPED_LENGTH_INVALID &&
>+		    !ep_ring->last_td_was_short) {
>+			xhci_warn(xhci, "Event TRB for slot %u ep %u with no TDs queued\n",
>+				  slot_id, ep_index);
> 		}
> 
>+		ep->skip = false;
>+		goto check_endpoint_halted;
>+	}
>+
>+	do {
> 		td = list_first_entry(&ep_ring->td_list, struct xhci_td,
> 				      td_list);
> 
>@@ -2800,7 +2790,14 @@ static int handle_tx_event(struct xhci_hcd *xhci,
> 
> 			if (ep->skip && usb_endpoint_xfer_isoc(&td->urb->ep->desc)) {
> 				skip_isoc_td(xhci, td, ep, status);
>-				continue;
>+				if (!list_empty(&ep_ring->td_list))
>+					continue;
>+
>+				xhci_dbg(xhci, "All TDs skipped for slot %u ep %u. Clear skip flag.\n",
>+					 slot_id, ep_index);
>+				ep->skip = false;
>+				td = NULL;
>+				goto check_endpoint_halted;
> 			}
> 
> 			/*

Kind regards,
FPS

On 06/09/2024 7.44, fps wrote:
> On September 5, 2024 4:32:58 PM GMT+02:00, Mathias Nyman <mathias.nyman@linux.intel.com> wrote:
>> From: Niklas Neronin <niklas.neronin@linux.intel.com>
>>
>> Introduce an initial check for an empty list prior to entering the while
>> loop. Which enables, the implementation of distinct warnings to
>> differentiate between scenarios where the list is initially empty and
>> when it has been emptied during processing skipped isoc TDs.
>>
>> These adjustments not only simplifies the large while loop, but also
>> facilitates future enhancements to the handle_tx_event() function.
>>
>> Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>> ---
>> drivers/usb/host/xhci-ring.c | 51 +++++++++++++++++-------------------
>> 1 file changed, 24 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>> index d37eeee74960..a4383735b16c 100644
>> --- a/drivers/usb/host/xhci-ring.c
>> +++ b/drivers/usb/host/xhci-ring.c
>> @@ -2761,35 +2761,25 @@ static int handle_tx_event(struct xhci_hcd *xhci,
>> 		return 0;
>> 	}
>>
>> -	do {
>> -		/* This TRB should be in the TD at the head of this ring's
>> -		 * TD list.
>> +	if (list_empty(&ep_ring->td_list)) {
>> +		/*
>> +		 * Don't print wanings if ring is empty due to a stopped endpoint generating an
> 
> "wanings" should be "warnings", no?

Thanks, yes it should be the latter.
It will fix it in a handle_tx_event() cleanup patch.

Thanks,
Niklas
> 
> 
>> +		 * extra completion event if the device was suspended. Or, a event for the last TRB
>> +		 * of a short TD we already got a short event for. The short TD is already removed
>> +		 * from the TD list.
>> 		 */
>> -		if (list_empty(&ep_ring->td_list)) {
>> -			/*
>> -			 * Don't print wanings if it's due to a stopped endpoint
>> -			 * generating an extra completion event if the device
>> -			 * was suspended. Or, a event for the last TRB of a
>> -			 * short TD we already got a short event for.
>> -			 * The short TD is already removed from the TD list.
>> -			 */
>> -
>> -			if (!(trb_comp_code == COMP_STOPPED ||
>> -			      trb_comp_code == COMP_STOPPED_LENGTH_INVALID ||
>> -			      ep_ring->last_td_was_short)) {
>> -				xhci_warn(xhci, "WARN Event TRB for slot %u ep %d with no TDs queued?\n",
>> -					  slot_id, ep_index);
>> -			}
>> -			if (ep->skip) {
>> -				ep->skip = false;
>> -				xhci_dbg(xhci, "td_list is empty while skip flag set. Clear skip flag for slot %u ep %u.\n",
>> -					 slot_id, ep_index);
>> -			}
>> -
>> -			td = NULL;
>> -			goto check_endpoint_halted;
>> +		if (trb_comp_code != COMP_STOPPED &&
>> +		    trb_comp_code != COMP_STOPPED_LENGTH_INVALID &&
>> +		    !ep_ring->last_td_was_short) {
>> +			xhci_warn(xhci, "Event TRB for slot %u ep %u with no TDs queued\n",
>> +				  slot_id, ep_index);
>> 		}
>>
>> +		ep->skip = false;
>> +		goto check_endpoint_halted;
>> +	}
>> +
>> +	do {
>> 		td = list_first_entry(&ep_ring->td_list, struct xhci_td,
>> 				      td_list);
>>
>> @@ -2800,7 +2790,14 @@ static int handle_tx_event(struct xhci_hcd *xhci,
>>
>> 			if (ep->skip && usb_endpoint_xfer_isoc(&td->urb->ep->desc)) {
>> 				skip_isoc_td(xhci, td, ep, status);
>> -				continue;
>> +				if (!list_empty(&ep_ring->td_list))
>> +					continue;
>> +
>> +				xhci_dbg(xhci, "All TDs skipped for slot %u ep %u. Clear skip flag.\n",
>> +					 slot_id, ep_index);
>> +				ep->skip = false;
>> +				td = NULL;
>> +				goto check_endpoint_halted;
>> 			}
>>
>> 			/*
> 
> Kind regards,
> FPS

>@@ -2761,35 +2761,25 @@ static int handle_tx_event(struct xhci_hcd *xhci,
> 		return 0;
> 	}
> 
>-	do {
>-		/* This TRB should be in the TD at the head of this ring's
>-		 * TD list.
>+	if (list_empty(&ep_ring->td_list)) {
>+		/*
>+		 * Don't print wanings if ring is empty due to a stopped endpoint generating an
>+		 * extra completion event if the device was suspended. Or, a event for the last TRB
Is changing this code perhaps an opportunity to clarify its comments?

This is just confusing. A stopped endpoint doesn't generate any "extra"
events since it can't be stopped again. Commit message of a83d6755814e4
suggests that this was about stopping running but idle EPs (as is the
case of EP0 before suspend). So briefly and to the point:

/* Ignore Force Stopped Event on an empty ring,
   or one containing only NOPs and Links */

>+		 * of a short TD we already got a short event for. The short TD is already removed
>+		 * from the TD list.
> 		 */
>-		if (list_empty(&ep_ring->td_list)) {
>-			/*
>-			 * Don't print wanings if it's due to a stopped endpoint
>-			 * generating an extra completion event if the device
>-			 * was suspended. Or, a event for the last TRB of a
>-			 * short TD we already got a short event for.
>-			 * The short TD is already removed from the TD list.
>-			 */
>-
>-			if (!(trb_comp_code == COMP_STOPPED ||
>-			      trb_comp_code == COMP_STOPPED_LENGTH_INVALID ||
>-			      ep_ring->last_td_was_short)) {
>-				xhci_warn(xhci, "WARN Event TRB for slot %u ep %d with no TDs queued?\n",
>-					  slot_id, ep_index);
>-			}
>-			if (ep->skip) {
>-				ep->skip = false;
>-				xhci_dbg(xhci, "td_list is empty while skip flag set. Clear skip flag for slot %u ep %u.\n",
>-					 slot_id, ep_index);
>-			}
>-
>-			td = NULL;
>-			goto check_endpoint_halted;
>+		if (trb_comp_code != COMP_STOPPED &&
>+		    trb_comp_code != COMP_STOPPED_LENGTH_INVALID &&
>+		    !ep_ring->last_td_was_short) {
>+			xhci_warn(xhci, "Event TRB for slot %u ep %u with no TDs queued\n",
>+				  slot_id, ep_index);
I would add trb_comp_code here if touching this line.

> 		}
> 
>+		ep->skip = false;
I don't like that the xhci_dbg() has been removed. If skip debugging is
to be reliable, it should report all state transitions. And this is an
unusual one, so maybe very interesting. Skip debugging is valuable, as
the logic is tricky and has known problem cases. More below.

>+		goto check_endpoint_halted;
>+	}
>+
>+	do {
> 		td = list_first_entry(&ep_ring->td_list, struct xhci_td,
> 				      td_list);
> 
>@@ -2800,7 +2790,14 @@ static int handle_tx_event(struct xhci_hcd *xhci,
> 
> 			if (ep->skip && usb_endpoint_xfer_isoc(&td->urb->ep->desc)) {
> 				skip_isoc_td(xhci, td, ep, status);
>-				continue;
>+				if (!list_empty(&ep_ring->td_list))
>+					continue;
>+
>+				xhci_dbg(xhci, "All TDs skipped for slot %u ep %u. Clear skip flag.\n",
>+					 slot_id, ep_index);
This used to get the empty list warning, but now it's mere xhci_dbg().
Throwing out all queued TDs is not the common case and it may easily
be a bug. Indeed, I can readily name two cases when it is a bug today:

1. Force Stopped Event on a NOP or Link following the missed TD. Then
trb_in_td() doesn't match subsequent TD and the rest is trashed.

Actually, this is a v6.11 regression since d56b0b2ab1429. Although past
behavior was bad and broken too, it was broken differently.

2. Ring Underrun/Overrun if new TDs were queued before we handled it.
If ep_trb_dma is NULL, nothing ever matches and everything goes out.

Arguably, these are rare events and I haven't observed them yet.
And one more problem that I don't think currently exists, but:

3. If you ever find yourself doing it on an ordinary event (Success,
Transaction Error, Babble, etc.) then, seriously, WTF?

Bottom line, empty list is a very suspicious thing to see here. I can
only think of two legitimate cases:

1. Ring X-run, only if nothing new was queued since it occurred.
2. FSE outside transfer TDs, if no transfer TDs existed after it.

>+				ep->skip = false;
>+				td = NULL;
>+				goto check_endpoint_halted;
Isoch EPs can't stall and aren't supposed to halt on errors, 4.10.2.

> 			}
> 
> 			/*