* Re: [BUG] on sh7372 (mackerel) with today's Linus' (github) master
2011-09-23 8:17 [BUG] on sh7372 (mackerel) with today's Linus' (github) master Guennadi Liakhovetski
@ 2011-09-23 22:21 ` Rafael J. Wysocki
2011-09-23 22:58 ` Rafael J. Wysocki
` (4 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Rafael J. Wysocki @ 2011-09-23 22:21 UTC (permalink / raw)
To: linux-sh
Hi,
On Friday, September 23, 2011, Guennadi Liakhovetski wrote:
> Hi
>
> Not a very pleasant message from an -rc7 kernel:
Well, I wonder why I can't reproduce this.
> [ 9.906250] BUG: sleeping function called from invalid context at /home/lyakh/software/project/24/src/linux-2.6/kernel/mutex.c:271
> [ 9.906250] in_atomic(): 1, irqs_disabled(): 128, pid: 167, name: modprobe
> [ 9.906250] 3 locks held by modprobe/167:
> [ 9.906250] #0: (&__lockdep_no_validate__){......}, at: [<c0176114>] __driver_attach+0x48/0x94
> [ 9.906250] #1: (&__lockdep_no_validate__){......}, at: [<c0176124>] __driver_attach+0x58/0x94
> [ 9.906250] #2: (&(&pcd->lock)->rlock){......}, at: [<c017f0d4>] pm_clk_resume+0x28/0x88
> [ 9.906250] Backtrace:
> [ 9.906250] [<c0012434>] (dump_backtrace+0x0/0x110) from [<c024f8f0>] (dump_stack+0x18/0x1c)
> [ 9.906250] r6:00000000 r5:cf0dde20 r4:cf798000 r3:60000093
> [ 9.906250] [<c024f8d8>] (dump_stack+0x0/0x1c) from [<c001da88>] (__might_sleep+0x100/0x120)
> [ 9.906250] [<c001d988>] (__might_sleep+0x0/0x120) from [<c025192c>] (mutex_lock_nested+0x2c/0x2fc)
> [ 9.906250] r4:c0356808
> [ 9.906250] [<c0251900>] (mutex_lock_nested+0x0/0x2fc) from [<c01a5a44>] (clk_get_sys+0x2c/0xf4)
> [ 9.906250] r8:00000000 r7:00000000 r6:00000000 r5:cf0dde20 r4:c0356808
> [ 9.906250] [<c01a5a18>] (clk_get_sys+0x0/0xf4) from [<c01a5b34>] (clk_get+0x28/0x2c)
> [ 9.906250] [<c01a5b0c>] (clk_get+0x0/0x2c) from [<c017f094>] (pm_clk_acquire+0x18/0x30)
> [ 9.906250] [<c017f07c>] (pm_clk_acquire+0x0/0x30) from [<c017f0f8>] (pm_clk_resume+0x4c/0x88)
> [ 9.906250] r4:cf0dcf40 r3:00000000
> [ 9.906250] [<c017f0ac>] (pm_clk_resume+0x0/0x88) from [<c017c6b8>] (rpm_callback+0x4c/0x6c)
> [ 9.906250] r8:cf798000 r7:c0333be0 r6:c0333be0 r5:c0333b50 r4:c017f0ac
> [ 9.906250] r3:00000001
> [ 9.906250] [<c017c66c>] (rpm_callback+0x0/0x6c) from [<c017d3f0>] (rpm_resume+0x300/0x3d4)
> [ 9.906250] r6:00000000 r5:c03520c8 r4:c0333b50 r3:c0331fb8
> [ 9.906250] [<c017d0f0>] (rpm_resume+0x0/0x3d4) from [<c017d750>] (__pm_runtime_resume+0x50/0x68)
> [ 9.906250] [<c017d700>] (__pm_runtime_resume+0x0/0x68) from [<bf016834>] (sh_mobile_i2c_xfer+0x3c/0x37c [i2c_sh_mobile])
> [ 9.906250] r7:00000002 r6:00000009 r5:cf799cc0 r4:cf64f800
> [ 9.906250] [<bf0167f8>] (sh_mobile_i2c_xfer+0x0/0x37c [i2c_sh_mobile]) from [<bf0016ec>] (i2c_transfer+0x98/0xe4 [i2c_core])
> [ 9.906250] [<bf001654>] (i2c_transfer+0x0/0xe4 [i2c_core]) from [<bf001bb0>] (i2c_smbus_xfer+0x3cc/0x504 [i2c_core])
> [ 9.906250] [<bf0017e4>] (i2c_smbus_xfer+0x0/0x504 [i2c_core]) from [<bf001fdc>] (i2c_smbus_read_byte_data+0x3c/0x4c [i2c_core])
> [ 9.906250] [<bf001fa0>] (i2c_smbus_read_byte_data+0x0/0x4c [i2c_core]) from [<bf0910d8>] (tca6416_read_reg+0x48/0x8c [tca6416_keypad])
> [ 9.906250] [<bf091090>] (tca6416_read_reg+0x0/0x8c [tca6416_keypad]) from [<bf091500>] (tca6416_keypad_probe+0x1dc/0x3a0 [tca6416_keypad])
> [ 9.906250] r7:00000030 r6:c033d204 r5:cf3dd800 r4:cf1ff0a0
> [ 9.906250] [<bf091324>] (tca6416_keypad_probe+0x0/0x3a0 [tca6416_keypad]) from [<bf0005cc>] (i2c_device_probe+0x7c/0xa4 [i2c_core])
> [ 9.906250] [<bf000550>] (i2c_device_probe+0x0/0xa4 [i2c_core]) from [<c0176010>] (driver_probe_device+0xd0/0x18c)
> [ 9.906250] r6:bf091878 r5:cf64f054 r4:cf64f020 r3:bf000550
> [ 9.906250] [<c0175f40>] (driver_probe_device+0x0/0x18c) from [<c017613c>] (__driver_attach+0x70/0x94)
> [ 9.906250] r7:00000000 r6:bf091878 r5:cf64f054 r4:cf64f020
> [ 9.906250] [<c01760cc>] (__driver_attach+0x0/0x94) from [<c01757a4>] (bus_for_each_dev+0x54/0x84)
> [ 9.906250] r6:00000000 r5:c01760cc r4:bf091878 r3:00000000
> [ 9.906250] [<c0175750>] (bus_for_each_dev+0x0/0x84) from [<c0175e34>] (driver_attach+0x20/0x28)
> [ 9.906250] r6:bf002df8 r5:cf3ef440 r4:bf091878
> [ 9.906250] [<c0175e14>] (driver_attach+0x0/0x28) from [<c017506c>] (bus_add_driver+0xa8/0x228)
> [ 9.906250] [<c0174fc4>] (bus_add_driver+0x0/0x228) from [<c01767c8>] (driver_register+0xb0/0x140)
> [ 9.906250] [<c0176718>] (driver_register+0x0/0x140) from [<bf0009a4>] (i2c_register_driver+0x48/0xb4 [i2c_core])
> [ 9.906250] r8:00000001 r7:00000000 r6:00000001 r5:bf093000 r4:bf091850
> [ 9.906250] r3:bf002df8
> [ 9.906250] [<bf00095c>] (i2c_register_driver+0x0/0xb4 [i2c_core]) from [<bf093018>] (tca6416_keypad_init+0x18/0x24 [tca6416_keypad])
> [ 9.906250] r5:bf093000 r4:bf0918c4
> [ 9.906250] [<bf093000>] (tca6416_keypad_init+0x0/0x24 [tca6416_keypad]) from [<c0009374>] (do_one_initcall+0xa0/0x170)
> [ 9.906250] [<c00092d4>] (do_one_initcall+0x0/0x170) from [<c005583c>] (sys_init_module+0xd24/0xf48)
> [ 9.906250] r9:0000001b r8:00000001 r7:cf5793a0 r6:00000001 r5:bf09190c
> [ 9.906250] r4:bf0918c4
> [ 9.906250] [<c0054b18>] (sys_init_module+0x0/0xf48) from [<c000efc0>] (ret_fast_syscall+0x0/0x30)
> [ 9.960937] input: tca6408-keys as /devices/platform/i2c-sh_mobile.0/i2c-0/0-0020/input/input0
There is a bug in pm_clk_suspend() (and analogously in pm_clk_resume())
that calls pm_clk_acquire() under a spinlock, but pm_clk_acquire()
calls clk_get().
I'm not sure how to fix this at the moment. We may need to revert commits
b7ab83e and 5a50a01, although I would hate that, because it would require us
to fix sh-sci runtime PM in a different way.
I'll try to figure out something over the weekend.
Thanks,
Rafael
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [BUG] on sh7372 (mackerel) with today's Linus' (github) master
2011-09-23 8:17 [BUG] on sh7372 (mackerel) with today's Linus' (github) master Guennadi Liakhovetski
2011-09-23 22:21 ` Rafael J. Wysocki
@ 2011-09-23 22:58 ` Rafael J. Wysocki
2011-09-24 8:09 ` Russell King - ARM Linux
` (3 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Rafael J. Wysocki @ 2011-09-23 22:58 UTC (permalink / raw)
To: linux-sh
On Saturday, September 24, 2011, Rafael J. Wysocki wrote:
> Hi,
>
> On Friday, September 23, 2011, Guennadi Liakhovetski wrote:
> > Hi
> >
> > Not a very pleasant message from an -rc7 kernel:
>
> Well, I wonder why I can't reproduce this.
>
> > [ 9.906250] BUG: sleeping function called from invalid context at /home/lyakh/software/project/24/src/linux-2.6/kernel/mutex.c:271
> > [ 9.906250] in_atomic(): 1, irqs_disabled(): 128, pid: 167, name: modprobe
> > [ 9.906250] 3 locks held by modprobe/167:
> > [ 9.906250] #0: (&__lockdep_no_validate__){......}, at: [<c0176114>] __driver_attach+0x48/0x94
> > [ 9.906250] #1: (&__lockdep_no_validate__){......}, at: [<c0176124>] __driver_attach+0x58/0x94
> > [ 9.906250] #2: (&(&pcd->lock)->rlock){......}, at: [<c017f0d4>] pm_clk_resume+0x28/0x88
> > [ 9.906250] Backtrace:
> > [ 9.906250] [<c0012434>] (dump_backtrace+0x0/0x110) from [<c024f8f0>] (dump_stack+0x18/0x1c)
> > [ 9.906250] r6:00000000 r5:cf0dde20 r4:cf798000 r3:60000093
> > [ 9.906250] [<c024f8d8>] (dump_stack+0x0/0x1c) from [<c001da88>] (__might_sleep+0x100/0x120)
> > [ 9.906250] [<c001d988>] (__might_sleep+0x0/0x120) from [<c025192c>] (mutex_lock_nested+0x2c/0x2fc)
> > [ 9.906250] r4:c0356808
> > [ 9.906250] [<c0251900>] (mutex_lock_nested+0x0/0x2fc) from [<c01a5a44>] (clk_get_sys+0x2c/0xf4)
> > [ 9.906250] r8:00000000 r7:00000000 r6:00000000 r5:cf0dde20 r4:c0356808
> > [ 9.906250] [<c01a5a18>] (clk_get_sys+0x0/0xf4) from [<c01a5b34>] (clk_get+0x28/0x2c)
> > [ 9.906250] [<c01a5b0c>] (clk_get+0x0/0x2c) from [<c017f094>] (pm_clk_acquire+0x18/0x30)
> > [ 9.906250] [<c017f07c>] (pm_clk_acquire+0x0/0x30) from [<c017f0f8>] (pm_clk_resume+0x4c/0x88)
> > [ 9.906250] r4:cf0dcf40 r3:00000000
> > [ 9.906250] [<c017f0ac>] (pm_clk_resume+0x0/0x88) from [<c017c6b8>] (rpm_callback+0x4c/0x6c)
> > [ 9.906250] r8:cf798000 r7:c0333be0 r6:c0333be0 r5:c0333b50 r4:c017f0ac
> > [ 9.906250] r3:00000001
> > [ 9.906250] [<c017c66c>] (rpm_callback+0x0/0x6c) from [<c017d3f0>] (rpm_resume+0x300/0x3d4)
> > [ 9.906250] r6:00000000 r5:c03520c8 r4:c0333b50 r3:c0331fb8
> > [ 9.906250] [<c017d0f0>] (rpm_resume+0x0/0x3d4) from [<c017d750>] (__pm_runtime_resume+0x50/0x68)
> > [ 9.906250] [<c017d700>] (__pm_runtime_resume+0x0/0x68) from [<bf016834>] (sh_mobile_i2c_xfer+0x3c/0x37c [i2c_sh_mobile])
> > [ 9.906250] r7:00000002 r6:00000009 r5:cf799cc0 r4:cf64f800
> > [ 9.906250] [<bf0167f8>] (sh_mobile_i2c_xfer+0x0/0x37c [i2c_sh_mobile]) from [<bf0016ec>] (i2c_transfer+0x98/0xe4 [i2c_core])
> > [ 9.906250] [<bf001654>] (i2c_transfer+0x0/0xe4 [i2c_core]) from [<bf001bb0>] (i2c_smbus_xfer+0x3cc/0x504 [i2c_core])
> > [ 9.906250] [<bf0017e4>] (i2c_smbus_xfer+0x0/0x504 [i2c_core]) from [<bf001fdc>] (i2c_smbus_read_byte_data+0x3c/0x4c [i2c_core])
> > [ 9.906250] [<bf001fa0>] (i2c_smbus_read_byte_data+0x0/0x4c [i2c_core]) from [<bf0910d8>] (tca6416_read_reg+0x48/0x8c [tca6416_keypad])
> > [ 9.906250] [<bf091090>] (tca6416_read_reg+0x0/0x8c [tca6416_keypad]) from [<bf091500>] (tca6416_keypad_probe+0x1dc/0x3a0 [tca6416_keypad])
> > [ 9.906250] r7:00000030 r6:c033d204 r5:cf3dd800 r4:cf1ff0a0
> > [ 9.906250] [<bf091324>] (tca6416_keypad_probe+0x0/0x3a0 [tca6416_keypad]) from [<bf0005cc>] (i2c_device_probe+0x7c/0xa4 [i2c_core])
> > [ 9.906250] [<bf000550>] (i2c_device_probe+0x0/0xa4 [i2c_core]) from [<c0176010>] (driver_probe_device+0xd0/0x18c)
> > [ 9.906250] r6:bf091878 r5:cf64f054 r4:cf64f020 r3:bf000550
> > [ 9.906250] [<c0175f40>] (driver_probe_device+0x0/0x18c) from [<c017613c>] (__driver_attach+0x70/0x94)
> > [ 9.906250] r7:00000000 r6:bf091878 r5:cf64f054 r4:cf64f020
> > [ 9.906250] [<c01760cc>] (__driver_attach+0x0/0x94) from [<c01757a4>] (bus_for_each_dev+0x54/0x84)
> > [ 9.906250] r6:00000000 r5:c01760cc r4:bf091878 r3:00000000
> > [ 9.906250] [<c0175750>] (bus_for_each_dev+0x0/0x84) from [<c0175e34>] (driver_attach+0x20/0x28)
> > [ 9.906250] r6:bf002df8 r5:cf3ef440 r4:bf091878
> > [ 9.906250] [<c0175e14>] (driver_attach+0x0/0x28) from [<c017506c>] (bus_add_driver+0xa8/0x228)
> > [ 9.906250] [<c0174fc4>] (bus_add_driver+0x0/0x228) from [<c01767c8>] (driver_register+0xb0/0x140)
> > [ 9.906250] [<c0176718>] (driver_register+0x0/0x140) from [<bf0009a4>] (i2c_register_driver+0x48/0xb4 [i2c_core])
> > [ 9.906250] r8:00000001 r7:00000000 r6:00000001 r5:bf093000 r4:bf091850
> > [ 9.906250] r3:bf002df8
> > [ 9.906250] [<bf00095c>] (i2c_register_driver+0x0/0xb4 [i2c_core]) from [<bf093018>] (tca6416_keypad_init+0x18/0x24 [tca6416_keypad])
> > [ 9.906250] r5:bf093000 r4:bf0918c4
> > [ 9.906250] [<bf093000>] (tca6416_keypad_init+0x0/0x24 [tca6416_keypad]) from [<c0009374>] (do_one_initcall+0xa0/0x170)
> > [ 9.906250] [<c00092d4>] (do_one_initcall+0x0/0x170) from [<c005583c>] (sys_init_module+0xd24/0xf48)
> > [ 9.906250] r9:0000001b r8:00000001 r7:cf5793a0 r6:00000001 r5:bf09190c
> > [ 9.906250] r4:bf0918c4
> > [ 9.906250] [<c0054b18>] (sys_init_module+0x0/0xf48) from [<c000efc0>] (ret_fast_syscall+0x0/0x30)
> > [ 9.960937] input: tca6408-keys as /devices/platform/i2c-sh_mobile.0/i2c-0/0-0020/input/input0
>
> There is a bug in pm_clk_suspend() (and analogously in pm_clk_resume())
> that calls pm_clk_acquire() under a spinlock, but pm_clk_acquire()
> calls clk_get().
>
> I'm not sure how to fix this at the moment. We may need to revert commits
> b7ab83e and 5a50a01, although I would hate that, because it would require us
> to fix sh-sci runtime PM in a different way.
>
> I'll try to figure out something over the weekend.
Well, since nobody seems to execute anything that may sleep under
clocks_mutex, perhaps we can do something like this:
---
drivers/clk/clkdev.c | 25 ++++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
Index: linux/drivers/clk/clkdev.c
=================================--- linux.orig/drivers/clk/clkdev.c
+++ linux/drivers/clk/clkdev.c
@@ -21,7 +21,7 @@
#include <linux/clkdev.h>
static LIST_HEAD(clocks);
-static DEFINE_MUTEX(clocks_mutex);
+static DEFINE_SPINLOCK(clocks_lock);
/*
* Find the correct struct clk for the device and connection ID.
@@ -64,12 +64,13 @@ static struct clk_lookup *clk_find(const
struct clk *clk_get_sys(const char *dev_id, const char *con_id)
{
struct clk_lookup *cl;
+ unsigned long flags;
- mutex_lock(&clocks_mutex);
+ spin_lock_irqsave(&clocks_lock, flags);
cl = clk_find(dev_id, con_id);
if (cl && !__clk_get(cl->clk))
cl = NULL;
- mutex_unlock(&clocks_mutex);
+ spin_unlock_irqrestore(&clocks_lock, flags);
return cl ? cl->clk : ERR_PTR(-ENOENT);
}
@@ -91,20 +92,24 @@ EXPORT_SYMBOL(clk_put);
void clkdev_add(struct clk_lookup *cl)
{
- mutex_lock(&clocks_mutex);
+ unsigned long flags;
+
+ spin_lock_irqsave(&clocks_lock, flags);
list_add_tail(&cl->node, &clocks);
- mutex_unlock(&clocks_mutex);
+ spin_unlock_irqrestore(&clocks_lock, flags);
}
EXPORT_SYMBOL(clkdev_add);
void __init clkdev_add_table(struct clk_lookup *cl, size_t num)
{
- mutex_lock(&clocks_mutex);
+ unsigned long flags;
+
+ spin_lock_irqsave(&clocks_lock, flags);
while (num--) {
list_add_tail(&cl->node, &clocks);
cl++;
}
- mutex_unlock(&clocks_mutex);
+ spin_unlock_irqrestore(&clocks_lock, flags);
}
#define MAX_DEV_ID 20
@@ -167,9 +172,11 @@ EXPORT_SYMBOL(clk_add_alias);
*/
void clkdev_drop(struct clk_lookup *cl)
{
- mutex_lock(&clocks_mutex);
+ unsigned long flags;
+
+ spin_lock_irqsave(&clocks_lock, flags);
list_del(&cl->node);
- mutex_unlock(&clocks_mutex);
+ spin_unlock_irqrestore(&clocks_lock, flags);
kfree(cl);
}
EXPORT_SYMBOL(clkdev_drop);
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [BUG] on sh7372 (mackerel) with today's Linus' (github) master
2011-09-23 8:17 [BUG] on sh7372 (mackerel) with today's Linus' (github) master Guennadi Liakhovetski
2011-09-23 22:21 ` Rafael J. Wysocki
2011-09-23 22:58 ` Rafael J. Wysocki
@ 2011-09-24 8:09 ` Russell King - ARM Linux
2011-09-24 13:07 ` Rafael J. Wysocki
` (2 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Russell King - ARM Linux @ 2011-09-24 8:09 UTC (permalink / raw)
To: linux-sh
On Sat, Sep 24, 2011 at 12:58:37AM +0200, Rafael J. Wysocki wrote:
> Well, since nobody seems to execute anything that may sleep under
> clocks_mutex, perhaps we can do something like this:
No - a spinlock prevents preemption, a mutex does not.
It's runtime PM which is doing stuff wrong here. You're not supposed to
give up the clk struct with it enabled.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [BUG] on sh7372 (mackerel) with today's Linus' (github) master
2011-09-23 8:17 [BUG] on sh7372 (mackerel) with today's Linus' (github) master Guennadi Liakhovetski
` (2 preceding siblings ...)
2011-09-24 8:09 ` Russell King - ARM Linux
@ 2011-09-24 13:07 ` Rafael J. Wysocki
2011-09-26 8:39 ` Russell King - ARM Linux
2011-09-26 17:25 ` Guennadi Liakhovetski
5 siblings, 0 replies; 7+ messages in thread
From: Rafael J. Wysocki @ 2011-09-24 13:07 UTC (permalink / raw)
To: linux-sh
On Saturday, September 24, 2011, Russell King - ARM Linux wrote:
> On Sat, Sep 24, 2011 at 12:58:37AM +0200, Rafael J. Wysocki wrote:
> > Well, since nobody seems to execute anything that may sleep under
> > clocks_mutex, perhaps we can do something like this:
>
> No - a spinlock prevents preemption, a mutex does not.
>
> It's runtime PM which is doing stuff wrong here. You're not supposed to
> give up the clk struct with it enabled.
OK, so below is a fix confined to clock_ops.c.
Thanks,
Rafael
---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: PM / Clocks: Do not acquire a mutex under a spinlock
Commit b7ab83e (PM: Use spinlock instead of mutex in clock
management functions) introduced a regression causing clocks_mutex
to be acquired under a spinlock. This happens because
pm_clk_suspend() and pm_clk_resume() call pm_clk_acquire() under
pcd->lock, but pm_clk_acquire() executes clk_get() which causes
clocks_mutex to be acquired. Similarly, __pm_clk_remove(),
executed under pcd->lock, calls clk_put(), which also causes
clocks_mutex to be acquired.
To fix those problems make pm_clk_add() call pm_clk_acquire(), so
that pm_clk_suspend() and pm_clk_resume() don't have to do that.
Change pm_clk_remove() and pm_clk_destroy() to separate
modifications of the pcd->clock_list list from the actual removal of
PM clock entry objects done by __pm_clk_remove().
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
drivers/base/power/clock_ops.c | 75 ++++++++++++++++++++---------------------
1 file changed, 38 insertions(+), 37 deletions(-)
Index: linux/drivers/base/power/clock_ops.c
=================================--- linux.orig/drivers/base/power/clock_ops.c
+++ linux/drivers/base/power/clock_ops.c
@@ -42,6 +42,22 @@ static struct pm_clk_data *__to_pcd(stru
}
/**
+ * pm_clk_acquire - Acquire a device clock.
+ * @dev: Device whose clock is to be acquired.
+ * @ce: PM clock entry corresponding to the clock.
+ */
+static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
+{
+ ce->clk = clk_get(dev, ce->con_id);
+ if (IS_ERR(ce->clk)) {
+ ce->status = PCE_STATUS_ERROR;
+ } else {
+ ce->status = PCE_STATUS_ACQUIRED;
+ dev_dbg(dev, "Clock %s managed by runtime PM.\n", ce->con_id);
+ }
+}
+
+/**
* pm_clk_add - Start using a device clock for power management.
* @dev: Device whose clock is going to be used for power management.
* @con_id: Connection ID of the clock.
@@ -73,6 +89,8 @@ int pm_clk_add(struct device *dev, const
}
}
+ pm_clk_acquire(dev, ce);
+
spin_lock_irq(&pcd->lock);
list_add_tail(&ce->node, &pcd->clock_list);
spin_unlock_irq(&pcd->lock);
@@ -82,17 +100,12 @@ int pm_clk_add(struct device *dev, const
/**
* __pm_clk_remove - Destroy PM clock entry.
* @ce: PM clock entry to destroy.
- *
- * This routine must be called under the spinlock protecting the PM list of
- * clocks corresponding the the @ce's device.
*/
static void __pm_clk_remove(struct pm_clock_entry *ce)
{
if (!ce)
return;
- list_del(&ce->node);
-
if (ce->status < PCE_STATUS_ERROR) {
if (ce->status = PCE_STATUS_ENABLED)
clk_disable(ce->clk);
@@ -126,18 +139,22 @@ void pm_clk_remove(struct device *dev, c
spin_lock_irq(&pcd->lock);
list_for_each_entry(ce, &pcd->clock_list, node) {
- if (!con_id && !ce->con_id) {
- __pm_clk_remove(ce);
- break;
- } else if (!con_id || !ce->con_id) {
+ if (!con_id && !ce->con_id)
+ goto remove;
+ else if (!con_id || !ce->con_id)
continue;
- } else if (!strcmp(con_id, ce->con_id)) {
- __pm_clk_remove(ce);
- break;
- }
+ else if (!strcmp(con_id, ce->con_id))
+ goto remove;
}
spin_unlock_irq(&pcd->lock);
+ return;
+
+ remove:
+ list_del(&ce->node);
+ spin_unlock_irq(&pcd->lock);
+
+ __pm_clk_remove(ce);
}
/**
@@ -175,20 +192,27 @@ void pm_clk_destroy(struct device *dev)
{
struct pm_clk_data *pcd = __to_pcd(dev);
struct pm_clock_entry *ce, *c;
+ struct list_head list;
if (!pcd)
return;
dev->power.subsys_data = NULL;
+ INIT_LIST_HEAD(&list);
spin_lock_irq(&pcd->lock);
list_for_each_entry_safe_reverse(ce, c, &pcd->clock_list, node)
- __pm_clk_remove(ce);
+ list_move(&ce->node, &list);
spin_unlock_irq(&pcd->lock);
kfree(pcd);
+
+ list_for_each_entry_safe_reverse(ce, c, &list, node) {
+ list_del(&ce->node);
+ __pm_clk_remove(ce);
+ }
}
#endif /* CONFIG_PM */
@@ -196,23 +220,6 @@ void pm_clk_destroy(struct device *dev)
#ifdef CONFIG_PM_RUNTIME
/**
- * pm_clk_acquire - Acquire a device clock.
- * @dev: Device whose clock is to be acquired.
- * @con_id: Connection ID of the clock.
- */
-static void pm_clk_acquire(struct device *dev,
- struct pm_clock_entry *ce)
-{
- ce->clk = clk_get(dev, ce->con_id);
- if (IS_ERR(ce->clk)) {
- ce->status = PCE_STATUS_ERROR;
- } else {
- ce->status = PCE_STATUS_ACQUIRED;
- dev_dbg(dev, "Clock %s managed by runtime PM.\n", ce->con_id);
- }
-}
-
-/**
* pm_clk_suspend - Disable clocks in a device's PM clock list.
* @dev: Device to disable the clocks for.
*/
@@ -230,9 +237,6 @@ int pm_clk_suspend(struct device *dev)
spin_lock_irqsave(&pcd->lock, flags);
list_for_each_entry_reverse(ce, &pcd->clock_list, node) {
- if (ce->status = PCE_STATUS_NONE)
- pm_clk_acquire(dev, ce);
-
if (ce->status < PCE_STATUS_ERROR) {
clk_disable(ce->clk);
ce->status = PCE_STATUS_ACQUIRED;
@@ -262,9 +266,6 @@ int pm_clk_resume(struct device *dev)
spin_lock_irqsave(&pcd->lock, flags);
list_for_each_entry(ce, &pcd->clock_list, node) {
- if (ce->status = PCE_STATUS_NONE)
- pm_clk_acquire(dev, ce);
-
if (ce->status < PCE_STATUS_ERROR) {
clk_enable(ce->clk);
ce->status = PCE_STATUS_ENABLED;
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [BUG] on sh7372 (mackerel) with today's Linus' (github) master
2011-09-23 8:17 [BUG] on sh7372 (mackerel) with today's Linus' (github) master Guennadi Liakhovetski
` (3 preceding siblings ...)
2011-09-24 13:07 ` Rafael J. Wysocki
@ 2011-09-26 8:39 ` Russell King - ARM Linux
2011-09-26 17:25 ` Guennadi Liakhovetski
5 siblings, 0 replies; 7+ messages in thread
From: Russell King - ARM Linux @ 2011-09-26 8:39 UTC (permalink / raw)
To: linux-sh
On Sat, Sep 24, 2011 at 03:07:07PM +0200, Rafael J. Wysocki wrote:
> On Saturday, September 24, 2011, Russell King - ARM Linux wrote:
> > On Sat, Sep 24, 2011 at 12:58:37AM +0200, Rafael J. Wysocki wrote:
> > > Well, since nobody seems to execute anything that may sleep under
> > > clocks_mutex, perhaps we can do something like this:
> >
> > No - a spinlock prevents preemption, a mutex does not.
> >
> > It's runtime PM which is doing stuff wrong here. You're not supposed to
> > give up the clk struct with it enabled.
>
> OK, so below is a fix confined to clock_ops.c.
Looks fine from a read-through the patch, thanks.
Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [BUG] on sh7372 (mackerel) with today's Linus' (github) master
2011-09-23 8:17 [BUG] on sh7372 (mackerel) with today's Linus' (github) master Guennadi Liakhovetski
` (4 preceding siblings ...)
2011-09-26 8:39 ` Russell King - ARM Linux
@ 2011-09-26 17:25 ` Guennadi Liakhovetski
5 siblings, 0 replies; 7+ messages in thread
From: Guennadi Liakhovetski @ 2011-09-26 17:25 UTC (permalink / raw)
To: linux-sh
On Sat, 24 Sep 2011, Rafael J. Wysocki wrote:
> On Saturday, September 24, 2011, Russell King - ARM Linux wrote:
> > On Sat, Sep 24, 2011 at 12:58:37AM +0200, Rafael J. Wysocki wrote:
> > > Well, since nobody seems to execute anything that may sleep under
> > > clocks_mutex, perhaps we can do something like this:
> >
> > No - a spinlock prevents preemption, a mutex does not.
> >
> > It's runtime PM which is doing stuff wrong here. You're not supposed to
> > give up the clk struct with it enabled.
>
> OK, so below is a fix confined to clock_ops.c.
Yes, works for me:
Tested-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Thanks
Guennadi
>
> Thanks,
> Rafael
>
>
> ---
> From: Rafael J. Wysocki <rjw@sisk.pl>
> Subject: PM / Clocks: Do not acquire a mutex under a spinlock
>
> Commit b7ab83e (PM: Use spinlock instead of mutex in clock
> management functions) introduced a regression causing clocks_mutex
> to be acquired under a spinlock. This happens because
> pm_clk_suspend() and pm_clk_resume() call pm_clk_acquire() under
> pcd->lock, but pm_clk_acquire() executes clk_get() which causes
> clocks_mutex to be acquired. Similarly, __pm_clk_remove(),
> executed under pcd->lock, calls clk_put(), which also causes
> clocks_mutex to be acquired.
>
> To fix those problems make pm_clk_add() call pm_clk_acquire(), so
> that pm_clk_suspend() and pm_clk_resume() don't have to do that.
> Change pm_clk_remove() and pm_clk_destroy() to separate
> modifications of the pcd->clock_list list from the actual removal of
> PM clock entry objects done by __pm_clk_remove().
>
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
> drivers/base/power/clock_ops.c | 75 ++++++++++++++++++++---------------------
> 1 file changed, 38 insertions(+), 37 deletions(-)
>
> Index: linux/drivers/base/power/clock_ops.c
> =================================> --- linux.orig/drivers/base/power/clock_ops.c
> +++ linux/drivers/base/power/clock_ops.c
> @@ -42,6 +42,22 @@ static struct pm_clk_data *__to_pcd(stru
> }
>
> /**
> + * pm_clk_acquire - Acquire a device clock.
> + * @dev: Device whose clock is to be acquired.
> + * @ce: PM clock entry corresponding to the clock.
> + */
> +static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
> +{
> + ce->clk = clk_get(dev, ce->con_id);
> + if (IS_ERR(ce->clk)) {
> + ce->status = PCE_STATUS_ERROR;
> + } else {
> + ce->status = PCE_STATUS_ACQUIRED;
> + dev_dbg(dev, "Clock %s managed by runtime PM.\n", ce->con_id);
> + }
> +}
> +
> +/**
> * pm_clk_add - Start using a device clock for power management.
> * @dev: Device whose clock is going to be used for power management.
> * @con_id: Connection ID of the clock.
> @@ -73,6 +89,8 @@ int pm_clk_add(struct device *dev, const
> }
> }
>
> + pm_clk_acquire(dev, ce);
> +
> spin_lock_irq(&pcd->lock);
> list_add_tail(&ce->node, &pcd->clock_list);
> spin_unlock_irq(&pcd->lock);
> @@ -82,17 +100,12 @@ int pm_clk_add(struct device *dev, const
> /**
> * __pm_clk_remove - Destroy PM clock entry.
> * @ce: PM clock entry to destroy.
> - *
> - * This routine must be called under the spinlock protecting the PM list of
> - * clocks corresponding the the @ce's device.
> */
> static void __pm_clk_remove(struct pm_clock_entry *ce)
> {
> if (!ce)
> return;
>
> - list_del(&ce->node);
> -
> if (ce->status < PCE_STATUS_ERROR) {
> if (ce->status = PCE_STATUS_ENABLED)
> clk_disable(ce->clk);
> @@ -126,18 +139,22 @@ void pm_clk_remove(struct device *dev, c
> spin_lock_irq(&pcd->lock);
>
> list_for_each_entry(ce, &pcd->clock_list, node) {
> - if (!con_id && !ce->con_id) {
> - __pm_clk_remove(ce);
> - break;
> - } else if (!con_id || !ce->con_id) {
> + if (!con_id && !ce->con_id)
> + goto remove;
> + else if (!con_id || !ce->con_id)
> continue;
> - } else if (!strcmp(con_id, ce->con_id)) {
> - __pm_clk_remove(ce);
> - break;
> - }
> + else if (!strcmp(con_id, ce->con_id))
> + goto remove;
> }
>
> spin_unlock_irq(&pcd->lock);
> + return;
> +
> + remove:
> + list_del(&ce->node);
> + spin_unlock_irq(&pcd->lock);
> +
> + __pm_clk_remove(ce);
> }
>
> /**
> @@ -175,20 +192,27 @@ void pm_clk_destroy(struct device *dev)
> {
> struct pm_clk_data *pcd = __to_pcd(dev);
> struct pm_clock_entry *ce, *c;
> + struct list_head list;
>
> if (!pcd)
> return;
>
> dev->power.subsys_data = NULL;
> + INIT_LIST_HEAD(&list);
>
> spin_lock_irq(&pcd->lock);
>
> list_for_each_entry_safe_reverse(ce, c, &pcd->clock_list, node)
> - __pm_clk_remove(ce);
> + list_move(&ce->node, &list);
>
> spin_unlock_irq(&pcd->lock);
>
> kfree(pcd);
> +
> + list_for_each_entry_safe_reverse(ce, c, &list, node) {
> + list_del(&ce->node);
> + __pm_clk_remove(ce);
> + }
> }
>
> #endif /* CONFIG_PM */
> @@ -196,23 +220,6 @@ void pm_clk_destroy(struct device *dev)
> #ifdef CONFIG_PM_RUNTIME
>
> /**
> - * pm_clk_acquire - Acquire a device clock.
> - * @dev: Device whose clock is to be acquired.
> - * @con_id: Connection ID of the clock.
> - */
> -static void pm_clk_acquire(struct device *dev,
> - struct pm_clock_entry *ce)
> -{
> - ce->clk = clk_get(dev, ce->con_id);
> - if (IS_ERR(ce->clk)) {
> - ce->status = PCE_STATUS_ERROR;
> - } else {
> - ce->status = PCE_STATUS_ACQUIRED;
> - dev_dbg(dev, "Clock %s managed by runtime PM.\n", ce->con_id);
> - }
> -}
> -
> -/**
> * pm_clk_suspend - Disable clocks in a device's PM clock list.
> * @dev: Device to disable the clocks for.
> */
> @@ -230,9 +237,6 @@ int pm_clk_suspend(struct device *dev)
> spin_lock_irqsave(&pcd->lock, flags);
>
> list_for_each_entry_reverse(ce, &pcd->clock_list, node) {
> - if (ce->status = PCE_STATUS_NONE)
> - pm_clk_acquire(dev, ce);
> -
> if (ce->status < PCE_STATUS_ERROR) {
> clk_disable(ce->clk);
> ce->status = PCE_STATUS_ACQUIRED;
> @@ -262,9 +266,6 @@ int pm_clk_resume(struct device *dev)
> spin_lock_irqsave(&pcd->lock, flags);
>
> list_for_each_entry(ce, &pcd->clock_list, node) {
> - if (ce->status = PCE_STATUS_NONE)
> - pm_clk_acquire(dev, ce);
> -
> if (ce->status < PCE_STATUS_ERROR) {
> clk_enable(ce->clk);
> ce->status = PCE_STATUS_ENABLED;
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 7+ messages in thread