[PATCH] usb: gadget: composite: split spinlock to avoid recursion

[PATCH] usb: gadget: composite: split spinlock to avoid recursion

[Michael Olbrich]

'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.

Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de>
---
 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;
+	/* protects deactivations counts*/
+	spinlock_t			deactivations_lock;
 
 	/* public: */
 	unsigned int			setup_pending:1;
-- 
2.20.1

Re: [PATCH] usb: gadget: composite: split spinlock to avoid recursion

[Peter Chen]

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@pengutronix.de>
> ---
>  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

Re: [PATCH] usb: gadget: composite: split spinlock to avoid recursion

[Alan Stern]

On Wed, 13 Nov 2019, Peter Chen wrote:

> 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.

I wasn't aware of the dual usage of that lock in the composite core 
(and 0a55187a1ec8c touches only the gadget core, not composite.c).

In any case, I don't have a good feel for how the locking is supposed 
to work in the composite core.  This is really something Felipe should 
look at.

Would a better fix be to change usb_function_deactivate() so that it
doesn't hold the lock while calling usb_gadget_deactivate()?  Maybe
increment cdev->deactivations unconditionally before dropping the lock
(for mutual exclusion) and then decrement it again if the call fails?

Alan Stern

Re: [PATCH] usb: gadget: composite: split spinlock to avoid recursion

[Michael Olbrich]

On Wed, Nov 13, 2019 at 10:36:25AM -0500, Alan Stern wrote:
> On Wed, 13 Nov 2019, Peter Chen wrote:
> > 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.
> 
> I wasn't aware of the dual usage of that lock in the composite core 
> (and 0a55187a1ec8c touches only the gadget core, not composite.c).
> 
> In any case, I don't have a good feel for how the locking is supposed 
> to work in the composite core.  This is really something Felipe should 
> look at.
> 
> Would a better fix be to change usb_function_deactivate() so that it
> doesn't hold the lock while calling usb_gadget_deactivate()?  Maybe
> increment cdev->deactivations unconditionally before dropping the lock
> (for mutual exclusion) and then decrement it again if the call fails?

Hmm, I think, that would mean that usb_gadget_activate() may be called
while usb_gadget_deactivate() is still running right? That's not
acceptable, is it?

Anyways. Something else is needed because executing usb_gadget_deactivate()
under the spinlock has another problem. It's hard to reproduce, but we've
seen this one:

BUG: scheduling while atomic: pipewire/260/0x00000002
Modules linked in: allegro(C) regmap_mmio v4l2_mem2mem xlnx_vcu st1232 uio_pdrv_genirq
Preemption disabled at: [<ffffff801061dc40>] usb_function_deactivate+0x30/0x80
CPU: 1 PID: 260 Comm: pipewire Tainted: G         C O 5.3.0-20191112-1 #2
Hardware name: Wolfvision ZynqMP PF4 (DT)
Call trace:
 dump_backtrace+0x0/0x178
 show_stack+0x24/0x30
 dump_stack+0xc0/0x104
 __schedule_bug+0xb0/0xc0
 __schedule+0x354/0x4d8
 schedule+0x44/0xd8
 schedule_timeout+0x1b4/0x380
 wait_for_common+0xc0/0x188
 wait_for_completion_timeout+0x2c/0x38
 dwc3_gadget_pullup+0x90/0xb0
 usb_gadget_disconnect+0x38/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+0x90/0x208
 ____fput+0x20/0x30
 task_work_run+0x98/0xb8
 do_notify_resume+0x2f4/0x340
 work_pending+0x8/0x14
dwc3 fe200000.usb: timed out waiting for SETUP phase

Or maybe it's incorrect for dwc3_gadget_pullup() to sleep?

Regards,
Michael

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] usb: gadget: composite: split spinlock to avoid recursion

[Peter Chen]

On 19-11-14 14:23:30, Michael Olbrich wrote:
> On Wed, Nov 13, 2019 at 10:36:25AM -0500, Alan Stern wrote:
> > On Wed, 13 Nov 2019, Peter Chen wrote:
> > > 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.
> > 
> > I wasn't aware of the dual usage of that lock in the composite core 
> > (and 0a55187a1ec8c touches only the gadget core, not composite.c).
> > 
> > In any case, I don't have a good feel for how the locking is supposed 
> > to work in the composite core.  This is really something Felipe should 
> > look at.
> > 
> > Would a better fix be to change usb_function_deactivate() so that it
> > doesn't hold the lock while calling usb_gadget_deactivate()?  Maybe
> > increment cdev->deactivations unconditionally before dropping the lock
> > (for mutual exclusion) and then decrement it again if the call fails?
> 
> Hmm, I think, that would mean that usb_gadget_activate() may be called
> while usb_gadget_deactivate() is still running right? That's not
> acceptable, is it?
> 

