musb RPM sleep-while-atomic in 4.9-rc1

Hi Tony,

I'm getting the splat below when booting 4.9-rc1 on a BBB and tracked it down to 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS glue layer") which added a synchronous RPM get in a timer callback:

[    6.466226] BUG: sleeping function called from invalid context at /home/johan/work/omicron/src/linux/drivers/base/power/runtime.c:955
[    6.478850] in_atomic(): 1, irqs_disabled(): 0, pid: 59, name: grep
[    6.485434] 1 lock held by grep/59:
[    6.489102]  #0: [    6.490966]  (
((&glue->timer))[    6.494206] ){+.-...}
, at: [    6.497194] [<c0191084>] call_timer_fn+0x0/0x41c
[    6.502048] Preemption disabled at:[    6.505540] [<c01015e0>] __do_softirq+0x80/0x594
[    6.510393] 
[    6.511974] CPU: 0 PID: 59 Comm: grep Not tainted 4.9.0-rc1 #46
[    6.518190] Hardware name: Generic AM33XX (Flattened Device Tree)
[    6.524613] [<c01116e4>] (unwind_backtrace) from [<c010dec0>] (show_stack+0x20/0x24)
[    6.532753] [<c010dec0>] (show_stack) from [<c0364f1c>] (dump_stack+0x24/0x28)
[    6.540351] [<c0364f1c>] (dump_stack) from [<c015ea58>] (___might_sleep+0x1d8/0x2c4)
[    6.548486] [<c015ea58>] (___might_sleep) from [<c015ebb4>] (__might_sleep+0x70/0xa8)
[    6.556720] [<c015ebb4>] (__might_sleep) from [<c0413d60>] (__pm_runtime_resume+0x9c/0xa0)
[    6.565404] [<c0413d60>] (__pm_runtime_resume) from [<c04d0bc4>] (otg_timer+0x3c/0x254)
[    6.573813] [<c04d0bc4>] (otg_timer) from [<c0191180>] (call_timer_fn+0xfc/0x41c)
[    6.581675] [<c0191180>] (call_timer_fn) from [<c01915c0>] (expire_timers+0x120/0x210)
[    6.589990] [<c01915c0>] (expire_timers) from [<c0191acc>] (run_timer_softirq+0xa4/0xdc)
[    6.598487] [<c0191acc>] (run_timer_softirq) from [<c010168c>] (__do_softirq+0x12c/0x594)
[    6.607079] [<c010168c>] (__do_softirq) from [<c01384cc>] (irq_exit+0xf4/0x130)
[    6.614766] [<c01384cc>] (irq_exit) from [<c017d910>] (__handle_domain_irq+0x84/0xec)
[    6.622991] [<c017d910>] (__handle_domain_irq) from [<c0101500>] (omap_intc_handle_irq+0x44/0xa0)
[    6.632306] [<c0101500>] (omap_intc_handle_irq) from [<c010ee78>] (__irq_usr+0x58/0x80)
[    6.640716] Exception stack(0xcf669fb0 to 0xcf669ff8)
[    6.646028] 9fa0:                                     b6fd5050 0012ebb0 b6e71058 b6e1c088
[    6.654614] 9fc0: b6e07000 00000001 b6e1cb10 b6fd5000 00000001 00000000 b6fd38e8 bec32a54
[    6.663198] 9fe0: b6e1c870 bec32990 b6fb0a20 b6fb9758 20030010 ffffffff

Setting the irq_safe flag seems to do the trick, but not sure that's what you intended to do.

I saw you posted some regression fixes lately, but they did not look related to this at first glance at least.

Quoted 6 lines · Johan Hovold

Hi Tony,

I'm getting the splat below when booting 4.9-rc1 on a BBB and tracked it down to 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS glue layer") which added a synchronous RPM get in a timer callback:

OK, sorry to hear about that. Care to email me your .config and how to reproduce and what do you have connected like a hub? Also do you use built-in gadgets or configure them via configfs?

Quoted 29 lines

[    6.466226] BUG: sleeping function called from invalid context at /home/johan/work/omicron/src/linux/drivers/base/power/runtime.c:955
[    6.478850] in_atomic(): 1, irqs_disabled(): 0, pid: 59, name: grep
[    6.485434] 1 lock held by grep/59:
[    6.489102]  #0: [    6.490966]  (
((&glue->timer))[    6.494206] ){+.-...}
, at: [    6.497194] [<c0191084>] call_timer_fn+0x0/0x41c
[    6.502048] Preemption disabled at:[    6.505540] [<c01015e0>] __do_softirq+0x80/0x594
[    6.510393] 
[    6.511974] CPU: 0 PID: 59 Comm: grep Not tainted 4.9.0-rc1 #46
[    6.518190] Hardware name: Generic AM33XX (Flattened Device Tree)
[    6.524613] [<c01116e4>] (unwind_backtrace) from [<c010dec0>] (show_stack+0x20/0x24)
[    6.532753] [<c010dec0>] (show_stack) from [<c0364f1c>] (dump_stack+0x24/0x28)
[    6.540351] [<c0364f1c>] (dump_stack) from [<c015ea58>] (___might_sleep+0x1d8/0x2c4)
[    6.548486] [<c015ea58>] (___might_sleep) from [<c015ebb4>] (__might_sleep+0x70/0xa8)
[    6.556720] [<c015ebb4>] (__might_sleep) from [<c0413d60>] (__pm_runtime_resume+0x9c/0xa0)
[    6.565404] [<c0413d60>] (__pm_runtime_resume) from [<c04d0bc4>] (otg_timer+0x3c/0x254)
[    6.573813] [<c04d0bc4>] (otg_timer) from [<c0191180>] (call_timer_fn+0xfc/0x41c)
[    6.581675] [<c0191180>] (call_timer_fn) from [<c01915c0>] (expire_timers+0x120/0x210)
[    6.589990] [<c01915c0>] (expire_timers) from [<c0191acc>] (run_timer_softirq+0xa4/0xdc)
[    6.598487] [<c0191acc>] (run_timer_softirq) from [<c010168c>] (__do_softirq+0x12c/0x594)
[    6.607079] [<c010168c>] (__do_softirq) from [<c01384cc>] (irq_exit+0xf4/0x130)
[    6.614766] [<c01384cc>] (irq_exit) from [<c017d910>] (__handle_domain_irq+0x84/0xec)
[    6.622991] [<c017d910>] (__handle_domain_irq) from [<c0101500>] (omap_intc_handle_irq+0x44/0xa0)
[    6.632306] [<c0101500>] (omap_intc_handle_irq) from [<c010ee78>] (__irq_usr+0x58/0x80)
[    6.640716] Exception stack(0xcf669fb0 to 0xcf669ff8)
[    6.646028] 9fa0:                                     b6fd5050 0012ebb0 b6e71058 b6e1c088
[    6.654614] 9fc0: b6e07000 00000001 b6e1cb10 b6fd5000 00000001 00000000 b6fd38e8 bec32a54
[    6.663198] 9fe0: b6e1c870 bec32990 b6fb0a20 b6fb9758 20030010 ffffffff

Quoted 2 lines

Setting the irq_safe flag seems to do the trick, but not sure that's what you intended to do.

