Infinite looping in omap2430.c USB driver

Infinite looping in omap2430.c USB driver

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2505 bytes --]


Hello `./scripts/get_maintainer.pl -f drivers/usb/musb/omap2430.c`

omap2430_musb_set_vbus in omap2430.c contains:

			while (musb_readb(musb->mregs, MUSB_DEVCTL) & 0x80) {

				cpu_relax();

				if (time_after(jiffies, timeout)) {
					dev_err(musb->controller,
					"configured as A device timeout");
					ret = -EINVAL;
					break;
				}
			}

having set
	unsigned long timeout = jiffies + msecs_to_jiffies(1000);

so it can busy-loop for up to 1 second.  Probably not ideal, but if it works
I wouldn't complain.

The
	if (int_usb & MUSB_INTR_SESSREQ) {
branch of musb_stage0_irq() called from musb_interrupt (from
generic_interrupt) calls this:

	if (musb->int_usb)
		retval |= musb_stage0_irq(musb, musb->int_usb,
				devctl, power);

so the busy loop can happen in an interrupt handler (not a threaded interrupt
handler), which is probably less ideal.

However this can be called with interrupt disabled, as happens at least
during resume when resume_irqs() calls:

		raw_spin_lock_irqsave(&desc->lock, flags);
		__enable_irq(desc, irq, true);
		raw_spin_unlock_irqrestore(&desc->lock, flags);

and an interrupt is found to be IRQS_PENDING.

In this case interrupts are disabled so 'jiffies' never changes so this loop
can continue forever.

This happens on my (GTA04) phone fairly regularly - between 1 in 10 and 1 in
30 resumes. The musb-hdrc interrupt is pending and reports

[ 4957.624176] musb-hdrc musb-hdrc: ** IRQ peripheral usb0040 tx0000 rx0000

'usb0040' is MUSB_INTR_SESSREQ.  I think this is triggered by detecting a
voltage change on the USB ID pin - is that right?  A short-to-earth would be
a request to switch to host mode, which is why it tries to enable VBUS.
Maybe there is some electrical noise which is being picked up?

In any case I get the interrupt despite nothing being plugged in, and the 0x80
bit of MUSB_DEVCTL never gets cleared.

I've added a simple loop counter which aborts the loop after 1000 loops -
this takes about 5 seconds, but includes some printks which probably slow it
down.

In 2 out of 2 cases, subsequent messages show that the hsmmc driver for the
uSD card that holds my root filesystem is messed up.  It seems to be waiting
for a request that is never going to complete.
So maybe the hsmmc is causing the noise that triggers the musb issue.

I can send a patch which add a loop count if you like, but I suspect you can
come up with a much better approach.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: Infinite looping in omap2430.c USB driver

[Tony Lindgren]

* NeilBrown <neilb@suse.de> [120706 15:44]:
> 
> Hello `./scripts/get_maintainer.pl -f drivers/usb/musb/omap2430.c`
> 
> omap2430_musb_set_vbus in omap2430.c contains:
> 
> 			while (musb_readb(musb->mregs, MUSB_DEVCTL) & 0x80) {
> 
> 				cpu_relax();
> 
> 				if (time_after(jiffies, timeout)) {
> 					dev_err(musb->controller,
> 					"configured as A device timeout");
> 					ret = -EINVAL;
> 					break;
> 				}
> 			}
> 
> having set
> 	unsigned long timeout = jiffies + msecs_to_jiffies(1000);
> 
> so it can busy-loop for up to 1 second.  Probably not ideal, but if it works
> I wouldn't complain.
> 
> The
> 	if (int_usb & MUSB_INTR_SESSREQ) {
> branch of musb_stage0_irq() called from musb_interrupt (from
> generic_interrupt) calls this:
> 
> 	if (musb->int_usb)
> 		retval |= musb_stage0_irq(musb, musb->int_usb,
> 				devctl, power);
> 
> so the busy loop can happen in an interrupt handler (not a threaded interrupt
> handler), which is probably less ideal.
> 
> However this can be called with interrupt disabled, as happens at least
> during resume when resume_irqs() calls:
> 
> 		raw_spin_lock_irqsave(&desc->lock, flags);
> 		__enable_irq(desc, irq, true);
> 		raw_spin_unlock_irqrestore(&desc->lock, flags);
> 
> and an interrupt is found to be IRQS_PENDING.
> 
> In this case interrupts are disabled so 'jiffies' never changes so this loop
> can continue forever.
> 
> This happens on my (GTA04) phone fairly regularly - between 1 in 10 and 1 in
> 30 resumes. The musb-hdrc interrupt is pending and reports
> 
> [ 4957.624176] musb-hdrc musb-hdrc: ** IRQ peripheral usb0040 tx0000 rx0000
> 
> 'usb0040' is MUSB_INTR_SESSREQ.  I think this is triggered by detecting a
> voltage change on the USB ID pin - is that right?  A short-to-earth would be
> a request to switch to host mode, which is why it tries to enable VBUS.
> Maybe there is some electrical noise which is being picked up?

I guess that could happen if the transceiver pins are floating during suspend?
 
> In any case I get the interrupt despite nothing being plugged in, and the 0x80
> bit of MUSB_DEVCTL never gets cleared.

As far as I remember, musb tries to be smart about changing to host mode,
and tries to do the session and vbus detection on it's own.. AFAIK, there's
nothing you can do until musb is done and detects the VBUS is not rising and
gives up. There are all kind of interrupt flag combinations trying to deal
with that mess, maybe you need to add yet another one?
 
> I've added a simple loop counter which aborts the loop after 1000 loops -
> this takes about 5 seconds, but includes some printks which probably slow it
> down.
> 
> In 2 out of 2 cases, subsequent messages show that the hsmmc driver for the
> uSD card that holds my root filesystem is messed up.  It seems to be waiting
> for a request that is never going to complete.
> So maybe the hsmmc is causing the noise that triggers the musb issue.
> 
> I can send a patch which add a loop count if you like, but I suspect you can
> come up with a much better approach.

Sounds like that loop should be fixed.

Regards,

Tony

Re: Infinite looping in omap2430.c USB driver

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3785 bytes --]


Hi Felipe,
 have you had a chance to look at this problem in omap2430_mbus_set_vbus yet?
Are you the person responsible?

thanks,
NeilBrown


On Mon, 9 Jul 2012 01:32:33 -0700 Tony Lindgren <tony@atomide.com> wrote:

> * NeilBrown <neilb@suse.de> [120706 15:44]:
> > 
> > Hello `./scripts/get_maintainer.pl -f drivers/usb/musb/omap2430.c`
> > 
> > omap2430_musb_set_vbus in omap2430.c contains:
> > 
> > 			while (musb_readb(musb->mregs, MUSB_DEVCTL) & 0x80) {
> > 
> > 				cpu_relax();
> > 
> > 				if (time_after(jiffies, timeout)) {
> > 					dev_err(musb->controller,
> > 					"configured as A device timeout");
> > 					ret = -EINVAL;
> > 					break;
> > 				}
> > 			}
> > 
> > having set
> > 	unsigned long timeout = jiffies + msecs_to_jiffies(1000);
> > 
> > so it can busy-loop for up to 1 second.  Probably not ideal, but if it works
> > I wouldn't complain.
> > 
> > The
> > 	if (int_usb & MUSB_INTR_SESSREQ) {
> > branch of musb_stage0_irq() called from musb_interrupt (from
> > generic_interrupt) calls this:
> > 
> > 	if (musb->int_usb)
> > 		retval |= musb_stage0_irq(musb, musb->int_usb,
> > 				devctl, power);
> > 
> > so the busy loop can happen in an interrupt handler (not a threaded interrupt
> > handler), which is probably less ideal.
> > 
> > However this can be called with interrupt disabled, as happens at least
> > during resume when resume_irqs() calls:
> > 
> > 		raw_spin_lock_irqsave(&desc->lock, flags);
> > 		__enable_irq(desc, irq, true);
> > 		raw_spin_unlock_irqrestore(&desc->lock, flags);
> > 
> > and an interrupt is found to be IRQS_PENDING.
> > 
> > In this case interrupts are disabled so 'jiffies' never changes so this loop
> > can continue forever.
> > 
> > This happens on my (GTA04) phone fairly regularly - between 1 in 10 and 1 in
> > 30 resumes. The musb-hdrc interrupt is pending and reports
> > 
> > [ 4957.624176] musb-hdrc musb-hdrc: ** IRQ peripheral usb0040 tx0000 rx0000
> > 
> > 'usb0040' is MUSB_INTR_SESSREQ.  I think this is triggered by detecting a
> > voltage change on the USB ID pin - is that right?  A short-to-earth would be
> > a request to switch to host mode, which is why it tries to enable VBUS.
> > Maybe there is some electrical noise which is being picked up?
> 
> I guess that could happen if the transceiver pins are floating during suspend?
>  
> > In any case I get the interrupt despite nothing being plugged in, and the 0x80
> > bit of MUSB_DEVCTL never gets cleared.
> 
> As far as I remember, musb tries to be smart about changing to host mode,
> and tries to do the session and vbus detection on it's own.. AFAIK, there's
> nothing you can do until musb is done and detects the VBUS is not rising and
> gives up. There are all kind of interrupt flag combinations trying to deal
> with that mess, maybe you need to add yet another one?
>  
> > I've added a simple loop counter which aborts the loop after 1000 loops -
> > this takes about 5 seconds, but includes some printks which probably slow it
> > down.
> > 
> > In 2 out of 2 cases, subsequent messages show that the hsmmc driver for the
> > uSD card that holds my root filesystem is messed up.  It seems to be waiting
> > for a request that is never going to complete.
> > So maybe the hsmmc is causing the noise that triggers the musb issue.
> > 
> > I can send a patch which add a loop count if you like, but I suspect you can
> > come up with a much better approach.
> 
> Sounds like that loop should be fixed.
> 
> Regards,
> 
> Tony
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: Infinite looping in omap2430.c USB driver

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 655 bytes --]

On Sat, Jul 07, 2012 at 08:39:49AM +1000, NeilBrown wrote:
> 
> Hello `./scripts/get_maintainer.pl -f drivers/usb/musb/omap2430.c`
> 
> omap2430_musb_set_vbus in omap2430.c contains:
> 
> 			while (musb_readb(musb->mregs, MUSB_DEVCTL) & 0x80) {
> 
> 				cpu_relax();
> 
> 				if (time_after(jiffies, timeout)) {
> 					dev_err(musb->controller,
> 					"configured as A device timeout");
> 					ret = -EINVAL;
> 					break;
> 				}
> 			}
> 
> having set
> 	unsigned long timeout = jiffies + msecs_to_jiffies(1000);

hehe, that's nasty. Please send a patch converting to a try count and a
udelay_range(), or something.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Infinite looping in omap2430.c USB driver

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1788 bytes --]

