On Thu, Sep 22, 2022 at 10:26:54PM +0800, eadavis@xxxxxxxx wrote:
> From: Edward Adam Davis <eadavis@xxxxxxxx>
>
> On Thu, 22 Sep 2022 15:04:45 +0200, Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > On Sun, Sep 18, 2022 at 11:50:37AM +0800, eadavis@xxxxxxxx wrote:
> > > From: Edward Adam Davis <eadavis@xxxxxxxx>
> > >
> > > ============================================
> > > WARNING: possible recursive locking detected
> > > 6.0.0-rc4+ #20 Not tainted
> > > --------------------------------------------
> > > kworker/0:1H/9 is trying to acquire lock:
> > > ffff888057ed9228 (&midi->transmit_lock){....}-{2:2}, at:
> > > f_midi_transmit+0x18c/0x1460 drivers/usb/gadget/function/f_midi.c:683
> > >
> > > but task is already holding lock:
> > > ffff888057ed9228 (&midi->transmit_lock){....}-{2:2}, at:
> > > f_midi_transmit+0x18c/0x1460 drivers/usb/gadget/function/f_midi.c:683
> > >
> > > other info that might help us debug this:
> > > Possible unsafe locking scenario:
> > >
> > > CPU0
> > > ----
> > > lock(&midi->transmit_lock);
> > > lock(&midi->transmit_lock);
> > >
> > > *** DEADLOCK ***
> > >
> > > May be due to missing lock nesting notation
> > >
> > > 3 locks held by kworker/0:1H/9:
> > > #0: ffff888011c65138 ((wq_completion)events_highpri){+.+.}-{0:0}, at:
> > > arch_atomic64_set arch/x86/include/asm/atomic64_64.h:34 [inline]
> > > #0: ffff888011c65138 ((wq_completion)events_highpri){+.+.}-{0:0}, at:
> > > arch_atomic_long_set include/linux/atomic/atomic-long.h:41 [inline]
> > > #0: ffff888011c65138 ((wq_completion)events_highpri){+.+.}-{0:0}, at:
> > > atomic_long_set include/linux/atomic/atomic-instrumented.h:1280
> > > [inline]
> > > #0: ffff888011c65138 ((wq_completion)events_highpri){+.+.}-{0:0}, at:
> > > set_work_data kernel/workqueue.c:636 [inline]
> > > #0: ffff888011c65138 ((wq_completion)events_highpri){+.+.}-{0:0}, at:
> > > set_work_pool_and_clear_pending kernel/workqueue.c:663 [inline]
> > > #0: ffff888011c65138 ((wq_completion)events_highpri){+.+.}-{0:0}, at:
> > > process_one_work+0x8b0/0x1650 kernel/workqueue.c:2260
> > > #1: ffffc900003afdb0 ((work_completion)(&midi->work)){+.+.}-{0:0},
> > > at: process_one_work+0x8e4/0x1650 kernel/workqueue.c:2264
> > > #2: ffff888057ed9228 (&midi->transmit_lock){....}-{2:2}, at:
> > > f_midi_transmit+0x18c/0x1460 drivers/usb/gadget/function/f_midi.c:683
> > >
> > > stack backtrace:
> > > CPU: 0 PID: 9 Comm: kworker/0:1H Not tainted 6.0.0-rc4+ #20
> > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> > > 1.13.0-1ubuntu1.1 04/01/2014
> > > Workqueue: events_highpri f_midi_in_work
> > > Call Trace:
> > > <TASK>
> > > __dump_stack lib/dump_stack.c:88 [inline]
> > > dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
> > > print_deadlock_bug kernel/locking/lockdep.c:2988 [inline]
> > > check_deadlock kernel/locking/lockdep.c:3031 [inline]
> > > validate_chain kernel/locking/lockdep.c:3816 [inline]
> > > __lock_acquire.cold+0x152/0x3c3 kernel/locking/lockdep.c:5053
> > > lock_acquire kernel/locking/lockdep.c:5666 [inline]
> > > lock_acquire+0x1ab/0x580 kernel/locking/lockdep.c:5631
> > > __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
> > > _raw_spin_lock_irqsave+0x39/0x50 kernel/locking/spinlock.c:162
> > > f_midi_transmit+0x18c/0x1460 drivers/usb/gadget/function/f_midi.c:683
> > > f_midi_complete+0x1bb/0x480 drivers/usb/gadget/function/f_midi.c:285
> > > dummy_queue+0x84a/0xb20 drivers/usb/gadget/udc/dummy_hcd.c:736
> > > usb_ep_queue+0xe8/0x3b0 drivers/usb/gadget/udc/core.c:288
> > > f_midi_do_transmit drivers/usb/gadget/function/f_midi.c:658 [inline]
> > > f_midi_transmit+0x7e4/0x1460 drivers/usb/gadget/function/f_midi.c:686
> > > process_one_work+0x9c7/0x1650 kernel/workqueue.c:2289
> > > worker_thread+0x623/0x1070 kernel/workqueue.c:2436
> > > kthread+0x2e9/0x3a0 kernel/kthread.c:376
> > > ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:306
> > > </TASK>
> > > Use nested notation for the spin_lock to avoid this warning.
> > >
> > > Signed-off-by: Edward Adam Davis <eadavis@xxxxxxxx>
> > > ---
> > > drivers/usb/gadget/function/f_midi.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
> > > index fddf539008a9..ad745fbd549e 100644
> > > --- a/drivers/usb/gadget/function/f_midi.c
> > > +++ b/drivers/usb/gadget/function/f_midi.c
> > > @@ -680,7 +680,8 @@ static void f_midi_transmit(struct f_midi *midi)
> > > if (!ep || !ep->enabled)
> > > goto drop_out;
> > >
> > > - spin_lock_irqsave(&midi->transmit_lock, flags);
> > > + spin_lock_irqsave_nested(&midi->transmit_lock, flags,
> > > + SINGLE_DEPTH_NESTING);
> >
> > This feels wrong (and you added a checkpatch warning at the same time.)
> >
> > If this is correct, please document this really really well why this is
> > the correct solution and we just don't really have a lockdep issue here
> > with the code itself.
> I want to assume the following scenario,
>
> CPU1
> ----
> spin_lock_irqsave(&midi->transmit_lock, f); <----- Task A
> ...
> ... <----- raise NMI and call Task B
> spin_lock_irqsave(&midi->transmit_lock, f); <----- Task B acquire same lock, OK?
Is that ok? Can you nest a spin lock like this? For some reason, I
didn't think you could, but I can't find anything in the documentation
about this, can you?
thanks,
greg k-h