I agree with Alan. Return error for usb_function_activate if
usb_function_deactive is running could be accepted, it seems there is
no other good way to fix this lock recursion and your below atomic
issue, let's see what Felipe will say.

Peter

> Anyways. Something else is needed because executing usb_gadget_deactivate()
> under the spinlock has another problem. It's hard to reproduce, but we've
> seen this one:
> 
> BUG: scheduling while atomic: pipewire/260/0x00000002
> Modules linked in: allegro(C) regmap_mmio v4l2_mem2mem xlnx_vcu st1232 uio_pdrv_genirq
> Preemption disabled at: [<ffffff801061dc40>] usb_function_deactivate+0x30/0x80
> CPU: 1 PID: 260 Comm: pipewire Tainted: G         C O 5.3.0-20191112-1 #2
> Hardware name: Wolfvision ZynqMP PF4 (DT)
> Call trace:
>  dump_backtrace+0x0/0x178
>  show_stack+0x24/0x30
>  dump_stack+0xc0/0x104
>  __schedule_bug+0xb0/0xc0
>  __schedule+0x354/0x4d8
>  schedule+0x44/0xd8
>  schedule_timeout+0x1b4/0x380
>  wait_for_common+0xc0/0x188
>  wait_for_completion_timeout+0x2c/0x38
>  dwc3_gadget_pullup+0x90/0xb0
>  usb_gadget_disconnect+0x38/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+0x90/0x208
>  ____fput+0x20/0x30
>  task_work_run+0x98/0xb8
>  do_notify_resume+0x2f4/0x340
>  work_pending+0x8/0x14
> dwc3 fe200000.usb: timed out waiting for SETUP phase
> 

Hi Michael,
   

-- 

Thanks,
Peter Chen

Re: [PATCH] usb: gadget: composite: split spinlock to avoid recursion

[Alan Stern]

On Thu, 14 Nov 2019, Michael Olbrich wrote:

> On Wed, Nov 13, 2019 at 10:36:25AM -0500, Alan Stern wrote:
> > On Wed, 13 Nov 2019, Peter Chen wrote:
> > > 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.
> > 
> > I wasn't aware of the dual usage of that lock in the composite core 
> > (and 0a55187a1ec8c touches only the gadget core, not composite.c).
> > 
> > In any case, I don't have a good feel for how the locking is supposed 
> > to work in the composite core.  This is really something Felipe should 
> > look at.
> > 
> > Would a better fix be to change usb_function_deactivate() so that it
> > doesn't hold the lock while calling usb_gadget_deactivate()?  Maybe
> > increment cdev->deactivations unconditionally before dropping the lock
> > (for mutual exclusion) and then decrement it again if the call fails?
> 
> Hmm, I think, that would mean that usb_gadget_activate() may be called
> while usb_gadget_deactivate() is still running right? That's not
> acceptable, is it?

It's a little tricky.  The lock in question belongs to the composite 
core, not the UDC core, so it doesn't really apply to the 
usb_gadget_{de}activate() routines.

As for mutual exclusion of usb_gadget_activate() and
usb_gadget_deactivate(), I don't know that anyone has ever considered
the matter.

> Anyways. Something else is needed because executing usb_gadget_deactivate()
> under the spinlock has another problem. It's hard to reproduce, but we've
> seen this one:
> 
> BUG: scheduling while atomic: pipewire/260/0x00000002
> Modules linked in: allegro(C) regmap_mmio v4l2_mem2mem xlnx_vcu st1232 uio_pdrv_genirq
> Preemption disabled at: [<ffffff801061dc40>] usb_function_deactivate+0x30/0x80
> CPU: 1 PID: 260 Comm: pipewire Tainted: G         C O 5.3.0-20191112-1 #2
> Hardware name: Wolfvision ZynqMP PF4 (DT)
> Call trace:
>  dump_backtrace+0x0/0x178
>  show_stack+0x24/0x30
>  dump_stack+0xc0/0x104
>  __schedule_bug+0xb0/0xc0
>  __schedule+0x354/0x4d8
>  schedule+0x44/0xd8
>  schedule_timeout+0x1b4/0x380
>  wait_for_common+0xc0/0x188
>  wait_for_completion_timeout+0x2c/0x38
>  dwc3_gadget_pullup+0x90/0xb0
>  usb_gadget_disconnect+0x38/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+0x90/0x208
>  ____fput+0x20/0x30
>  task_work_run+0x98/0xb8
>  do_notify_resume+0x2f4/0x340
>  work_pending+0x8/0x14
> dwc3 fe200000.usb: timed out waiting for SETUP phase
> 
> Or maybe it's incorrect for dwc3_gadget_pullup() to sleep?

It isn't documented, so there's no definitive answer.  My feeling is 
that the UDC driver pullup routines should not sleep, but that's not 
official.

Of course, Felipe should have the last word on this.

Alan Stern