LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* Re: [patch 2.6.25-rc3] lockdep:  add spin_lock_irq_nested()
@ 2008-02-25 22:33 David Brownell
  2008-02-26  9:53 ` Peter Zijlstra
  0 siblings, 1 reply; 8+ messages in thread
From: David Brownell @ 2008-02-25 22:33 UTC (permalink / raw)
  To: peterz; +Cc: mingo, linux-kernel

> So I'm think that the reason this only _changes_ the false
> recursion notification ...

Whoops, it's because of the following typo:

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

should be		mark_gpio_locking(j).

That's an off-by-one error in ASCII or QWERTY (adjacency), and
in terms of typefaces it's a few pixels.  Sigh.

Since that seems to behave, I'll send along a couple patches
like this ... unless you say this isn't really, after all, the
right way to address this problem.  Note that neither IRQ has
ever been triggered at this point ... I'll move the marking up
a bit to ensure the lock has never been taken either.

- Dave


>  		}
>  		set_irq_chained_handler(bank->irq, gpio_irq_handler);
>  		set_irq_data(bank->irq, bank);
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [patch 2.6.25-rc3] lockdep:  add spin_lock_irq_nested()
  2008-02-25 22:33 [patch 2.6.25-rc3] lockdep: add spin_lock_irq_nested() David Brownell
@ 2008-02-26  9:53 ` Peter Zijlstra
  2008-02-26 10:36   ` David Brownell
  0 siblings, 1 reply; 8+ messages in thread
From: Peter Zijlstra @ 2008-02-26  9:53 UTC (permalink / raw)
  To: David Brownell; +Cc: mingo, linux-kernel


On Mon, 2008-02-25 at 14:33 -0800, David Brownell wrote:

> > +#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

Glad to hear this works out for you.

Just one note, you don't need the #ifdef mess here. struct
lock_class_key is 0 bytes on !LOCKDEP and lockdep_set_class*() is
defined away as well.



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [patch 2.6.25-rc3] lockdep:  add spin_lock_irq_nested()
  2008-02-26  9:53 ` Peter Zijlstra
@ 2008-02-26 10:36   ` David Brownell
  0 siblings, 0 replies; 8+ messages in thread
From: David Brownell @ 2008-02-26 10:36 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: mingo, linux-kernel

On Tuesday 26 February 2008, Peter Zijlstra wrote:
> 
> On Mon, 2008-02-25 at 14:33 -0800, David Brownell wrote:
> 
> > > +#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
> > > +
> > > +	...
> 
> Glad to hear this works out for you.

Yeah, glad to have a localized fix rather than mucking with genirq.


> Just one note, you don't need the #ifdef mess here. struct
> lock_class_key is 0 bytes on !LOCKDEP and lockdep_set_class*() is
> defined away as well.

Hmm, then kernel/irq/handle.c shouldn't need #ifdefs either ... ;)


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [patch 2.6.25-rc3] lockdep:  add spin_lock_irq_nested()
  2008-02-25 13:17           ` Peter Zijlstra
@ 2008-02-25 21:10             ` David Brownell
  0 siblings, 0 replies; 8+ messages in thread
From: David Brownell @ 2008-02-25 21:10 UTC (permalink / raw)
  To: peterz; +Cc: mingo, linux-kernel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [patch 2.6.25-rc3] lockdep:  add spin_lock_irq_nested()
  2008-02-25 11:21         ` David Brownell
@ 2008-02-25 13:17           ` Peter Zijlstra
  2008-02-25 21:10             ` David Brownell
  0 siblings, 1 reply; 8+ messages in thread
From: Peter Zijlstra @ 2008-02-25 13:17 UTC (permalink / raw)
  To: David Brownell; +Cc: mingo, linux-kernel


On Mon, 2008-02-25 at 03:21 -0800, David Brownell wrote:
> > > > > >     ==> 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. 
> > > 
> > > Does this look about right?  Or, I suppose it could just call
> > > the _spin_lock_irqsave_nested() routine and discard the result.
> >
> > Before I look at the code, and with a notice that I haven't had my
> > morning juice yet...
> >
> > It seems to me a spin_lock_irq_nested() thing is redundant, because:
> >
> > The lock must obviously be held hardirq safe and nested implies one is
> > already held.
> 
> 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.