That's what we want to avoid as it keep the parent device permanently enabled. To avoid that we want to just queue things and deal with them from pm_runtime_resume.

Quoted 2 lines

I saw you posted some regression fixes lately, but they did not look related to this at first glance at least.

Yeah this seems different. Can you still try v4.9-rc + patches from thread "[PATCH 0/2] Fixes for two more musb regressions"?

Quoted 11 lines · Tony Lindgren

Quoted 6 lines · Johan Hovold

Hi Tony,

I'm getting the splat below when booting 4.9-rc1 on a BBB and tracked it down to 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS glue layer") which added a synchronous RPM get in a timer callback:

OK, sorry to hear about that. Care to email me your .config and how to reproduce and what do you have connected like a hub? Also do you use built-in gadgets or configure them via configfs?

It happens even with no devices connected to the host port, and with no gadgets configured (or the peripheral port simply disabled). Whenever the glue timer fires and calls pm_runtime_get_sync() I get the splat. For some reason the might_sleep() in get_sync() does not trigger the first time, which means that I see this four seconds after probe, and then every other second when the timer fires.

Attaching my defconfig.

Quoted 6 lines

Quoted 2 lines

Setting the irq_safe flag seems to do the trick, but not sure that's what you intended to do.

That's what we want to avoid as it keep the parent device permanently enabled. To avoid that we want to just queue things and deal with them from pm_runtime_resume.

I figured, so then that pm_runtime_get_sync() in the dsps timer callback needs to go.

Quoted 5 lines

Quoted 2 lines

I saw you posted some regression fixes lately, but they did not look related to this at first glance at least.

Yeah this seems different. Can you still try v4.9-rc + patches from thread "[PATCH 0/2] Fixes for two more musb regressions"?

As expected, those two fixes makes no difference.

Quoted 13 lines · Johan Hovold

Quoted 11 lines · Tony Lindgren

Quoted 6 lines · Johan Hovold

Hi Tony,

I'm getting the splat below when booting 4.9-rc1 on a BBB and tracked it down to 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS glue layer") which added a synchronous RPM get in a timer callback:

OK, sorry to hear about that. Care to email me your .config and how to reproduce and what do you have connected like a hub? Also do you use built-in gadgets or configure them via configfs?

Quoted 6 lines

It happens even with no devices connected to the host port, and with no gadgets configured (or the peripheral port simply disabled). Whenever the glue timer fires and calls pm_runtime_get_sync() I get the splat. For some reason the might_sleep() in get_sync() does not trigger the first time, which means that I see this four seconds after probe, and then every other second when the timer fires.

OK. I'm totally baffled how come I did not hit this earlier with my tests. I did have some extra patches for using the pmic vbus irq for cable detection, but that's only for the peripheral instance and the host instance is still using timer.

Quoted 7 lines

Quoted 6 lines

Quoted 2 lines

Setting the irq_safe flag seems to do the trick, but not sure that's what you intended to do.

That's what we want to avoid as it keep the parent device permanently enabled. To avoid that we want to just queue things and deal with them from pm_runtime_resume.

Quoted 2 lines

I figured, so then that pm_runtime_get_sync() in the dsps timer callback needs to go.

Agreed, that is clearly wrong to call from softirq context. I need to figure out what is right fix here but don't have access to my bbb until next week.

Quoted 21 lines · Tony Lindgren

Quoted 19 lines · Johan Hovold

Quoted 11 lines · Tony Lindgren

Quoted 6 lines · Johan Hovold

Hi Tony,

I'm getting the splat below when booting 4.9-rc1 on a BBB and tracked it down to 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS glue layer") which added a synchronous RPM get in a timer callback:

OK, sorry to hear about that. Care to email me your .config and how to reproduce and what do you have connected like a hub? Also do you use built-in gadgets or configure them via configfs?

It happens even with no devices connected to the host port, and with no gadgets configured (or the peripheral port simply disabled). Whenever the glue timer fires and calls pm_runtime_get_sync() I get the splat. For some reason the might_sleep() in get_sync() does not trigger the first time, which means that I see this four seconds after probe, and then every other second when the timer fires.

Quoted 4 lines

OK. I'm totally baffled how come I did not hit this earlier with my tests. I did have some extra patches for using the pmic vbus irq for cable detection, but that's only for the peripheral instance and the host instance is still using timer.

Yeah, I was a bit surprised as well given that other people have also apparently run this code. Guess CONFIG_DEBUG_ATOMIC_SLEEP must have been disabled?

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

Quoted 2 lines

Setting the irq_safe flag seems to do the trick, but not sure that's what you intended to do.

That's what we want to avoid as it keep the parent device permanently enabled. To avoid that we want to just queue things and deal with them from pm_runtime_resume.

Quoted 6 lines

Quoted 2 lines

I figured, so then that pm_runtime_get_sync() in the dsps timer callback needs to go.

Agreed, that is clearly wrong to call from softirq context. I need to figure out what is right fix here but don't have access to my bbb until next week.

Ok, I'll make do with irq_safe meanwhile.

Quoted 5 lines · Johan Hovold

Quoted 3 lines

Agreed, that is clearly wrong to call from softirq context. I need to figure out what is right fix here but don't have access to my bbb until next week.

Ok, I'll make do with irq_safe meanwhile.

OK

From tony Mon Sep 17 00:00:00 2001

From: Tony Lindgren <[email protected]>
Date: Mon, 24 Oct 2016 09:18:02 -0700
Subject: [PATCH] usb: musb: Fix sleeping function called from invalid
context for hdrc glue

Commit 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS glue layer") wrongly added a call for pm_runtime_get_sync to otg_timer that runs in softirq context. That causes a "BUG: sleeping function called from invalid context" every time when polling the cable status:

[<c015ebb4>] (__might_sleep) from [<c0413d60>] (__pm_runtime_resume+0x9c/0xa0) [<c0413d60>] (__pm_runtime_resume) from [<c04d0bc4>] (otg_timer+0x3c/0x254) [<c04d0bc4>] (otg_timer) from [<c0191180>] (call_timer_fn+0xfc/0x41c) [<c0191180>] (call_timer_fn) from [<c01915c0>] (expire_timers+0x120/0x210) [<c01915c0>] (expire_timers) from [<c0191acc>] (run_timer_softirq+0xa4/0xdc) [<c0191acc>] (run_timer_softirq) from [<c010168c>] (__do_softirq+0x12c/0x594)

I did not notice that as I did not have CONFIG_DEBUG_ATOMIC_SLEEP enabled.

Let's fix the issue by adding dsps_check_status() and dsps_runtime_resume() functions. And let's have otg_timer() check for the PM runtime status, and if not already enabled, have dsps_runtime_resume() call dsps_check_status() directly.

Quoted 17 lines · Tony Lindgren

From: Tony Lindgren <[email protected]>
Date: Mon, 24 Oct 2016 09:18:02 -0700
Subject: [PATCH] usb: musb: Fix sleeping function called from invalid
context for hdrc glue

Commit 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS glue layer") wrongly added a call for pm_runtime_get_sync to otg_timer that runs in softirq context. That causes a "BUG: sleeping function called from invalid context" every time when polling the cable status:

