Bug in "genirq: record trigger type"

PATCH New Benjamin Herrenschmidt 4 participants 8 messages Oct 21, 2008 1 message not in the archive IRQ

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Benjamin Herrenschmidt

author

2008-10-21 06:32 UTC #1 lore raw

Quoted 7 lines · Linux Kernel Mailing List

Gitweb:     http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=0c5d1eb77a8be917b638344a22afe1398236482b
Commit:     0c5d1eb77a8be917b638344a22afe1398236482b
Parent:     d6d5aeb661fc14655c417f3582ae7ec52985d2a8
Author:     David Brownell <[email protected]>
AuthorDate: Wed Oct 1 14:46:18 2008 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu Oct 2 10:24:09 2008 +0200

This one is obviously broken and breaks booting on a whole bunch of machines (including powermac's and thus my G5, it's never good when my own machine breaks !).

Nice to see 3 SOB's and one Ack and nobody caught the obvious bug :-)

Review

Quoted 11 lines

(quoted diff) +5 -5

 	desc = irq_desc + irq;
-	if (desc->chip->set_type) {
-		spin_lock_irqsave(&desc->lock, flags);
-		ret = desc->chip->set_type(irq, type);
-		spin_unlock_irqrestore(&desc->lock, flags);
-	}
+	if (type == IRQ_TYPE_NONE)
+		return 0;
+
+	spin_lock_irqsave(&desc->lock, flags);
+	ret = __irq_set_trigger(desc, irq, flags);

                                             ^^^^ type maybe ?

Review

Quoted 3 lines

(quoted diff) +1 -0

+	spin_unlock_irqrestore(&desc->lock, flags);
 	return ret;
 }

I have to run so no patch until tomorrow unless somebody beats me to it.

Cheers,
Ben.

Yinghai Lu

2008-10-21 07:22 UTC #2 lore raw

Quoted 26 lines · Benjamin Herrenschmidt

Quoted 7 lines · Linux Kernel Mailing List

Gitweb:     http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=0c5d1eb77a8be917b638344a22afe1398236482b
Commit:     0c5d1eb77a8be917b638344a22afe1398236482b
Parent:     d6d5aeb661fc14655c417f3582ae7ec52985d2a8
Author:     David Brownell <[email protected]>
AuthorDate: Wed Oct 1 14:46:18 2008 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu Oct 2 10:24:09 2008 +0200

This one is obviously broken and breaks booting on a whole bunch of machines (including powermac's and thus my G5, it's never good when my own machine breaks !).

Nice to see 3 SOB's and one Ack and nobody caught the obvious bug :-)

Review

Quoted 11 lines

      desc = irq_desc + irq;

(quoted diff) +5 -5

-     if (desc->chip->set_type) {
-             spin_lock_irqsave(&desc->lock, flags);
-             ret = desc->chip->set_type(irq, type);
-             spin_unlock_irqrestore(&desc->lock, flags);
-     }
+     if (type == IRQ_TYPE_NONE)
+             return 0;
+
+     spin_lock_irqsave(&desc->lock, flags);
+     ret = __irq_set_trigger(desc, irq, flags);

Quoted 7 lines

                                            ^^^^ type maybe ?

Quoted 3 lines

+     spin_unlock_irqrestore(&desc->lock, flags);
      return ret;

}

I have to run so no patch until tomorrow unless somebody beats me to it.

there is patch about it, but somehow get lost.

Ingo Molnar

2008-10-21 07:29 UTC #3 lore raw

Quoted 30 lines · Yinghai Lu

Quoted 28 lines · Benjamin Herrenschmidt

Quoted 7 lines · Linux Kernel Mailing List

Gitweb:     http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=0c5d1eb77a8be917b638344a22afe1398236482b
Commit:     0c5d1eb77a8be917b638344a22afe1398236482b
Parent:     d6d5aeb661fc14655c417f3582ae7ec52985d2a8
Author:     David Brownell <[email protected]>
AuthorDate: Wed Oct 1 14:46:18 2008 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu Oct 2 10:24:09 2008 +0200

This one is obviously broken and breaks booting on a whole bunch of machines (including powermac's and thus my G5, it's never good when my own machine breaks !).

Nice to see 3 SOB's and one Ack and nobody caught the obvious bug :-)

Review

