USB: musb: fix session-bit runtime-PM quirk

PATCH New Johan Hovold 4 participants 6 messages Sep 7, 2017 – Oct 9, 2017 4 messages not in the archive

Subsystems (3)

Stable branch USB MUSB multipoint high speed dual-role controller

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Johan Hovold

author

2017-09-07 13:39 UTC #1 lore raw

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

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 <[email protected]> # 4.9

Cc Tony Lindgren <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/musb/musb_core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/usb/musb/musb_core.c +4 -4

index b67692857daf..38fa3603554f 100644
@@ -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)

Signature · 1 line

2.14.1

Johan Hovold

author

2017-09-07 13:39 UTC #2 lore raw

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

In reply to a message not in the archive

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 <[email protected]> # 4.9

Cc Felipe Balbi <[email protected]>

Cc Tony Lindgren <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/musb/musb_core.c | 11 +++++++++--
 drivers/usb/musb/musb_core.h |  2 ++
 2 files changed, 11 insertions(+), 2 deletions(-)

drivers/usb/musb/musb_core.c +9 -2

index 38fa3603554f..e083d0cce670 100644
@@ -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);

drivers/usb/musb/musb_core.h +2 -0

index 9f22c5b8ce37..1830a571d025 100644
@@ -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 */

Signature · 1 line

2.14.1

A message that is not in the archive (sent off-list or not yet received): 20171005091436.GA2618@localhost

Tony Lindgren

Tested-by

2017-10-05 15:11 UTC #4 lore raw

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

Quoted 18 lines · Johan Hovold

Quoted 17 lines · Johan Hovold
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).

Quoted 8 lines

Quoted 6 lines
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 <[email protected]>

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Johan Hovold

2017-10-05 15:14 UTC #6 lore raw

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

Quoted 26 lines · Tony Lindgren

Quoted 25 lines · Johan Hovold
Quoted 23 lines · Johan Hovold
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.

Quoted 6 lines

Quoted 1 line
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 <[email protected]>

Great, thanks for testing!

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

Bin Liu

2017-10-06 16:27 UTC #7 lore raw

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

Quoted 27 lines · Johan Hovold

Quoted 26 lines · Tony Lindgren
Quoted 25 lines · Johan Hovold
Quoted 23 lines · Johan Hovold
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.

Quoted 8 lines

Quoted 6 lines
Quoted 1 line
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 <[email protected]>
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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20171006162756.GD12182-zlS79ln5qqxp6PWD+TyudpdHMjK6IpyN@public.gmane.org

Johan Hovold

2017-10-09 08:09 UTC #9 lore raw

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

Quoted 13 lines · Bin Liu

Quoted 11 lines · Johan Hovold
Quoted 8 lines · Tony Lindgren
Quoted 1 line · Johan Hovold
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 <[email protected]>
Great, thanks for testing!

Quoted 8 lines

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html