[RFC PATCH v1 3/6] USB: URB documentation: claim complete() may be run with IRQs enabled

[Ming Lei]

On Wed, Jun 19, 2013 at 12:42 AM, Alan Stern <stern at 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