On 19-11-12 10:33:18, Michael Olbrich wrote: > 'delayed_status' and 'deactivations' are used completely independent but > they share the same spinlock. This can result in spinlock recursion: > > BUG: spinlock recursion on CPU#1, uvc-gadget/322 > lock: 0xffffffc0570364e0, .magic: dead4ead, .owner: uvc-gadget/322, .owner_cpu: 1 > CPU: 1 PID: 322 Comm: uvc-gadget Tainted: G C O 5.3.0-20190916-1+ #55 > Hardware name: XXXXX (DT) > Call trace: > dump_backtrace+0x0/0x178 > show_stack+0x24/0x30 > dump_stack+0xc0/0x104 > spin_dump+0x90/0xa0 > do_raw_spin_lock+0xd8/0x108 > _raw_spin_lock_irqsave+0x40/0x50 > composite_disconnect+0x2c/0x80 > usb_gadget_disconnect+0x84/0x150 > usb_gadget_deactivate+0x64/0x120 > usb_function_deactivate+0x70/0x80 > uvc_function_disconnect+0x20/0x58 > uvc_v4l2_release+0x34/0x90 > v4l2_release+0xbc/0xf0 > __fput+0xb0/0x218 > ____fput+0x20/0x30 > task_work_run+0xa0/0xd0 > do_notify_resume+0x2f4/0x340 > work_pending+0x8/0x14 > > Fix this by using separate spinlocks. This issue may be introduced by 0a55187a1ec8c ("USB: gadget core: Issue ->disconnect() callback from usb_gadget_disconnect()"), which adds gadget's disconnect at usb_gadget_disconnect. Add Alan, if he is Ok with your patch, you may cc to stable tree. > > Signed-off-by: Michael Olbrich <m.olbrich@xxxxxxxxxxxxxx> > --- > drivers/usb/gadget/composite.c | 9 +++++---- > drivers/usb/gadget/configfs.c | 1 + > include/linux/usb/composite.h | 4 +++- > 3 files changed, 9 insertions(+), 5 deletions(-) > > diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c > index 76883ff4f5bb..35c792e5b408 100644 > --- a/drivers/usb/gadget/composite.c > +++ b/drivers/usb/gadget/composite.c > @@ -346,14 +346,14 @@ int usb_function_deactivate(struct usb_function *function) > unsigned long flags; > int status = 0; > > - spin_lock_irqsave(&cdev->lock, flags); > + spin_lock_irqsave(&cdev->deactivations_lock, flags); > > if (cdev->deactivations == 0) > status = usb_gadget_deactivate(cdev->gadget); > if (status == 0) > cdev->deactivations++; > > - spin_unlock_irqrestore(&cdev->lock, flags); > + spin_unlock_irqrestore(&cdev->deactivations_lock, flags); > return status; > } > EXPORT_SYMBOL_GPL(usb_function_deactivate); > @@ -374,7 +374,7 @@ int usb_function_activate(struct usb_function *function) > unsigned long flags; > int status = 0; > > - spin_lock_irqsave(&cdev->lock, flags); > + spin_lock_irqsave(&cdev->deactivations_lock, flags); > > if (WARN_ON(cdev->deactivations == 0)) > status = -EINVAL; > @@ -384,7 +384,7 @@ int usb_function_activate(struct usb_function *function) > status = usb_gadget_activate(cdev->gadget); > } > > - spin_unlock_irqrestore(&cdev->lock, flags); > + spin_unlock_irqrestore(&cdev->deactivations_lock, flags); > return status; > } > EXPORT_SYMBOL_GPL(usb_function_activate); > @@ -2196,6 +2196,7 @@ static int composite_bind(struct usb_gadget *gadget, > return status; > > spin_lock_init(&cdev->lock); > + spin_lock_init(&cdev->deactivations_lock); > cdev->gadget = gadget; > set_gadget_data(gadget, cdev); > INIT_LIST_HEAD(&cdev->configs); > diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c > index 025129942894..45f717fcdb89 100644 > --- a/drivers/usb/gadget/configfs.c > +++ b/drivers/usb/gadget/configfs.c > @@ -521,6 +521,7 @@ static const struct config_item_type gadget_root_type = { > static void composite_init_dev(struct usb_composite_dev *cdev) > { > spin_lock_init(&cdev->lock); > + spin_lock_init(&cdev->deactivations_lock); > INIT_LIST_HEAD(&cdev->configs); > INIT_LIST_HEAD(&cdev->gstrings); > } > diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h > index 8675e145ea8b..86eb6f2c03ac 100644 > --- a/include/linux/usb/composite.h > +++ b/include/linux/usb/composite.h > @@ -505,8 +505,10 @@ struct usb_composite_dev { > */ > int delayed_status; > > - /* protects deactivations and delayed_status counts*/ > + /* protects delayed_status counts*/ > spinlock_t lock; In fact, this lock is used many places, not only at setup delayed cases. You may change comments or delete the comments. > + /* protects deactivations counts*/ Add one space after 'counts'. > + spinlock_t deactivations_lock; > -- Thanks, Peter Chen