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
