subject:"\[PATCH v2\] usb\: gadget\: fix spinlock dead lock in gadgetfs"

Re: [PATCH v2] usb: gadget: fix spinlock dead lock in gadgetfs

2016-05-26 Thread Bin Liu

Hi,

On Thu, May 26, 2016 at 07:26:44PM +0300, Felipe Balbi wrote:
> 
> >> Bin Liu  writes:
> >> > On Wed, May 25, 2016 at 02:18:34PM -0500, Bin Liu wrote:
> >> >> [   40.467381] =
> >> >> [   40.473013] [ INFO: possible recursive locking detected ]
> >> >> [   40.478651] 4.6.0-08691-g7f3db9a #37 Not tainted
> >> >> [   40.483466] -
> >> >> [   40.489098] usb/733 is trying to acquire lock:
> >> >> [   40.493734]  (&(>lock)->rlock){-.}, at: [] 
> >> >> ep0_complete+0x18/0xdc [gadgetfs]
> >> >> [   40.502882]
> >> >> [   40.502882] but task is already holding lock:
> >> >> [   40.508967]  (&(>lock)->rlock){-.}, at: [] 
> >> >> ep0_read+0x20/0x5e0 [gadgetfs]
> >> >> [   40.517811]
> >> >> [   40.517811] other info that might help us debug this:
> >> >> [   40.524623]  Possible unsafe locking scenario:
> >> >> [   40.524623]
> >> >> [   40.530798]CPU0
> >> >> [   40.533346]
> >> >> [   40.535894]   lock(&(>lock)->rlock);
> >> >> [   40.540088]   lock(&(>lock)->rlock);
> >> >> [   40.544284]
> >> >> [   40.544284]  *** DEADLOCK ***
> >> >> [   40.544284]
> >> >> [   40.550461]  May be due to missing lock nesting notation
> >> >> [   40.550461]
> >> >> [   40.557544] 2 locks held by usb/733:
> >> >> [   40.561271]  #0:  (>f_pos_lock){+.+.+.}, at: [] 
> >> >> __fdget_pos+0x40/0x48
> >> >> [   40.569219]  #1:  (&(>lock)->rlock){-.}, at: [] 
> >> >> ep0_read+0x20/0x5e0 [gadgetfs]
> >> >> [   40.578523]
> >> >> [   40.578523] stack backtrace:
> >> >> [   40.583075] CPU: 0 PID: 733 Comm: usb Not tainted 
> >> >> 4.6.0-08691-g7f3db9a #37
> >> >> [   40.590246] Hardware name: Generic AM33XX (Flattened Device Tree)
> >> >> [   40.596625] [] (unwind_backtrace) from [] 
> >> >> (show_stack+0x10/0x14)
> >> >> [   40.604718] [] (show_stack) from [] 
> >> >> (dump_stack+0xb0/0xe4)
> >> >> [   40.612267] [] (dump_stack) from [] 
> >> >> (__lock_acquire+0xf68/0x1994)
> >> >> [   40.620440] [] (__lock_acquire) from [] 
> >> >> (lock_acquire+0xd8/0x238)
> >> >> [   40.628621] [] (lock_acquire) from [] 
> >> >> (_raw_spin_lock_irqsave+0x38/0x4c)
> >> >> [   40.637440] [] (_raw_spin_lock_irqsave) from [] 
> >> >> (ep0_complete+0x18/0xdc [gadgetfs])
> >> >> [   40.647339] [] (ep0_complete [gadgetfs]) from [] 
> >> >> (musb_g_giveback+0x118/0x1b0 [musb_hdrc])
> >> >> [   40.657842] [] (musb_g_giveback [musb_hdrc]) from 
> >> >> [] (musb_g_ep0_queue+0x16c/0x188 [musb_hdrc])
> >> >> [   40.668772] [] (musb_g_ep0_queue [musb_hdrc]) from 
> >> >> [] (ep0_read+0x544/0x5e0 [gadgetfs])
> >> >> [   40.678963] [] (ep0_read [gadgetfs]) from [] 
> >> >> (__vfs_read+0x20/0x110)
> >> >> [   40.687414] [] (__vfs_read) from [] 
> >> >> (vfs_read+0x88/0x114)
> >> >> [   40.694864] [] (vfs_read) from [] 
> >> >> (SyS_read+0x44/0x9c)
> >> >> [   40.702051] [] (SyS_read) from [] 
> >> >> (ret_fast_syscall+0x0/0x1c)
> >> >> 
> >> >> Cc:  # v3.16+
> >> >> Signed-off-by: Bin Liu 
> >> >> ---
> >> >> v2:
> >> >>   - lock protects setup_req(), only unlock for usb_ep_queue()
> >> >>   - use GFP_KERNEL in usb_ep_queue()
> >> >>   - fix the same in two places in gadgetfs_setup() too
> >> >
> >> > Never mind. Sent the patch too soon. It gives the following problem.
> >> >
> >> > [  179.810367] WARNING: CPU: 0 PID: 0 at kernel/locking/lockdep.c:2724
> >> > _raw_spin_unlock_irq+0x24/0x2c
> >> > [  179.819719] DEBUG_LOCKS_WARN_ON(current->hardirq_context)
> >> 
> >> thanks for letting us know. I'm dropping this from my queue. Please
> >> resend final patch once you have it ;-)
> >
> > It turned out to be a very easy fix ;)
> >
> > I have v3 ready, but checkpatch.pl complains the followings. I don't
> > think this patch should fix them, since the whole driver is coded like
> > that. Any comments?
> 
> Let's fix it with a follow-up patch. Can you do that one too, btw? I can
> do it, if you don't wanna deal with that; but I'm also open to receiving
> free patches heh

