On Tue, 4 Oct 2016, Bin Liu wrote: > local_irq_save() disables irq completely on non-SMP which causes the > hard interrupt handler unable to serve interrupts, which loses the > benefit that the giveback tasklet brings in usb_hcd_giveback_urb(). It doesn't lose all the benefit. Interrupts are still enabled before and after the local_irq_save region. > One example of the issues caused by this disabling local irq is that > urb->complete in uvc driver takes about 1ms, which holds the host > controller to not receive packets for so long that causes webcam drops > data. > > I don't see any callers of usb_hcd_giveback_urb() holding a spin_lock, > so it is safe to remove this local_irq_save/restore() around > urb->complete. How carefully did you look? ehci_urb_done() holds ehci->lock while calling usb_hcd_giveback_urb(). Anyway, that's not the problem this code was meant to solve. The problem is that the completion routine in a USB driver might expect to be called with interrupts disabled, and therefore might use spin_lock() instead of spin_lock_irqsave(). You can't remove this local_irq_save() until you have checked all the completion routines in all the USB drivers to make sure they don't make this mistake. Alan Stern > Signed-off-by: Bin Liu <b-liu@xxxxxx> > --- > v2: fix build warning > > drivers/usb/core/hcd.c | 14 -------------- > 1 file changed, 14 deletions(-) > > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c > index 479e223..c18d480 100644 > --- a/drivers/usb/core/hcd.c > +++ b/drivers/usb/core/hcd.c > @@ -1733,7 +1733,6 @@ static void __usb_hcd_giveback_urb(struct urb *urb) > struct usb_hcd *hcd = bus_to_hcd(urb->dev->bus); > struct usb_anchor *anchor = urb->anchor; > int status = urb->unlinked; > - unsigned long flags; > > urb->hcpriv = NULL; > if (unlikely((urb->transfer_flags & URB_SHORT_NOT_OK) && > @@ -1751,20 +1750,7 @@ static void __usb_hcd_giveback_urb(struct urb *urb) > /* pass ownership to the completion handler */ > urb->status = status; > > - /* > - * We disable local IRQs here avoid possible deadlock because > - * drivers may call spin_lock() to hold lock which might be > - * acquired in one hard interrupt handler. > - * > - * The local_irq_save()/local_irq_restore() around complete() > - * will be removed if current USB drivers have been cleaned up > - * and no one may trigger the above deadlock situation when > - * running complete() in tasklet. > - */ > - local_irq_save(flags); > urb->complete(urb); > - local_irq_restore(flags); > - > usb_anchor_resume_wakeups(anchor); > atomic_dec(&urb->use_count); > if (unlikely(atomic_read(&urb->reject))) -- 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
