ARM: OMAP2+: hwmod: fix _idle() hwmod state sanity check sequence

PATCH New Suman Anna 2 participants 4 messages Apr 4, 2016 – Apr 10, 2016

Subsystems (4)

Arm (32-bit) Arm sub-architectures OMAP2+ support OMAP hwmod support

lore

Share & tools

lore

Suman Anna

author

2016-04-04 23:30 UTC #0 lore raw

PATCH 1/1 ARM: OMAP2+: hwmod: fix _idle() hwmod state sanity check sequence

The omap_hwmod _enable() function can return success without setting the hwmod state to _HWMOD_STATE_ENABLED for IPs with reset lines when all of the reset lines are asserted. The omap_hwmod _idle() function also performs a similar check, but after checking for the hwmod state first. This triggers the WARN when pm_runtime_get and pm_runtime_put are invoked on IPs with all reset lines asserted. Reverse the checks for hwmod state and reset lines status to fix this.

Issue found during a unbind operation on a device with reset lines still asserted, example backtrace below

 ------------[ cut here ]------------
 WARNING: CPU: 1 PID: 879 at arch/arm/mach-omap2/omap_hwmod.c:2207 _idle+0x1e4/0x240()
 omap_hwmod: mmu_dsp: idle state can only be entered from enabled state
 Modules linked in:
 CPU: 1 PID: 879 Comm: sh Not tainted 4.4.0-00008-ga989d951331a #3
 Hardware name: Generic OMAP5 (Flattened Device Tree)
 [<c0018e60>] (unwind_backtrace) from [<c0014dc4>] (show_stack+0x10/0x14)
 [<c0014dc4>] (show_stack) from [<c037ac28>] (dump_stack+0x90/0xc0)
 [<c037ac28>] (dump_stack) from [<c003f420>] (warn_slowpath_common+0x78/0xb4)
 [<c003f420>] (warn_slowpath_common) from [<c003f48c>] (warn_slowpath_fmt+0x30/0x40)
 [<c003f48c>] (warn_slowpath_fmt) from [<c0028c20>] (_idle+0x1e4/0x240)
 [<c0028c20>] (_idle) from [<c0029080>] (omap_hwmod_idle+0x28/0x48)
 [<c0029080>] (omap_hwmod_idle) from [<c002a5a4>] (omap_device_idle+0x3c/0x90)
 [<c002a5a4>] (omap_device_idle) from [<c0427a90>] (__rpm_callback+0x2c/0x60)
 [<c0427a90>] (__rpm_callback) from [<c0427ae4>] (rpm_callback+0x20/0x80)
 [<c0427ae4>] (rpm_callback) from [<c0427f84>] (rpm_suspend+0x138/0x74c)
 [<c0427f84>] (rpm_suspend) from [<c0428b78>] (__pm_runtime_idle+0x78/0xa8)
 [<c0428b78>] (__pm_runtime_idle) from [<c041f514>] (__device_release_driver+0x64/0x100)
 [<c041f514>] (__device_release_driver) from [<c041f5d0>] (device_release_driver+0x20/0x2c)
 [<c041f5d0>] (device_release_driver) from [<c041d85c>] (unbind_store+0x78/0xf8)
 [<c041d85c>] (unbind_store) from [<c0206df8>] (kernfs_fop_write+0xc0/0x1c4)
 [<c0206df8>] (kernfs_fop_write) from [<c018a120>] (__vfs_write+0x20/0xdc)
 [<c018a120>] (__vfs_write) from [<c018a9cc>] (vfs_write+0x90/0x164)
 [<c018a9cc>] (vfs_write) from [<c018b1f0>] (SyS_write+0x44/0x9c)
 [<c018b1f0>] (SyS_write) from [<c0010420>] (ret_fast_syscall+0x0/0x1c)
 ---[ end trace a4182013c75a9f50 ]---

While at this, fix the sequence in _shutdown() as well, though there is no easy reproducible scenario.

Fixes 747834ab8347 ("ARM: OMAP2+: hwmod: revise hardreset behavior")

Signed-off-by Suman Anna <[email protected]>

---
Hi Paul,

There are couple of pr_debug statements that only print once the code flow passes through certain conditions. Not sure if you wanted them to be printed first thing when entering the functions or only if the function is really doing something. But it is still not consistent between _enable(), _idle() and _shutdown().

regards
Suman

 arch/arm/mach-omap2/omap_hwmod.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

arch/arm/mach-omap2/omap_hwmod.c +6 -6

index b6d62e4cdfdd..027371c29fb9 100644
@@ -2207,15 +2207,15 @@ static int _idle(struct omap_hwmod *oh)
 
 	pr_debug("omap_hwmod: %s: idling\n", oh->name);
 
+	if (_are_all_hardreset_lines_asserted(oh))
+		return 0;
+
 	if (oh->_state != _HWMOD_STATE_ENABLED) {
 		WARN(1, "omap_hwmod: %s: idle state can only be entered from enabled state\n",
 			oh->name);
 		return -EINVAL;
 	}
 
