Date: Fri, 5 Sep 2025 08:56:44 +0200
From: =?UTF-8?B?TWljaGHFgg==?= Pecio <michal.pecio@gmail.com>
To: Mathias Nyman <mathias.nyman@linux.intel.com>
Cc: Mathias Nyman <mathias.nyman@intel.com>, Niklas Neronin
 <niklas.neronin@linux.intel.com>, linux-usb@vger.kernel.org
Subject: Re: [PATCH RFC 1/2] usb: xhci: Drop the TD_CLEARED cancel status
Message-ID: <20250905085644.4f68e718.michal.pecio@gmail.com>
In-Reply-To: <feff2c31-344e-4147-8c9f-081b634dc82f@linux.intel.com>
References: <20250901084642.2b42c0e7.michal.pecio@gmail.com>
	<20250901084800.35252e61.michal.pecio@gmail.com>
	<feff2c31-344e-4147-8c9f-081b634dc82f@linux.intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

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@gmail.com>
> > ---  
> 
> 
> 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