Quoted 11 lines

      desc = irq_desc + irq;

(quoted diff) +5 -5

-     if (desc->chip->set_type) {
-             spin_lock_irqsave(&desc->lock, flags);
-             ret = desc->chip->set_type(irq, type);
-             spin_unlock_irqrestore(&desc->lock, flags);
-     }
+     if (type == IRQ_TYPE_NONE)
+             return 0;
+
+     spin_lock_irqsave(&desc->lock, flags);
+     ret = __irq_set_trigger(desc, irq, flags);

                                            ^^^^ type maybe ?

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
+     spin_unlock_irqrestore(&desc->lock, flags);
      return ret;
}
I have to run so no patch until tomorrow unless somebody beats me to it.
there is patch about it, but somehow get lost.

should be all sorted now, the fix is below.

Ingo

------------------>

Quoted 1 line

From aac4ddd11a8d0e402ddc3fbc75204cb64efa0aac Mon Sep 17 00:00:00 2001

From: Chris Friesen <[email protected]>
Date: Mon, 20 Oct 2008 12:41:58 -0600
Subject: [PATCH] genirq: fix set_irq_type() when recording trigger type

In set_irq_type() we want to pass the type rather than the current interrupt state.

Signed-off-by Chris Friesen <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

---
 kernel/irq/chip.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

kernel/irq/chip.c +1 -1

index 4895fde..3de6ea3 100644
@@ -127,7 +127,7 @@ int set_irq_type(unsigned int irq, unsigned int type)
 		return 0;
 
 	spin_lock_irqsave(&desc->lock, flags);
-	ret = __irq_set_trigger(desc, irq, flags);
+	ret = __irq_set_trigger(desc, irq, type);
 	spin_unlock_irqrestore(&desc->lock, flags);
 	return ret;
 }

Benjamin Herrenschmidt

author Acked-by

2008-10-21 07:32 UTC #4 lore raw

Quoted 9 lines · Ingo Molnar

From: Chris Friesen <[email protected]>
Date: Mon, 20 Oct 2008 12:41:58 -0600
Subject: [PATCH] genirq: fix set_irq_type() when recording trigger type
In set_irq_type() we want to pass the type rather than the current interrupt state.
Signed-off-by Chris Friesen <[email protected]>
Signed-off-by Ingo Molnar <[email protected]>

Acked-by Benjamin Herrenschmidt <[email protected]>

Quoted 17 lines

---
 kernel/irq/chip.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

kernel/irq/chip.c +1 -1

index 4895fde..3de6ea3 100644
@@ -127,7 +127,7 @@ int set_irq_type(unsigned int irq, unsigned int type)
 		return 0;
 
 	spin_lock_irqsave(&desc->lock, flags);
-	ret = __irq_set_trigger(desc, irq, flags);
+	ret = __irq_set_trigger(desc, irq, type);
 	spin_unlock_irqrestore(&desc->lock, flags);
 	return ret;
 }

Ingo Molnar

2008-10-21 07:34 UTC #5 lore raw

Quoted 6 lines · Benjamin Herrenschmidt

Quoted 4 lines · Ingo Molnar
From: Chris Friesen <[email protected]>
Date: Mon, 20 Oct 2008 12:41:58 -0600
Subject: [PATCH] genirq: fix set_irq_type() when recording trigger type

Quoted 7 lines

Quoted 5 lines
In set_irq_type() we want to pass the type rather than the current interrupt state.
Signed-off-by Chris Friesen <[email protected]>
Signed-off-by Ingo Molnar <[email protected]>
Acked-by Benjamin Herrenschmidt <[email protected]>

thx, added your ack to the commit as well.

Ingo

Ingo Molnar

2008-10-21 07:23 UTC #6 lore raw

PATCH 1/1 genirq: fix set_irq_type() when recording trigger type

In reply to Benjamin Herrenschmidt

Quoted 8 lines · Benjamin Herrenschmidt

Quoted 7 lines · Linux Kernel Mailing List

Gitweb:     http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=0c5d1eb77a8be917b638344a22afe1398236482b
Commit:     0c5d1eb77a8be917b638344a22afe1398236482b
Parent:     d6d5aeb661fc14655c417f3582ae7ec52985d2a8
Author:     David Brownell <[email protected]>
AuthorDate: Wed Oct 1 14:46:18 2008 -0700
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu Oct 2 10:24:09 2008 +0200