I'd like to do it, but I have a pile of other suff to do. (I still
remember I owe you a g-zero test with dwc3...)

Regards,
-Bin.


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] usb: gadget: fix spinlock dead lock in gadgetfs

2016-05-26 Thread Felipe Balbi


>> Bin Liu  writes:
>> > On Wed, May 25, 2016 at 02:18:34PM -0500, Bin Liu wrote:
>> >> [   40.467381] =
>> >> [   40.473013] [ INFO: possible recursive locking detected ]
>> >> [   40.478651] 4.6.0-08691-g7f3db9a #37 Not tainted
>> >> [   40.483466] -
>> >> [   40.489098] usb/733 is trying to acquire lock:
>> >> [   40.493734]  (&(>lock)->rlock){-.}, at: [] 
>> >> ep0_complete+0x18/0xdc [gadgetfs]
>> >> [   40.502882]
>> >> [   40.502882] but task is already holding lock:
>> >> [   40.508967]  (&(>lock)->rlock){-.}, at: [] 
>> >> ep0_read+0x20/0x5e0 [gadgetfs]
>> >> [   40.517811]
>> >> [   40.517811] other info that might help us debug this:
>> >> [   40.524623]  Possible unsafe locking scenario:
>> >> [   40.524623]
>> >> [   40.530798]CPU0
>> >> [   40.533346]
>> >> [   40.535894]   lock(&(>lock)->rlock);
>> >> [   40.540088]   lock(&(>lock)->rlock);
>> >> [   40.544284]
>> >> [   40.544284]  *** DEADLOCK ***
>> >> [   40.544284]
>> >> [   40.550461]  May be due to missing lock nesting notation
>> >> [   40.550461]
>> >> [   40.557544] 2 locks held by usb/733:
>> >> [   40.561271]  #0:  (>f_pos_lock){+.+.+.}, at: [] 
>> >> __fdget_pos+0x40/0x48
>> >> [   40.569219]  #1:  (&(>lock)->rlock){-.}, at: [] 
>> >> ep0_read+0x20/0x5e0 [gadgetfs]
>> >> [   40.578523]
>> >> [   40.578523] stack backtrace:
>> >> [   40.583075] CPU: 0 PID: 733 Comm: usb Not tainted 4.6.0-08691-g7f3db9a 
>> >> #37
>> >> [   40.590246] Hardware name: Generic AM33XX (Flattened Device Tree)
>> >> [   40.596625] [] (unwind_backtrace) from [] 
>> >> (show_stack+0x10/0x14)
>> >> [   40.604718] [] (show_stack) from [] 
>> >> (dump_stack+0xb0/0xe4)
>> >> [   40.612267] [] (dump_stack) from [] 
>> >> (__lock_acquire+0xf68/0x1994)
>> >> [   40.620440] [] (__lock_acquire) from [] 
>> >> (lock_acquire+0xd8/0x238)
>> >> [   40.628621] [] (lock_acquire) from [] 
>> >> (_raw_spin_lock_irqsave+0x38/0x4c)
>> >> [   40.637440] [] (_raw_spin_lock_irqsave) from [] 
>> >> (ep0_complete+0x18/0xdc [gadgetfs])
>> >> [   40.647339] [] (ep0_complete [gadgetfs]) from [] 
>> >> (musb_g_giveback+0x118/0x1b0 [musb_hdrc])
>> >> [   40.657842] [] (musb_g_giveback [musb_hdrc]) from 
>> >> [] (musb_g_ep0_queue+0x16c/0x188 [musb_hdrc])
>> >> [   40.668772] [] (musb_g_ep0_queue [musb_hdrc]) from 
>> >> [] (ep0_read+0x544/0x5e0 [gadgetfs])
>> >> [   40.678963] [] (ep0_read [gadgetfs]) from [] 
>> >> (__vfs_read+0x20/0x110)
>> >> [   40.687414] [] (__vfs_read) from [] 
>> >> (vfs_read+0x88/0x114)
>> >> [   40.694864] [] (vfs_read) from [] 
>> >> (SyS_read+0x44/0x9c)
>> >> [   40.702051] [] (SyS_read) from [] 
>> >> (ret_fast_syscall+0x0/0x1c)
>> >> 
>> >> Cc:  # v3.16+
>> >> Signed-off-by: Bin Liu 
>> >> ---
>> >> v2:
>> >>   - lock protects setup_req(), only unlock for usb_ep_queue()
>> >>   - use GFP_KERNEL in usb_ep_queue()
>> >>   - fix the same in two places in gadgetfs_setup() too
>> >
>> > Never mind. Sent the patch too soon. It gives the following problem.
>> >
>> > [  179.810367] WARNING: CPU: 0 PID: 0 at kernel/locking/lockdep.c:2724
>> > _raw_spin_unlock_irq+0x24/0x2c
>> > [  179.819719] DEBUG_LOCKS_WARN_ON(current->hardirq_context)
>> 
>> thanks for letting us know. I'm dropping this from my queue. Please
>> resend final patch once you have it ;-)
>
> It turned out to be a very easy fix ;)
>
> I have v3 ready, but checkpatch.pl complains the followings. I don't
> think this patch should fix them, since the whole driver is coded like
> that. Any comments?

