On Fri, 1 Jun 2018, Sebastian Andrzej Siewior wrote: > On 2018-05-22 15:14:17 [-0400], Alan Stern wrote: > > Sorry, I don't understand that sentence at all. And I don't see how it > > could be relevant to the point I was trying to make. > > > > Consider, for example, drivers/hid/usbhid/hid-core.c. In that file, > > hid_io_error() is called by hid_irq_in(), which is an URB completion > > handler. hid_io_error() acquires usbhid_lock. Therefore it would be > > necessary to audit the usbhid driver to see whether interrupts are > > enabled or disabled any place where usbhid_lock is acquired. And in > > fact, hid_ctrl() (another completion handler) calls > > spin_lock(&usbhid->lock) -- this could cause problems for you. And > > usbhid->lock is acquired in other places, ones that are not inside > > completion handlers. > > To pick up this example. hid_io_error() is called from process context > (usb_hid_open()) or the completion handler and disables interrupts while > acquiring the lock. hid_ctrl() acquires the lock without disabling the > interrupts. This is not a problem because hid_ctrl() can not be > preempted while it is holding the lock by anything that also wants the > lock. So hid-core could stay as-is and we would be fine. However > lockdep would complain if hid-core would be used on ehci > (softirq/tasklet completion) and ohci (IRQ-handler completion) because > then lockdep would think that hid_ctrl() invoked by ehci could be > preempted by the ohci driver. In any case, it's generally bad form for two routines to obtain the same lock in process context, if one of them uses spin_lock and the other uses spin_lock_irqsave. It indicates that the programmer didn't have a firm grasp on how the lock would be used. > > None of this has anything to do with IRQ usage or hrtimers. > hid-core sets up a timer_list timer hid_retry_timeout(). This timer runs > in softirq context which means it can't preempt the completion handler > (hid_ctrlhid_ctrl()) which does just spin_lock() (both run in BH). > However, if hid_retry_timeout() were a hrtimer timer then it would be > invoked in IRQ-context. In that case it could preempt hid_ctrlhid_ctrl() > and _irqsave() would be required in hid_ctrlhid_ctrl(). > > My counting reveals ~250 USB drivers. I think it is best to audit them > first and make sure that completion handler like hid_ctrl() do And any routines they call, obviously. > _irqsave(). Once that is done, the local_irq_save() in > __usb_hcd_giveback_urb() could go. Agreed. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