> The IRQ framework uses spin_lock_irq() in only one place that I saw:
> in kernel/irq/autoprobe.c for the probe_irq_{on,off,mask}() calls.
>
> Those calls will grab locks at their "top level", and then the
> irqchip methods they call might need to grab locks for other irqs.
> Potential example:  chip->startup() and chip->shutdown() could
> need to ensure a *parent* controller starts/stops, and that should
> involve mutual exclusion using the parent's irq lock (as well as
> the child's).  So the chip and its parent should be in different
> lock classes, else lockdep will wrongly warn of recursion.

Quite, but we must also take note of the irq state.

> >	Hence the context is already hardirq safe thus using
> > spin_lock_irq/spin_unlock_irq is wrong because it will enable irqs and
> > destroy the irqsafe guarantee for the parent lock.
> 
> That's not how the autoprobe() stuff works.  The other calls in
> the genirq framework don't use the *_irq() variants though, so
> your intuition is right there.  (Only the autoprobe paths had
> the FIXME comments in that patch I sent earlier, related to the
> lack of the $SUBJECT primitive.)

Right, which is where I gleaned your usage from..

> > Obviously I'm missing something here.. otherwise you wouldn't need it.
> >
> > As I'm very much not familiar with the IRQ code, could you spell it out
> > to me?
> 
> The probe_irq_*() calls are made from task context, not hardirq
> context, but they access the same locks involved in IRQ management
> and processing.  So either they need to pass the same lock class
> annodations to lockdep, or there's something that's unusually
> counter-intuitive going on with respect to those annotations in
> simple tree data structures.

Ah, you can play tricks here :-) All you need to ensure are consisent
class uses. So if your normal usage is 0->1->2->3 etc.. all you need to
ensure is that the reverse never happens.

Also, have you looked at explicit lock_class_key usage per chip? 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*().


Look at it this way:

 spin_lock_irq()   := local_irq_disable(); spin_lock();
 spin_unlock_irq() := spin_unlock(); local_irq_enable();

 spin_lock_irq_nested() : local_irq_disable(); spin_lock_nested();


 spin_lock_irq(&parent_desc->lock);	local_irq_disable();
					spin_lock(&parent_desc->lock);

 spin_lock_irq_nested(&desc->lock, 1);	local_irq_disable();
 					spin_lock_nested(&desc->lock, 1)

 spin_unlock_irq(&desc->lock);		spin_unlock(&desc->lock);
					local_irq_enable(); <--- BUG!

 spin_unlock_irq(&parent_desc->lock);	spin_unlock(&parent_desc->lock);
					local_irq_enable();


At the BUG site lockdep will warn because parent_desc->lock is still
held as hardirq-safe lock, but the context is hardirq-unsafe.

It will become an actual deadlock if at that point an IRQ happens and
tries to acquire parent_desc->lock.

The safe approach is using spin_lock_irqsave{,_nested}().


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [patch 2.6.25-rc3] lockdep:  add spin_lock_irq_nested()
  2008-02-25 10:20       ` Peter Zijlstra
@ 2008-02-25 11:21         ` David Brownell
  2008-02-25 13:17           ` Peter Zijlstra
  0 siblings, 1 reply; 8+ messages in thread
From: David Brownell @ 2008-02-25 11:21 UTC (permalink / raw)
  To: peterz; +Cc: mingo, linux-kernel

> > > > >     ==> 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. 
> > 
> > Does this look about right?  Or, I suppose it could just call
> > the _spin_lock_irqsave_nested() routine and discard the result.
>
> Before I look at the code, and with a notice that I haven't had my
> morning juice yet...
>
> It seems to me a spin_lock_irq_nested() thing is redundant, because:
>
> The lock must obviously be held hardirq safe and nested implies one is
> already held.

I thought the way to use the *_nested() calls was "consistently"!

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.

The IRQ framework uses spin_lock_irq() in only one place that I saw:
in kernel/irq/autoprobe.c for the probe_irq_{on,off,mask}() calls.

Those calls will grab locks at their "top level", and then the
irqchip methods they call might need to grab locks for other irqs.
Potential example:  chip->startup() and chip->shutdown() could
need to ensure a *parent* controller starts/stops, and that should
involve mutual exclusion using the parent's irq lock (as well as
the child's).  So the chip and its parent should be in different
lock classes, else lockdep will wrongly warn of recursion.


>	Hence the context is already hardirq safe thus using
> spin_lock_irq/spin_unlock_irq is wrong because it will enable irqs and
> destroy the irqsafe guarantee for the parent lock.

That's not how the autoprobe() stuff works.  The other calls in
the genirq framework don't use the *_irq() variants though, so
your intuition is right there.  (Only the autoprobe paths had
the FIXME comments in that patch I sent earlier, related to the
lack of the $SUBJECT primitive.)


> Obviously I'm missing something here.. otherwise you wouldn't need it.
>
> As I'm very much not familiar with the IRQ code, could you spell it out
> to me?

The probe_irq_*() calls are made from task context, not hardirq
context, but they access the same locks involved in IRQ management
and processing.  So either they need to pass the same lock class
annodations to lockdep, or there's something that's unusually
counter-intuitive going on with respect to those annotations in
simple tree data structures.

- Dave


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [patch 2.6.25-rc3] lockdep:  add spin_lock_irq_nested()
  2008-02-25  4:33     ` [patch 2.6.25-rc3] lockdep: add spin_lock_irq_nested() David Brownell