Let's fix it with a follow-up patch. Can you do that one too, btw? I can
do it, if you don't wanna deal with that; but I'm also open to receiving
free patches heh

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v2] usb: gadget: fix spinlock dead lock in gadgetfs

2016-05-26 Thread Bin Liu

Hi,

On Thu, May 26, 2016 at 09:27:26AM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Bin Liu  writes:
> > On Wed, May 25, 2016 at 02:18:34PM -0500, Bin Liu wrote:
> >> [   40.467381] =
> >> [   40.473013] [ INFO: possible recursive locking detected ]
> >> [   40.478651] 4.6.0-08691-g7f3db9a #37 Not tainted
> >> [   40.483466] -
> >> [   40.489098] usb/733 is trying to acquire lock:
> >> [   40.493734]  (&(>lock)->rlock){-.}, at: [] 
> >> ep0_complete+0x18/0xdc [gadgetfs]
> >> [   40.502882]
> >> [   40.502882] but task is already holding lock:
> >> [   40.508967]  (&(>lock)->rlock){-.}, at: [] 
> >> ep0_read+0x20/0x5e0 [gadgetfs]
> >> [   40.517811]
> >> [   40.517811] other info that might help us debug this:
> >> [   40.524623]  Possible unsafe locking scenario:
> >> [   40.524623]
> >> [   40.530798]CPU0
> >> [   40.533346]
> >> [   40.535894]   lock(&(>lock)->rlock);
> >> [   40.540088]   lock(&(>lock)->rlock);
> >> [   40.544284]
> >> [   40.544284]  *** DEADLOCK ***
> >> [   40.544284]
> >> [   40.550461]  May be due to missing lock nesting notation
> >> [   40.550461]
> >> [   40.557544] 2 locks held by usb/733:
> >> [   40.561271]  #0:  (>f_pos_lock){+.+.+.}, at: [] 
> >> __fdget_pos+0x40/0x48
> >> [   40.569219]  #1:  (&(>lock)->rlock){-.}, at: [] 
> >> ep0_read+0x20/0x5e0 [gadgetfs]
> >> [   40.578523]
> >> [   40.578523] stack backtrace:
> >> [   40.583075] CPU: 0 PID: 733 Comm: usb Not tainted 4.6.0-08691-g7f3db9a 
> >> #37
> >> [   40.590246] Hardware name: Generic AM33XX (Flattened Device Tree)
> >> [   40.596625] [] (unwind_backtrace) from [] 
> >> (show_stack+0x10/0x14)
> >> [   40.604718] [] (show_stack) from [] 
> >> (dump_stack+0xb0/0xe4)
> >> [   40.612267] [] (dump_stack) from [] 
> >> (__lock_acquire+0xf68/0x1994)
> >> [   40.620440] [] (__lock_acquire) from [] 
> >> (lock_acquire+0xd8/0x238)
> >> [   40.628621] [] (lock_acquire) from [] 
> >> (_raw_spin_lock_irqsave+0x38/0x4c)
> >> [   40.637440] [] (_raw_spin_lock_irqsave) from [] 
> >> (ep0_complete+0x18/0xdc [gadgetfs])
> >> [   40.647339] [] (ep0_complete [gadgetfs]) from [] 
> >> (musb_g_giveback+0x118/0x1b0 [musb_hdrc])
> >> [   40.657842] [] (musb_g_giveback [musb_hdrc]) from 
> >> [] (musb_g_ep0_queue+0x16c/0x188 [musb_hdrc])
> >> [   40.668772] [] (musb_g_ep0_queue [musb_hdrc]) from 
> >> [] (ep0_read+0x544/0x5e0 [gadgetfs])
> >> [   40.678963] [] (ep0_read [gadgetfs]) from [] 
> >> (__vfs_read+0x20/0x110)
> >> [   40.687414] [] (__vfs_read) from [] 
> >> (vfs_read+0x88/0x114)
> >> [   40.694864] [] (vfs_read) from [] 
> >> (SyS_read+0x44/0x9c)
> >> [   40.702051] [] (SyS_read) from [] 
> >> (ret_fast_syscall+0x0/0x1c)
> >> 
> >> Cc:  # v3.16+
> >> Signed-off-by: Bin Liu 
> >> ---
> >> v2:
> >>   - lock protects setup_req(), only unlock for usb_ep_queue()
> >>   - use GFP_KERNEL in usb_ep_queue()
> >>   - fix the same in two places in gadgetfs_setup() too
> >
> > Never mind. Sent the patch too soon. It gives the following problem.
> >
> > [  179.810367] WARNING: CPU: 0 PID: 0 at kernel/locking/lockdep.c:2724
> > _raw_spin_unlock_irq+0x24/0x2c
> > [  179.819719] DEBUG_LOCKS_WARN_ON(current->hardirq_context)
> 
> thanks for letting us know. I'm dropping this from my queue. Please
> resend final patch once you have it ;-)

