On Monday 21 January 2008, Peter Zijlstra wrote:
>
> On Fri, 2008-01-18 at 14:29 -0800, David Brownell wrote:
> > EXPERIMENTAL and incomplete patch to make LOCKDEP behave better in
> > the face of irq chaining, by annotating irqs with a lock_class which
> > can reflect that hierarchy.
>
> This is too short for me (who has no clue about genirq) to understand
> what this patch does or why it does it.
I thought the example was complete ...
> > This version of the patch is incomplete in at least two respects:
> >
> > - There's no spin_lock_irq_nested() primitive, so that locking calls
> > on irq probing (yeech!) paths must ignore the annotations.
> >
> > ==> LOCKDEP feature is evidently missing:
> > spin_lock_irq_nested(lock_ptr, lock_class)
>
> This rant is more lines than adding the API :-/ the reason for it not
> being there is simple, it wasn't needed up until now.
I suspected that was the case, but for all I knew there was some
religious objection. But in any case, there'd be little point in
fixing that if the more significant issue can't get resolved.
> > - We'd really need new API to declare the parent/child relationships
> > between IRQs to handle this properly. Lacking such calls, this just
> > piggybacks set_irq_chained_handler() to modify the parent's class.
> > That's a problem, since only the lowest level of any lock tree gets
> > accurate nesting annotations. On the plus side: no platform changes
> > are needed this way, so testing is easy.
> >
> > ==> GENIRQ programming interface evidently missing
> > set_irq_parent(parent_irqnum, child_irqnum) (?)
... that one's the real dirty bit of this example patch; having
multiple levels of IRQ chaining is quite realistic.
> > Example: when a GPIO controller's set_wake() handler needs to call its
> > parent's set_irq_wake() method to ensure all appropriate signal paths are
> > set up, that currently generates a bogus lockdep warning. This patch
> > removes those bogus warnings ... when there's only one level of parent.
>
> OK.... still need a bit more detail here.
What, like an example of such a bogus warning? See below.
First set_irq_wake() call owns child's lock; bogus warning
issued when getting parent's lock.
This is on AT91, but ISTR seeing similar warnings (different
drivers of course) on OMAP too.
- Dave
# echo standby > state
Syncing filesystems ... done.
PM: Preparing system for standby sleep
Freezing user space processes ... (elapsed 0.00 seconds) done.
Freezing remaining freezable tasks ... (elapsed 0.00 seconds) done.
PM: Entering standby sleep
Suspending console(s)
=============================================
[ INFO: possible recursive locking detected ]
2.6.24-rc8 #57
---------------------------------------------
sh/474 is trying to acquire lock:
(&irq_desc_lock_class){++..}, at: [<c005ca90>] set_irq_wake+0x34/0xfc
but task is already holding lock:
(&irq_desc_lock_class){++..}, at: [<c005ca90>] set_irq_wake+0x34/0xfc
other info that might help us debug this:
4 locks held by sh/474:
#0: (&buffer->mutex){--..}, at: [<c00b9044>] sysfs_write_file+0x30/0x148
#1: (pm_mutex){--..}, at: [<c005b788>] enter_state+0x13c/0x19c
#2: (dpm_mtx){--..}, at: [<c010beac>] device_suspend+0x28/0x250
#3: (&irq_desc_lock_class){++..}, at: [<c005ca90>] set_irq_wake+0x34/0xfc
stack backtrace:
[<c0022034>] (dump_stack+0x0/0x14) from [<c0056454>]
(__lock_acquire+0x94c/0xd80)
[<c0055b08>] (__lock_acquire+0x0/0xd80) from [<c0056d64>]
(lock_acquire+0x70/0x88)
[<c0056cf4>] (lock_acquire+0x0/0x88) from [<c01bb51c>]
(_spin_lock_irqsave+0x48/0x5c)
r6:20000093 r5:c005ca90 r4:c02446cc
[<c01bb4d4>] (_spin_lock_irqsave+0x0/0x5c) from [<c005ca90>]
(set_irq_wake+0x34/0xfc)
r6:00000001 r5:00000005 r4:c024469c
[<c005ca5c>] (set_irq_wake+0x0/0xfc) from [<c0026a9c>]
(gpio_irq_set_wake+0x6c/0x78)
[<c0026a30>] (gpio_irq_set_wake+0x0/0x78) from [<c005cb24>]
(set_irq_wake+0xc8/0xfc)
[<c005ca5c>] (set_irq_wake+0x0/0xfc) from [<c014534c>]
(at91_mci_suspend+0x3c/0x58)
[<c0145310>] (at91_mci_suspend+0x0/0x58) from [<c0109ae8>]
(platform_drv_suspend+0x20/0x24)
r5:c023ed64 r4:c024e0c0
[<c0109ac8>] (platform_drv_suspend+0x0/0x24) from [<c0109b3c>]
(platform_suspend+0x2c/0x38)
[<c0109b10>] (platform_suspend+0x0/0x38) from [<c010bfcc>]
(device_suspend+0x148/0x250)
[<c010be84>] (device_suspend+0x0/0x250) from [<c005b574>]
(suspend_devices_and_enter+0x4c/0x124)
r8:00000008 r7:00000001 r6:00000001 r5:c047ca14 r4:00000000
[<c005b528>] (suspend_devices_and_enter+0x0/0x124) from [<c005b744>]
(enter_state+0xf8/0x19c)
r6:c020c33b r5:c047cc58 r4:00001602
[<c005b64c>] (enter_state+0x0/0x19c) from [<c005b888>] (state_store+0xa0/0xbc)
r7:c1d4e000 r6:00000001 r5:00000007 r4:c020c33b
[<c005b7e8>] (state_store+0x0/0xbc) from [<c00b8d20>]
(subsys_attr_store+0x34/0x38)
r8:c024a260 r7:c0244338 r6:00000008 r5:c1d02b6c r4:c1c0b630
[<c00b8cec>] (subsys_attr_store+0x0/0x38) from [<c00b9124>]
(sysfs_write_file+0x110/0x148)
[<c00b9014>] (sysfs_write_file+0x0/0x148) from [<c007ea20>]
(vfs_write+0xb8/0xe0)
[<c007e968>] (vfs_write+0x0/0xe0) from [<c007ef54>] (sys_write+0x4c/0x7c)
r6:c1d11540 r5:0
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
