diff mbox

[RFC,v1,2/6] USB: disable IRQs deliberately when calling complete()

Message ID 1371567833-9077-3-git-send-email-ming.lei@canonical.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ming Lei June 18, 2013, 3:03 p.m. UTC
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(-)

Comments

Sergei Shtylyov June 18, 2013, 3:13 p.m. UTC | #1
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
Alan Stern June 18, 2013, 4:36 p.m. UTC | #2
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
Ming Lei June 19, 2013, 3:02 a.m. UTC | #3
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
Alan Stern June 19, 2013, 3:30 p.m. UTC | #4
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 mbox

Patch

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);