It turned out to be a very easy fix ;)

I have v3 ready, but checkpatch.pl complains the followings. I don't
think this patch should fix them, since the whole driver is coded like
that. Any comments?

WARNING: space prohibited between function name and open parenthesis '('
#68: FILE: drivers/usb/gadget/legacy/inode.c:941:
+   if ((retval = setup_req (ep, req, 0)) == 0) {

ERROR: do not use assignment in if condition
#68: FILE: drivers/usb/gadget/legacy/inode.c:941:
+   if ((retval = setup_req (ep, req, 0)) == 0) {

WARNING: space prohibited between function name and open parenthesis '('
#69: FILE: drivers/usb/gadget/legacy/inode.c:942:
+   spin_unlock_irq (>lock);

WARNING: space prohibited between function name and open parenthesis '('
#70: FILE: drivers/usb/gadget/legacy/inode.c:943:
+   retval = usb_ep_queue (ep, req, GFP_KERNEL);

WARNING: space prohibited between function name and open parenthesis '('
#71: FILE: drivers/usb/gadget/legacy/inode.c:944:
+   spin_lock_irq (>lock);

WARNING: space prohibited between function name and open parenthesis '('
#81: FILE: drivers/usb/gadget/legacy/inode.c:1464:
+   spin_unlock (>lock);

WARNING: space prohibited between function name and open parenthesis '('
#85: FILE: drivers/usb/gadget/legacy/inode.c:1467:
+   spin_lock (>lock);

WARNING: space prohibited between function name and open parenthesis '('
#95: FILE:

Re: [PATCH v2] usb: gadget: fix spinlock dead lock in gadgetfs

2016-05-26 Thread Felipe Balbi


Hi,

Bin Liu  writes:
> On Wed, May 25, 2016 at 02:18:34PM -0500, Bin Liu wrote:
>> [   40.467381] =
>> [   40.473013] [ INFO: possible recursive locking detected ]
>> [   40.478651] 4.6.0-08691-g7f3db9a #37 Not tainted
>> [   40.483466] -
>> [   40.489098] usb/733 is trying to acquire lock:
>> [   40.493734]  (&(>lock)->rlock){-.}, at: [] 
>> ep0_complete+0x18/0xdc [gadgetfs]
>> [   40.502882]
>> [   40.502882] but task is already holding lock:
>> [   40.508967]  (&(>lock)->rlock){-.}, at: [] 
>> ep0_read+0x20/0x5e0 [gadgetfs]
>> [   40.517811]
>> [   40.517811] other info that might help us debug this:
>> [   40.524623]  Possible unsafe locking scenario:
>> [   40.524623]
>> [   40.530798]CPU0
>> [   40.533346]
>> [   40.535894]   lock(&(>lock)->rlock);
>> [   40.540088]   lock(&(>lock)->rlock);
>> [   40.544284]
>> [   40.544284]  *** DEADLOCK ***
>> [   40.544284]
>> [   40.550461]  May be due to missing lock nesting notation
>> [   40.550461]
>> [   40.557544] 2 locks held by usb/733:
>> [   40.561271]  #0:  (>f_pos_lock){+.+.+.}, at: [] 
>> __fdget_pos+0x40/0x48
>> [   40.569219]  #1:  (&(>lock)->rlock){-.}, at: [] 
>> ep0_read+0x20/0x5e0 [gadgetfs]
>> [   40.578523]
>> [   40.578523] stack backtrace:
>> [   40.583075] CPU: 0 PID: 733 Comm: usb Not tainted 4.6.0-08691-g7f3db9a #37
>> [   40.590246] Hardware name: Generic AM33XX (Flattened Device Tree)
>> [   40.596625] [] (unwind_backtrace) from [] 
>> (show_stack+0x10/0x14)
>> [   40.604718] [] (show_stack) from [] 
>> (dump_stack+0xb0/0xe4)
>> [   40.612267] [] (dump_stack) from [] 
>> (__lock_acquire+0xf68/0x1994)
>> [   40.620440] [] (__lock_acquire) from [] 
>> (lock_acquire+0xd8/0x238)
>> [   40.628621] [] (lock_acquire) from [] 
>> (_raw_spin_lock_irqsave+0x38/0x4c)
>> [   40.637440] [] (_raw_spin_lock_irqsave) from [] 
>> (ep0_complete+0x18/0xdc [gadgetfs])
>> [   40.647339] [] (ep0_complete [gadgetfs]) from [] 
>> (musb_g_giveback+0x118/0x1b0 [musb_hdrc])
>> [   40.657842] [] (musb_g_giveback [musb_hdrc]) from [] 
>> (musb_g_ep0_queue+0x16c/0x188 [musb_hdrc])
>> [   40.668772] [] (musb_g_ep0_queue [musb_hdrc]) from [] 
>> (ep0_read+0x544/0x5e0 [gadgetfs])
>> [   40.678963] [] (ep0_read [gadgetfs]) from [] 
>> (__vfs_read+0x20/0x110)
>> [   40.687414] [] (__vfs_read) from [] 
>> (vfs_read+0x88/0x114)
>> [   40.694864] [] (vfs_read) from [] (SyS_read+0x44/0x9c)
>> [   40.702051] [] (SyS_read) from [] 
>> (ret_fast_syscall+0x0/0x1c)
>> 
>> Cc:  # v3.16+
>> Signed-off-by: Bin Liu 
>> ---
>> v2:
>>   - lock protects setup_req(), only unlock for usb_ep_queue()
>>   - use GFP_KERNEL in usb_ep_queue()
>>   - fix the same in two places in gadgetfs_setup() too
>
> Never mind. Sent the patch too soon. It gives the following problem.
>
> [  179.810367] WARNING: CPU: 0 PID: 0 at kernel/locking/lockdep.c:2724
> _raw_spin_unlock_irq+0x24/0x2c
> [  179.819719] DEBUG_LOCKS_WARN_ON(current->hardirq_context)

thanks for letting us know. I'm dropping this from my queue. Please
resend final patch once you have it ;-)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v2] usb: gadget: fix spinlock dead lock in gadgetfs