[<c015ebb4>] (__might_sleep) from [<c0413d60>] (__pm_runtime_resume+0x9c/0xa0) [<c0413d60>] (__pm_runtime_resume) from [<c04d0bc4>] (otg_timer+0x3c/0x254) [<c04d0bc4>] (otg_timer) from [<c0191180>] (call_timer_fn+0xfc/0x41c) [<c0191180>] (call_timer_fn) from [<c01915c0>] (expire_timers+0x120/0x210) [<c01915c0>] (expire_timers) from [<c0191acc>] (run_timer_softirq+0xa4/0xdc) [<c0191acc>] (run_timer_softirq) from [<c010168c>] (__do_softirq+0x12c/0x594)

Quoted 6 lines

I did not notice that as I did not have CONFIG_DEBUG_ATOMIC_SLEEP enabled.

Let's fix the issue by adding dsps_check_status() and dsps_runtime_resume() functions. And let's have otg_timer() check for the PM runtime status, and if not already enabled, have dsps_runtime_resume() call dsps_check_status() directly.

While this makes the sleeping-while-atomic warning go away, it does not seem correct. In case we were suspended, the glue layer will now call dsps_check_status while the controller (child) is still suspended:

[    8.577538] musb-hdrc musb-hdrc.0.auto: otg_timer
[    8.582642] musb-dsps 47401400.usb: dsps_runtime_resume
[    8.600651] musb-hdrc musb-hdrc.0.auto: dsps_check_status
[    8.630617] musb-hdrc musb-hdrc.0.auto: musb_runtime_resume

Quoted 26 lines · Johan Hovold

Quoted 17 lines · Tony Lindgren

From: Tony Lindgren <[email protected]>
Date: Mon, 24 Oct 2016 09:18:02 -0700
Subject: [PATCH] usb: musb: Fix sleeping function called from invalid
context for hdrc glue

Commit 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS glue layer") wrongly added a call for pm_runtime_get_sync to otg_timer that runs in softirq context. That causes a "BUG: sleeping function called from invalid context" every time when polling the cable status:

[<c015ebb4>] (__might_sleep) from [<c0413d60>] (__pm_runtime_resume+0x9c/0xa0) [<c0413d60>] (__pm_runtime_resume) from [<c04d0bc4>] (otg_timer+0x3c/0x254) [<c04d0bc4>] (otg_timer) from [<c0191180>] (call_timer_fn+0xfc/0x41c) [<c0191180>] (call_timer_fn) from [<c01915c0>] (expire_timers+0x120/0x210) [<c01915c0>] (expire_timers) from [<c0191acc>] (run_timer_softirq+0xa4/0xdc) [<c0191acc>] (run_timer_softirq) from [<c010168c>] (__do_softirq+0x12c/0x594)

Quoted 6 lines

I did not notice that as I did not have CONFIG_DEBUG_ATOMIC_SLEEP enabled.

Let's fix the issue by adding dsps_check_status() and dsps_runtime_resume() functions. And let's have otg_timer() check for the PM runtime status, and if not already enabled, have dsps_runtime_resume() call dsps_check_status() directly.

Quoted 8 lines

While this makes the sleeping-while-atomic warning go away, it does not seem correct. In case we were suspended, the glue layer will now call dsps_check_status while the controller (child) is still suspended:

[    8.577538] musb-hdrc musb-hdrc.0.auto: otg_timer
[    8.582642] musb-dsps 47401400.usb: dsps_runtime_resume
[    8.600651] musb-hdrc musb-hdrc.0.auto: dsps_check_status
[    8.630617] musb-hdrc musb-hdrc.0.auto: musb_runtime_resume

Hmm that's a good point yeah. If we start doing something generic in musb-core.c musb_runtime_resume, things could go wrong in the future.

With this particular hardware musb registers are always accessible though when the parent of the two musb instances musb_am335x is done.

We could just reprogam the otg timer if !pm_runtime_active(dev). But the sucky things with that approach is that when idle we have two timers, one to wake-up, then another one to check the status.

Quoted 42 lines · Tony Lindgren

Quoted 26 lines · Johan Hovold

Quoted 17 lines · Tony Lindgren

From: Tony Lindgren <[email protected]>
Date: Mon, 24 Oct 2016 09:18:02 -0700
Subject: [PATCH] usb: musb: Fix sleeping function called from invalid
context for hdrc glue

Commit 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS glue layer") wrongly added a call for pm_runtime_get_sync to otg_timer that runs in softirq context. That causes a "BUG: sleeping function called from invalid context" every time when polling the cable status:

[<c015ebb4>] (__might_sleep) from [<c0413d60>] (__pm_runtime_resume+0x9c/0xa0) [<c0413d60>] (__pm_runtime_resume) from [<c04d0bc4>] (otg_timer+0x3c/0x254) [<c04d0bc4>] (otg_timer) from [<c0191180>] (call_timer_fn+0xfc/0x41c) [<c0191180>] (call_timer_fn) from [<c01915c0>] (expire_timers+0x120/0x210) [<c01915c0>] (expire_timers) from [<c0191acc>] (run_timer_softirq+0xa4/0xdc) [<c0191acc>] (run_timer_softirq) from [<c010168c>] (__do_softirq+0x12c/0x594)

Quoted 6 lines

I did not notice that as I did not have CONFIG_DEBUG_ATOMIC_SLEEP enabled.

Let's fix the issue by adding dsps_check_status() and dsps_runtime_resume() functions. And let's have otg_timer() check for the PM runtime status, and if not already enabled, have dsps_runtime_resume() call dsps_check_status() directly.

Quoted 8 lines

While this makes the sleeping-while-atomic warning go away, it does not seem correct. In case we were suspended, the glue layer will now call dsps_check_status while the controller (child) is still suspended:

[    8.577538] musb-hdrc musb-hdrc.0.auto: otg_timer
[    8.582642] musb-dsps 47401400.usb: dsps_runtime_resume
[    8.600651] musb-hdrc musb-hdrc.0.auto: dsps_check_status
[    8.630617] musb-hdrc musb-hdrc.0.auto: musb_runtime_resume

Hmm that's a good point yeah. If we start doing something generic in musb-core.c musb_runtime_resume, things could go wrong in the future.

With this particular hardware musb registers are always accessible though when the parent of the two musb instances musb_am335x is done.

Quoted 3 lines

We could just reprogam the otg timer if !pm_runtime_active(dev). But the sucky things with that approach is that when idle we have two timers, one to wake-up, then another one to check the status.

Maybe add another callback from musb_runtime_resume that can be used for such deferred work instead?

Quoted 43 lines · Johan Hovold

Quoted 42 lines · Tony Lindgren

Quoted 26 lines · Johan Hovold

Quoted 17 lines · Tony Lindgren

From: Tony Lindgren <[email protected]>
Date: Mon, 24 Oct 2016 09:18:02 -0700
Subject: [PATCH] usb: musb: Fix sleeping function called from invalid
context for hdrc glue

Commit 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS glue layer") wrongly added a call for pm_runtime_get_sync to otg_timer that runs in softirq context. That causes a "BUG: sleeping function called from invalid context" every time when polling the cable status:

