Re: [PATCH] OMAP: omap_hwmod: remove locking from hwmod_for_each iterators
Paul Walmsley writes:
> Hi Kevin,
>
> On Tue, 10 Aug 2010, Kevin Hilman wrote:
>
>> Remove unnecessary locking in the 'for_each' iterators. Any locking should
>> be taken care of by using hwmod API functions in the functions called by the
>> iterators.
>>
>> In addition, having locking here causes lockdep to detect possible circular
>> dependencies (originally reported by Partha Basak .)
>>
>> For example, during init (#0 below) you have the hwmod_mutex acquired
>> (hwmod_for_each_by_class()) then the dpm_list_mtx acquired
>> (device_pm_add()). Later, during suspend the dpm_list_mtx is aquired
>> first (dpm_suspend_noirq()), then the omap_hwmod_mutex is acquired
>> (omap_hwmod_idle()).
>>
>> [ 810.170593] ===
>> [ 810.170593] [ INFO: possible circular locking dependency detected ]
>> [ 810.170623] 2.6.35-rc5-00131-g56e767c-dirty #34
>> [ 810.170654] ---
>> [ 810.170654] sh/670 is trying to acquire lock:
>> [ 810.170684] (omap_hwmod_mutex){+.+.+.}, at: []
>> omap_hwmod_idle+0x1c/0x38
>> [ 810.170745]
>> [ 810.170745] but task is already holding lock:
>> [ 810.170776] (dpm_list_mtx){+.+...}, at: []
>> dpm_suspend_noirq+0x28/0x188
>> [ 810.170806]
>> [ 810.170837] which lock already depends on the new lock.
>> [ 810.170837]
>> [ 810.170837]
>> [ 810.170837] the existing dependency chain (in reverse order) is:
>> [ 810.170867]
>> [ 810.170867] -> #1 (dpm_list_mtx){+.+...}:
>> [ 810.170898][] lock_acquire+0x60/0x74
>> [ 810.170959][] mutex_lock_nested+0x58/0x2e4
>> [ 810.170989][] device_pm_add+0x14/0xcc
>> [ 810.171020][] device_add+0x3b8/0x564
>> [ 810.171051][] platform_device_add+0x104/0x160
>> [ 810.171112][] omap_device_build_ss+0x14c/0x1c8
>> [ 810.171142][] omap_device_build+0x48/0x50
>> [ 810.171173][] omap2_init_gpio+0xf0/0x15c
>> [ 810.171203][] omap_hwmod_for_each_by_class+0x60/0xa4
>> [ 810.171264][] do_one_initcall+0x58/0x1b4
>> [ 810.171295][] kernel_init+0x98/0x150
>> [ 810.171325][] kernel_thread_exit+0x0/0x8
>> [ 810.171356]
>> [ 810.171356] -> #0 (omap_hwmod_mutex){+.+.+.}:
>> [ 810.171386][] __lock_acquire+0x108c/0x1784
>> [ 810.171447][] lock_acquire+0x60/0x74
>> [ 810.171478][] mutex_lock_nested+0x58/0x2e4
>> [ 810.171508][] omap_hwmod_idle+0x1c/0x38
>> [ 810.171539][] omap_device_idle_hwmods+0x20/0x3c
>> [ 810.171600][] _omap_device_deactivate+0x58/0x14c
>> [ 810.171630][] omap_device_idle+0x4c/0x6c
>> [ 810.171661][] platform_pm_runtime_suspend+0x4c/0x74
>> [ 810.171691][] __pm_runtime_suspend+0x204/0x34c
>> [ 810.171722][] pm_runtime_suspend+0x20/0x34
>> [ 810.171752][] platform_pm_runtime_idle+0x8/0x10
>> [ 810.171783][] __pm_runtime_idle+0x15c/0x198
>> [ 810.171813][] pm_runtime_idle+0x1c/0x30
>> [ 810.171844][] platform_pm_suspend_noirq+0x48/0x50
>> [ 810.171875][] pm_noirq_op+0xa0/0x184
>> [ 810.171905][] dpm_suspend_noirq+0xac/0x188
>> [ 810.171936][] suspend_devices_and_enter+0x94/0x1d8
>> [ 810.171966][] enter_state+0xbc/0x120
>> [ 810.171997][] state_store+0xa4/0xb8
>> [ 810.172027][] kobj_attr_store+0x18/0x1c
>> [ 810.172088][] sysfs_write_file+0x10c/0x144
>> [ 810.172119][] vfs_write+0xb0/0x148
>> [ 810.172149][] sys_write+0x3c/0x68
>> [ 810.172180][] ret_fast_syscall+0x0/0x3c
>
> The intention of the mutex_lock() in the omap_hwmod_for_each*() case is to
> protect against changes to omap_hwmod_list during the list iteration. It
> is true that omap_hwmod_list is only likely to change very early in boot,
> as the hwmods are registered, so perhaps this is not necessary in
> practice. But at least theoretically it seems necessary, since I don't
> think that list_for_each_entry() is safe if a list addition is in progress
> simultaneously.
Good point. It wasn't clear to me exactly what the mutex was trying to
protect, thanks for the clarification.
> On the other hand, taking the mutex during most of the other
> omap_hwmod calls, such as
> omap_hwmod_{enable,idle,shutdown,enable_clocks,disable_clocks,reset,enable_wakeup,disable_wakeup}
> is definitely not needed since those functions do not affect
> omap_hwmod_list.
Agreed.
> The goal of the mutex there was simply to protect
> against concurrent calls to those functions. To do that, perhaps the lock
> could be moved into the struct omap_hwmod? I believe you suggested this
> during our PM discussions in Bengaluru. That would carry a slight memory
> space penalty, but would also deserialize hwmod operations on an SMP
> system. If you agree, perhaps you might spin a patch for that instead?
Yes, will do.
Thanks,
Kevin
--
To unsubscribe from this list: send the
Re: [PATCH] OMAP: omap_hwmod: remove locking from hwmod_for_each iterators
Hi Kevin,
On Tue, 10 Aug 2010, Kevin Hilman wrote:
> Remove unnecessary locking in the 'for_each' iterators. Any locking should
> be taken care of by using hwmod API functions in the functions called by the
> iterators.
>
> In addition, having locking here causes lockdep to detect possible circular
> dependencies (originally reported by Partha Basak .)
>
> For example, during init (#0 below) you have the hwmod_mutex acquired
> (hwmod_for_each_by_class()) then the dpm_list_mtx acquired
> (device_pm_add()). Later, during suspend the dpm_list_mtx is aquired
> first (dpm_suspend_noirq()), then the omap_hwmod_mutex is acquired
> (omap_hwmod_idle()).
>
> [ 810.170593] ===
> [ 810.170593] [ INFO: possible circular locking dependency detected ]
> [ 810.170623] 2.6.35-rc5-00131-g56e767c-dirty #34
> [ 810.170654] ---
> [ 810.170654] sh/670 is trying to acquire lock:
> [ 810.170684] (omap_hwmod_mutex){+.+.+.}, at: []
> omap_hwmod_idle+0x1c/0x38
> [ 810.170745]
> [ 810.170745] but task is already holding lock:
> [ 810.170776] (dpm_list_mtx){+.+...}, at: []
> dpm_suspend_noirq+0x28/0x188
> [ 810.170806]
> [ 810.170837] which lock already depends on the new lock.
> [ 810.170837]
> [ 810.170837]
> [ 810.170837] the existing dependency chain (in reverse order) is:
> [ 810.170867]
> [ 810.170867] -> #1 (dpm_list_mtx){+.+...}:
> [ 810.170898][] lock_acquire+0x60/0x74
> [ 810.170959][] mutex_lock_nested+0x58/0x2e4
> [ 810.170989][] device_pm_add+0x14/0xcc
> [ 810.171020][] device_add+0x3b8/0x564
> [ 810.171051][] platform_device_add+0x104/0x160
> [ 810.171112][] omap_device_build_ss+0x14c/0x1c8
> [ 810.171142][] omap_device_build+0x48/0x50
> [ 810.171173][] omap2_init_gpio+0xf0/0x15c
> [ 810.171203][] omap_hwmod_for_each_by_class+0x60/0xa4
> [ 810.171264][] do_one_initcall+0x58/0x1b4
> [ 810.171295][] kernel_init+0x98/0x150
> [ 810.171325][] kernel_thread_exit+0x0/0x8
> [ 810.171356]
> [ 810.171356] -> #0 (omap_hwmod_mutex){+.+.+.}:
> [ 810.171386][] __lock_acquire+0x108c/0x1784
> [ 810.171447][] lock_acquire+0x60/0x74
> [ 810.171478][] mutex_lock_nested+0x58/0x2e4
> [ 810.171508][] omap_hwmod_idle+0x1c/0x38
> [ 810.171539][] omap_device_idle_hwmods+0x20/0x3c
> [ 810.171600][] _omap_device_deactivate+0x58/0x14c
> [ 810.171630][] omap_device_idle+0x4c/0x6c
> [ 810.171661][] platform_pm_runtime_suspend+0x4c/0x74
> [ 810.171691][] __pm_runtime_suspend+0x204/0x34c
> [ 810.171722][] pm_runtime_suspend+0x20/0x34
> [ 810.171752][] platform_pm_runtime_idle+0x8/0x10
> [ 810.171783][] __pm_runtime_idle+0x15c/0x198
> [ 810.171813][] pm_runtime_idle+0x1c/0x30
> [ 810.171844][] platform_pm_suspend_noirq+0x48/0x50
> [ 810.171875][] pm_noirq_op+0xa0/0x184
> [ 810.171905][] dpm_suspend_noirq+0xac/0x188
> [ 810.171936][] suspend_devices_and_enter+0x94/0x1d8
> [ 810.171966][] enter_state+0xbc/0x120
> [ 810.171997][] state_store+0xa4/0xb8
> [ 810.172027][] kobj_attr_store+0x18/0x1c
> [ 810.172088][] sysfs_write_file+0x10c/0x144
> [ 810.172119][] vfs_write+0xb0/0x148
> [ 810.172149][] sys_write+0x3c/0x68
> [ 810.172180][] ret_fast_syscall+0x0/0x3c
The intention of the mutex_lock() in the omap_hwmod_for_each*() case is to
protect against changes to omap_hwmod_list during the list iteration. It
is true that omap_hwmod_list is only likely to change very early in boot,
as the hwmods are registered, so perhaps this is not necessary in
practice. But at least theoretically it seems necessary, since I don't
think that list_for_each_entry() is safe if a list addition is in progress
simultaneously.
On the other hand, taking the mutex during most of the other
omap_hwmod calls, such as
omap_hwmod_{enable,idle,shutdown,enable_clocks,disable_clocks,reset,enable_wakeup,disable_wakeup}
is definitely not needed since those functions do not affect
omap_hwmod_list. The goal of the mutex there was simply to protect
against concurrent calls to those functions. To do that, perhaps the lock
could be moved into the struct omap_hwmod? I believe you suggested this
during our PM discussions in Bengaluru. That would carry a slight memory
space penalty, but would also deserialize hwmod operations on an SMP
system. If you agree, perhaps you might spin a patch for that instead?
- Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] OMAP: omap_hwmod: remove locking from hwmod_for_each iterators
Remove unnecessary locking in the 'for_each' iterators. Any locking should
be taken care of by using hwmod API functions in the functions called by the
iterators.
In addition, having locking here causes lockdep to detect possible circular
dependencies (originally reported by Partha Basak .)
For example, during init (#0 below) you have the hwmod_mutex acquired
(hwmod_for_each_by_class()) then the dpm_list_mtx acquired
(device_pm_add()). Later, during suspend the dpm_list_mtx is aquired
first (dpm_suspend_noirq()), then the omap_hwmod_mutex is acquired
(omap_hwmod_idle()).
[ 810.170593] ===
[ 810.170593] [ INFO: possible circular locking dependency detected ]
[ 810.170623] 2.6.35-rc5-00131-g56e767c-dirty #34
[ 810.170654] ---
[ 810.170654] sh/670 is trying to acquire lock:
[ 810.170684] (omap_hwmod_mutex){+.+.+.}, at: []
omap_hwmod_idle+0x1c/0x38
[ 810.170745]
[ 810.170745] but task is already holding lock:
[ 810.170776] (dpm_list_mtx){+.+...}, at: []
dpm_suspend_noirq+0x28/0x188
[ 810.170806]
[ 810.170837] which lock already depends on the new lock.
[ 810.170837]
[ 810.170837]
[ 810.170837] the existing dependency chain (in reverse order) is:
[ 810.170867]
[ 810.170867] -> #1 (dpm_list_mtx){+.+...}:
[ 810.170898][] lock_acquire+0x60/0x74
[ 810.170959][] mutex_lock_nested+0x58/0x2e4
[ 810.170989][] device_pm_add+0x14/0xcc
[ 810.171020][] device_add+0x3b8/0x564
[ 810.171051][] platform_device_add+0x104/0x160
[ 810.171112][] omap_device_build_ss+0x14c/0x1c8
[ 810.171142][] omap_device_build+0x48/0x50
[ 810.171173][] omap2_init_gpio+0xf0/0x15c
[ 810.171203][] omap_hwmod_for_each_by_class+0x60/0xa4
[ 810.171264][] do_one_initcall+0x58/0x1b4
[ 810.171295][] kernel_init+0x98/0x150
[ 810.171325][] kernel_thread_exit+0x0/0x8
[ 810.171356]
[ 810.171356] -> #0 (omap_hwmod_mutex){+.+.+.}:
[ 810.171386][] __lock_acquire+0x108c/0x1784
[ 810.171447][] lock_acquire+0x60/0x74
[ 810.171478][] mutex_lock_nested+0x58/0x2e4
[ 810.171508][] omap_hwmod_idle+0x1c/0x38
[ 810.171539][] omap_device_idle_hwmods+0x20/0x3c
[ 810.171600][] _omap_device_deactivate+0x58/0x14c
[ 810.171630][] omap_device_idle+0x4c/0x6c
[ 810.171661][] platform_pm_runtime_suspend+0x4c/0x74
[ 810.171691][] __pm_runtime_suspend+0x204/0x34c
[ 810.171722][] pm_runtime_suspend+0x20/0x34
[ 810.171752][] platform_pm_runtime_idle+0x8/0x10
[ 810.171783][] __pm_runtime_idle+0x15c/0x198
[ 810.171813][] pm_runtime_idle+0x1c/0x30
[ 810.171844][] platform_pm_suspend_noirq+0x48/0x50
[ 810.171875][] pm_noirq_op+0xa0/0x184
[ 810.171905][] dpm_suspend_noirq+0xac/0x188
[ 810.171936][] suspend_devices_and_enter+0x94/0x1d8
[ 810.171966][] enter_state+0xbc/0x120
[ 810.171997][] state_store+0xa4/0xb8
[ 810.172027][] kobj_attr_store+0x18/0x1c
[ 810.172088][] sysfs_write_file+0x10c/0x144
[ 810.172119][] vfs_write+0xb0/0x148
[ 810.172149][] sys_write+0x3c/0x68
[ 810.172180][] ret_fast_syscall+0x0/0x3c
[ 810.172210]
[ 810.172210] other info that might help us debug this:
[ 810.172210]
[ 810.172241] 4 locks held by sh/670:
[ 810.172241] #0: (&buffer->mutex){+.+.+.}, at: []
sysfs_write_file+0x28/0x144
[ 810.172302] #1: (s_active#5){.+.+.+}, at: []
sysfs_write_file+0xe8/0x144
[ 810.172363] #2: (pm_mutex){+.+.+.}, at: [] enter_state+0x20/0x120
[ 810.172393] #3: (dpm_list_mtx){+.+...}, at: []
dpm_suspend_noirq+0x28/0x188
Tested-by: Partha Basak
Signed-off-by: Kevin Hilman
---
arch/arm/mach-omap2/omap_hwmod.c |6 --
1 files changed, 0 insertions(+), 6 deletions(-)
diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index cb911d7..5941d5b 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -1204,13 +1204,11 @@ int omap_hwmod_for_each(int (*fn)(struct omap_hwmod
*oh, void *data),
if (!fn)
return -EINVAL;
- mutex_lock(&omap_hwmod_mutex);
list_for_each_entry(temp_oh, &omap_hwmod_list, node) {
ret = (*fn)(temp_oh, data);
if (ret)
break;
}
- mutex_unlock(&omap_hwmod_mutex);
return ret;
}
@@ -1703,8 +1701,6 @@ int omap_hwmod_for_each_by_class(const char *classname,
pr_debug("omap_hwmod: %s: looking for modules of class %s\n",
__func__, classname);
- mutex_lock(&omap_hwmod_mutex);
-
list_for_each_entry(temp_oh, &omap_hwmod_list, node) {
if (!strcmp(temp_oh->class->name, classname)) {
pr_debug("omap_hwmod: %s: %s: calling callback fn\n",
@@ -1715,8 +1711,6 @@
