From: David Brownell <david-b@pacbell.net>
To: Linux Kernel list <linux-kernel@vger.kernel.org>
Cc: peterz@infradead.org, mingo@redhat.com
Subject: [patch/rfc 2.6.24-rc8-git] genirq: partial lockdep fixes
Date: Fri, 18 Jan 2008 14:29:00 -0800 [thread overview]
Message-ID: <200801181429.00485.david-b@pacbell.net> (raw)
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;
next reply other threads:[~2008-01-18 22:29 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2008-01-18 22:29 David Brownell [this message]
2008-01-21 8:31 ` [patch/rfc 2.6.24-rc8-git] genirq: partial lockdep fixes 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=200801181429.00485.david-b@pacbell.net \
--to=david-b@pacbell.net \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.