@ 2008-02-25 10:20       ` Peter Zijlstra
  2008-02-25 11:21         ` David Brownell
  0 siblings, 1 reply; 8+ messages in thread
From: Peter Zijlstra @ 2008-02-25 10:20 UTC (permalink / raw)
  To: David Brownell; +Cc: Linux Kernel list, mingo


On Sun, 2008-02-24 at 20:33 -0800, David Brownell wrote:
> > > >     ==> 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. 
> 
> Does this look about right?  Or, I suppose it could just call
> the _spin_lock_irqsave_nested() routine and discard the result.

Before I look at the code, and with a notice that I haven't had my
morning juice yet...

It seems to me a spin_lock_irq_nested() thing is redundant, because:

The lock must obviously be held hardirq safe and nested implies one is
already held. Hence the context is already hardirq safe thus using
spin_lock_irq/spin_unlock_irq is wrong because it will enable irqs and
destroy the irqsafe guarantee for the parent lock.

Obviously I'm missing something here.. otherwise you wouldn't need it.

As I'm very much not familiar with the IRQ code, could you spell it out
to me?


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [patch 2.6.25-rc3] lockdep:  add spin_lock_irq_nested()
  2008-01-21 18:22   ` David Brownell
@ 2008-02-25  4:33     ` David Brownell
  2008-02-25 10:20       ` Peter Zijlstra
  0 siblings, 1 reply; 8+ messages in thread
From: David Brownell @ 2008-02-25  4:33 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: Linux Kernel list, mingo

> > >     ==> 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. 

Does this look about right?  Or, I suppose it could just call
the _spin_lock_irqsave_nested() routine and discard the result.

- Dave

=========	 CUT HERE
Add new spin_lock_irq_nested() call, so that lockdep can work with the
code which uses spin_*_irq() calls that don't save/restore flags.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
Against 2.6.25-rc3

 include/linux/spinlock.h         |    6 ++++++
 include/linux/spinlock_api_smp.h |    2 ++
 kernel/spinlock.c                |   21 +++++++++++++++++++--
 3 files changed, 27 insertions(+), 2 deletions(-)

--- a/include/linux/spinlock.h	2008-02-24 18:50:50.000000000 -0800
+++ b/include/linux/spinlock.h	2008-02-24 19:02:39.000000000 -0800
@@ -196,9 +196,13 @@ do {								\
 #define write_lock_irqsave(lock, flags)	flags = _write_lock_irqsave(lock)
 
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
+#define spin_lock_irq_nested(lock, subclass) \
+	_spin_lock_irq_nested(lock, subclass)
 #define spin_lock_irqsave_nested(lock, flags, subclass) \
 	flags = _spin_lock_irqsave_nested(lock, subclass)
 #else
+#define spin_lock_irq_nested(lock, subclass) \
+	_spin_lock_irq(lock)
 #define spin_lock_irqsave_nested(lock, flags, subclass) \
 	flags = _spin_lock_irqsave(lock)
 #endif
@@ -208,6 +212,8 @@ do {								\
 #define spin_lock_irqsave(lock, flags)	_spin_lock_irqsave(lock, flags)
 #define read_lock_irqsave(lock, flags)	_read_lock_irqsave(lock, flags)
 #define write_lock_irqsave(lock, flags)	_write_lock_irqsave(lock, flags)
+#define spin_lock_irq_nested(lock, subclass) \
+	spin_lock_irq(lock)
 #define spin_lock_irqsave_nested(lock, flags, subclass)	\
 	spin_lock_irqsave(lock, flags)
 
--- a/include/linux/spinlock_api_smp.h	2008-02-24 18:50:50.000000000 -0800
+++ b/include/linux/spinlock_api_smp.h	2008-02-24 19:02:39.000000000 -0800
@@ -28,6 +28,8 @@ void __lockfunc _spin_lock_bh(spinlock_t
 void __lockfunc _read_lock_bh(rwlock_t *lock)		__acquires(lock);
 void __lockfunc _write_lock_bh(rwlock_t *lock)		__acquires(lock);
 void __lockfunc _spin_lock_irq(spinlock_t *lock)	__acquires(lock);
+void __lockfunc _spin_lock_irq_nested(spinlock_t *lock, int subclass)
+							__acquires(lock);
 void __lockfunc _read_lock_irq(rwlock_t *lock)		__acquires(lock);
 void __lockfunc _write_lock_irq(rwlock_t *lock)		__acquires(lock);
 unsigned long __lockfunc _spin_lock_irqsave(spinlock_t *lock)
--- a/kernel/spinlock.c	2008-02-24 18:50:50.000000000 -0800
+++ b/kernel/spinlock.c	2008-02-24 19:02:39.000000000 -0800
@@ -290,8 +290,26 @@ void __lockfunc _spin_lock_nested(spinlo
 	spin_acquire(&lock->dep_map, subclass, 0, _RET_IP_);
 	LOCK_CONTENDED(lock, _raw_spin_trylock, _raw_spin_lock);
 }
-
 EXPORT_SYMBOL(_spin_lock_nested);
+
+void __lockfunc _spin_lock_irq_nested(spinlock_t *lock, int subclass)
+{
+	local_irq_disable();
+	preempt_disable();
+	spin_acquire(&lock->dep_map, subclass, 0, _RET_IP_);
+	/*
+	 * On lockdep we dont want the hand-coded irq-enable of
+	 * _raw_spin_lock_flags() code, because lockdep assumes
+	 * that interrupts are not re-enabled during lock-acquire:
+	 */
+#ifdef CONFIG_LOCKDEP
+	LOCK_CONTENDED(lock, _raw_spin_trylock, _raw_spin_lock);
+#else
+	_raw_spin_lock_flags(lock, &flags);
+#endif
+}
+EXPORT_SYMBOL(_spin_lock_irq_nested);
+
 unsigned long __lockfunc _spin_lock_irqsave_nested(spinlock_t *lock, int subclass)
 {
 	unsigned long flags;
@@ -311,7 +329,6 @@ unsigned long __lockfunc _spin_lock_irqs
 #endif
 	return flags;
 }
-
 EXPORT_SYMBOL(_spin_lock_irqsave_nested);
 
 #endif

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2008-02-26 10:36 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-02-25 22:33 [patch 2.6.25-rc3] lockdep: add spin_lock_irq_nested() David Brownell
2008-02-26  9:53 ` Peter Zijlstra
2008-02-26 10:36   ` David Brownell
  -- strict thread matches above, loose matches on Subject: below --
2008-01-18 22:29 [patch/rfc 2.6.24-rc8-git] genirq: partial lockdep fixes David Brownell
2008-01-21  8:31 ` Peter Zijlstra
2008-01-21 18:22   ` David Brownell
2008-02-25  4:33     ` [patch 2.6.25-rc3] lockdep: add spin_lock_irq_nested() David Brownell
2008-02-25 10:20       ` Peter Zijlstra
2008-02-25 11:21         ` David Brownell
2008-02-25 13:17           ` Peter Zijlstra
2008-02-25 21:10             ` David Brownell

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).