[<c015ebb4>] (__might_sleep) from [<c0413d60>] (__pm_runtime_resume+0x9c/0xa0) [<c0413d60>] (__pm_runtime_resume) from [<c04d0bc4>] (otg_timer+0x3c/0x254) [<c04d0bc4>] (otg_timer) from [<c0191180>] (call_timer_fn+0xfc/0x41c) [<c0191180>] (call_timer_fn) from [<c01915c0>] (expire_timers+0x120/0x210) [<c01915c0>] (expire_timers) from [<c0191acc>] (run_timer_softirq+0xa4/0xdc) [<c0191acc>] (run_timer_softirq) from [<c010168c>] (__do_softirq+0x12c/0x594)

Quoted 6 lines

I did not notice that as I did not have CONFIG_DEBUG_ATOMIC_SLEEP enabled.

Let's fix the issue by adding dsps_check_status() and dsps_runtime_resume() functions. And let's have otg_timer() check for the PM runtime status, and if not already enabled, have dsps_runtime_resume() call dsps_check_status() directly.

Quoted 8 lines

While this makes the sleeping-while-atomic warning go away, it does not seem correct. In case we were suspended, the glue layer will now call dsps_check_status while the controller (child) is still suspended:

[    8.577538] musb-hdrc musb-hdrc.0.auto: otg_timer
[    8.582642] musb-dsps 47401400.usb: dsps_runtime_resume
[    8.600651] musb-hdrc musb-hdrc.0.auto: dsps_check_status
[    8.630617] musb-hdrc musb-hdrc.0.auto: musb_runtime_resume

Hmm that's a good point yeah. If we start doing something generic in musb-core.c musb_runtime_resume, things could go wrong in the future.

With this particular hardware musb registers are always accessible though when the parent of the two musb instances musb_am335x is done.

Quoted 6 lines

Quoted 3 lines

We could just reprogam the otg timer if !pm_runtime_active(dev). But the sucky things with that approach is that when idle we have two timers, one to wake-up, then another one to check the status.

Maybe add another callback from musb_runtime_resume that can be used for such deferred work instead?

OK that's a good idea :)

From tony Mon Sep 17 00:00:00 2001

* Tony Lindgren <[email protected]> [161026 07:32]:

8< -------------------------------
From tony Mon Sep 17 00:00:00 2001
From: Tony Lindgren <[email protected]>
Date: Tue, 25 Oct 2016 08:42:00 -0700
Subject: [PATCH] usb: musb: Fix sleeping function called from invalid
context for hdrc glue

Commit 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS glue layer") wrongly added a call for pm_runtime_get_sync to otg_timer that runs in softirq context. That causes a "BUG: sleeping function called from invalid context" every time when polling the cable status:

[<c015ebb4>] (__might_sleep) from [<c0413d60>] (__pm_runtime_resume+0x9c/0xa0) [<c0413d60>] (__pm_runtime_resume) from [<c04d0bc4>] (otg_timer+0x3c/0x254) [<c04d0bc4>] (otg_timer) from [<c0191180>] (call_timer_fn+0xfc/0x41c) [<c0191180>] (call_timer_fn) from [<c01915c0>] (expire_timers+0x120/0x210) [<c01915c0>] (expire_timers) from [<c0191acc>] (run_timer_softirq+0xa4/0xdc) [<c0191acc>] (run_timer_softirq) from [<c010168c>] (__do_softirq+0x12c/0x594)

I did not notice that as I did not have CONFIG_DEBUG_ATOMIC_SLEEP enabled.

Let's fix the issue by adding dsps_check_status() and then register a callback with musb_runtime_resume() so it gets called only when musb core and it's parent devices are awake. Note that we don't want to do this from PM runtime resume in musb_dsps.c as musb core is not awake yet at that point as noted by Johan Hovold <[email protected]>.

Note that musb_gadget_queue() also suffers from a similar issue when connecting the cable and cannot use pm_runtime_get_sync().

Fixes 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS

glue layer")

Reported-by Johan Hovold <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

---
 drivers/usb/musb/musb_core.c   | 44 +++++++++++++++++++++++++++++++++++++++++-
 drivers/usb/musb/musb_core.h   |  7 +++++++
 drivers/usb/musb/musb_dsps.c   | 29 +++++++++++++++++++++-------
 drivers/usb/musb/musb_gadget.c | 16 +++++++++++++--
 4 files changed, 86 insertions(+), 10 deletions(-)

drivers/usb/musb/musb_core.c +43 -1

@@ -122,7 +122,6 @@ MODULE_AUTHOR(DRIVER_AUTHOR);
 MODULE_LICENSE("GPL");
 MODULE_ALIAS("platform:" MUSB_DRIVER_NAME);
 
-
 /*-------------------------------------------------------------------------*/
 
 static inline struct musb *dev_to_musb(struct device *dev)

@@ -1896,6 +1895,46 @@ static void musb_pm_runtime_check_session(struct musb *musb)
 	musb->session = s;
 }
 
