[PATCH 0/2] MUSB: tracking down locking bugs

[PATCH 0/2] MUSB: tracking down locking bugs

[Kevin Hilman]

With the help of the lock debugging features of the -rt patch, I'm
trying to track down locking issues in the MUSB driver.

For starters, here's one fix and a patch which BUGs under potential
deadlocks for aid in tracking down more potential deadlock paths.

Kevin
--

[PATCH 1/2] ARM: OMAP: MUSB: dont exit with spinlock held

[Kevin Hilman]

[-- Attachment #1: musb-locking-vbus-store.patch --]
[-- Type: text/plain, Size: 974 bytes --]

Signed-off-by: Kevin Hilman <khilman@mvista.com>

---
 drivers/usb/musb/musb_core.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Index: dev/drivers/usb/musb/musb_core.c
===================================================================
--- dev.orig/drivers/usb/musb/musb_core.c
+++ dev/drivers/usb/musb/musb_core.c
@@ -1682,19 +1682,22 @@ musb_vbus_store(struct device *dev, stru
 	struct musb	*musb = dev_to_musb(dev);
 	unsigned long	flags;
 	unsigned long	val;
+	ssize_t retval = n;
 
 	spin_lock_irqsave(&musb->lock, flags);
 	if (sscanf(buf, "%lu", &val) < 1) {
 		printk(KERN_ERR "Invalid VBUS timeout ms value\n");
-		return -EINVAL;
+		retval = -EINVAL;
+		goto out;
 	}
 	musb->a_wait_bcon = val;
 	if (musb->xceiv.state == OTG_STATE_A_WAIT_BCON)
 		musb->is_active = 0;
 	musb_platform_try_idle(musb, jiffies + msecs_to_jiffies(val));
+out:
 	spin_unlock_irqrestore(&musb->lock, flags);
 
-	return n;
+	return retval;
 }
 
 static ssize_t

--

Re: [PATCH 1/2] ARM: OMAP: MUSB: dont exit with spinlock held

[David Brownell]

> From linux-omap-open-source-bounces@linux.omap.com  Thu Aug 30 20:07:19 2007
> Date: Thu, 30 Aug 2007 19:54:00 -0700
> From: Kevin Hilman <khilman@mvista.com>
> To: linux-omap-open-source@linux.omap.com
> Subject: [PATCH 1/2] ARM: OMAP: MUSB: dont exit with spinlock held
>
> Signed-off-by: Kevin Hilman <khilman@mvista.com>
>
> ---
>  drivers/usb/musb/musb_core.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> Index: dev/drivers/usb/musb/musb_core.c
> ===================================================================
> --- dev.orig/drivers/usb/musb/musb_core.c
> +++ dev/drivers/usb/musb/musb_core.c
> @@ -1682,19 +1682,22 @@ musb_vbus_store(struct device *dev, stru
>  	struct musb	*musb = dev_to_musb(dev);
>  	unsigned long	flags;
>  	unsigned long	val;
> +	ssize_t retval = n;
>  
>  	spin_lock_irqsave(&musb->lock, flags);
>  	if (sscanf(buf, "%lu", &val) < 1) {

What's it doing grabbing the spinlock before sscanf() anyway?
It's not loke any protected data is handed off to sscanf()...
This should instead be:

	if (sscanf(...)...)
		return -EINVAL

	spin_lock_irqsave(...)

General rule of thumb:  don't grab spinlocks earlier than necessary.


>  		printk(KERN_ERR "Invalid VBUS timeout ms value\n");
> -		return -EINVAL;
> +		retval = -EINVAL;
> +		goto out;
>  	}
>  	musb->a_wait_bcon = val;
>  	if (musb->xceiv.state == OTG_STATE_A_WAIT_BCON)
>  		musb->is_active = 0;
>  	musb_platform_try_idle(musb, jiffies + msecs_to_jiffies(val));
> +out:
>  	spin_unlock_irqrestore(&musb->lock, flags);
>  
> -	return n;
> +	return retval;
>  }
>  
>  static ssize_t
>

Re: [PATCH 1/2] ARM: OMAP: MUSB: dont exit with spinlock held

[Kevin Hilman]

David Brownell wrote:
>> From linux-omap-open-source-bounces@linux.omap.com  Thu Aug 30 20:07:19 2007
>> Date: Thu, 30 Aug 2007 19:54:00 -0700
>> From: Kevin Hilman <khilman@mvista.com>
>> To: linux-omap-open-source@linux.omap.com
>> Subject: [PATCH 1/2] ARM: OMAP: MUSB: dont exit with spinlock held
>>
>> Signed-off-by: Kevin Hilman <khilman@mvista.com>
>>
>> ---
>>  drivers/usb/musb/musb_core.c |    7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> Index: dev/drivers/usb/musb/musb_core.c
>> ===================================================================
>> --- dev.orig/drivers/usb/musb/musb_core.c
>> +++ dev/drivers/usb/musb/musb_core.c
>> @@ -1682,19 +1682,22 @@ musb_vbus_store(struct device *dev, stru
>>  	struct musb	*musb = dev_to_musb(dev);
>>  	unsigned long	flags;
>>  	unsigned long	val;
>> +	ssize_t retval = n;
>>  
>>  	spin_lock_irqsave(&musb->lock, flags);
>>  	if (sscanf(buf, "%lu", &val) < 1) {
> 
> What's it doing grabbing the spinlock before sscanf() anyway?
> It's not loke any protected data is handed off to sscanf()...
> This should instead be:
> 
> 	if (sscanf(...)...)
> 		return -EINVAL
> 
> 	spin_lock_irqsave(...)
> 
> General rule of thumb:  don't grab spinlocks earlier than necessary.
> 

Agreed, will re-submit.

[PATCH 2/2] ARM: OMAP: MUSB: BUG on potential deadlocks

[Kevin Hilman]

[-- Attachment #1: musb-lock-debug.patch --]
[-- Type: text/plain, Size: 3458 bytes --]

Help track down locking bugs.

There are several places where locks are assumed to be held and are
unlocked.  BUG-out when the lock is not actually held.

Signed-off-by: Kevin Hilman <khilman@mvista.com>

---
 drivers/usb/musb/musb_gadget.c     |    7 +++++++
 drivers/usb/musb/musb_gadget_ep0.c |    4 ++++
 drivers/usb/musb/musb_host.c       |    1 +
 3 files changed, 12 insertions(+)

Index: dev/drivers/usb/musb/musb_gadget.c
===================================================================
--- dev.orig/drivers/usb/musb/musb_gadget.c
+++ dev/drivers/usb/musb/musb_gadget.c
@@ -118,6 +118,7 @@ __acquires(ep->musb->lock)
 	musb = req->musb;
 
 	ep->busy = 1;
+	BUG_ON(!spin_is_locked(&musb->lock));
 	spin_unlock(&musb->lock);
 	if (is_dma_capable()) {
 		if (req->mapped) {
@@ -1812,6 +1813,7 @@ stop_activity(struct musb *musb, struct 
 			}
 		}
 
+		BUG_ON(!spin_is_locked(&musb->lock));
 		spin_unlock(&musb->lock);
 		driver->disconnect (&musb->g);
 		spin_lock(&musb->lock);
@@ -1851,6 +1853,8 @@ int usb_gadget_unregister_driver(struct 
 		stop_activity(musb, driver);
 
 		DBG(3, "unregistering driver %s\n", driver->function);
+
+		BUG_ON(!spin_is_locked(&musb->lock));
 		spin_unlock_irqrestore(&musb->lock, flags);
 		driver->unbind(&musb->g);
 		spin_lock_irqsave(&musb->lock, flags);
@@ -1891,6 +1895,7 @@ void musb_g_resume(struct musb *musb)
 	case OTG_STATE_B_PERIPHERAL:
 		musb->is_active = 1;
 		if (musb->gadget_driver && musb->gadget_driver->resume) {
+			BUG_ON(!spin_is_locked(&musb->lock));
 			spin_unlock(&musb->lock);
 			musb->gadget_driver->resume(&musb->g);
 			spin_lock(&musb->lock);
@@ -1918,6 +1923,7 @@ void musb_g_suspend(struct musb *musb)
 	case OTG_STATE_B_PERIPHERAL:
 		musb->is_suspended = 1;
 		if (musb->gadget_driver && musb->gadget_driver->suspend) {
+			BUG_ON(!spin_is_locked(&musb->lock));
 			spin_unlock(&musb->lock);
 			musb->gadget_driver->suspend(&musb->g);
 			spin_lock(&musb->lock);
@@ -1954,6 +1960,7 @@ void musb_g_disconnect(struct musb *musb
 
 	musb->g.speed = USB_SPEED_UNKNOWN;
 	if (musb->gadget_driver && musb->gadget_driver->disconnect) {
+		BUG_ON(!spin_is_locked(&musb->lock));
 		spin_unlock(&musb->lock);
 		musb->gadget_driver->disconnect(&musb->g);
 		spin_lock(&musb->lock);
Index: dev/drivers/usb/musb/musb_gadget_ep0.c
===================================================================
--- dev.orig/drivers/usb/musb/musb_gadget_ep0.c
+++ dev/drivers/usb/musb/musb_gadget_ep0.c
@@ -273,7 +273,9 @@ __acquires(musb->lock)
 				if (!musb_ep->desc)
 					break;
 
+
 				/* REVISIT do it directly, no locking games */
+				BUG_ON(!spin_is_locked(&musb->lock));
 				spin_unlock(&musb->lock);
 				musb_gadget_set_halt(&musb_ep->end_point, 0);
 				spin_lock(&musb->lock);
@@ -586,6 +588,8 @@ __acquires(musb->lock)
 	int retval;
 	if (!musb->gadget_driver)
 		return -EOPNOTSUPP;
+
+	BUG_ON(!spin_is_locked(&musb->lock));
 	spin_unlock(&musb->lock);
 	retval = musb->gadget_driver->setup(&musb->g, ctrlrequest);
 	spin_lock(&musb->lock);
Index: dev/drivers/usb/musb/musb_host.c
===================================================================
--- dev.orig/drivers/usb/musb/musb_host.c
+++ dev/drivers/usb/musb/musb_host.c
@@ -305,6 +305,7 @@ __acquires(musb->lock)
 			urb->actual_length, urb->transfer_buffer_length
 			);
 
+	BUG_ON(!spin_is_locked(&musb->lock));
 	spin_unlock(&musb->lock);
 	usb_hcd_giveback_urb(musb_to_hcd(musb), urb);
 	spin_lock(&musb->lock);

--

Re: [PATCH 2/2] ARM: OMAP: MUSB: BUG on potential deadlocks

[David Brownell]

> There are several places where locks are assumed to be held and are
> unlocked.  BUG-out when the lock is not actually held.

This isn't a good patch.  *ADDING* a BUG() is wrong, and that's
especially true for things like spin_is_locked() ... which has
historically lied, and in any case doesn't reflect the requirement
that the lock holder be the current execution context instead of
some other context.  And if it

And unfortunately all of these use spin_is_locked()...

Also:


> --- dev.orig/drivers/usb/musb/musb_gadget.c
> +++ dev/drivers/usb/musb/musb_gadget.c
> @@ -118,6 +118,7 @@ __acquires(ep->musb->lock)
                     ^^^^^^^^^^^
Functions with "sparse" annotations get static checking from that
tool.  I'm not sure how much I trust it, but it *has* reported a
few locking bugs.  Currently, "-Wcontext" is the default, and this
whole driver passes "sparse" fine (other than a few warnings bout
not understanding CPP).

Several a few of these are in functions with the "sparse" annotations,
since they need to do things like temporarily dropping the spinlock so
a callback could reenter the driver.


It'd be much better to actually submit fixes for any locking bugs
than to add BUG_ON() statements.

- Dave

Re: [PATCH 2/2] ARM: OMAP: MUSB: BUG on potential deadlocks

[Kevin Hilman]

David Brownell wrote:
>> There are several places where locks are assumed to be held and are
>> unlocked.  BUG-out when the lock is not actually held.
> 
> This isn't a good patch.  *ADDING* a BUG() is wrong, and that's
> especially true for things like spin_is_locked() 

I probably should've made this an RFC instead of a patch.  I meant for
this to be a debug aid, not to actually go into the tree.  I'm using
this to try to track down some deadlocks and thought it may be useful to
others.  It has already turned up a couple problems (see below.)

>> --- dev.orig/drivers/usb/musb/musb_gadget.c
>> +++ dev/drivers/usb/musb/musb_gadget.c
>> @@ -118,6 +118,7 @@ __acquires(ep->musb->lock)
>                      ^^^^^^^^^^^
> Functions with "sparse" annotations get static checking from that
> tool.  I'm not sure how much I trust it, but it *has* reported a
> few locking bugs.  Currently, "-Wcontext" is the default, and this
> whole driver passes "sparse" fine (other than a few warnings bout
> not understanding CPP).
>
> Several a few of these are in functions with the "sparse" annotations,
> since they need to do things like temporarily dropping the spinlock so
> a callback could reenter the driver.

OK, I was wondering what those annotations were used for.  I hadn't used
sparse for this before.  Several of the functions which drop and re-take
the lock are not annotated.  I'll add annotations and see if sparse
turns up any more problems.

> It'd be much better to actually submit fixes for any locking bugs
> than to add BUG_ON() statements.

I agree, and I'm working on the fixes.  Just not sure I have the right
fix yet.

For exaple, I've found a couple paths where the BUG_ON() has definitely
turned up a problem:

dma_controller_irq
 musb_dma_completion
  musb_g_rx  (or musb_host_rx in host mode)
   musb_g_giveback (musb_advance_schedule --> musb_giveback)

In both host and gadget cases, the giveback function is entered without
the lock being held and tries to unlock the lock.  The comment in
musb_dma_completion says that the lock is held, but it's not when called
from the DMA IRQ ( but is when called from davinci_irq via
cppi_completion.)

I've tried putting a lock around this in a few ways, and can get gadget
to work without a deadlock, but doing so causes host mode to stop
detecting devices.  I'm still trying to figure out why.

Kevin

Re: [PATCH 2/2] ARM: OMAP: MUSB: BUG on potential deadlocks

[David Brownell]

> > This isn't a good patch.  *ADDING* a BUG() is wrong, and that's
> > especially true for things like spin_is_locked() 
>
> I probably should've made this an RFC instead of a patch.  I meant for
> this to be a debug aid, not to actually go into the tree.  I'm using
> this to try to track down some deadlocks and thought it may be useful to
> others.  It has already turned up a couple problems (see below.)

OK, that's better.  As I presume you've deduced, a lot of the recent
cleanup effort in this driver is so we can try pushing it upstream
for 2.6.24 ... abuse of BUG() wouldn't help that!


> > It'd be much better to actually submit fixes for any locking bugs
> > than to add BUG_ON() statements.
>
> I agree, and I'm working on the fixes.  Just not sure I have the right
> fix yet.
>
> For exaple, I've found a couple paths where the BUG_ON() has definitely
> turned up a problem:
>
> dma_controller_irq
>  musb_dma_completion
>   musb_g_rx  (or musb_host_rx in host mode)
>    musb_g_giveback (musb_advance_schedule --> musb_giveback)
>
> In both host and gadget cases, the giveback function is entered without
> the lock being held and tries to unlock the lock.  The comment in
> musb_dma_completion says that the lock is held, but it's not when called
> from the DMA IRQ ( but is when called from davinci_irq via
> cppi_completion.)

I presume you mean the OMAP2430 support?  Looked to me like TUSB6010
got that OK too.  That code for Mentor's DMA engine still has obvious
bugs; and I don't know that it's ever had more than basic cleanups.


> I've tried putting a lock around this in a few ways, and can get gadget
> to work without a deadlock, but doing so causes host mode to stop
> detecting devices.  I'm still trying to figure out why.

And more power to you!  I've not looked at that particular issue.

- Dave

Re: [PATCH 2/2] ARM: OMAP: MUSB: BUG on potential deadlocks

[Kevin Hilman]

David Brownell wrote:
>>> This isn't a good patch.  *ADDING* a BUG() is wrong, and that's
>>> especially true for things like spin_is_locked() 
>> I probably should've made this an RFC instead of a patch.  I meant for
>> this to be a debug aid, not to actually go into the tree.  I'm using
>> this to try to track down some deadlocks and thought it may be useful to
>> others.  It has already turned up a couple problems (see below.)
> 
> OK, that's better.  As I presume you've deduced, a lot of the recent
> cleanup effort in this driver is so we can try pushing it upstream
> for 2.6.24 ... abuse of BUG() wouldn't help that!
> 
> 
>>> It'd be much better to actually submit fixes for any locking bugs
>>> than to add BUG_ON() statements.
>> I agree, and I'm working on the fixes.  Just not sure I have the right
>> fix yet.
>>
>> For exaple, I've found a couple paths where the BUG_ON() has definitely
>> turned up a problem:
>>
>> dma_controller_irq
>>  musb_dma_completion
>>   musb_g_rx  (or musb_host_rx in host mode)
>>    musb_g_giveback (musb_advance_schedule --> musb_giveback)
>>
>> In both host and gadget cases, the giveback function is entered without
>> the lock being held and tries to unlock the lock.  The comment in
>> musb_dma_completion says that the lock is held, but it's not when called
>> from the DMA IRQ ( but is when called from davinci_irq via
>> cppi_completion.)
> 
> I presume you mean the OMAP2430 support?  Looked to me like TUSB6010
> got that OK too.  That code for Mentor's DMA engine still has obvious
> bugs; and I don't know that it's ever had more than basic cleanups.

Yes, this is 2430.

So is nobody else trying to use the Mentor's DMA engine?  I know it's
typically disabled on DaVinci, but is it used on tusb?

Kevin

Re: [PATCH 2/2] ARM: OMAP: MUSB: BUG on potential deadlocks

[David Brownell]

> >> dma_controller_irq
> >>  musb_dma_completion
> >>   musb_g_rx  (or musb_host_rx in host mode)
> >>    musb_g_giveback (musb_advance_schedule --> musb_giveback)
> >>
> >> In both host and gadget cases, the giveback function is entered without
> >> the lock being held and tries to unlock the lock.  The comment in
> >> musb_dma_completion says that the lock is held, but it's not when called
> >> from the DMA IRQ ( but is when called from davinci_irq via
> >> cppi_completion.)
> > 
> > I presume you mean the OMAP2430 support?  Looked to me like TUSB6010
> > got that OK too.  That code for Mentor's DMA engine still has obvious
> > bugs; and I don't know that it's ever had more than basic cleanups.
>
> Yes, this is 2430.
>
> So is nobody else trying to use the Mentor's DMA engine?

It's not available on most other TI silicon.  As I understand it,
Mentor licenses their IP "a la carte" with their DMA being one of
the optional parts.  (Ditto the capability of working through an
external hub ...)


> I know it's typically disabled on DaVinci, but is it used on tusb?

Nope.  "Disabled" being the wrong word; "unavailable" (see above).


>From what I can tell, the TUSB60x0 team took the DaVinci design and
moved it into a discrete chip.  They upgraded the RTL for the MHDRC
core (getting bugfixes).  The 6010 chip uses a relatively conventional
external DMA interface, although it's mostly broken on the first revision
(only one channel works).  The 6020 uses VLYNQ and CPPI ... I can hope
they fixed some of the broken-as-designed aspects of CPPI; that DMA
interface is really counterproductive for most USB cases.

- Dave

Re: [PATCH 0/2] MUSB: tracking down locking bugs

[David Brownell]

> With the help of the lock debugging features of the -rt patch, I'm
> trying to track down locking issues in the MUSB driver.

It was last done about 18 months ago, so it's clearly Time to
check those locks again.  :)

Re: [PATCH 0/2] MUSB: tracking down locking bugs

[Kevin Hilman]

David Brownell wrote:
>> With the help of the lock debugging features of the -rt patch, I'm
>> trying to track down locking issues in the MUSB driver.
> 
> It was last done about 18 months ago, so it's clearly Time to
> check those locks again.  :)

Agreed.

In the short term, I'll have a couple more "band-aid" style patches for
you to sanity check.  They fix a couple deadlocks uncovered by my
'BUG_ON' patch, but since I don't know this driver all that well, there
may be better ways to do things.

For the long term, more lock granularity should probably be used.  For
example, currently the entire interrupt handler wrapped with a
spin_lock_irqsave().  When moving to the threaded interrupts of
PREEMPT_RT, this effictively negates the benefit of having threaded
handlers in the first place.

Kevin

Re: [PATCH 0/2] MUSB: tracking down locking bugs

[David Brownell]

> >> With the help of the lock debugging features of the -rt patch, I'm
> >> trying to track down locking issues in the MUSB driver.
> > 
> > It was last done about 18 months ago, so it's clearly Time to
> > check those locks again.  :)
>
> Agreed.
>
> In the short term, I'll have a couple more "band-aid" style patches for
> you to sanity check.  They fix a couple deadlocks uncovered by my
> 'BUG_ON' patch, but since I don't know this driver all that well, there
> may be better ways to do things.

The simple solution is to just analyse the call paths.  Remember too
that a BUG_ON(f(spin_is_locked(L))) is likely to be a false alarm...

Also, nowadays I run most things with lockdep, which helps establish
that there can't be *that* many locking bugs.


> For the long term, more lock granularity should probably be used.  For
> example, currently the entire interrupt handler wrapped with a
> spin_lock_irqsave().  When moving to the threaded interrupts of
> PREEMPT_RT, this effictively negates the benefit of having threaded
> handlers in the first place.

I've heard that conventional wisdom -- conventional to advocates
of threaded IRQ handlers, that is! -- but I don't exactly see how
it would apply in cases like this.

Having multiple execution contexts trip over each others' accesses
to registers or data structures isn't a way to have a safe driver.

- Dave