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. > 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 _irqsave(). Once that is done, the local_irq_save() in __usb_hcd_giveback_urb() could go. Sebastian -- 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
