On Tue, Oct 04, 2016 at 02:50:09PM -0400, Alan Stern wrote: > 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. Yeah, but the problem is the irq disabled region is sometimes huge (in ms range in uvc device driver). > > > 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(). Obviously not much :( I checked a few level up from ehci_urb_done(), but didn't reach echi->lock... > > 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. Ok, I will try to understand it more. Thanks for the comments. Regards, -Bin. > > 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
