usb: musb: dsps: kill OTG timer on suspend

Message ID 1410791949-32460-1-git-send-email-balbi@ti.com
State Accepted
Commit 468bcc2a2ca071f652009d2d20d97f2437630cae
Headers show

Commit Message

Felipe Balbi Sept. 15, 2014, 2:39 p.m.
if we don't make sure to kill the timer, it could
expire after we have already gated our clocks.

That will trigger a Data Abort exception because
we would try to access register while clock is gated.

Fix that bug.

Cc: <stable@vger.kernel.org> # v3.14+
Fixes 869c597 (usb: musb: dsps: add support for suspend and resume)
Tested-by: Dave Gerlach <d-gerlach@ti.com>
Signed-off-by: Felipe Balbi <balbi@ti.com>
---

I'll send this one together with my v3.18 pull.

 drivers/usb/musb/musb_dsps.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Sebastian Andrzej Siewior Oct. 13, 2014, 9:13 a.m. | #1
On 2014-09-15 09:39:09 [-0500], Felipe Balbi wrote:
> if we don't make sure to kill the timer, it could
> expire after we have already gated our clocks.
> 
> That will trigger a Data Abort exception because
> we would try to access register while clock is gated.

That timer deserves to be killed because nobody wants it to wakeup the
system from suspend. However the Data Abort wouldn't happen if the timer
would use pm_runtime_get_sync() right?
 
> --- a/drivers/usb/musb/musb_dsps.c
> +++ b/drivers/usb/musb/musb_dsps.c
> @@ -870,6 +870,7 @@ static int dsps_suspend(struct device *dev)
>  	struct musb *musb = platform_get_drvdata(glue->musb);
>  	void __iomem *mbase = musb->ctrl_base;
>  
> +	del_timer_sync(&glue->timer);
>  	glue->context.control = dsps_readl(mbase, wrp->control);
>  	glue->context.epintr = dsps_readl(mbase, wrp->epintr_set);
>  	glue->context.coreintr = dsps_readl(mbase, wrp->coreintr_set);
> @@ -895,6 +896,7 @@ static int dsps_resume(struct device *dev)
>  	dsps_writel(mbase, wrp->mode, glue->context.mode);
>  	dsps_writel(mbase, wrp->tx_mode, glue->context.tx_mode);
>  	dsps_writel(mbase, wrp->rx_mode, glue->context.rx_mode);
> +	setup_timer(&glue->timer, otg_timer, (unsigned long) musb);

You need a mod_timer() here instead. I will send a patch in a few.

>  	return 0;
>  }

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felipe Balbi Oct. 13, 2014, 10:41 p.m. | #2
On Mon, Oct 13, 2014 at 11:13:40AM +0200, Sebastian Andrzej Siewior wrote:
> On 2014-09-15 09:39:09 [-0500], Felipe Balbi wrote:
> > if we don't make sure to kill the timer, it could
> > expire after we have already gated our clocks.
> > 
> > That will trigger a Data Abort exception because
> > we would try to access register while clock is gated.
> 
> That timer deserves to be killed because nobody wants it to wakeup the
> system from suspend. However the Data Abort wouldn't happen if the timer
> would use pm_runtime_get_sync() right?

correct, but we want to suspend ;-) there is a race here:

mod_timer();

/* before mod_timer() expires */
"echo mem > /sys/power/suspend"

->runtime_suspend()
/* clocks are now gated */

/* mod_timer() expires and BOOM! */

> > --- a/drivers/usb/musb/musb_dsps.c
> > +++ b/drivers/usb/musb/musb_dsps.c
> > @@ -870,6 +870,7 @@ static int dsps_suspend(struct device *dev)
> >  	struct musb *musb = platform_get_drvdata(glue->musb);
> >  	void __iomem *mbase = musb->ctrl_base;
> >  
> > +	del_timer_sync(&glue->timer);
> >  	glue->context.control = dsps_readl(mbase, wrp->control);
> >  	glue->context.epintr = dsps_readl(mbase, wrp->epintr_set);
> >  	glue->context.coreintr = dsps_readl(mbase, wrp->coreintr_set);
> > @@ -895,6 +896,7 @@ static int dsps_resume(struct device *dev)
> >  	dsps_writel(mbase, wrp->mode, glue->context.mode);
> >  	dsps_writel(mbase, wrp->tx_mode, glue->context.tx_mode);
> >  	dsps_writel(mbase, wrp->rx_mode, glue->context.rx_mode);
> > +	setup_timer(&glue->timer, otg_timer, (unsigned long) musb);
> 
> You need a mod_timer() here instead. I will send a patch in a few.

yeah, I was confused if it should be setup_timer() or mod_timer(). Since
I deleted the timer on suspend, I thought setup_timer() was more
appropriate, no ?
Sebastian Andrzej Siewior Oct. 14, 2014, 8:10 a.m. | #3
On 2014-10-13 17:41:50 [-0500], Felipe Balbi wrote:
> > That timer deserves to be killed because nobody wants it to wakeup the
> > system from suspend. However the Data Abort wouldn't happen if the timer
> > would use pm_runtime_get_sync() right?
> 
> correct, but we want to suspend ;-) there is a race here:
No doubt about that but I was confused that the timer routine didn't do
pm_get_sync() at all. Usually this done before every register access so
I was just asking if this piece should be added or if it is not required
at all. But then I don't see any pm_get_sync() in URB enqueue for
instance so there might be a different strategy :)

> mod_timer();
> 
> /* before mod_timer() expires */
> "echo mem > /sys/power/suspend"
> 
> ->runtime_suspend()
> /* clocks are now gated */
> 
> /* mod_timer() expires and BOOM! */

yes.

> > You need a mod_timer() here instead. I will send a patch in a few.
> 
> yeah, I was confused if it should be setup_timer() or mod_timer(). Since
> I deleted the timer on suspend, I thought setup_timer() was more
> appropriate, no ?
Look for
	"[PATCH] usb: musb: dsps: start OTG timer on resume again"

setup_timer() does nothing but assign the private data argument.
mod_timer() enqueues the timer back into the timer wheel which is the
reverse of del_timer(). On top of that you should check if you we want
to the enqueue timer at all (you don't want this in HOSTMODE or if it is
not in OTG mode).

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
index c791ba5..154bcf1 100644
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -870,6 +870,7 @@  static int dsps_suspend(struct device *dev)
 	struct musb *musb = platform_get_drvdata(glue->musb);
 	void __iomem *mbase = musb->ctrl_base;
 
+	del_timer_sync(&glue->timer);
 	glue->context.control = dsps_readl(mbase, wrp->control);
 	glue->context.epintr = dsps_readl(mbase, wrp->epintr_set);
 	glue->context.coreintr = dsps_readl(mbase, wrp->coreintr_set);
@@ -895,6 +896,7 @@  static int dsps_resume(struct device *dev)
 	dsps_writel(mbase, wrp->mode, glue->context.mode);
 	dsps_writel(mbase, wrp->tx_mode, glue->context.tx_mode);
 	dsps_writel(mbase, wrp->rx_mode, glue->context.rx_mode);
+	setup_timer(&glue->timer, otg_timer, (unsigned long) musb);
 
 	return 0;
 }