[PATCH v2 1/2] USB: musb: fix session-bit runtime-PM quirk

[PATCH v2 1/2] USB: musb: fix session-bit runtime-PM quirk

[Johan Hovold]

The current session-bit quirk implementation does not prevent the retry
counter from underflowing, something which could break runtime PM and
keep the device active for a very long time (about 2^32 seconds) after a
disconnect.

This notably breaks the B-device timeout case, but could potentially
cause problems also when the controller is operating as an A-device.

Fixes: 2bff3916fda9 ("usb: musb: Fix PM for hub disconnect")
Cc: stable <stable@vger.kernel.org>     # 4.9
Cc: Tony Lindgren <tony@atomide.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/musb/musb_core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index b67692857daf..38fa3603554f 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -1861,22 +1861,22 @@ static void musb_pm_runtime_check_session(struct musb *musb)
 		MUSB_DEVCTL_HR;
 	switch (devctl & ~s) {
 	case MUSB_QUIRK_B_INVALID_VBUS_91:
-		if (musb->quirk_retries--) {
+		if (musb->quirk_retries) {
 			musb_dbg(musb,
 				 "Poll devctl on invalid vbus, assume no session");
 			schedule_delayed_work(&musb->irq_work,
 					      msecs_to_jiffies(1000));
-
+			musb->quirk_retries--;
 			return;
 		}
 		/* fall through */
 	case MUSB_QUIRK_A_DISCONNECT_19:
-		if (musb->quirk_retries--) {
+		if (musb->quirk_retries) {
 			musb_dbg(musb,
 				 "Poll devctl on possible host mode disconnect");
 			schedule_delayed_work(&musb->irq_work,
 					      msecs_to_jiffies(1000));
-
+			musb->quirk_retries--;
 			return;
 		}
 		if (!musb->session)
-- 
2.14.1

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

[Johan Hovold]

The musb delayed irq work was never flushed on suspend, something which
since 4.9 can lead to an external abort if the work is scheduled after
the grandparent's clock has been disabled:

PM: Suspending system (mem)
PM: suspend of devices complete after 125.224 msecs
PM: suspend devices took 0.132 seconds
PM: late suspend of devices complete after 7.423 msecs
PM: noirq suspend of devices complete after 7.083 msecs
suspend debug: Waiting for 5 second(s).
Unhandled fault: external abort on non-linefetch (0x1008) at 0xd0262c60
...
[<c054880c>] (musb_default_readb) from [<c0547b5c>] (musb_irq_work+0x48/0x220)
[<c0547b5c>] (musb_irq_work) from [<c014f8a4>] (process_one_work+0x1f4/0x758)
[<c014f8a4>] (process_one_work) from [<c014fe5c>] (worker_thread+0x54/0x514)
[<c014fe5c>] (worker_thread) from [<c015704c>] (kthread+0x128/0x158)
[<c015704c>] (kthread) from [<c0109330>] (ret_from_fork+0x14/0x24)

Commit 2bff3916fda9 ("usb: musb: Fix PM for hub disconnect") started
scheduling musb_irq_work with a delay of up to a second and with
retries thereby making this easy to trigger, for example, by suspending
shortly after a disconnect.

Note that we set a flag to prevent the irq work from rescheduling itself
during suspend and instead process a disconnect immediately. This takes
care of the case where we are disconnected shortly before suspending.

However, when in host mode, a disconnect while suspended will still
go unnoticed and thus prevent the controller from runtime suspending
upon resume as the session bit is always set. This will need to be
addressed separately.

Fixes: 550a7375fe72 ("USB: Add MUSB and TUSB support")
Fixes: 467d5c980709 ("usb: musb: Implement session bit based runtime PM for musb-core")
Fixes: 2bff3916fda9 ("usb: musb: Fix PM for hub disconnect")
Cc: stable <stable@vger.kernel.org>     # 4.9
Cc: Felipe Balbi <felipe.balbi@linux.intel.com>
Cc: Tony Lindgren <tony@atomide.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/musb/musb_core.c | 11 +++++++++--
 drivers/usb/musb/musb_core.h |  2 ++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index 38fa3603554f..e083d0cce670 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -1861,7 +1861,7 @@ static void musb_pm_runtime_check_session(struct musb *musb)
 		MUSB_DEVCTL_HR;
 	switch (devctl & ~s) {
 	case MUSB_QUIRK_B_INVALID_VBUS_91:
-		if (musb->quirk_retries) {
+		if (musb->quirk_retries && !musb->flush_irq_work) {
 			musb_dbg(musb,
 				 "Poll devctl on invalid vbus, assume no session");
 			schedule_delayed_work(&musb->irq_work,
@@ -1871,7 +1871,7 @@ static void musb_pm_runtime_check_session(struct musb *musb)
 		}
 		/* fall through */
 	case MUSB_QUIRK_A_DISCONNECT_19:
-		if (musb->quirk_retries) {
+		if (musb->quirk_retries && !musb->flush_irq_work) {
 			musb_dbg(musb,
 				 "Poll devctl on possible host mode disconnect");
 			schedule_delayed_work(&musb->irq_work,
@@ -2681,8 +2681,15 @@ static int musb_suspend(struct device *dev)
 
 	musb_platform_disable(musb);
 	musb_disable_interrupts(musb);
+
+	musb->flush_irq_work = true;
+	while (flush_delayed_work(&musb->irq_work))
+		;
+	musb->flush_irq_work = false;
+
 	if (!(musb->io.quirks & MUSB_PRESERVE_SESSION))
 		musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
+
 	WARN_ON(!list_empty(&musb->pending_list));
 
 	spin_lock_irqsave(&musb->lock, flags);
diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
index 9f22c5b8ce37..1830a571d025 100644
--- a/drivers/usb/musb/musb_core.h
+++ b/drivers/usb/musb/musb_core.h
@@ -428,6 +428,8 @@ struct musb {
 	unsigned		test_mode:1;
 	unsigned		softconnect:1;
 
+	unsigned		flush_irq_work:1;
+
 	u8			address;
 	u8			test_mode_nr;
 	u16			ackpend;		/* ep0 */
-- 
2.14.1

Re: [PATCH v2 0/2] USB: musb: PM fixes

[Tony Lindgren]

* Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [171005 02:15]:
> On Thu, Sep 07, 2017 at 03:37:46PM +0200, Johan Hovold wrote:
> > These patches fix a couple of bugs introduced by the recent runtime-PM
> > work (details in the individual commit messages).
> > 
> > Note that the external abort was due to the irq work never being flushed
> > on suspend, and that we may need similar fixes for the delayed reset and
> > resume work which are likewise never cancelled on suspend.
> > 
> > As I just mentioned in the v1 thread, there are a number of further issues with
> > musb suspend (e.g. on BBB):
> > 
> >  1. System suspend appears to break any active gadget (which then can be
> >     restarted manually).
> > 
> >  2. The early_tx polling in musb_cppi41 lacks a timeout, something which can
> >     lead to the hrtimer rescheduling itself indefinitely if the fifo never
> >     empties (e.g. if a transfer is is initiated post suspend due to issue 1).
> > 
> >     See commit a655f481d83d ("usb: musb: musb_cppi41: handle pre-mature TX
> >     complete interrupt").
> > 
> >  3. In host mode, if a device is disconnected while the system is suspended,
> >     this will keep the controller runtime active after resume as the session
> >     bit is always set.
> 
> Bin and Tony, any comments to this series?

Oops sorry I forgot to test these after two recent conferences. I just gave
these a try and they both behave for me. Nice fixes, for both:

Tested-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/2] USB: musb: PM fixes

[Johan Hovold]

On Thu, Oct 05, 2017 at 08:11:55AM -0700, Tony Lindgren wrote:
> * Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [171005 02:15]:
> > On Thu, Sep 07, 2017 at 03:37:46PM +0200, Johan Hovold wrote:
> > > These patches fix a couple of bugs introduced by the recent runtime-PM
> > > work (details in the individual commit messages).
> > > 
> > > Note that the external abort was due to the irq work never being flushed
> > > on suspend, and that we may need similar fixes for the delayed reset and
> > > resume work which are likewise never cancelled on suspend.
> > > 
> > > As I just mentioned in the v1 thread, there are a number of further issues with
> > > musb suspend (e.g. on BBB):
> > > 
> > >  1. System suspend appears to break any active gadget (which then can be
> > >     restarted manually).
> > > 
> > >  2. The early_tx polling in musb_cppi41 lacks a timeout, something which can
> > >     lead to the hrtimer rescheduling itself indefinitely if the fifo never
> > >     empties (e.g. if a transfer is is initiated post suspend due to issue 1).
> > > 
> > >     See commit a655f481d83d ("usb: musb: musb_cppi41: handle pre-mature TX
> > >     complete interrupt").
> > > 
> > >  3. In host mode, if a device is disconnected while the system is suspended,
> > >     this will keep the controller runtime active after resume as the session
> > >     bit is always set.
> > 
> > Bin and Tony, any comments to this series?
> 
> Oops sorry I forgot to test these after two recent conferences. I just gave
> these a try and they both behave for me. Nice fixes, for both:
> 
> Tested-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>

Great, thanks for testing!

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

Re: [PATCH v2 0/2] USB: musb: PM fixes

[Bin Liu]

On Thu, Oct 05, 2017 at 05:14:24PM +0200, Johan Hovold wrote:
> On Thu, Oct 05, 2017 at 08:11:55AM -0700, Tony Lindgren wrote:
> > * Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [171005 02:15]:
> > > On Thu, Sep 07, 2017 at 03:37:46PM +0200, Johan Hovold wrote:
> > > > These patches fix a couple of bugs introduced by the recent runtime-PM
> > > > work (details in the individual commit messages).
> > > > 
> > > > Note that the external abort was due to the irq work never being flushed
> > > > on suspend, and that we may need similar fixes for the delayed reset and
> > > > resume work which are likewise never cancelled on suspend.
> > > > 
> > > > As I just mentioned in the v1 thread, there are a number of further issues with
> > > > musb suspend (e.g. on BBB):
> > > > 
> > > >  1. System suspend appears to break any active gadget (which then can be
> > > >     restarted manually).
> > > > 
> > > >  2. The early_tx polling in musb_cppi41 lacks a timeout, something which can
> > > >     lead to the hrtimer rescheduling itself indefinitely if the fifo never
> > > >     empties (e.g. if a transfer is is initiated post suspend due to issue 1).
> > > > 
> > > >     See commit a655f481d83d ("usb: musb: musb_cppi41: handle pre-mature TX
> > > >     complete interrupt").
> > > > 
> > > >  3. In host mode, if a device is disconnected while the system is suspended,
> > > >     this will keep the controller runtime active after resume as the session
> > > >     bit is always set.
> > > 
> > > Bin and Tony, any comments to this series?
> > 
> > Oops sorry I forgot to test these after two recent conferences. I just gave
> > these a try and they both behave for me. Nice fixes, for both:
> > 
> > Tested-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> 
> Great, thanks for testing!

Tony, thanks for testing them.

Johan, I recently have been over loaded with a non-usb work, and it
probably will last for another month or two. But I will try my best to
get these patches and others accumulated recently into upstream in next
week.

Sorry for the delay.

Regards,
-Bin.

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

Re: [PATCH v2 0/2] USB: musb: PM fixes

[Johan Hovold]

On Fri, Oct 06, 2017 at 11:27:56AM -0500, Bin Liu wrote:
> On Thu, Oct 05, 2017 at 05:14:24PM +0200, Johan Hovold wrote:
> > On Thu, Oct 05, 2017 at 08:11:55AM -0700, Tony Lindgren wrote:
> > > * Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [171005 02:15]:

> > > > Bin and Tony, any comments to this series?
> > > 
> > > Oops sorry I forgot to test these after two recent conferences. I just gave
> > > these a try and they both behave for me. Nice fixes, for both:
> > > 
> > > Tested-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> > 
> > Great, thanks for testing!
> 
> Tony, thanks for testing them.
> 
> Johan, I recently have been over loaded with a non-usb work, and it
> probably will last for another month or two. But I will try my best to
> get these patches and others accumulated recently into upstream in next
> week.
> 
> Sorry for the delay.

No worries, thanks for letting me know.

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