[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-usb
Subject:    Re: [PATCH] USB: core: Make usb_start_wait_urb() interruptible
From:       Alan Stern <stern () rowland ! harvard ! edu>
Date:       2021-08-31 17:03:38
Message-ID: 20210831170338.GA371511 () rowland ! harvard ! edu
[Download RAW message or body]

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@devserv.devel.redhat.com/
> 
> 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
[prev in list] [next in list] [prev in thread] [next in thread]