2016-05-25 Thread Bin Liu

On Wed, May 25, 2016 at 02:18:34PM -0500, Bin Liu wrote:
> [   40.467381] =
> [   40.473013] [ INFO: possible recursive locking detected ]
> [   40.478651] 4.6.0-08691-g7f3db9a #37 Not tainted
> [   40.483466] -
> [   40.489098] usb/733 is trying to acquire lock:
> [   40.493734]  (&(>lock)->rlock){-.}, at: [] 
> ep0_complete+0x18/0xdc [gadgetfs]
> [   40.502882]
> [   40.502882] but task is already holding lock:
> [   40.508967]  (&(>lock)->rlock){-.}, at: [] 
> ep0_read+0x20/0x5e0 [gadgetfs]
> [   40.517811]
> [   40.517811] other info that might help us debug this:
> [   40.524623]  Possible unsafe locking scenario:
> [   40.524623]
> [   40.530798]CPU0
> [   40.533346]
> [   40.535894]   lock(&(>lock)->rlock);
> [   40.540088]   lock(&(>lock)->rlock);
> [   40.544284]
> [   40.544284]  *** DEADLOCK ***
> [   40.544284]
> [   40.550461]  May be due to missing lock nesting notation
> [   40.550461]
> [   40.557544] 2 locks held by usb/733:
> [   40.561271]  #0:  (>f_pos_lock){+.+.+.}, at: [] 
> __fdget_pos+0x40/0x48
> [   40.569219]  #1:  (&(>lock)->rlock){-.}, at: [] 
> ep0_read+0x20/0x5e0 [gadgetfs]
> [   40.578523]
> [   40.578523] stack backtrace:
> [   40.583075] CPU: 0 PID: 733 Comm: usb Not tainted 4.6.0-08691-g7f3db9a #37
> [   40.590246] Hardware name: Generic AM33XX (Flattened Device Tree)
> [   40.596625] [] (unwind_backtrace) from [] 
> (show_stack+0x10/0x14)
> [   40.604718] [] (show_stack) from [] 
> (dump_stack+0xb0/0xe4)
> [   40.612267] [] (dump_stack) from [] 
> (__lock_acquire+0xf68/0x1994)
> [   40.620440] [] (__lock_acquire) from [] 
> (lock_acquire+0xd8/0x238)
> [   40.628621] [] (lock_acquire) from [] 
> (_raw_spin_lock_irqsave+0x38/0x4c)
> [   40.637440] [] (_raw_spin_lock_irqsave) from [] 
> (ep0_complete+0x18/0xdc [gadgetfs])
> [   40.647339] [] (ep0_complete [gadgetfs]) from [] 
> (musb_g_giveback+0x118/0x1b0 [musb_hdrc])
> [   40.657842] [] (musb_g_giveback [musb_hdrc]) from [] 
> (musb_g_ep0_queue+0x16c/0x188 [musb_hdrc])
> [   40.668772] [] (musb_g_ep0_queue [musb_hdrc]) from [] 
> (ep0_read+0x544/0x5e0 [gadgetfs])
> [   40.678963] [] (ep0_read [gadgetfs]) from [] 
> (__vfs_read+0x20/0x110)
> [   40.687414] [] (__vfs_read) from [] 
> (vfs_read+0x88/0x114)
> [   40.694864] [] (vfs_read) from [] (SyS_read+0x44/0x9c)
> [   40.702051] [] (SyS_read) from [] 
> (ret_fast_syscall+0x0/0x1c)
> 
> Cc:  # v3.16+
> Signed-off-by: Bin Liu 
> ---
> v2:
>   - lock protects setup_req(), only unlock for usb_ep_queue()
>   - use GFP_KERNEL in usb_ep_queue()
>   - fix the same in two places in gadgetfs_setup() too

