[BUG] on sh7372 (mackerel) with today's Linus' (github) master

[BUG] on sh7372 (mackerel) with today's Linus' (github) master

[Guennadi Liakhovetski]

Hi

Not a very pleasant message from an -rc7 kernel:


[    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

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [BUG] on sh7372 (mackerel) with today's Linus' (github) master

[Rafael J. Wysocki]

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

Re: [BUG] on sh7372 (mackerel) with today's Linus' (github) master

[Rafael J. Wysocki]

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);

Re: [BUG] on sh7372 (mackerel) with today's Linus' (github) master

[Russell King - ARM Linux]

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.

Re: [BUG] on sh7372 (mackerel) with today's Linus' (github) master

[Rafael J. Wysocki]

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;

Re: [BUG] on sh7372 (mackerel) with today's Linus' (github) master

[Russell King - ARM Linux]

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>

Re: [BUG] on sh7372 (mackerel) with today's Linus' (github) master

[Guennadi Liakhovetski]

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/