On 06/09/2024 15.23, Michał Pecio wrote:
>> @@ -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 */
Thanks, for the suggestion. Indeed the comment should be updated.
>
>> + * 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.
Sure, I'll add a debug message when the skip flag is toggled.
>
>> + 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.
I can change it from a debug to a warning. Then the edge case should be more visible.
>
>> + 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.
Good point, thanks.
>
>> }
>>
>> /*
