On Mon, Jul 27, 2020 at 03:58:05PM +0200, Oliver Neukum wrote: > Am Montag, den 27.07.2020, 14:27 +0300 schrieb Eli Billauer: > > Hello, Oliver. > > > > On 27/07/20 13:14, Oliver Neukum wrote: > > > That however is really a kludge we cannot have in usbcore. > > > I am afraid as is the patch should_not_ be applied. > > > > > > > Could you please explain further why the suggested patch is unsuitable? > > Hi, > > certainly. > > 1. timeouts are generally a bad idea, especially if the timeout does > not come out of a spec. > > 2. That involves quoting you: > > Alternatively, if the driver submits URBs to the same anchor while > usb_kill_anchored_urbs() is called, this timeout might be reached. This That would be a bug in the driver, though. In such a situation, a WARN is worth having. > could happen, for example, if the completer function that ran in the > racy situation resubmits the URB. If that situation isn't cleared within > 1000ms, it means that there's a URB in the system that the driver isn't > aware of. Maybe that situation is worth more than a WARN. > > That is an entirely valid use case. And a bulk URB may take a potentially > unbounded time to complete. It is _not_ a valid use case. Since usb_kill_anchored_urbs() doesn't' specify whether it will kill URBs that are added to the anchor after it is called (and before it returns), a driver that anchors URBs at such a time is buggy. Maybe this should be mentioned in the kerneldoc for the routine: Drivers must not add URBs to the anchor while the routine is running. > My failure in this case is simply overengineering. > If this line: > > usb_unanchor_urb(urb); > > In __usb_hcd_giveback_urb(struct urb *urb) weren't there, the issue > would not exist. I misdesigned the API in automatically unanchoring > a completing URB. > Simply removing it now is no longer possible, so we need to come up with > a more complex solution. Given that this timeout-based API is already present and being used in a separate context, I don't see anything wrong with using it here as well. Alan Stern