-	if (_are_all_hardreset_lines_asserted(oh))
-		return 0;
-
 	if (oh->class->sysc)
 		_idle_sysc(oh);
 	_del_initiator_dep(oh, mpu_oh);

@@ -2262,6 +2262,9 @@ static int _shutdown(struct omap_hwmod *oh)
 	int ret, i;
 	u8 prev_state;
 
+	if (_are_all_hardreset_lines_asserted(oh))
+		return 0;
+
 	if (oh->_state != _HWMOD_STATE_IDLE &&
 	    oh->_state != _HWMOD_STATE_ENABLED) {
 		WARN(1, "omap_hwmod: %s: disabled state can only be entered from idle, or enabled state\n",

@@ -2269,9 +2272,6 @@ static int _shutdown(struct omap_hwmod *oh)
 		return -EINVAL;
 	}
 
-	if (_are_all_hardreset_lines_asserted(oh))
-		return 0;
-
 	pr_debug("omap_hwmod: %s: disabling\n", oh->name);
 
 	if (oh->class->pre_shutdown) {

Signature · 1 line

2.7.4

Paul Walmsley

maintainer

2016-04-10 18:42 UTC #1 lore raw

PATCH 1/1 ARM: OMAP2+: hwmod: fix _idle() hwmod state sanity check sequence

Hi Suman

Quoted 38 lines · Suman Anna

Issue found during a unbind operation on a device with reset lines still asserted, example backtrace below

 ------------[ cut here ]------------
 WARNING: CPU: 1 PID: 879 at arch/arm/mach-omap2/omap_hwmod.c:2207 _idle+0x1e4/0x240()
 omap_hwmod: mmu_dsp: idle state can only be entered from enabled state
 Modules linked in:
 CPU: 1 PID: 879 Comm: sh Not tainted 4.4.0-00008-ga989d951331a #3
 Hardware name: Generic OMAP5 (Flattened Device Tree)
 [<c0018e60>] (unwind_backtrace) from [<c0014dc4>] (show_stack+0x10/0x14)
 [<c0014dc4>] (show_stack) from [<c037ac28>] (dump_stack+0x90/0xc0)
 [<c037ac28>] (dump_stack) from [<c003f420>] (warn_slowpath_common+0x78/0xb4)
 [<c003f420>] (warn_slowpath_common) from [<c003f48c>] (warn_slowpath_fmt+0x30/0x40)
 [<c003f48c>] (warn_slowpath_fmt) from [<c0028c20>] (_idle+0x1e4/0x240)
 [<c0028c20>] (_idle) from [<c0029080>] (omap_hwmod_idle+0x28/0x48)
 [<c0029080>] (omap_hwmod_idle) from [<c002a5a4>] (omap_device_idle+0x3c/0x90)
 [<c002a5a4>] (omap_device_idle) from [<c0427a90>] (__rpm_callback+0x2c/0x60)
 [<c0427a90>] (__rpm_callback) from [<c0427ae4>] (rpm_callback+0x20/0x80)
 [<c0427ae4>] (rpm_callback) from [<c0427f84>] (rpm_suspend+0x138/0x74c)
 [<c0427f84>] (rpm_suspend) from [<c0428b78>] (__pm_runtime_idle+0x78/0xa8)
 [<c0428b78>] (__pm_runtime_idle) from [<c041f514>] (__device_release_driver+0x64/0x100)
 [<c041f514>] (__device_release_driver) from [<c041f5d0>] (device_release_driver+0x20/0x2c)
 [<c041f5d0>] (device_release_driver) from [<c041d85c>] (unbind_store+0x78/0xf8)
 [<c041d85c>] (unbind_store) from [<c0206df8>] (kernfs_fop_write+0xc0/0x1c4)
 [<c0206df8>] (kernfs_fop_write) from [<c018a120>] (__vfs_write+0x20/0xdc)
 [<c018a120>] (__vfs_write) from [<c018a9cc>] (vfs_write+0x90/0x164)
 [<c018a9cc>] (vfs_write) from [<c018b1f0>] (SyS_write+0x44/0x9c)
 [<c018b1f0>] (SyS_write) from [<c0010420>] (ret_fast_syscall+0x0/0x1c)
 ---[ end trace a4182013c75a9f50 ]---

Quoted 5 lines

While at this, fix the sequence in _shutdown() as well, though there is no easy reproducible scenario.
Fixes 747834ab8347 ("ARM: OMAP2+: hwmod: revise hardreset behavior")
Signed-off-by Suman Anna <[email protected]>

Thanks, I'm going to queue this for v4.7, mostly to get in more testing and integration time. Let me know if you think it's important enough to go into v4.6-rc.

Quoted 5 lines

There are couple of pr_debug statements that only print once the code flow passes through certain conditions. Not sure if you wanted them to be printed first thing when entering the functions or only if the function is really doing something. But it is still not consistent between _enable(), _idle() and _shutdown().

Yeah those need to be cleaned up. Seems like the best thing to do is to move them all to the beginning of their respective functions.

- Paul

Suman Anna

author

2016-04-10 21:13 UTC #2 lore raw

PATCH 1/1 ARM: OMAP2+: hwmod: fix _idle() hwmod state sanity check sequence

Hi Paul,

Quoted 45 lines · Paul Walmsley

Hi Suman

Quoted 41 lines · Suman Anna

Issue found during a unbind operation on a device with reset lines still asserted, example backtrace below

 ------------[ cut here ]------------
 WARNING: CPU: 1 PID: 879 at arch/arm/mach-omap2/omap_hwmod.c:2207 _idle+0x1e4/0x240()
 omap_hwmod: mmu_dsp: idle state can only be entered from enabled state
 Modules linked in:
 CPU: 1 PID: 879 Comm: sh Not tainted 4.4.0-00008-ga989d951331a #3
 Hardware name: Generic OMAP5 (Flattened Device Tree)
 [<c0018e60>] (unwind_backtrace) from [<c0014dc4>] (show_stack+0x10/0x14)
 [<c0014dc4>] (show_stack) from [<c037ac28>] (dump_stack+0x90/0xc0)
 [<c037ac28>] (dump_stack) from [<c003f420>] (warn_slowpath_common+0x78/0xb4)
 [<c003f420>] (warn_slowpath_common) from [<c003f48c>] (warn_slowpath_fmt+0x30/0x40)
 [<c003f48c>] (warn_slowpath_fmt) from [<c0028c20>] (_idle+0x1e4/0x240)
 [<c0028c20>] (_idle) from [<c0029080>] (omap_hwmod_idle+0x28/0x48)
 [<c0029080>] (omap_hwmod_idle) from [<c002a5a4>] (omap_device_idle+0x3c/0x90)
 [<c002a5a4>] (omap_device_idle) from [<c0427a90>] (__rpm_callback+0x2c/0x60)
 [<c0427a90>] (__rpm_callback) from [<c0427ae4>] (rpm_callback+0x20/0x80)
 [<c0427ae4>] (rpm_callback) from [<c0427f84>] (rpm_suspend+0x138/0x74c)
 [<c0427f84>] (rpm_suspend) from [<c0428b78>] (__pm_runtime_idle+0x78/0xa8)
 [<c0428b78>] (__pm_runtime_idle) from [<c041f514>] (__device_release_driver+0x64/0x100)
 [<c041f514>] (__device_release_driver) from [<c041f5d0>] (device_release_driver+0x20/0x2c)
 [<c041f5d0>] (device_release_driver) from [<c041d85c>] (unbind_store+0x78/0xf8)
 [<c041d85c>] (unbind_store) from [<c0206df8>] (kernfs_fop_write+0xc0/0x1c4)
 [<c0206df8>] (kernfs_fop_write) from [<c018a120>] (__vfs_write+0x20/0xdc)
 [<c018a120>] (__vfs_write) from [<c018a9cc>] (vfs_write+0x90/0x164)
 [<c018a9cc>] (vfs_write) from [<c018b1f0>] (SyS_write+0x44/0x9c)
 [<c018b1f0>] (SyS_write) from [<c0010420>] (ret_fast_syscall+0x0/0x1c)
 ---[ end trace a4182013c75a9f50 ]---

While at this, fix the sequence in _shutdown() as well, though there is no easy reproducible scenario.

Quoted 6 lines

Quoted 2 lines
Fixes 747834ab8347 ("ARM: OMAP2+: hwmod: revise hardreset behavior")
Signed-off-by Suman Anna <[email protected]>
Thanks, I'm going to queue this for v4.7, mostly to get in more testing and integration time. Let me know if you think it's important enough to go into v4.6-rc.

Nope, not essential for the -rc cycle. I am fine with it getting queued for v4.7.

Quoted 10 lines

Quoted 7 lines
There are couple of pr_debug statements that only print once the code flow passes through certain conditions. Not sure if you wanted them to be printed first thing when entering the functions or only if the function is really doing something. But it is still not consistent between _enable(), _idle() and _shutdown().
Yeah those need to be cleaned up. Seems like the best thing to do is to move them all to the beginning of their respective functions.

You want that fixed in this patch or a separate patch?

regards
Suman

Paul Walmsley

maintainer

2016-04-10 23:02 UTC #3 lore raw

PATCH 1/1 ARM: OMAP2+: hwmod: fix _idle() hwmod state sanity check sequence

Hi Suman

Quoted 10 lines · Suman Anna

Quoted 8 lines
Quoted 5 lines
There are couple of pr_debug statements that only print once the code flow passes through certain conditions. Not sure if you wanted them to be printed first thing when entering the functions or only if the function is really doing something. But it is still not consistent between _enable(), _idle() and _shutdown().
Yeah those need to be cleaned up. Seems like the best thing to do is to move them all to the beginning of their respective functions.
You want that fixed in this patch or a separate patch?

A separate one would be great. No rush

- Paul