Never mind. Sent the patch too soon. It gives the following problem.

[  179.810367] WARNING: CPU: 0 PID: 0 at kernel/locking/lockdep.c:2724
_raw_spin_unlock_irq+0x24/0x2c
[  179.819719] DEBUG_LOCKS_WARN_ON(current->hardirq_context)

Regards,
-Bin.

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] usb: gadget: fix spinlock dead lock in gadgetfs

2016-05-25 Thread Bin Liu

[   40.467381] =
[   40.473013] [ INFO: possible recursive locking detected ]
[   40.478651] 4.6.0-08691-g7f3db9a #37 Not tainted
[   40.483466] -
[   40.489098] usb/733 is trying to acquire lock:
[   40.493734]  (&(>lock)->rlock){-.}, at: [] 
ep0_complete+0x18/0xdc [gadgetfs]
[   40.502882]
[   40.502882] but task is already holding lock:
[   40.508967]  (&(>lock)->rlock){-.}, at: [] 
ep0_read+0x20/0x5e0 [gadgetfs]
[   40.517811]
[   40.517811] other info that might help us debug this:
[   40.524623]  Possible unsafe locking scenario:
[   40.524623]
[   40.530798]CPU0
[   40.533346]
[   40.535894]   lock(&(>lock)->rlock);
[   40.540088]   lock(&(>lock)->rlock);
[   40.544284]
[   40.544284]  *** DEADLOCK ***
[   40.544284]
[   40.550461]  May be due to missing lock nesting notation
[   40.550461]
[   40.557544] 2 locks held by usb/733:
[   40.561271]  #0:  (>f_pos_lock){+.+.+.}, at: [] 
__fdget_pos+0x40/0x48
[   40.569219]  #1:  (&(>lock)->rlock){-.}, at: [] 
ep0_read+0x20/0x5e0 [gadgetfs]
[   40.578523]
[   40.578523] stack backtrace:
[   40.583075] CPU: 0 PID: 733 Comm: usb Not tainted 4.6.0-08691-g7f3db9a #37
[   40.590246] Hardware name: Generic AM33XX (Flattened Device Tree)
[   40.596625] [] (unwind_backtrace) from [] 
(show_stack+0x10/0x14)
[   40.604718] [] (show_stack) from [] 
(dump_stack+0xb0/0xe4)
[   40.612267] [] (dump_stack) from [] 
(__lock_acquire+0xf68/0x1994)
[   40.620440] [] (__lock_acquire) from [] 
(lock_acquire+0xd8/0x238)
[   40.628621] [] (lock_acquire) from [] 
(_raw_spin_lock_irqsave+0x38/0x4c)
[   40.637440] [] (_raw_spin_lock_irqsave) from [] 
(ep0_complete+0x18/0xdc [gadgetfs])
[   40.647339] [] (ep0_complete [gadgetfs]) from [] 
(musb_g_giveback+0x118/0x1b0 [musb_hdrc])
[   40.657842] [] (musb_g_giveback [musb_hdrc]) from [] 
(musb_g_ep0_queue+0x16c/0x188 [musb_hdrc])
[   40.668772] [] (musb_g_ep0_queue [musb_hdrc]) from [] 
(ep0_read+0x544/0x5e0 [gadgetfs])
[   40.678963] [] (ep0_read [gadgetfs]) from [] 
(__vfs_read+0x20/0x110)
[   40.687414] [] (__vfs_read) from [] (vfs_read+0x88/0x114)
[   40.694864] [] (vfs_read) from [] (SyS_read+0x44/0x9c)
[   40.702051] [] (SyS_read) from [] 
(ret_fast_syscall+0x0/0x1c)

