On Tue, Aug 31, 2021 at 01:10:32PM +0200, Johan Hovold wrote: > On Tue, Aug 31, 2021 at 11:13:59AM +0200, Johan Hovold wrote: > > On Mon, Aug 30, 2021 at 10:46:13AM -0400, Alan Stern wrote: > > > > Consider that a control message in a driver is likely to use the > > > default USB_CTRL_[GS]ET_TIMEOUT value of 5 seconds. Does it make sense > > > to allow uninterruptible wait states to last as long as that? > > > > Perhaps sometimes? I don't have a use case at hand, but I haven't > > reviewed all drivers either. > > > > The comment above usb_start_wait_urb() (which also needs to be updated, > > by the way) even suggests that drivers should "implement their own > > interruptible routines" so perhaps this has just gone unnoticed for 20 > > odd years. And the question then becomes, why didn't we use > > interruptible sleep from the start? > > > > And trying to answer that I find that that's precisely what we did, but > > for some reason it was changed to uninterruptible sleep in v2.4.11 > > without a motivation (that I could easily find spelled out). > > Here it is: > > https://lore.kernel.org/lkml/20010818013101.A7058@xxxxxxxxxxxxxxxxxxxxxxxx/ > > It's rationale does not seem valid anymore (i.e. the NULL deref), but > the example is still instructive. > > If you close for example a v4l application you still expect the device > to be shut down orderly but with interruptible sleep all control > requests during shutdown will be aborted immediately. > > Similar for USB serial devices which would for example fail to deassert > DTR/RTS, etc. (I just verified with the patch applied.) On Tue, Aug 31, 2021 at 01:02:11PM +0200, Oliver Neukum wrote: > Upon further considerations forcing user space to handle signals also > breaks the API, albeit in a more subtle manner. I'd suggest that we use > wait_event_killable_timeout(). And do it the way Alan initially disliked, > that is code a version for use by usbfs. > > Thus we'd avoid the issue of having an unkillable process, but we do > not impose a need to handle signals. Okay, I'll play it safe and rewrite the patch, adding special-purpose routines just for usbfs. Will wait_event_killable_timeout() prevent complaints about tasks being blocked for too long (the reason syzbot reported this in the first place)? Alan Stern