[...]

Quoted 10 lines

Quoted 4 lines
Signed-off-by David Brownell <[email protected]>
Signed-off-by Andrew Morton <[email protected]>
Acked-by Thomas Gleixner <[email protected]>
Signed-off-by Ingo Molnar <[email protected]>
This one is obviously broken and breaks booting on a whole bunch of machines (including powermac's and thus my G5, it's never good when my own machine breaks !).
Nice to see 3 SOB's and one Ack and nobody caught the obvious bug :-)

that patch came from -mm towards us shortly before the merge window. Once we had it integrated Chris Friesen reported a boot hang on a G5, and there's a fix pending for the bug, see it below.

Will send it to Linus expressly. Fortunately only a minority of Linux users will ever run a box that uses set_irq_type() - but yes it needs to be fixed.

Ingo

------------------------------>

Quoted 1 line

From aac4ddd11a8d0e402ddc3fbc75204cb64efa0aac Mon Sep 17 00:00:00 2001

From: Chris Friesen <[email protected]>
Date: Mon, 20 Oct 2008 12:41:58 -0600
Subject: [PATCH] genirq: fix set_irq_type() when recording trigger type

In set_irq_type() we want to pass the type rather than the current interrupt state.

Signed-off-by Chris Friesen <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

---
 kernel/irq/chip.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

kernel/irq/chip.c +1 -1

index 4895fde..3de6ea3 100644
@@ -127,7 +127,7 @@ int set_irq_type(unsigned int irq, unsigned int type)
 		return 0;
 
 	spin_lock_irqsave(&desc->lock, flags);
-	ret = __irq_set_trigger(desc, irq, flags);
+	ret = __irq_set_trigger(desc, irq, type);
 	spin_unlock_irqrestore(&desc->lock, flags);
 	return ret;
 }

David Brownell

2008-10-21 08:01 UTC #7 lore raw

In reply to Benjamin Herrenschmidt

Quoted 5 lines · Benjamin Herrenschmidt

This one is obviously broken and breaks booting on a whole bunch of machines (including powermac's and thus my G5, it's never good when my own machine breaks !).
Nice to see 3 SOB's and one Ack and nobody caught the obvious bug :-)

As you saw, that one's fixed. Chris' patch unfortunately didn't get integrated right away.

I'm a bit more curious about another potential issue though ... as described in the patch comment:

- Make set_irq_type() usage match request_irq() usage:
* IRQ_TYPE_NONE should be a NOP; succeed, so irq_chip methods won't have to handle that case any more (many do it wrong).

It might be a bit more accurate to say irq_chip.set_type() methods are *inconsistent* in handling IRQ_TYPE_NONE. Previously the set_irq_type() method would pass that down to irq_chip code.

I had observed two behaviors, but I thought I observed a third one in some of the PowerPC code:

(1) ignore it ... matching request_irq() usage
(2) return an error ... nasty
(3) assign some irq_chip-specific trigger mode

That third behavior might cause a bit of trouble, but I think it was only used during platform init. Someone more attuned to PowerPC might want to check ...

- Dave

Benjamin Herrenschmidt

author

2008-10-21 08:31 UTC #8 lore raw

Quoted 15 lines · David Brownell

I'm a bit more curious about another potential issue though ... as described in the patch comment:
- Make set_irq_type() usage match request_irq() usage:
* IRQ_TYPE_NONE should be a NOP; succeed, so irq_chip methods won't have to handle that case any more (many do it wrong).
It might be a bit more accurate to say irq_chip.set_type() methods are *inconsistent* in handling IRQ_TYPE_NONE. Previously the set_irq_type() method would pass that down to irq_chip code.
I had observed two behaviors, but I thought I observed a third one in some of the PowerPC code:

Quoted 7 lines

(1) ignore it ... matching request_irq() usage
(2) return an error ... nasty
(3) assign some irq_chip-specific trigger mode
That third behavior might cause a bit of trouble, but I think it was only used during platform init. Someone more attuned to PowerPC might want to check ...

Ok, I wrote a lot of the port of powerpc stuff to genirq so I supposed I'm the person to do that sweep :-) I'll have a look tomorrow.

Thanks for the heads up,

Cheers,
Ben.