Cc:  # v3.16+
Signed-off-by: Bin Liu 
---
v2:
  - lock protects setup_req(), only unlock for usb_ep_queue()
  - use GFP_KERNEL in usb_ep_queue()
  - fix the same in two places in gadgetfs_setup() too

 drivers/usb/gadget/legacy/inode.c | 17 +
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/gadget/legacy/inode.c 
b/drivers/usb/gadget/legacy/inode.c
index e64479f..a333e42 100644
--- a/drivers/usb/gadget/legacy/inode.c
+++ b/drivers/usb/gadget/legacy/inode.c
@@ -938,8 +938,11 @@ ep0_read (struct file *fd, char __user *buf, size_t len, 
loff_t *ptr)
struct usb_ep   *ep = dev->gadget->ep0;
struct usb_request  *req = dev->req;
 
-   if ((retval = setup_req (ep, req, 0)) == 0)
-   retval = usb_ep_queue (ep, req, GFP_ATOMIC);
+   if ((retval = setup_req (ep, req, 0)) == 0) {
+   spin_unlock_irq (>lock);
+   retval = usb_ep_queue (ep, req, GFP_KERNEL);
+   spin_lock_irq (>lock);
+   }
dev->state = STATE_DEV_CONNECTED;
 
/* assume that was SET_CONFIGURATION */
@@ -1457,8 +1460,11 @@ delegate:
w_length);
if (value < 0)
break;
+
+   spin_unlock_irq (>lock);
value = usb_ep_queue (gadget->ep0, dev->req,
-   GFP_ATOMIC);
+   GFP_KERNEL);
+   spin_lock_irq (>lock);
if (value < 0) {
clean_req (gadget->ep0, dev->req);
break;
@@ -1481,7 +1487,10 @@ delegate:
if (value >= 0 && dev->state != STATE_DEV_SETUP) {
req->length = value;
req->zero = value < w_length;
-   value = usb_ep_queue (gadget->ep0, req, GFP_ATOMIC);
+
+   spin_unlock_irq (>lock);
+   value = usb_ep_queue (gadget->ep0, req, GFP_KERNEL);
+   spin_lock_irq (>lock);
if (value < 0) {
DBG (dev, "ep_queue --> %d\n", value);
req->status = 0;
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More

Re: [PATCH v2] usb: gadget: fix spinlock dead lock in gadgetfs

Re: [PATCH v2] usb: gadget: fix spinlock dead lock in gadgetfs

Re: [PATCH v2] usb: gadget: fix spinlock dead lock in gadgetfs

Re: [PATCH v2] usb: gadget: fix spinlock dead lock in gadgetfs

Re: [PATCH v2] usb: gadget: fix spinlock dead lock in gadgetfs

[PATCH v2] usb: gadget: fix spinlock dead lock in gadgetfs

6 matches

Site Navigation

Mail list logo

Footer information