On Mon, 1 Sep 2025 12:47:10 +0300, Mathias Nyman wrote: > On 1.9.2025 9.48, Michal Pecio wrote: > > There seems to be no need to keep invalidated TDs around and require > > callers to follow up with xhci_giveback_invalidated_tds(), so make > > xhci_invalidate_cancelled_tds() give back invalidated TDs right away. > > > > TD_CLEARED cancel status is no longer useful and therefore removed. > > Code complexity and overhead of repeated list traversals are reduced. > > > > There is no more need to debug interactions between these functions, > > so a big source of xhci_dbg() noise (and potential bugs) goes away. > > From now, "Removing cancelled TD starting at ..." really means that > > the TD is being removed, unless one of the errors below is logged or > > dynamic debug indicates that Set TR Dequeue is queued or deferred. > > > > Also clean up some debug noise from xhci_handle_cmd_set_deq(), which > > practically duplicates xhci_invalidate_cancelled_tds() messages that > > will be printed just before this completion handler returns. > > > > Signed-off-by: Michal Pecio <michal.pecio@xxxxxxxxx> > > --- > > > Looks good to me. > > xhci_giveback_invalidated_tds() was anyways called right after > xhci_invalidate_cancelled_tds() under the same lock protection > so we can as well give back those non-cached cancelled TDs right away. Thank you, that's what it looked like to me. The only reason I could imagine for possibly wanting this state to exist is if somebody somehow demanded that cancelled TDs are given back in some particular order (submit order, cancellation order), but this driver has been delaying pending TD giveback due to Set Deq for years. Nobody seems to be complaining and I couldn't find any mention of giveback order in places like usb_unlink_urb(). I'm thikning about a v2 with small changes: 1. add a dynamic debug when a cached TD is selected for clearing - currently only the deffered cached TDs are logged instantly and we need to look for "Set TR Deq ptr ..." announcement later to know which TD was chosen. 2. add a patch to replace all 'break' with 'continue' in this function, because they break out of a switch nested in a loop, skip over an inactive 'else' clause and trigger a new iteration immediately. Regards, Michal
