> > I thought the way to use the *_nested() calls was "consistently"!
>
> Very much depends on your view of consistent :-)
>
> > That is, if one instance of a lock access uses it, they all should,
> > since that's the only way lockdep learns about equivalence classes.
> > Also, locks shouldn't move between those equivalence classes... so
> > the raw lockdep data stays correct.
>
> Ah, see, you're missing a detail (see below for the full story). Its the
> irq state that must be consistent. So if at any one point you take the
> lock in an irq safe context, you must always take it irq safe.
IRQ state is not a factor; that's already coded correctly.
The issue here is lockdep falsely reporting recursive locking.
Again, the locking is correct; it's lockdep getting confused,
because it puts all irq_desc[].lock instances in the same bag
instead of treating parent and child locks differently. So
when something holding a child lock makes a call that grabs
a parent lock, it thinks that could be recursive.
> Also, have you looked at explicit lock_class_key usage per chip?
Never had reason to look at such stuff; you didn't mention the
option in the earlier chitchat...
> That
> way you can avoid using the _nested annotation. You can set a class on a
> lock right after spin_lock_init() using lockdep_set_class*().
All this stuff is set up in kernel/irq/handle.c ... spinlocks
are initialized statically, lock classes are set up even before
the kernel banner is printed, by early_init_irq_lock_class().
So I'm think that the reason this only _changes_ the false
recursion notification is that it doesn't support overriding
such class annotations; it doesn't seem to forget about the
first one set up. Note that the code I posted earlier, using
the _nested annotations, isn't confused like this...
- Dave
=================== CUT
Try to remove some false lockdep warnings about lock recursion,
by marking putting GPIO irq_desc locks into their own class.
But that doesn't seem to work; all it does is change the fault
report to be more confusing:
=============================================
[ INFO: possible recursive locking detected ]
2.6.25-rc3 #80
---------------------------------------------
swapper/1 is trying to acquire lock:
(&irq_desc_lock_class){+...}, at: [<c007150c>] set_irq_wake+0x34/0xe8
but task is already holding lock:
(&irq_desc_lock_class){+...}, at: [<c007150c>] set_irq_wake+0x34/0xe8
other info that might help us debug this:
2 locks held by swapper/1:
#0: (&irq_desc_lock_class){+...}, at: [<c007150c>] set_irq_wake+0x34/0xe8
#1: (&bank->lock){....}, at: [<c0038d80>] _set_gpio_wakeup+0x38/0xa4
stack backtrace:
[<c0029d78>] (dump_stack+0x0/0x14) from [<c0064a48>]
(print_deadlock_bug+0xa0/0xcc)
[<c00649a8>] (print_deadlock_bug+0x0/0xcc) from [<c0064ae0>]
(check_deadlock+0x6c/0x84)
r6:c1c1834c r5:00000000 r4:00000000
[<c0064a74>] (check_deadlock+0x0/0x84) from [<c0066500>]
(validate_chain+0x1d8/0x2c4)
r7:c1c1834c r6:00000000 r5:01403805 r4:c04c0658
[<c0066328>] (validate_chain+0x0/0x2c4) from [<c0066aec>]
(__lock_acquire+0x500/0x5bc)
[<c00665ec>] (__lock_acquire+0x0/0x5bc) from [<c006705c>]
(lock_acquire+0x70/0x88)
[<c0066fec>] (lock_acquire+0x0/0x88) from [<c02260d0>]
(_spin_lock_irqsave+0x50/0x64)
r6:20000093 r5:c007150c r4:c02cf6c8
[<c0226080>] (_spin_lock_irqsave+0x0/0x64) from [<c007150c>]
(set_irq_wake+0x34/0xe8)
r6:00000001 r5:00000025 r4:c02cf698
[<c00714d8>] (set_irq_wake+0x0/0xe8) from [<c0038da4>]
(_set_gpio_wakeup+0x5c/0xa4)
[<c0038d48>] (_set_gpio_wakeup+0x0/0xa4) from [<c0039410>]
(gpio_wake_enable+0x48/0x50)
r8:c00393c8 r7:c02d5ecc r6:00000001 r5:00000162 r4:000000c2
[<c00393c8>] (gpio_wake_enable+0x0/0x50) from [<c0071594>]
(set_irq_wake+0xbc/0xe8)
r6:00000001 r5:00000162 r4:c02d5e9c
[<c00714d8>] (set_irq_wake+0x0/0xe8) from [<c000c8d0>]
(osk_mistral_init+0x160/0x1c8)
[<c000c770>] (osk_mistral_init+0x0/0x1c8) from [<c000c9e4>]
(osk_init+0xac/0xd4)
r4:c001f3f8
[<c000c938>] (osk_init+0x0/0xd4) from [<c0009db4>]
(customize_machine+0x20/0x2c)
r4:c001e000
[<c0009d94>] (customize_machine+0x0/0x2c) from [<c0008684>]
(do_initcalls+0x78/0x200)
[<c000860c>] (do_initcalls+0x0/0x200) from [<c000882c>]
(do_basic_setup+0x20/0x24)
[<c000880c>] (do_basic_setup+0x0/0x24) from [<c0008bd0>]
(kernel_init+0x44/0x90)
[<c0008b8c>] (kernel_init+0x0/0x90) from [<c004576c>] (do_exit+0x0/0x30c)
r4:00000000
---
arch/arm/plat-omap/gpio.c | 23 +++++++++++++++++++++++
1 files changed, 23 insertions(+)
--- a/arch/arm/plat-omap/gpio.c 2008-02-24 19:02:32.000000000 -0800
+++ b/arch/arm/plat-omap/gpio.c 2008-02-25 10:54:29.000000000 -0800
@@ -1332,6 +1332,28 @@ static struct clk *gpio_fclks[OMAP34XX_N
static struct clk *gpio_iclks[OMAP34XX_NR_GPIOS];
#endif
+#ifdef CONFIG_LOCKDEP
+
+/* tell lockdep that this IRQ's locks and its parent's locks are in
+ * different categories, so that it won't detect false recursion.
+ */
+static struct lock_class_key gpio_lock_class;
+
+static inline void mark_gpio_locking(unsigned gpio_irq)
+{
+ lockdep_set_class(&irq_desc[gpio_irq].lock, &gpio_lock_class);
+}
+
+#else
+
+static inline void mark_gpio_locking(unsigned gpio_irq)
+{
+ /* NOP */
+}
+
+#endif
+
+
static int __init _omap_gpio_init(void)
{
int i;
@@ -1532,6 +1554,7 @@ static int __init _omap_gpio_init(void)
set_irq_chip(j, &gpio_irq_chip);
set_irq_handler(j, handle_simple_irq);
set_irq_flags(j, IRQF_VALID);
+ mark_gpio_locking(i);
}
set_irq_chained_handler(bank->irq, gpio_irq_handler);
set_irq_data(bank->irq, bank);
--
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/