On Thu, 9 Aug 2012 14:15:51 +0300 Felipe Balbi <balbi@ti.com> wrote:


> hehe, that's nasty. Please send a patch converting to a try count and a
> udelay_range(), or something.
> 

how's this?

Thanks,
NeilBrown


From: NeilBrown <neilb@suse.de>
Date: Mon, 13 Aug 2012 12:32:58 +1000
Subject: [PATCH] omap2430: don't loop indefinitely in interrupt.

When called during resume_irqs, omap2430_musb_set_vbus() is run with
interrupts disabled,  In that case 'jiffies' never changes so the loop
can loop forever.

So impose a maximum loop count and add an 'mdelay' to ensure we wait
a reasonable amount of time for bit to be cleared.

This fixes a hang on resume.

Signed-of-by: NeilBrown <neilb@suse.de>

diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
index c7785e8..8a93381 100644
--- a/drivers/usb/musb/omap2430.c
+++ b/drivers/usb/musb/omap2430.c
@@ -34,6 +34,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/pm_runtime.h>
 #include <linux/err.h>
+#include <linux/delay.h>
 
 #include "musb_core.h"
 #include "omap2430.h"
@@ -145,6 +146,7 @@ static void omap2430_musb_set_vbus(struct musb *musb, int is_on)
 
 	if (is_on) {
 		if (musb->xceiv->state == OTG_STATE_A_IDLE) {
+			int loops = 100;
 			/* start the session */
 			devctl |= MUSB_DEVCTL_SESSION;
 			musb_writeb(musb->mregs, MUSB_DEVCTL, devctl);
@@ -154,9 +156,11 @@ static void omap2430_musb_set_vbus(struct musb *musb, int is_on)
 			 */
 			while (musb_readb(musb->mregs, MUSB_DEVCTL) & 0x80) {
 
+				mdelay(5);
 				cpu_relax();
 
-				if (time_after(jiffies, timeout)) {
+				if (time_after(jiffies, timeout)
+				    || loops-- <= 0) {
 					dev_err(musb->controller,
 					"configured as A device timeout");
 					ret = -EINVAL;

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: Infinite looping in omap2430.c USB driver

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1888 bytes --]

Hi,

On Mon, Aug 13, 2012 at 12:34:53PM +1000, NeilBrown wrote:
> On Thu, 9 Aug 2012 14:15:51 +0300 Felipe Balbi <balbi@ti.com> wrote:
> 
> 
> > hehe, that's nasty. Please send a patch converting to a try count and a
> > udelay_range(), or something.
> > 
> 
> how's this?
> 
> Thanks,
> NeilBrown
> 
> 
> From: NeilBrown <neilb@suse.de>
> Date: Mon, 13 Aug 2012 12:32:58 +1000
> Subject: [PATCH] omap2430: don't loop indefinitely in interrupt.
> 
> When called during resume_irqs, omap2430_musb_set_vbus() is run with
> interrupts disabled,  In that case 'jiffies' never changes so the loop
> can loop forever.
> 
> So impose a maximum loop count and add an 'mdelay' to ensure we wait
> a reasonable amount of time for bit to be cleared.
> 
> This fixes a hang on resume.
> 
> Signed-of-by: NeilBrown <neilb@suse.de>
> 
> diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
> index c7785e8..8a93381 100644
> --- a/drivers/usb/musb/omap2430.c
> +++ b/drivers/usb/musb/omap2430.c
> @@ -34,6 +34,7 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/err.h>
> +#include <linux/delay.h>
>  
>  #include "musb_core.h"
>  #include "omap2430.h"
> @@ -145,6 +146,7 @@ static void omap2430_musb_set_vbus(struct musb *musb, int is_on)
>  
>  	if (is_on) {
>  		if (musb->xceiv->state == OTG_STATE_A_IDLE) {
> +			int loops = 100;
>  			/* start the session */
>  			devctl |= MUSB_DEVCTL_SESSION;
>  			musb_writeb(musb->mregs, MUSB_DEVCTL, devctl);
> @@ -154,9 +156,11 @@ static void omap2430_musb_set_vbus(struct musb *musb, int is_on)
>  			 */
>  			while (musb_readb(musb->mregs, MUSB_DEVCTL) & 0x80) {
>  
> +				mdelay(5);

I would prefer udelay_range() as it will let scheduler group timers.
Something like:

udelay_range(3000, 5000);

should do, I gues...

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Infinite looping in omap2430.c USB driver

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2675 bytes --]

On Mon, 13 Aug 2012 17:32:34 +0300 Felipe Balbi <balbi@ti.com> wrote:

> Hi,
> 
> On Mon, Aug 13, 2012 at 12:34:53PM +1000, NeilBrown wrote:
> > On Thu, 9 Aug 2012 14:15:51 +0300 Felipe Balbi <balbi@ti.com> wrote:
> > 
> > 
> > > hehe, that's nasty. Please send a patch converting to a try count and a
> > > udelay_range(), or something.
> > > 
> > 
> > how's this?
> > 
> > Thanks,
> > NeilBrown
> > 
> > 
> > From: NeilBrown <neilb@suse.de>
> > Date: Mon, 13 Aug 2012 12:32:58 +1000
> > Subject: [PATCH] omap2430: don't loop indefinitely in interrupt.
> > 
> > When called during resume_irqs, omap2430_musb_set_vbus() is run with
> > interrupts disabled,  In that case 'jiffies' never changes so the loop
> > can loop forever.
> > 
> > So impose a maximum loop count and add an 'mdelay' to ensure we wait
> > a reasonable amount of time for bit to be cleared.
> > 
> > This fixes a hang on resume.
> > 
> > Signed-of-by: NeilBrown <neilb@suse.de>
> > 
> > diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
> > index c7785e8..8a93381 100644
> > --- a/drivers/usb/musb/omap2430.c
> > +++ b/drivers/usb/musb/omap2430.c
> > @@ -34,6 +34,7 @@
> >  #include <linux/dma-mapping.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/err.h>
> > +#include <linux/delay.h>
> >  
> >  #include "musb_core.h"
> >  #include "omap2430.h"
> > @@ -145,6 +146,7 @@ static void omap2430_musb_set_vbus(struct musb *musb, int is_on)
> >  
> >  	if (is_on) {
> >  		if (musb->xceiv->state == OTG_STATE_A_IDLE) {
> > +			int loops = 100;
> >  			/* start the session */
> >  			devctl |= MUSB_DEVCTL_SESSION;
> >  			musb_writeb(musb->mregs, MUSB_DEVCTL, devctl);
> > @@ -154,9 +156,11 @@ static void omap2430_musb_set_vbus(struct musb *musb, int is_on)
> >  			 */
> >  			while (musb_readb(musb->mregs, MUSB_DEVCTL) & 0x80) {
> >  
> > +				mdelay(5);
> 
> I would prefer udelay_range() as it will let scheduler group timers.
> Something like:
> 
> udelay_range(3000, 5000);
> 
> should do, I gues...
> 

Except that there is no udelay_range :-(
There is a usleep_range, but that can only be used from non-atomic context
and in the problem case interrupts are disabled and a spinlock is held so we
very definitely are not in non-atomic context.  If we need a delay at all, it
has to be udelay or mdelay.
If we could do this in a work function rather than directly from the
interrupt handler that would be best but I have no idea what dependencies
there are..  Would it be safe for musb_stage0_irq() to ask a workqueue to run
musb_platform_set_vbus rather than doing it directly?

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: Infinite looping in omap2430.c USB driver

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2560 bytes --]

On Tue, Aug 14, 2012 at 07:46:59AM +1000, NeilBrown wrote:
> On Mon, 13 Aug 2012 17:32:34 +0300 Felipe Balbi <balbi@ti.com> wrote:
> 
> > Hi,
> > 
> > On Mon, Aug 13, 2012 at 12:34:53PM +1000, NeilBrown wrote:
> > > On Thu, 9 Aug 2012 14:15:51 +0300 Felipe Balbi <balbi@ti.com> wrote:
> > > 
> > > 
> > > > hehe, that's nasty. Please send a patch converting to a try count and a
> > > > udelay_range(), or something.
> > > > 
> > > 
> > > how's this?
> > > 
> > > Thanks,
> > > NeilBrown
> > > 
> > > 
> > > From: NeilBrown <neilb@suse.de>
> > > Date: Mon, 13 Aug 2012 12:32:58 +1000
> > > Subject: [PATCH] omap2430: don't loop indefinitely in interrupt.
> > > 
> > > When called during resume_irqs, omap2430_musb_set_vbus() is run with
> > > interrupts disabled,  In that case 'jiffies' never changes so the loop
> > > can loop forever.
> > > 
> > > So impose a maximum loop count and add an 'mdelay' to ensure we wait
> > > a reasonable amount of time for bit to be cleared.
> > > 
> > > This fixes a hang on resume.
> > > 
> > > Signed-of-by: NeilBrown <neilb@suse.de>
> > > 
> > > diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
> > > index c7785e8..8a93381 100644
> > > --- a/drivers/usb/musb/omap2430.c
> > > +++ b/drivers/usb/musb/omap2430.c
> > > @@ -34,6 +34,7 @@
> > >  #include <linux/dma-mapping.h>
> > >  #include <linux/pm_runtime.h>
> > >  #include <linux/err.h>
> > > +#include <linux/delay.h>
> > >  
> > >  #include "musb_core.h"
> > >  #include "omap2430.h"
> > > @@ -145,6 +146,7 @@ static void omap2430_musb_set_vbus(struct musb *musb, int is_on)
> > >  
> > >  	if (is_on) {
> > >  		if (musb->xceiv->state == OTG_STATE_A_IDLE) {
> > > +			int loops = 100;
> > >  			/* start the session */
> > >  			devctl |= MUSB_DEVCTL_SESSION;
> > >  			musb_writeb(musb->mregs, MUSB_DEVCTL, devctl);
> > > @@ -154,9 +156,11 @@ static void omap2430_musb_set_vbus(struct musb *musb, int is_on)
> > >  			 */
> > >  			while (musb_readb(musb->mregs, MUSB_DEVCTL) & 0x80) {
> > >  
> > > +				mdelay(5);
> > 
> > I would prefer udelay_range() as it will let scheduler group timers.
> > Something like:
> > 
> > udelay_range(3000, 5000);
> > 
> > should do, I gues...
> > 
> 
> Except that there is no udelay_range :-(
> There is a usleep_range, but that can only be used from non-atomic context
> and in the problem case interrupts are disabled and a spinlock is held so we

my bad. Got confused. I will apply the original patch then. Thanks.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]