Re: Problematic Set TR Deq error handling series in xhci-next

["Michał Pecio" <michal.pecio@gmail.com>]

On Sat, 10 May 2025 11:52:49 -0400, Alan Stern wrote:
> On Fri, May 09, 2025 at 11:41:38AM +0200, Michał Pecio wrote:
> > Hi,
> > 
> > I noticed that xhci/for-usb-next now includes a series which tries
> > to handle Set TR Deq errors. It strikes me as a solution looking for
> > a problem, and frankly creating new problems where none existed ;)
> > 
> > I am aware of three cases leading to errors being handled here, and
> > none of them is addressed. One is harmless and easy to fix properly,
> > but this series appears to turn it into a "never give back the URB"
> > disaster. Tests pending, I hope to find some time this weekend.
> > 
> > There should be no need to handle these errors, they are prevented
> > by not queuing the command in wrong states. When the command fails,
> > it means the driver screwed up tracking endpoint state and other
> > things are on fire too, so the actual bug should be fixed instead.
> > 
> > The case of disabled endpoints is clear: no URBs are allowed, the
> > core is broken. It would be more productive to sanity-check core:
> > detect and nuke lingering URBs in places like endpoint_disable(),
> > drop_endpoint(), reset_device(), free_dev(). If Set Deq is already
> > pending at the time, give back the URB and let the command fail.  
> 
> The core already does this for endpoint_disable.  If the others have 
> problems, could you provide a tracebacks so we can see the pathways 
> where the problem occurs?

I'm not aware of problems, this paragraph was hypothetical: if someone
thinks that problems exist or should be monitored for, there are better
places to do it than handle_cmd_set_deq().

Today I patched those HCD methods locally to check for pending URBs.
Nothing caught so far, but I will leave this code running long term.

This was discussed before and you said that device reset should be OK.
Users of hub_free_dev() also appear to be OK (they call things which
call disable_endpoint() on EP0 or all EPs).

Mathias fixed usb_set_interface() a few years ago. Not sure if similar
use of usb_hcd_alloc_bandwidth() in usb_set_configuration() is safe?

Michal