linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] usb: gadget: composite: split spinlock to avoid recursion
@ 2019-11-12  9:33 Michael Olbrich
  2019-11-13  6:31 ` Peter Chen
  0 siblings, 1 reply; 6+ messages in thread
From: Michael Olbrich @ 2019-11-12  9:33 UTC (permalink / raw)
  To: linux-usb
  Cc: Felipe Balbi, Greg Kroah-Hartman, linux-kernel, kernel, 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


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] usb: gadget: composite: split spinlock to avoid recursion
  2019-11-12  9:33 [PATCH] usb: gadget: composite: split spinlock to avoid recursion Michael Olbrich
@ 2019-11-13  6:31 ` Peter Chen
  2019-11-13 15:36   ` Alan Stern
  0 siblings, 1 reply; 6+ messages in thread
From: Peter Chen @ 2019-11-13  6:31 UTC (permalink / raw)
  To: Michael Olbrich; +Cc: linux-usb, Felipe Balbi, Greg Kroah-Hartman, linux-kernel

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] usb: gadget: composite: split spinlock to avoid recursion
  2019-11-13  6:31 ` Peter Chen
@ 2019-11-13 15:36   ` Alan Stern
  2019-11-14 13:23     ` Michael Olbrich
  0 siblings, 1 reply; 6+ messages in thread
From: Alan Stern @ 2019-11-13 15:36 UTC (permalink / raw)
  To: Peter Chen
  Cc: Michael Olbrich, linux-usb, Felipe Balbi, Greg Kroah-Hartman,
	linux-kernel

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


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] usb: gadget: composite: split spinlock to avoid recursion
  2019-11-13 15:36   ` Alan Stern
@ 2019-11-14 13:23     ` Michael Olbrich
  2019-11-15  6:59       ` Peter Chen
  2019-11-18 20:56       ` Alan Stern
  0 siblings, 2 replies; 6+ messages in thread
From: Michael Olbrich @ 2019-11-14 13:23 UTC (permalink / raw)
  To: Alan Stern
  Cc: Peter Chen, linux-usb, Felipe Balbi, Greg Kroah-Hartman, linux-kernel

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 |

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] usb: gadget: composite: split spinlock to avoid recursion
  2019-11-14 13:23     ` Michael Olbrich
@ 2019-11-15  6:59       ` Peter Chen
  2019-11-18 20:56       ` Alan Stern
  1 sibling, 0 replies; 6+ messages in thread
From: Peter Chen @ 2019-11-15  6:59 UTC (permalink / raw)
  To: Alan Stern, linux-usb, Felipe Balbi, Greg Kroah-Hartman,
	linux-kernel, Michael Olbrich

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] usb: gadget: composite: split spinlock to avoid recursion
  2019-11-14 13:23     ` Michael Olbrich
  2019-11-15  6:59       ` Peter Chen
@ 2019-11-18 20:56       ` Alan Stern
  1 sibling, 0 replies; 6+ messages in thread
From: Alan Stern @ 2019-11-18 20:56 UTC (permalink / raw)
  To: Michael Olbrich, Felipe Balbi
  Cc: Peter Chen, linux-usb, Greg Kroah-Hartman, linux-kernel

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


^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-11-18 20:56 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-12  9:33 [PATCH] usb: gadget: composite: split spinlock to avoid recursion Michael Olbrich
2019-11-13  6:31 ` Peter Chen
2019-11-13 15:36   ` Alan Stern
2019-11-14 13:23     ` Michael Olbrich
2019-11-15  6:59       ` Peter Chen
2019-11-18 20:56       ` Alan Stern

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).