| Message ID | 1410791949-32460-1-git-send-email-balbi@ti.com |
|---|---|
| State | Accepted |
| Commit | 468bcc2a2ca071f652009d2d20d97f2437630cae |
| Headers | show |
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
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 ?
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
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; }