+struct musb_resume_work {
+	void (*callback)(struct musb *musb, void *data);
+	void *data;
+	struct list_head node;
+};
+
+void musb_queue_on_resume(struct musb *musb,
+			  void (*callback)(struct musb *musb, void *data),
+			  void *data)
+{
+	struct musb_resume_work *w;
+	unsigned long flags;
+
+	w = devm_kzalloc(musb->controller, sizeof(*w), GFP_ATOMIC);
+	if (!w)
+		return;
+
+	w->callback = callback;
+	w->data = data;
+	spin_lock_irqsave(&musb->list_lock, flags);
+	list_add_tail(&w->node, &musb->resume_work);
+	spin_unlock_irqrestore(&musb->list_lock, flags);
+}
+EXPORT_SYMBOL_GPL(musb_queue_on_resume);
+
+static void musb_run_pending(struct musb *musb)
+{
+	struct musb_resume_work *w, *_w;
+	unsigned long flags;
+
+	spin_lock_irqsave(&musb->list_lock, flags);
+	list_for_each_entry_safe(w, _w, &musb->resume_work, node) {
+		if (w->callback)
+			w->callback(musb, w->data);
+		list_del(&w->node);
+		devm_kfree(musb->controller, w);
+	}
+	spin_unlock_irqrestore(&musb->list_lock, flags);
+}
+
 /* Only used to provide driver mode change events */
 static void musb_irq_work(struct work_struct *data)
 {

@@ -1969,6 +2008,7 @@ static struct musb *allocate_instance(struct device *dev,
 	INIT_LIST_HEAD(&musb->control);
 	INIT_LIST_HEAD(&musb->in_bulk);
 	INIT_LIST_HEAD(&musb->out_bulk);
+	INIT_LIST_HEAD(&musb->resume_work);
 
 	musb->vbuserr_retry = VBUSERR_RETRY_COUNT;
 	musb->a_wait_bcon = OTG_TIME_A_WAIT_BCON;

@@ -2065,6 +2105,7 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
 	}
 
 	spin_lock_init(&musb->lock);
+	spin_lock_init(&musb->list_lock);
 	musb->board_set_power = plat->set_power;
 	musb->min_power = plat->min_power;
 	musb->ops = plat->platform_ops;

@@ -2374,6 +2415,7 @@ static int musb_remove(struct platform_device *pdev)
 	 *  - Peripheral mode: peripheral is deactivated (or never-activated)
 	 *  - OTG mode: both roles are deactivated (or never-activated)
 	 */
+	musb_run_pending(musb);
 	musb_exit_debugfs(musb);
 
 	cancel_work_sync(&musb->irq_work);

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

@@ -303,6 +303,7 @@ struct musb_context_registers {
 struct musb {
 	/* device lock */
 	spinlock_t		lock;
+	spinlock_t		list_lock;	/* resume work list lock */
 
 	struct musb_io		io;
 	const struct musb_platform_ops *ops;

@@ -337,6 +338,7 @@ struct musb {
 	struct list_head	control;	/* of musb_qh */
 	struct list_head	in_bulk;	/* of musb_qh */
 	struct list_head	out_bulk;	/* of musb_qh */
+	struct list_head	resume_work;	/* pending work on resume */
 
 	struct timer_list	otg_timer;
 	struct notifier_block	nb;

@@ -540,6 +542,11 @@ extern irqreturn_t musb_interrupt(struct musb *);
 
 extern void musb_hnp_stop(struct musb *musb);
 
+extern void
+musb_queue_on_resume(struct musb *musb,
+		     void (*callback)(struct musb *musb, void *data),
+		     void *data);
+
 static inline void musb_platform_set_vbus(struct musb *musb, int is_on)
 {
 	if (musb->ops->set_vbus)

drivers/usb/musb/musb_dsps.c +22 -7

@@ -188,9 +188,8 @@ static void dsps_musb_disable(struct musb *musb)
 	musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
 }
 
-static void otg_timer(unsigned long _musb)
+static void dsps_check_status(struct musb *musb)
 {
-	struct musb *musb = (void *)_musb;
 	void __iomem *mregs = musb->mregs;
 	struct device *dev = musb->controller;
 	struct dsps_glue *glue = dev_get_drvdata(dev->parent);

@@ -198,11 +197,6 @@ static void otg_timer(unsigned long _musb)
 	u8 devctl;
 	unsigned long flags;
 	int skip_session = 0;
-	int err;
-
-	err = pm_runtime_get_sync(dev);
-	if (err < 0)
-		dev_err(dev, "Poll could not pm_runtime_get: %i\n", err);
 
 	/*
 	 * We poll because DSPS IP's won't expose several OTG-critical

@@ -246,6 +240,27 @@ static void otg_timer(unsigned long _musb)
 		break;
 	}
 	spin_unlock_irqrestore(&musb->lock, flags);
+}
+
+static void dsps_check_status_resume_work(struct musb *musb, void *unused)
+{
+	dsps_check_status(musb);
+}
+
+static void otg_timer(unsigned long _musb)
+{
+	struct musb *musb = (void *)_musb;
+	struct device *dev = musb->controller;
+	int err;
+
+	err = pm_runtime_get(dev);
+	if (err < 0)
+		dev_err(dev, "Poll could not pm_runtime_get: %i\n", err);
+
+	if (pm_runtime_active(dev))
+		dsps_check_status(musb);
+	else
+		musb_queue_on_resume(musb, dsps_check_status_resume_work, NULL);
 
 	pm_runtime_mark_last_busy(dev);
 	pm_runtime_put_autosuspend(dev);

drivers/usb/musb/musb_gadget.c +2 -2

+}
+
 static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
 			gfp_t gfp_flags)
 {

@@ -1271,8 +1278,13 @@ static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
 	list_add_tail(&request->list, &musb_ep->req_list);
 
 	/* it this is the head of the queue, start i/o ... */
-	if (!musb_ep->busy && &request->list == musb_ep->req_list.next)
-		musb_ep_restart(musb, request);

Quoted 21 lines · Tony Lindgren

* Tony Lindgren <[email protected]> [161026 07:32]:

8< -------------------------------
From tony Mon Sep 17 00:00:00 2001
From: Tony Lindgren <[email protected]>
Date: Tue, 25 Oct 2016 08:42:00 -0700
Subject: [PATCH] usb: musb: Fix sleeping function called from invalid
context for hdrc glue

Commit 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS glue layer") wrongly added a call for pm_runtime_get_sync to otg_timer that runs in softirq context. That causes a "BUG: sleeping function called from invalid context" every time when polling the cable status:

[<c015ebb4>] (__might_sleep) from [<c0413d60>] (__pm_runtime_resume+0x9c/0xa0) [<c0413d60>] (__pm_runtime_resume) from [<c04d0bc4>] (otg_timer+0x3c/0x254) [<c04d0bc4>] (otg_timer) from [<c0191180>] (call_timer_fn+0xfc/0x41c) [<c0191180>] (call_timer_fn) from [<c01915c0>] (expire_timers+0x120/0x210) [<c01915c0>] (expire_timers) from [<c0191acc>] (run_timer_softirq+0xa4/0xdc) [<c0191acc>] (run_timer_softirq) from [<c010168c>] (__do_softirq+0x12c/0x594)

Quoted 7 lines

I did not notice that as I did not have CONFIG_DEBUG_ATOMIC_SLEEP enabled.

Let's fix the issue by adding dsps_check_status() and then register a callback with musb_runtime_resume() so it gets called only when musb core and it's parent devices are awake. Note that we don't want to do this from PM runtime resume in musb_dsps.c as musb core is not awake yet at that point as noted by Johan Hovold <[email protected]>.

It seems some chunks are missing since this patch only runs the deferred work at remove and not at runtime resume, effectively breaking detection.

Quoted 2 lines

Note that musb_gadget_queue() also suffers from a similar issue when connecting the cable and cannot use pm_runtime_get_sync().

You seem to have left that pm_runtime_get_sync() in there though.

From tony Mon Sep 17 00:00:00 2001

Quoted 30 lines · Johan Hovold

Quoted 21 lines · Tony Lindgren

* Tony Lindgren <[email protected]> [161026 07:32]:

8< -------------------------------
From tony Mon Sep 17 00:00:00 2001
From: Tony Lindgren <[email protected]>
Date: Tue, 25 Oct 2016 08:42:00 -0700
Subject: [PATCH] usb: musb: Fix sleeping function called from invalid
context for hdrc glue

Commit 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS glue layer") wrongly added a call for pm_runtime_get_sync to otg_timer that runs in softirq context. That causes a "BUG: sleeping function called from invalid context" every time when polling the cable status:

[<c015ebb4>] (__might_sleep) from [<c0413d60>] (__pm_runtime_resume+0x9c/0xa0) [<c0413d60>] (__pm_runtime_resume) from [<c04d0bc4>] (otg_timer+0x3c/0x254) [<c04d0bc4>] (otg_timer) from [<c0191180>] (call_timer_fn+0xfc/0x41c) [<c0191180>] (call_timer_fn) from [<c01915c0>] (expire_timers+0x120/0x210) [<c01915c0>] (expire_timers) from [<c0191acc>] (run_timer_softirq+0xa4/0xdc) [<c0191acc>] (run_timer_softirq) from [<c010168c>] (__do_softirq+0x12c/0x594)

Quoted 7 lines

I did not notice that as I did not have CONFIG_DEBUG_ATOMIC_SLEEP enabled.

Let's fix the issue by adding dsps_check_status() and then register a callback with musb_runtime_resume() so it gets called only when musb core and it's parent devices are awake. Note that we don't want to do this from PM runtime resume in musb_dsps.c as musb core is not awake yet at that point as noted by Johan Hovold <[email protected]>.

Quoted 3 lines

It seems some chunks are missing since this patch only runs the deferred work at remove and not at runtime resume, effectively breaking detection.

Oops sorry yeah looks like I had that in a separate debug hacks patch..

Quoted 4 lines

Quoted 2 lines

Note that musb_gadget_queue() also suffers from a similar issue when connecting the cable and cannot use pm_runtime_get_sync().

You seem to have left that pm_runtime_get_sync() in there though.

And that one I must have hosed when cleaning up, thanks for noticing these. Updated patch below.

drivers/usb/musb/musb_gadget.c +3 -1

+}
+
 static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
 			gfp_t gfp_flags)
 {

@@ -1255,7 +1262,7 @@ static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
 
 	map_dma_buffer(request, musb, musb_ep);
 
-	pm_runtime_get_sync(musb->controller);
+	pm_runtime_get(musb->controller);
 	spin_lock_irqsave(&musb->lock, lockflags);
 
 	/* don't queue if the ep is down */

@@ -1271,8 +1278,13 @@ static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
 	list_add_tail(&request->list, &musb_ep->req_list);
 

Quoted 2 lines · Tony Lindgren

And that one I must have hosed when cleaning up, thanks for noticing these. Updated patch below.

I had a couple of inline comments to the previous version about locking in the gadget code as well (hidden after too much context). Looks like there's a lock missing for the deferred work, and something that seems like a possible ABBA deadlock.

Quoted 7 lines

drivers/usb/musb/musb_gadget.c +0 -0

@@ -1222,6 +1222,13 @@ void musb_ep_restart(struct musb *musb, struct musb_request *req)
 		rxstate(musb, req);
 }
 

drivers/usb/musb/musb_gadget.c void musb_ep_restart(struct musb *musb, struct musb_request *req) · line 1229

Quoted 5 lines

drivers/usb/musb/musb_gadget.c +5 -0

+void musb_ep_restart_resume_work(struct musb *musb, void *data)
+{
+	struct musb_request *req = data;
+
+	musb_ep_restart(musb, req);

This one is supposed to be called with musb->lock held (according to the function header anyway).

Quoted 15 lines

drivers/usb/musb/musb_gadget.c +1 -1

 static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
 			gfp_t gfp_flags)
 {

@@ -1255,7 +1262,7 @@ static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
 
 	map_dma_buffer(request, musb, musb_ep);
 
-	pm_runtime_get_sync(musb->controller);
+	pm_runtime_get(musb->controller);
 	spin_lock_irqsave(&musb->lock, lockflags);
 
 	/* don't queue if the ep is down */

@@ -1271,8 +1278,13 @@ static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
 	list_add_tail(&request->list, &musb_ep->req_list);
 

drivers/usb/musb/musb_gadget.c static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req, · line 1287

Quoted 10 lines

drivers/usb/musb/musb_gadget.c +7 -2

 	/* it this is the head of the queue, start i/o ... */
-	if (!musb_ep->busy && &request->list == musb_ep->req_list.next)
-		musb_ep_restart(musb, request);
+	if (!musb_ep->busy && &request->list == musb_ep->req_list.next) {
+		if (pm_runtime_active(musb->controller))
+			musb_ep_restart(musb, request);
+		else
+			musb_queue_on_resume(musb, musb_ep_restart_resume_work,
+					     request);
+	}

But then this looks like it could trigger an ABBA deadlock as musb->lock is held while queue_on_resume() takes musb->list_lock, and musb_run_pending() would take the same locks in the reverse order.

Quoted 9 lines · Johan Hovold

Quoted 7 lines · Tony Lindgren

drivers/usb/musb/musb_gadget.c +0 -0

@@ -1222,6 +1222,13 @@ void musb_ep_restart(struct musb *musb, struct musb_request *req)
 		rxstate(musb, req);
 }
 

Quoted 8 lines

drivers/usb/musb/musb_gadget.c void musb_ep_restart(struct musb *musb, struct musb_request *req) · line 1229

Quoted 5 lines

drivers/usb/musb/musb_gadget.c +5 -0

+void musb_ep_restart_resume_work(struct musb *musb, void *data)
+{
+	struct musb_request *req = data;
+
+	musb_ep_restart(musb, req);

This one is supposed to be called with musb->lock held (according to the function header anyway).

Good point, yeah that calls the monster functions txstate and rxstate.

Quoted 15 lines

Quoted 15 lines

drivers/usb/musb/musb_gadget.c +1 -1

 static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
 			gfp_t gfp_flags)
 {

@@ -1255,7 +1262,7 @@ static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
 
 	map_dma_buffer(request, musb, musb_ep);
 
-	pm_runtime_get_sync(musb->controller);
+	pm_runtime_get(musb->controller);
 	spin_lock_irqsave(&musb->lock, lockflags);
 
 	/* don't queue if the ep is down */

@@ -1271,8 +1278,13 @@ static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
 	list_add_tail(&request->list, &musb_ep->req_list);
 

Quoted 14 lines

drivers/usb/musb/musb_gadget.c static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req, · line 1287

Quoted 10 lines

drivers/usb/musb/musb_gadget.c +7 -2

 	/* it this is the head of the queue, start i/o ... */
-	if (!musb_ep->busy && &request->list == musb_ep->req_list.next)
-		musb_ep_restart(musb, request);
+	if (!musb_ep->busy && &request->list == musb_ep->req_list.next) {
+		if (pm_runtime_active(musb->controller))
+			musb_ep_restart(musb, request);
+		else
+			musb_queue_on_resume(musb, musb_ep_restart_resume_work,
+					     request);
+	}

But then this looks like it could trigger an ABBA deadlock as musb->lock is held while queue_on_resume() takes musb->list_lock, and musb_run_pending() would take the same locks in the reverse order.

It seems we can avoid that by locking only list_add_tail() and list_del():

list_for_each_entry_safe(w, _w, &musb->resume_work, node) {
	spin_lock_irqsave(&musb->list_lock, flags);
	list_del(&w->node);
	spin_unlock_irqrestore(&musb->list_lock, flags);
	if (w->callback)
		w->callback(musb, w->data);
	devm_kfree(musb->controller, w);
}

Or do you have some better ideas?

Quoted 7 lines · Tony Lindgren

Quoted 3 lines · Johan Hovold

But then this looks like it could trigger an ABBA deadlock as musb->lock is held while queue_on_resume() takes musb->list_lock, and musb_run_pending() would take the same locks in the reverse order.

It seems we can avoid that by locking only list_add_tail() and list_del():

Quoted 8 lines

list_for_each_entry_safe(w, _w, &musb->resume_work, node) {
	spin_lock_irqsave(&musb->list_lock, flags);
	list_del(&w->node);
	spin_unlock_irqrestore(&musb->list_lock, flags);
	if (w->callback)
		w->callback(musb, w->data);
	devm_kfree(musb->controller, w);
}

I think you still need to hold the lock while traversing the list (even if you temporarily release it during the callback).

From tony Mon Sep 17 00:00:00 2001

Quoted 17 lines · Johan Hovold

Quoted 15 lines · Tony Lindgren

Quoted 3 lines · Johan Hovold

But then this looks like it could trigger an ABBA deadlock as musb->lock is held while queue_on_resume() takes musb->list_lock, and musb_run_pending() would take the same locks in the reverse order.

It seems we can avoid that by locking only list_add_tail() and list_del():

list_for_each_entry_safe(w, _w, &musb->resume_work, node) {
	spin_lock_irqsave(&musb->list_lock, flags);
	list_del(&w->node);
	spin_unlock_irqrestore(&musb->list_lock, flags);
	if (w->callback)
		w->callback(musb, w->data);
	devm_kfree(musb->controller, w);
}

Quoted 2 lines

I think you still need to hold the lock while traversing the list (even if you temporarily release it during the callback).

Hmm yeah we need iterate through the list again to avoid missing new elements being added. I've updated the patch to use a the common while (!list_empty(&musb->resume_work)) loop. Does that solve the concern you had or did you also had some other concern there?

Quoted 21 lines · Tony Lindgren

Quoted 19 lines · Johan Hovold

Quoted 7 lines · Tony Lindgren

Quoted 3 lines · Johan Hovold

But then this looks like it could trigger an ABBA deadlock as musb->lock is held while queue_on_resume() takes musb->list_lock, and musb_run_pending() would take the same locks in the reverse order.

It seems we can avoid that by locking only list_add_tail() and list_del():

Quoted 8 lines

list_for_each_entry_safe(w, _w, &musb->resume_work, node) {
	spin_lock_irqsave(&musb->list_lock, flags);
	list_del(&w->node);
	spin_unlock_irqrestore(&musb->list_lock, flags);
	if (w->callback)
		w->callback(musb, w->data);
	devm_kfree(musb->controller, w);
}

I think you still need to hold the lock while traversing the list (even if you temporarily release it during the callback).

Quoted 4 lines

Hmm yeah we need iterate through the list again to avoid missing new elements being added. I've updated the patch to use a the common while (!list_empty(&musb->resume_work)) loop. Does that solve the concern you had or did you also had some other concern there?

Yeah, while that minimises the race window it is still possible that the timer callback checks pm_runtime_active() after the queue has been processed but before the rpm status is updated.

How about using a work struct and synchronous get for the deferred case?

From tony Mon Sep 17 00:00:00 2001

Here's the patch updated to use the existing finish_resume_work. Is that along the lines you were thinking?

Adding also Ladis to Cc too.

Quoted 27 lines · Johan Hovold

Quoted 25 lines · Tony Lindgren

Quoted 17 lines · Johan Hovold

Quoted 7 lines · Tony Lindgren

Quoted 3 lines · Johan Hovold

But then this looks like it could trigger an ABBA deadlock as musb->lock is held while queue_on_resume() takes musb->list_lock, and musb_run_pending() would take the same locks in the reverse order.

It seems we can avoid that by locking only list_add_tail() and list_del():

Quoted 8 lines

list_for_each_entry_safe(w, _w, &musb->resume_work, node) {
	spin_lock_irqsave(&musb->list_lock, flags);
	list_del(&w->node);
	spin_unlock_irqrestore(&musb->list_lock, flags);
	if (w->callback)
		w->callback(musb, w->data);
	devm_kfree(musb->controller, w);
}

Quoted 2 lines

I think you still need to hold the lock while traversing the list (even if you temporarily release it during the callback).

Hmm yeah we need iterate through the list again to avoid missing new elements being added. I've updated the patch to use a the common while (!list_empty(&musb->resume_work)) loop. Does that solve the concern you had or did you also had some other concern there?

Quoted 3 lines

Yeah, while that minimises the race window it is still possible that the timer callback checks pm_runtime_active() after the queue has been processed but before the rpm status is updated.

OK. Sorry for the delay responding, had my motherboard fail over the weekend..

Quoted 1 line

How about using a work struct and synchronous get for the deferred case?

Here's the patch updated to use the existing finish_resume_work. Is that along the lines you were thinking?

8< ----------------------------
From tony Mon Sep 17 00:00:00 2001
From: Tony Lindgren <[email protected]>
Date: Wed, 2 Nov 2016 19:59:05 -0700
Subject: [PATCH] usb: musb: Fix sleeping function called from invalid
context for hdrc glue

Commit 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS glue layer") wrongly added a call for pm_runtime_get_sync to otg_timer that runs in softirq context. That causes a "BUG: sleeping function called from invalid context" every time when polling the cable status:

[<c015ebb4>] (__might_sleep) from [<c0413d60>] (__pm_runtime_resume+0x9c/0xa0) [<c0413d60>] (__pm_runtime_resume) from [<c04d0bc4>] (otg_timer+0x3c/0x254) [<c04d0bc4>] (otg_timer) from [<c0191180>] (call_timer_fn+0xfc/0x41c) [<c0191180>] (call_timer_fn) from [<c01915c0>] (expire_timers+0x120/0x210) [<c01915c0>] (expire_timers) from [<c0191acc>] (run_timer_softirq+0xa4/0xdc) [<c0191acc>] (run_timer_softirq) from [<c010168c>] (__do_softirq+0x12c/0x594)

I did not notice that as I did not have CONFIG_DEBUG_ATOMIC_SLEEP enabled. And looks like also musb_gadget_queue() suffers from the same problem.

Let's fix the issue by using a list of delayed work then call it on
resume. We can use the existing finish_resume_work for that. Note that
we want to do this only when both musb core and it's parent devices are
awake as noted by Johan Hovold <[email protected]>. This allows us also to get rid of musb_gadget_work and need_finish_resume flag.

Note that we now also need to get rid of static int first as that won't work right on devices with two musb instances like am335x.

Note that we don't want to mess with deassert_reset_work as that's more time sensitive and USB spec related instead of PM runtime related.

Fixes 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS

glue layer")

Reported-by Johan Hovold <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

---
 drivers/usb/musb/musb_core.c    | 89 +++++++++++++++++++++++++++++++++--------
 drivers/usb/musb/musb_core.h    |  9 ++++-
 drivers/usb/musb/musb_dsps.c    | 24 +++++++----
 drivers/usb/musb/musb_gadget.c  | 31 +++++++++-----
 drivers/usb/musb/musb_host.h    |  6 ++-
 drivers/usb/musb/musb_virthub.c |  5 +--
 6 files changed, 123 insertions(+), 41 deletions(-)

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

@@ -578,8 +578,9 @@ static irqreturn_t musb_stage0_irq(struct musb *musb, u8 int_usb,
 						| MUSB_PORT_STAT_RESUME;
 				musb->rh_timer = jiffies
 					+ msecs_to_jiffies(USB_RESUME_TIMEOUT);
-				musb->need_finish_resume = 1;
-
+				musb_queue_resume_work(musb,
+						       musb_host_finish_resume,
+						       NULL);
 				musb->xceiv->otg->state = OTG_STATE_A_HOST;
 				musb->is_active = 1;
 				musb_host_resume_root_hub(musb);

@@ -1969,6 +1970,7 @@ static struct musb *allocate_instance(struct device *dev,
 	INIT_LIST_HEAD(&musb->control);
 	INIT_LIST_HEAD(&musb->in_bulk);
 	INIT_LIST_HEAD(&musb->out_bulk);
+	INIT_LIST_HEAD(&musb->pending_list);
 
 	musb->vbuserr_retry = VBUSERR_RETRY_COUNT;
 	musb->a_wait_bcon = OTG_TIME_A_WAIT_BCON;

@@ -2018,6 +2020,64 @@ static void musb_free(struct musb *musb)
 	musb_host_free(musb);
 }
 
+struct musb_pending_work {
+	void (*callback)(struct musb *musb, void *data);
+	void *data;
+	struct list_head node;

drivers/usb/musb/musb_core.c +16 -0

+	spin_lock_irqsave(&musb->list_lock, flags);
+	while (!list_empty(&musb->pending_list)) {
+		w = list_first_entry(&musb->pending_list,
+				     struct musb_pending_work,
+				     node);
+		list_del(&w->node);
+		spin_unlock_irqrestore(&musb->list_lock, flags);
+		if (w->callback)
+			w->callback(musb, w->data);
+		devm_kfree(musb->controller, w);
+		spin_lock_irqsave(&musb->list_lock, flags);
+	}
+	spin_unlock_irqrestore(&musb->list_lock, flags);
+	pm_runtime_mark_last_busy(musb->controller);
+	pm_runtime_put_autosuspend(musb->controller);
+}

drivers/usb/musb/musb_core.c +27 -10

+
+	w = devm_kzalloc(musb->controller, sizeof(*w), GFP_ATOMIC);
+	if (!w)
+		return;
+
+	w->callback = callback;
+	w->data = data;
+	spin_lock_irqsave(&musb->list_lock, flags);
+	list_add_tail(&w->node, &musb->pending_list);
+	spin_unlock_irqrestore(&musb->list_lock, flags);
+}
+EXPORT_SYMBOL_GPL(musb_queue_resume_work);
+
+void musb_cancel_resume_work(struct musb *musb)
+{
+	cancel_delayed_work_sync(&musb->finish_resume_work);
+}
+
 static void musb_deassert_reset(struct work_struct *work)
 {
 	struct musb *musb;

@@ -2065,6 +2125,7 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
 	}
 
 	spin_lock_init(&musb->lock);
+	spin_lock_init(&musb->list_lock);
 	musb->board_set_power = plat->set_power;
 	musb->min_power = plat->min_power;
 	musb->ops = plat->platform_ops;

@@ -2215,7 +2276,7 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
 	/* Init IRQ workqueue before request_irq */
 	INIT_WORK(&musb->irq_work, musb_irq_work);
 	INIT_DELAYED_WORK(&musb->deassert_reset_work, musb_deassert_reset);
-	INIT_DELAYED_WORK(&musb->finish_resume_work, musb_host_finish_resume);
+	INIT_DELAYED_WORK(&musb->finish_resume_work, musb_pending_work);
 
 	/* setup musb parts of the core (especially endpoints) */
 	status = musb_core_init(plat->config->multipoint

@@ -2310,7 +2371,7 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
 
 fail3:
 	cancel_work_sync(&musb->irq_work);
-	cancel_delayed_work_sync(&musb->finish_resume_work);
+	musb_cancel_resume_work(musb);
 	cancel_delayed_work_sync(&musb->deassert_reset_work);
 	if (musb->dma_controller)
 		musb_dma_controller_destroy(musb->dma_controller);

@@ -2377,7 +2438,7 @@ static int musb_remove(struct platform_device *pdev)
 	musb_exit_debugfs(musb);
 
 	cancel_work_sync(&musb->irq_work);
-	cancel_delayed_work_sync(&musb->finish_resume_work);
+	musb_cancel_resume_work(musb);
 	cancel_delayed_work_sync(&musb->deassert_reset_work);
 	pm_runtime_get_sync(musb->controller);
 	musb_host_cleanup(musb);

@@ -2603,11 +2664,9 @@ static int musb_resume(struct device *dev)
 	mask = MUSB_DEVCTL_BDEVICE | MUSB_DEVCTL_FSDEV | MUSB_DEVCTL_LSDEV;
 	if ((devctl & mask) != (musb->context.devctl & mask))
 		musb->port1_status = 0;
-	if (musb->need_finish_resume) {
-		musb->need_finish_resume = 0;
-		schedule_delayed_work(&musb->finish_resume_work,
-				      msecs_to_jiffies(USB_RESUME_TIMEOUT));
-	}
+
+	schedule_delayed_work(&musb->finish_resume_work,
+			      msecs_to_jiffies(USB_RESUME_TIMEOUT));
 
 	/*
 	 * The USB HUB code expects the device to be in RPM_ACTIVE once it came

@@ -2633,8 +2692,8 @@ static int musb_runtime_suspend(struct device *dev)
 
 static int musb_runtime_resume(struct device *dev)
 {
-	struct musb	*musb = dev_to_musb(dev);
-	static int	first = 1;
+	struct musb *musb = dev_to_musb(dev);
+	struct delayed_work *d = &musb->finish_resume_work;
 
 	/*
 	 * When pm_runtime_get_sync called for the first time in driver

drivers/usb/musb/musb_core.c +0 -1

 	}
 	spin_unlock_irqrestore(&musb->lock, flags);
 

@@ -1849,7 +1863,6 @@ int musb_gadget_setup(struct musb *musb)
 #elif IS_ENABLED(CONFIG_USB_MUSB_GADGET)
 	musb->g.is_otg = 0;
 #endif
-	INIT_DELAYED_WORK(&musb->gadget_work, musb_gadget_work);
 	musb_g_init_endpoints(musb);
 
 	musb->is_active = 0;

Quoted 2 lines · Tony Lindgren

OK. Sorry for the delay responding, had my motherboard fail over the weekend..

Ouch. Hope the recovery process wasn't too painful.

Quoted 4 lines

Quoted 1 line

How about using a work struct and synchronous get for the deferred case?

Here's the patch updated to use the existing finish_resume_work. Is that along the lines you were thinking?

Along those lines, yes, but I'm not sure about reusing finish_resume_work currently used to deassert resume signalling only.

If work is already queued it seems you could end up deasserting resume prematurely for example. It currently also add unnecessary latency to other deferred work.

You also forgot to queue musb_host_finish_resume() from musb_port_suspend() which looks like it would break port resume.

drivers/usb/musb/musb_core.c static void musb_free(struct musb *musb) · line 2062

Quoted 5 lines

drivers/usb/musb/musb_core.c +5 -0

+	struct musb_pending_work *w;
+	unsigned long flags;
+
+	if (!callback)
+		return;

WARN_ON (e.g. in case someone switches the arguments)?

Perhaps sleep in musb_host_finish_resume() instead of always adding this delay which is only needed for deasserting resume signalling and not for generic (runtime) resume work (if you decide to use a common work queue).

But why schedule from resume at all? It seems you could just schedule the deferred-work processing directly after having registered a callback.