[patch/rfc 2.6.24-rc8-git] genirq: partial lockdep fixes

[patch/rfc 2.6.24-rc8-git] genirq: partial lockdep fixes

[David Brownell]

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

 - 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) (?)

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.

---
 include/linux/irq.h    |    3 +++
 kernel/irq/autoprobe.c |    5 +++++
 kernel/irq/chip.c      |   44 ++++++++++++++++++++++++++++----------------
 kernel/irq/handle.c    |    4 ++--
 kernel/irq/manage.c    |   12 ++++++------
 kernel/irq/migration.c |    2 +-
 kernel/irq/proc.c      |    2 +-
 kernel/irq/spurious.c  |    6 +++---
 8 files changed, 49 insertions(+), 29 deletions(-)

--- a/include/linux/irq.h	2008-01-17 22:25:53.000000000 -0800
+++ b/include/linux/irq.h	2008-01-18 00:08:57.000000000 -0800
@@ -164,6 +164,9 @@ struct irq_desc {
 	unsigned int		irqs_unhandled;
 	unsigned long		last_unhandled;	/* Aging timer for unhandled count */
 	spinlock_t		lock;
+#ifdef CONFIG_LOCKDEP
+	unsigned int		lock_class;	/* for chained irqs */
+#endif
 #ifdef CONFIG_SMP
 	cpumask_t		affinity;
 	unsigned int		cpu;
--- a/kernel/irq/autoprobe.c	2008-01-17 22:25:53.000000000 -0800
+++ b/kernel/irq/autoprobe.c	2008-01-18 00:08:57.000000000 -0800
@@ -41,6 +41,7 @@ unsigned long probe_irq_on(void)
 	for (i = NR_IRQS-1; i > 0; i--) {
 		desc = irq_desc + i;
 
+/* FIXME nested */
 		spin_lock_irq(&desc->lock);
 		if (!desc->action && !(desc->status & IRQ_NOPROBE)) {
 			/*
@@ -71,6 +72,7 @@ unsigned long probe_irq_on(void)
 	for (i = NR_IRQS-1; i > 0; i--) {
 		desc = irq_desc + i;
 
+/* FIXME nested */
 		spin_lock_irq(&desc->lock);
 		if (!desc->action && !(desc->status & IRQ_NOPROBE)) {
 			desc->status |= IRQ_AUTODETECT | IRQ_WAITING;
@@ -93,6 +95,7 @@ unsigned long probe_irq_on(void)
 		unsigned int status;
 
 		desc = irq_desc + i;
+/* FIXME nested */
 		spin_lock_irq(&desc->lock);
 		status = desc->status;
 
@@ -134,6 +137,7 @@ unsigned int probe_irq_mask(unsigned lon
 		struct irq_desc *desc = irq_desc + i;
 		unsigned int status;
 
+/* FIXME nested */
 		spin_lock_irq(&desc->lock);
 		status = desc->status;
 
@@ -177,6 +181,7 @@ int probe_irq_off(unsigned long val)
 		struct irq_desc *desc = irq_desc + i;
 		unsigned int status;
 
+/* FIXME nested */
 		spin_lock_irq(&desc->lock);
 		status = desc->status;
 
--- a/kernel/irq/chip.c	2008-01-17 22:25:52.000000000 -0800
+++ b/kernel/irq/chip.c	2008-01-18 14:20:21.000000000 -0800
@@ -35,7 +35,7 @@ void dynamic_irq_init(unsigned int irq)
 
 	/* Ensure we don't have left over values from a previous use of this irq */
 	desc = irq_desc + irq;
-	spin_lock_irqsave(&desc->lock, flags);
+	spin_lock_irqsave_nested(&desc->lock, flags, desc->lock_class);
 	desc->status = IRQ_DISABLED;
 	desc->chip = &no_irq_chip;
 	desc->handle_irq = handle_bad_irq;
@@ -68,7 +68,7 @@ void dynamic_irq_cleanup(unsigned int ir
 	}
 
 	desc = irq_desc + irq;
-	spin_lock_irqsave(&desc->lock, flags);
+	spin_lock_irqsave_nested(&desc->lock, flags, desc->lock_class);
 	if (desc->action) {
 		spin_unlock_irqrestore(&desc->lock, flags);
 		printk(KERN_ERR "Destroying IRQ%d without calling free_irq\n",
@@ -105,7 +105,7 @@ int set_irq_chip(unsigned int irq, struc
 		chip = &no_irq_chip;
 
 	desc = irq_desc + irq;
-	spin_lock_irqsave(&desc->lock, flags);
+	spin_lock_irqsave_nested(&desc->lock, flags, desc->lock_class);
 	irq_chip_set_defaults(chip);
 	desc->chip = chip;
 	spin_unlock_irqrestore(&desc->lock, flags);
@@ -132,7 +132,7 @@ int set_irq_type(unsigned int irq, unsig
 
 	desc = irq_desc + irq;
 	if (desc->chip->set_type) {
-		spin_lock_irqsave(&desc->lock, flags);
+		spin_lock_irqsave_nested(&desc->lock, flags, desc->lock_class);
 		ret = desc->chip->set_type(irq, type);
 		spin_unlock_irqrestore(&desc->lock, flags);
 	}
@@ -159,7 +159,7 @@ int set_irq_data(unsigned int irq, void 
 	}
 
 	desc = irq_desc + irq;
-	spin_lock_irqsave(&desc->lock, flags);
+	spin_lock_irqsave_nested(&desc->lock, flags, desc->lock_class);
 	desc->handler_data = data;
 	spin_unlock_irqrestore(&desc->lock, flags);
 	return 0;
@@ -184,7 +184,7 @@ int set_irq_msi(unsigned int irq, struct
 		return -EINVAL;
 	}
 	desc = irq_desc + irq;
-	spin_lock_irqsave(&desc->lock, flags);
+	spin_lock_irqsave_nested(&desc->lock, flags, desc->lock_class);
 	desc->msi_desc = entry;
 	if (entry)
 		entry->irq = irq;
@@ -209,7 +209,7 @@ int set_irq_chip_data(unsigned int irq, 
 		return -EINVAL;
 	}
 
-	spin_lock_irqsave(&desc->lock, flags);
+	spin_lock_irqsave_nested(&desc->lock, flags, desc->lock_class);
 	desc->chip_data = data;
 	spin_unlock_irqrestore(&desc->lock, flags);
 
@@ -293,7 +293,7 @@ handle_simple_irq(unsigned int irq, stru
 	irqreturn_t action_ret;
 	const unsigned int cpu = smp_processor_id();
 
-	spin_lock(&desc->lock);
+	spin_lock_nested(&desc->lock, desc->lock_class);
 
 	if (unlikely(desc->status & IRQ_INPROGRESS))
 		goto out_unlock;
@@ -311,7 +311,7 @@ handle_simple_irq(unsigned int irq, stru
 	if (!noirqdebug)
 		note_interrupt(irq, desc, action_ret);
 
-	spin_lock(&desc->lock);
+	spin_lock_nested(&desc->lock, desc->lock_class);
 	desc->status &= ~IRQ_INPROGRESS;
 out_unlock:
 	spin_unlock(&desc->lock);
@@ -334,7 +334,7 @@ handle_level_irq(unsigned int irq, struc
 	struct irqaction *action;
 	irqreturn_t action_ret;
 
-	spin_lock(&desc->lock);
+	spin_lock_nested(&desc->lock, desc->lock_class);
 	mask_ack_irq(desc, irq);
 
 	if (unlikely(desc->status & IRQ_INPROGRESS))
@@ -357,7 +357,7 @@ handle_level_irq(unsigned int irq, struc
 	if (!noirqdebug)
 		note_interrupt(irq, desc, action_ret);
 
-	spin_lock(&desc->lock);
+	spin_lock_nested(&desc->lock, desc->lock_class);
 	desc->status &= ~IRQ_INPROGRESS;
 	if (!(desc->status & IRQ_DISABLED) && desc->chip->unmask)
 		desc->chip->unmask(irq);
@@ -382,7 +382,7 @@ handle_fasteoi_irq(unsigned int irq, str
 	struct irqaction *action;
 	irqreturn_t action_ret;
 
-	spin_lock(&desc->lock);
+	spin_lock_nested(&desc->lock, desc->lock_class);
 
 	if (unlikely(desc->status & IRQ_INPROGRESS))
 		goto out;
@@ -410,7 +410,7 @@ handle_fasteoi_irq(unsigned int irq, str
 	if (!noirqdebug)
 		note_interrupt(irq, desc, action_ret);
 
-	spin_lock(&desc->lock);
+	spin_lock_nested(&desc->lock, desc->lock_class);
 	desc->status &= ~IRQ_INPROGRESS;
 out:
 	desc->chip->eoi(irq);
@@ -439,7 +439,7 @@ handle_edge_irq(unsigned int irq, struct
 {
 	const unsigned int cpu = smp_processor_id();
 
-	spin_lock(&desc->lock);
+	spin_lock_nested(&desc->lock, desc->lock_class);
 
 	desc->status &= ~(IRQ_REPLAY | IRQ_WAITING);
 
@@ -489,7 +489,7 @@ handle_edge_irq(unsigned int irq, struct
 		action_ret = handle_IRQ_event(irq, action);
 		if (!noirqdebug)
 			note_interrupt(irq, desc, action_ret);
-		spin_lock(&desc->lock);
+		spin_lock_nested(&desc->lock, desc->lock_class);
 
 	} while ((desc->status & (IRQ_PENDING | IRQ_DISABLED)) == IRQ_PENDING);
 
@@ -553,7 +553,7 @@ __set_irq_handler(unsigned int irq, irq_
 		desc->chip = &dummy_irq_chip;
 	}
 
-	spin_lock_irqsave(&desc->lock, flags);
+	spin_lock_irqsave_nested(&desc->lock, flags, desc->lock_class);
 
 	/* Uninstall? */
 	if (handle == handle_bad_irq) {
@@ -570,6 +570,18 @@ __set_irq_handler(unsigned int irq, irq_
 		desc->status |= IRQ_NOREQUEST | IRQ_NOPROBE;
 		desc->depth = 0;
 		desc->chip->unmask(irq);
+
+#ifdef CONFIG_LOCKDEP
+		/* REVISIT this only handles a single level of chaining.
+		 * A better solution would use a new interface setting
+		 * the child's lock_class to one more than the parent's;
+		 * but genirq doesn't currently link parents to children.
+		 *
+		 * REVISIT should this even work?  We already grabbed the
+		 * lock with the wrong class annotation, above...
+		 */
+		desc->lock_class++;
+#endif
 	}
 	spin_unlock_irqrestore(&desc->lock, flags);
 }
--- a/kernel/irq/handle.c	2008-01-17 22:25:52.000000000 -0800
+++ b/kernel/irq/handle.c	2008-01-18 00:08:57.000000000 -0800
@@ -187,7 +187,7 @@ fastcall unsigned int __do_IRQ(unsigned 
 		return 1;
 	}
 
-	spin_lock(&desc->lock);
+	spin_lock_nested(&desc->lock, desc->lock_class);
 	if (desc->chip->ack)
 		desc->chip->ack(irq);
 	/*
@@ -237,7 +237,7 @@ fastcall unsigned int __do_IRQ(unsigned 
 		if (!noirqdebug)
 			note_interrupt(irq, desc, action_ret);
 
-		spin_lock(&desc->lock);
+		spin_lock_nested(&desc->lock, desc->lock_class);
 		if (likely(!(desc->status & IRQ_PENDING)))
 			break;
 		desc->status &= ~IRQ_PENDING;
--- a/kernel/irq/manage.c	2008-01-17 22:25:52.000000000 -0800
+++ b/kernel/irq/manage.c	2008-01-18 00:08:57.000000000 -0800
@@ -45,7 +45,7 @@ void synchronize_irq(unsigned int irq)
 			cpu_relax();
 
 		/* Ok, that indicated we're done: double-check carefully. */
-		spin_lock_irqsave(&desc->lock, flags);
+		spin_lock_irqsave_nested(&desc->lock, flags, desc->lock_class);
 		status = desc->status;
 		spin_unlock_irqrestore(&desc->lock, flags);
 
@@ -115,7 +115,7 @@ void disable_irq_nosync(unsigned int irq
 	if (irq >= NR_IRQS)
 		return;
 
-	spin_lock_irqsave(&desc->lock, flags);
+	spin_lock_irqsave_nested(&desc->lock, flags, desc->lock_class);
 	if (!desc->depth++) {
 		desc->status |= IRQ_DISABLED;
 		desc->chip->disable(irq);
@@ -167,7 +167,7 @@ void enable_irq(unsigned int irq)
 	if (irq >= NR_IRQS)
 		return;
 
-	spin_lock_irqsave(&desc->lock, flags);
+	spin_lock_irqsave_nested(&desc->lock, flags, desc->lock_class);
 	switch (desc->depth) {
 	case 0:
 		printk(KERN_WARNING "Unbalanced enable for IRQ %d\n", irq);
@@ -210,7 +210,7 @@ int set_irq_wake(unsigned int irq, unsig
 	/* wakeup-capable irqs can be shared between drivers that
 	 * don't need to have the same sleep mode behaviors.
 	 */
-	spin_lock_irqsave(&desc->lock, flags);
+	spin_lock_irqsave_nested(&desc->lock, flags, desc->lock_class);
 	if (on) {
 		if (desc->wake_depth++ == 0)
 			desc->status |= IRQ_WAKEUP;
@@ -301,7 +301,7 @@ int setup_irq(unsigned int irq, struct i
 	/*
 	 * The following block of code has to be executed atomically
 	 */
-	spin_lock_irqsave(&desc->lock, flags);
+	spin_lock_irqsave_nested(&desc->lock, flags, desc->lock_class);
 	p = &desc->action;
 	old = *p;
 	if (old) {
@@ -427,7 +427,7 @@ void free_irq(unsigned int irq, void *de
 		return;
 
 	desc = irq_desc + irq;
-	spin_lock_irqsave(&desc->lock, flags);
+	spin_lock_irqsave_nested(&desc->lock, flags, desc->lock_class);
 	p = &desc->action;
 	for (;;) {
 		struct irqaction *action = *p;
--- a/kernel/irq/migration.c	2008-01-17 22:25:53.000000000 -0800
+++ b/kernel/irq/migration.c	2008-01-18 00:08:57.000000000 -0800
@@ -6,7 +6,7 @@ void set_pending_irq(unsigned int irq, c
 	struct irq_desc *desc = irq_desc + irq;
 	unsigned long flags;
 
-	spin_lock_irqsave(&desc->lock, flags);
+	spin_lock_irqsave_nested(&desc->lock, flags, desc->lock_class);
 	desc->status |= IRQ_MOVE_PENDING;
 	irq_desc[irq].pending_mask = mask;
 	spin_unlock_irqrestore(&desc->lock, flags);
--- a/kernel/irq/proc.c	2008-01-17 22:25:53.000000000 -0800
+++ b/kernel/irq/proc.c	2008-01-18 00:08:57.000000000 -0800
@@ -84,7 +84,7 @@ static int name_unique(unsigned int irq,
 	unsigned long flags;
 	int ret = 1;
 
-	spin_lock_irqsave(&desc->lock, flags);
+	spin_lock_irqsave_nested(&desc->lock, flags, desc->lock_class);
 	for (action = desc->action ; action; action = action->next) {
 		if ((action != new_action) && action->name &&
 				!strcmp(new_action->name, action->name)) {
--- a/kernel/irq/spurious.c	2008-01-17 22:25:52.000000000 -0800
+++ b/kernel/irq/spurious.c	2008-01-18 00:08:57.000000000 -0800
@@ -29,7 +29,7 @@ static int misrouted_irq(int irq)
 		if (i == irq)	/* Already tried */
 			continue;
 
-		spin_lock(&desc->lock);
+		spin_lock_nested(&desc->lock, desc->lock_class);
 		/* Already running on another processor */
 		if (desc->status & IRQ_INPROGRESS) {
 			/*
@@ -57,7 +57,7 @@ static int misrouted_irq(int irq)
 		}
 		local_irq_disable();
 		/* Now clean up the flags */
-		spin_lock(&desc->lock);
+		spin_lock_nested(&desc->lock, desc->lock_class);
 		action = desc->action;
 
 		/*
@@ -71,7 +71,7 @@ static int misrouted_irq(int irq)
 			work = 1;
 			spin_unlock(&desc->lock);
 			handle_IRQ_event(i, action);
-			spin_lock(&desc->lock);
+			spin_lock_nested(&desc->lock, desc->lock_class);
 			desc->status &= ~IRQ_PENDING;
 		}
 		desc->status &= ~IRQ_INPROGRESS;

Re: [patch/rfc 2.6.24-rc8-git] genirq: partial lockdep fixes

[Peter Zijlstra]

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.

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

>  - 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) (?)
> 
> 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.

Re: [patch/rfc 2.6.24-rc8-git] genirq: partial lockdep fixes

[David Brownell]

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
 

[patch 2.6.25-rc3] lockdep: add spin_lock_irq_nested()

[David Brownell]

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

Re: [patch 2.6.25-rc3] lockdep: add spin_lock_irq_nested()

[Peter Zijlstra]

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?

Re: [patch 2.6.25-rc3] lockdep: add spin_lock_irq_nested()

[David Brownell]

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

Re: [patch 2.6.25-rc3] lockdep: add spin_lock_irq_nested()

[Peter Zijlstra]

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}().

Re: [patch 2.6.25-rc3] lockdep: add spin_lock_irq_nested()

[David Brownell]

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

Re: [patch 2.6.25-rc3] lockdep: add spin_lock_irq_nested()

[David Brownell]

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

Re: [patch 2.6.25-rc3] lockdep: add spin_lock_irq_nested()

[Peter Zijlstra]

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.

Re: [patch 2.6.25-rc3] lockdep: add spin_lock_irq_nested()

[David Brownell]

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