| Message ID | 20120813123453.4cba14ca@notabene.brown (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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...
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
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.
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;