Hi,
On Thu, May 26, 2016 at 09:27:26AM +0300, Felipe Balbi wrote:
>
> Hi,
>
> Bin Liu <[email protected]> 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] (&(&dev->lock)->rlock){-.....}, at: [<bf129288>]
> >> ep0_complete+0x18/0xdc [gadgetfs]
> >> [ 40.502882]
> >> [ 40.502882] but task is already holding lock:
> >> [ 40.508967] (&(&dev->lock)->rlock){-.....}, at: [<bf12a420>]
> >> 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(&(&dev->lock)->rlock);
> >> [ 40.540088] lock(&(&dev->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->f_pos_lock){+.+.+.}, at: [<c02a6114>]
> >> __fdget_pos+0x40/0x48
> >> [ 40.569219] #1: (&(&dev->lock)->rlock){-.....}, at: [<bf12a420>]
> >> 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] [<c010ffbc>] (unwind_backtrace) from [<c010c1bc>]
> >> (show_stack+0x10/0x14)
> >> [ 40.604718] [<c010c1bc>] (show_stack) from [<c04207fc>]
> >> (dump_stack+0xb0/0xe4)
> >> [ 40.612267] [<c04207fc>] (dump_stack) from [<c01886ec>]
> >> (__lock_acquire+0xf68/0x1994)
> >> [ 40.620440] [<c01886ec>] (__lock_acquire) from [<c0189528>]
> >> (lock_acquire+0xd8/0x238)
> >> [ 40.628621] [<c0189528>] (lock_acquire) from [<c06ad6b4>]
> >> (_raw_spin_lock_irqsave+0x38/0x4c)
> >> [ 40.637440] [<c06ad6b4>] (_raw_spin_lock_irqsave) from [<bf129288>]
> >> (ep0_complete+0x18/0xdc [gadgetfs])
> >> [ 40.647339] [<bf129288>] (ep0_complete [gadgetfs]) from [<bf10a728>]
> >> (musb_g_giveback+0x118/0x1b0 [musb_hdrc])
> >> [ 40.657842] [<bf10a728>] (musb_g_giveback [musb_hdrc]) from
> >> [<bf108768>] (musb_g_ep0_queue+0x16c/0x188 [musb_hdrc])
> >> [ 40.668772] [<bf108768>] (musb_g_ep0_queue [musb_hdrc]) from
> >> [<bf12a944>] (ep0_read+0x544/0x5e0 [gadgetfs])
> >> [ 40.678963] [<bf12a944>] (ep0_read [gadgetfs]) from [<c0284470>]
> >> (__vfs_read+0x20/0x110)
> >> [ 40.687414] [<c0284470>] (__vfs_read) from [<c0285324>]
> >> (vfs_read+0x88/0x114)
> >> [ 40.694864] [<c0285324>] (vfs_read) from [<c0286150>]
> >> (SyS_read+0x44/0x9c)
> >> [ 40.702051] [<c0286150>] (SyS_read) from [<c0107820>]
> >> (ret_fast_syscall+0x0/0x1c)
> >>
> >> Cc: <[email protected]> # v3.16+
> >> Signed-off-by: Bin Liu <[email protected]>
> >> ---
> >> 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 (&dev->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 (&dev->lock);
WARNING: space prohibited between function name and open parenthesis '('
#81: FILE: drivers/usb/gadget/legacy/inode.c:1464:
+ spin_unlock (&dev->lock);
WARNING: space prohibited between function name and open parenthesis '('
#85: FILE: drivers/usb/gadget/legacy/inode.c:1467:
+ spin_lock (&dev->lock);
WARNING: space prohibited between function name and open parenthesis '('
#95: FILE: drivers/usb/gadget/legacy/inode.c:1491:
+ spin_unlock (&dev->lock);
WARNING: space prohibited between function name and open parenthesis '('
#96: FILE: drivers/usb/gadget/legacy/inode.c:1492:
+ value = usb_ep_queue (gadget->ep0, req, GFP_KERNEL);
total: 1 errors, 8 warnings, 40 lines checked
Regards,
-Bin.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
