| Message ID | 1371567833-9077-3-git-send-email-ming.lei@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello. On 18-06-2013 19:03, Ming Lei wrote: > We disable local IRQs here in case of running complete() by > tasklet to 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. > Cc: Oliver Neukum <oliver@neukum.org> > Cc: Alan Stern <stern@rowland.harvard.edu> > Signed-off-by: Ming Lei <ming.lei@canonical.com> > --- > drivers/usb/core/hcd.c | 23 ++++++++++++++++++++++- > 1 file changed, 22 insertions(+), 1 deletion(-) > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c > index a272968..09a8263 100644 > --- a/drivers/usb/core/hcd.c > +++ b/drivers/usb/core/hcd.c > @@ -1673,7 +1673,28 @@ static void __usb_hcd_giveback_urb(struct urb *urb) > > /* pass ownership to the completion handler */ > urb->status = status; > - urb->complete (urb); > + > + /* > + * We disable local IRQs here in case of running complete() by > + * tasklet to 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. > + */ > + if (hcd_giveback_urb_in_bh(hcd)) { > + unsigned long flags; > + > + local_irq_save(flags); > + urb->complete (urb); I guess you didn't run the patch thru scripts/checkpatch.pl, did you? It would complain about the space before (. WBR, Sergei
On Tue, 18 Jun 2013, Ming Lei wrote: > We disable local IRQs here in case of running complete() by > tasklet to 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. This should be part of the first patch, not added on separately. > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c > index a272968..09a8263 100644 > --- a/drivers/usb/core/hcd.c > +++ b/drivers/usb/core/hcd.c > @@ -1673,7 +1673,28 @@ static void __usb_hcd_giveback_urb(struct urb *urb) > > /* pass ownership to the completion handler */ > urb->status = status; > - urb->complete (urb); > + > + /* > + * We disable local IRQs here in case of running complete() by > + * tasklet to 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. > + */ > + if (hcd_giveback_urb_in_bh(hcd)) { > + unsigned long flags; > + > + local_irq_save(flags); > + urb->complete (urb); > + local_irq_restore(flags); > + } else { > + urb->complete (urb); > + } > + There's no reason to bother with the test. You might as well always do the local_irq_save. Alan Stern
On Wed, Jun 19, 2013 at 12:36 AM, Alan Stern <stern@rowland.harvard.edu> wrote: > On Tue, 18 Jun 2013, Ming Lei wrote: > >> We disable local IRQs here in case of running complete() by >> tasklet to 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. > > This should be part of the first patch, not added on separately. Yes, but I want to highlight the change since that will be removed after drivers have been cleaned up. > >> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c >> index a272968..09a8263 100644 >> --- a/drivers/usb/core/hcd.c >> +++ b/drivers/usb/core/hcd.c >> @@ -1673,7 +1673,28 @@ static void __usb_hcd_giveback_urb(struct urb *urb) >> >> /* pass ownership to the completion handler */ >> urb->status = status; >> - urb->complete (urb); >> + >> + /* >> + * We disable local IRQs here in case of running complete() by >> + * tasklet to 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. >> + */ >> + if (hcd_giveback_urb_in_bh(hcd)) { >> + unsigned long flags; >> + >> + local_irq_save(flags); >> + urb->complete (urb); >> + local_irq_restore(flags); >> + } else { >> + urb->complete (urb); >> + } >> + > > There's no reason to bother with the test. You might as well always do > the local_irq_save. Looks fine. Thanks, -- Ming Lei
On Wed, 19 Jun 2013, Ming Lei wrote: > On Wed, Jun 19, 2013 at 12:36 AM, Alan Stern <stern@rowland.harvard.edu> wrote: > > On Tue, 18 Jun 2013, Ming Lei wrote: > > > >> We disable local IRQs here in case of running complete() by > >> tasklet to 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. > > > > This should be part of the first patch, not added on separately. > > Yes, but I want to highlight the change since that will be removed > after drivers have been cleaned up. I don't think it's necessary to highlight anything, and it seems silly to add new code in one patch and then change it in the very next patch. Alan Stern
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c index a272968..09a8263 100644 --- a/drivers/usb/core/hcd.c +++ b/drivers/usb/core/hcd.c @@ -1673,7 +1673,28 @@ static void __usb_hcd_giveback_urb(struct urb *urb) /* pass ownership to the completion handler */ urb->status = status; - urb->complete (urb); + + /* + * We disable local IRQs here in case of running complete() by + * tasklet to 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. + */ + if (hcd_giveback_urb_in_bh(hcd)) { + unsigned long flags; + + local_irq_save(flags); + urb->complete (urb); + local_irq_restore(flags); + } else { + urb->complete (urb); + } + atomic_dec (&urb->use_count); if (unlikely(atomic_read(&urb->reject))) wake_up (&usb_kill_urb_queue);
We disable local IRQs here in case of running complete() by tasklet to 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. Cc: Oliver Neukum <oliver@neukum.org> Cc: Alan Stern <stern@rowland.harvard.edu> Signed-off-by: Ming Lei <ming.lei@canonical.com> --- drivers/usb/core/hcd.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)