[PATCH v2 0/2] USB: musb: fix external abort on suspend

[PATCH v2 0/2] USB: musb: fix external abort on suspend

[Johan Hovold]

This is a v2 of the fix for an external abort during suspend which I submitted
on Monday.

The commit message of patch 1/2 has only been slightly reworded, while
patch 2/2 is new.

Johan


Johan Hovold (2):
  USB: musb: fix external abort on suspend
  USB: musb: dsps: add explicit runtime resume at suspend

 drivers/usb/musb/musb_core.c | 18 ++++++++++--------
 drivers/usb/musb/musb_dsps.c | 13 +++++++++++--
 2 files changed, 21 insertions(+), 10 deletions(-)

-- 
2.13.3

[PATCH v2 1/2] USB: musb: fix external abort on suspend

[Johan Hovold]

Make sure that the controller is runtime resumed when system suspending
to avoid an external abort when accessing the interrupt registers:

  Unhandled fault: external abort on non-linefetch (0x1008) at 0xd025840a
  ...
  [<c05481a4>] (musb_default_readb) from [<c0545abc>] (musb_disable_interrupts+0x84/0xa8)
  [<c0545abc>] (musb_disable_interrupts) from [<c0546b08>] (musb_suspend+0x38/0xb8)
  [<c0546b08>] (musb_suspend) from [<c04a57f8>] (platform_pm_suspend+0x3c/0x64)

This is easily reproduced on a BBB by enabling the peripheral port only
(as the host port may enable the shared clock) and keeping it
disconnected so that the controller is runtime suspended. (Well, you
would also need to the not-yet-merged am33xx-suspend patches by Dave
Gerlach to be able to suspend the BBB.)

This is a regression that was introduced by commit 1c4d0b4e1806 ("usb:
musb: Remove pm_runtime_set_irq_safe") which allowed the parent glue
device to runtime suspend and thereby exposed a couple of older issues:

Register accesses without explicitly making sure the controller is
runtime resumed during suspend was first introduced by commit
c338412b5ded ("usb: musb: unconditionally save and restore the context
on suspend") in 3.14.

Commit a1fc1920aaaa ("usb: musb: core: make sure musb is in RPM_ACTIVE on
resume") later started setting the RPM status to active during resume,
and this was also implicitly relying on the parent always being active.
Since commit 71723f95463d ("PM / runtime: print error when activating a
child to unactive parent") this now also results in the following
warning:

  musb-hdrc musb-hdrc.0: runtime PM trying to activate child device
    musb-hdrc.0 but parent (47401400.usb) is not active

This patch has been verified on 4.13-rc2, 4.12 and 4.9 using a BBB
(the dsps glue would always be active also in 4.8).

Fixes: c338412b5ded ("usb: musb: unconditionally save and restore the context on suspend")
Fixes: a1fc1920aaaa ("usb: musb: core: make sure musb is in RPM_ACTIVE on resume")
Fixes: 1c4d0b4e1806 ("usb: musb: Remove pm_runtime_set_irq_safe")
Cc: stable <stable@vger.kernel.org>	# 4.8
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Daniel Mack <zonque@gmail.com>
Cc: Dave Gerlach <d-gerlach@ti.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Tony Lindgren <tony@atomide.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/musb/musb_core.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index 87cbd56cc761..b67692857daf 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -2671,6 +2671,13 @@ static int musb_suspend(struct device *dev)
 {
 	struct musb	*musb = dev_to_musb(dev);
 	unsigned long	flags;
+	int ret;
+
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(dev);
+		return ret;
+	}
 
 	musb_platform_disable(musb);
 	musb_disable_interrupts(musb);
@@ -2721,14 +2728,6 @@ static int musb_resume(struct device *dev)
 	if ((devctl & mask) != (musb->context.devctl & mask))
 		musb->port1_status = 0;
 
-	/*
-	 * The USB HUB code expects the device to be in RPM_ACTIVE once it came
-	 * out of suspend
-	 */
-	pm_runtime_disable(dev);
-	pm_runtime_set_active(dev);
-	pm_runtime_enable(dev);
-
 	musb_start(musb);
 
 	spin_lock_irqsave(&musb->lock, flags);
@@ -2738,6 +2737,9 @@ static int musb_resume(struct device *dev)
 			error);
 	spin_unlock_irqrestore(&musb->lock, flags);
 
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
 	return 0;
 }
 
-- 
2.13.3

[PATCH v2 2/2] USB: musb: dsps: add explicit runtime resume at suspend

[Johan Hovold]

The musb_dsps driver is special in that the parent (glue) device's
driver is accessing registers mapped by the child. The clock is however
shared and is managed by the grandparent device.

Since commit 869c59782981 ("usb: musb: dsps: add support for suspend and
resume") the dsps driver has been accessing these registers as part of
suspend and resume.

The parent driver obviously cannot runtime resume the child during
system suspend and is currently relying on the fact that the child will
be RPM_ACTIVE throughout suspend. The suspend implementation also makes
sure to check that the child is indeed present (and hence the clock
enabled) before accessing the registers.

Let's add an explicit runtime resume of the glue device itself to enable the
clock before doing the register accesses in case these assumptions ever change
(i.e. if the child is left runtime suspended).

Note that the glue-timer cancellation is moved after the child-presence
check to keep error handling simple. This should be fine as the timer is
not setup until the controller is being registered and at that time
glue->musb and its driver data have already been initialised.

Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Daniel Mack <zonque@gmail.com>
Cc: Tony Lindgren <tony@atomide.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/musb/musb_dsps.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
index bc6a9be2ccc5..f6b526606ad1 100644
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -1015,13 +1015,20 @@ static int dsps_suspend(struct device *dev)
 	const struct dsps_musb_wrapper *wrp = glue->wrp;
 	struct musb *musb = platform_get_drvdata(glue->musb);
 	void __iomem *mbase;
-
-	del_timer_sync(&glue->timer);
+	int ret;
 
 	if (!musb)
 		/* This can happen if the musb device is in -EPROBE_DEFER */
 		return 0;
 
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(dev);
+		return ret;
+	}
+
+	del_timer_sync(&glue->timer);
+
 	mbase = musb->ctrl_base;
 	glue->context.control = musb_readl(mbase, wrp->control);
 	glue->context.epintr = musb_readl(mbase, wrp->epintr_set);
@@ -1060,6 +1067,8 @@ static int dsps_resume(struct device *dev)
 	    musb->port_mode == MUSB_PORT_MODE_DUAL_ROLE)
 		dsps_mod_timer(glue, -1);
 
+	pm_runtime_put(dev);
+
 	return 0;
 }
 #endif
-- 
2.13.3