| Message ID | 1371567833-9077-4-git-send-email-ming.lei@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 18 Jun 2013, Ming Lei wrote: > There is no good reason to run complete() in hard interrupt > disabled context. > > After running complete() in tasklet, we will enable local IRQs > when calling complete() since we can do it now. > > Even though we still disable IRQs now when calling complete() > in tasklet, the URB documentation is updated to claim complete() > may be run in tasklet context and local IRQs may be enabled, so > that USB drivers can know the change and avoid one deadlock caused > by: assume IRQs disabled in complete() and call spin_lock() to > hold lock which might be acquired in interrupt context. > > Current spin_lock() usages in drivers' complete() will be cleaned > up at the same time, and when the cleanup is finished, local IRQs > will be enabled when calling complete() in tasklet. > > Also fix description about type of usb_complete_t. > @@ -210,12 +209,19 @@ have transferred successfully before the completion was called. > > > NOTE: ***** WARNING ***** > -NEVER SLEEP IN A COMPLETION HANDLER. These are normally called > -during hardware interrupt processing. If you can, defer substantial > -work to a tasklet (bottom half) to keep system latencies low. You'll > -probably need to use spinlocks to protect data structures you manipulate > -in completion handlers. > - > +NEVER SLEEP IN A COMPLETION HANDLER. These are called during hardware > +interrupt processing if HCD_BH isn't set in hcd->driver->flags, otherwise > +called in tasklet context. If you can, defer substantial work to a tasklet > +(bottom half) to keep system latencies low. You'll probably need to use > +spinlocks to protect data structures you manipulate in completion handlers. You shouldn't go into so much detail here, partly because it doesn't matter for driver authors and partly because it is subject to change. Just say that completion handlers are usually called in an atomic context, so they must not sleep. Also, the advice about deferring work to a tasklet seems out of place now, since many completion handlers will already be running in a tasklet. > +Driver can't assume that local IRQs are disabled any more when running > +complete(), for example, if drivers want to hold a lock which might be > +acquired in hard interrupt handler, they have to use spin_lock_irqsave() > +instead of spin_lock() to hold the lock. Don't say so much. Just mention that in the current kernel (3.10), completion handlers always run with local interrupts disabled, but in the future this is likely to change. Therefore drivers should not make any assumptions. > +In the future, all HCD might set HCD_BH to run complete() in tasklet so > +that system latencies can be kept low. This does not need to be in the documentation. Alan Stern
On Wed, Jun 19, 2013 at 12:42 AM, Alan Stern <stern@rowland.harvard.edu> wrote: > On Tue, 18 Jun 2013, Ming Lei wrote: > >> There is no good reason to run complete() in hard interrupt >> disabled context. >> >> After running complete() in tasklet, we will enable local IRQs >> when calling complete() since we can do it now. >> >> Even though we still disable IRQs now when calling complete() >> in tasklet, the URB documentation is updated to claim complete() >> may be run in tasklet context and local IRQs may be enabled, so >> that USB drivers can know the change and avoid one deadlock caused >> by: assume IRQs disabled in complete() and call spin_lock() to >> hold lock which might be acquired in interrupt context. >> >> Current spin_lock() usages in drivers' complete() will be cleaned >> up at the same time, and when the cleanup is finished, local IRQs >> will be enabled when calling complete() in tasklet. >> >> Also fix description about type of usb_complete_t. > >> @@ -210,12 +209,19 @@ have transferred successfully before the completion was called. >> >> >> NOTE: ***** WARNING ***** >> -NEVER SLEEP IN A COMPLETION HANDLER. These are normally called >> -during hardware interrupt processing. If you can, defer substantial >> -work to a tasklet (bottom half) to keep system latencies low. You'll >> -probably need to use spinlocks to protect data structures you manipulate >> -in completion handlers. >> - >> +NEVER SLEEP IN A COMPLETION HANDLER. These are called during hardware >> +interrupt processing if HCD_BH isn't set in hcd->driver->flags, otherwise >> +called in tasklet context. If you can, defer substantial work to a tasklet >> +(bottom half) to keep system latencies low. You'll probably need to use >> +spinlocks to protect data structures you manipulate in completion handlers. > > You shouldn't go into so much detail here, partly because it doesn't > matter for driver authors and partly because it is subject to change. > Just say that completion handlers are usually called in an atomic > context, so they must not sleep. Right. > > Also, the advice about deferring work to a tasklet seems out of place > now, since many completion handlers will already be running in a > tasklet. Good catch. > >> +Driver can't assume that local IRQs are disabled any more when running >> +complete(), for example, if drivers want to hold a lock which might be >> +acquired in hard interrupt handler, they have to use spin_lock_irqsave() >> +instead of spin_lock() to hold the lock. > > Don't say so much. Just mention that in the current kernel (3.10), > completion handlers always run with local interrupts disabled, but in > the future this is likely to change. Therefore drivers should not make > any assumptions. OK. > >> +In the future, all HCD might set HCD_BH to run complete() in tasklet so >> +that system latencies can be kept low. > > This does not need to be in the documentation. Right, since drivers don't care HCD. Thanks, -- Ming Lei
diff --git a/Documentation/usb/URB.txt b/Documentation/usb/URB.txt index 00d2c64..454db87 100644 --- a/Documentation/usb/URB.txt +++ b/Documentation/usb/URB.txt @@ -195,13 +195,12 @@ by the completion handler. The handler is of the following type: - typedef void (*usb_complete_t)(struct urb *, struct pt_regs *) + typedef void (*usb_complete_t)(struct urb *) -I.e., it gets the URB that caused the completion call, plus the -register values at the time of the corresponding interrupt (if any). -In the completion handler, you should have a look at urb->status to -detect any USB errors. Since the context parameter is included in the URB, -you can pass information to the completion handler. +I.e., it gets the URB that caused the completion call. In the completion +handler, you should have a look at urb->status to detect any USB errors. +Since the context parameter is included in the URB, you can pass +information to the completion handler. Note that even when an error (or unlink) is reported, data may have been transferred. That's because USB transfers are packetized; it might take @@ -210,12 +209,19 @@ have transferred successfully before the completion was called. NOTE: ***** WARNING ***** -NEVER SLEEP IN A COMPLETION HANDLER. These are normally called -during hardware interrupt processing. If you can, defer substantial -work to a tasklet (bottom half) to keep system latencies low. You'll -probably need to use spinlocks to protect data structures you manipulate -in completion handlers. - +NEVER SLEEP IN A COMPLETION HANDLER. These are called during hardware +interrupt processing if HCD_BH isn't set in hcd->driver->flags, otherwise +called in tasklet context. If you can, defer substantial work to a tasklet +(bottom half) to keep system latencies low. You'll probably need to use +spinlocks to protect data structures you manipulate in completion handlers. + +Driver can't assume that local IRQs are disabled any more when running +complete(), for example, if drivers want to hold a lock which might be +acquired in hard interrupt handler, they have to use spin_lock_irqsave() +instead of spin_lock() to hold the lock. + +In the future, all HCD might set HCD_BH to run complete() in tasklet so +that system latencies can be kept low. 1.8. How to do isochronous (ISO) transfers?
There is no good reason to run complete() in hard interrupt disabled context. After running complete() in tasklet, we will enable local IRQs when calling complete() since we can do it now. Even though we still disable IRQs now when calling complete() in tasklet, the URB documentation is updated to claim complete() may be run in tasklet context and local IRQs may be enabled, so that USB drivers can know the change and avoid one deadlock caused by: assume IRQs disabled in complete() and call spin_lock() to hold lock which might be acquired in interrupt context. Current spin_lock() usages in drivers' complete() will be cleaned up at the same time, and when the cleanup is finished, local IRQs will be enabled when calling complete() in tasklet. Also fix description about type of usb_complete_t. Cc: Oliver Neukum <oliver@neukum.org> Cc: Alan Stern <stern@rowland.harvard.edu> Signed-off-by: Ming Lei <ming.lei@canonical.com> --- Documentation/usb/URB.txt | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-)