[PATCH] usb: musb: dsps: kill OTG timer on suspend

Discussion:

Felipe Balbi

2014-09-15 14:39:09 UTC

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: <***@vger.kernel.org> # v3.14+
Fixes 869c597 (usb: musb: dsps: add support for suspend and resume)
Tested-by: Dave Gerlach <d-***@ti.com>
Signed-off-by: Felipe 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(+)

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;
}

--
2.0.1.563.g66f467c

Sebastian Andrzej Siewior

2014-10-13 09:13:40 UTC

Permalink

Post by Felipe Balbi
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?

Post by Felipe Balbi
--- 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.

Post by Felipe Balbi
return 0;
}

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

Felipe Balbi

2014-10-13 22:41:50 UTC

Permalink

Post by Sebastian Andrzej Siewior

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! */

Post by Sebastian Andrzej Siewior

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 ?

--
balbi

Sebastian Andrzej Siewior

2014-10-14 08:10:08 UTC

Permalink

Post by Felipe Balbi

Post by Sebastian Andrzej Siewior
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?

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 :)

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

yes.

Post by Felipe Balbi

Post by Sebastian Andrzej Siewior
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-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html