Re: [patch] genirq: do not mask interrupts by default

Re: [patch] genirq: do not mask interrupts by default

[Linus Torvalds]

On Wed, 15 Nov 2006, Ingo Molnar wrote:
> 
> problem is, we dont know /for a fact/ that something is "APIC-edge". We 
> only know that the BIOS claims it that it's so.

This is incorrect. We will have _programmed_ the APIC with whatever the 
BIOS said in the MP tables, so if we think it's level triggered, it _is_ 
level triggered.

So I really think that all the arguments for i8259 not wanting replay 
weigh equally on level-triggered PCI irq's too.

Now, the one thing that makes me think your approach is the right one is 
that it's potentially going to be better performance - if people disable 
irq's and the normal case is that no irq will actually happen, then 
optimistically not doing anything at all (except marking the irq disabled, 
of course) is always good.

However, because it's a semantic change, I _really_ don't want to do it 
right now. We're maybe a week away from 2.6.19, and the "ISA irq's don't 
work" report is one of the things that is holding things up right now.

So that's why I'd much rather go with Eric's patch for now - because it 
keeps the semantics that we've always had.

			Linus

Re: [patch] genirq: do not mask interrupts by default

[Ingo Molnar]

* Linus Torvalds <torvalds@osdl.org> wrote:

> On Wed, 15 Nov 2006, Ingo Molnar wrote:
> > 
> > problem is, we dont know /for a fact/ that something is "APIC-edge". 
> > We only know that the BIOS claims it that it's so.
> 
> This is incorrect. We will have _programmed_ the APIC with whatever 
> the BIOS said in the MP tables, so if we think it's level triggered, 
> it _is_ level triggered.

yeah. I was thinking about the low 16 irqs (those are really the problem 
spots most of the time, not the normal IO-APIC irqs) - which are routed 
all across the southbridge and might end up being handled by a 
i8259A-lookalike entity. Right now we default to level-triggered IRQ 
flow handling:

                if (i < 16) {
                        /*
                         * 16 old-style INTA-cycle interrupts:
                         */
                        set_irq_chip_and_handler_name(i, &i8259A_chip,
                                                      handle_level_irq, "XT");

because that's the best we can do (it's also what our i8259 code did 
historically). But it would be one step safer to also do the 
lazy-disable. Just in case things might get lost while masked. Or is 
that an absolutely horrible hardware breakage that i shouldnt worry 
about?

> So I really think that all the arguments for i8259 not wanting replay 
> weigh equally on level-triggered PCI irq's too.
> 
> Now, the one thing that makes me think your approach is the right one 
> is that it's potentially going to be better performance - if people 
> disable irq's and the normal case is that no irq will actually happen, 
> then optimistically not doing anything at all (except marking the irq 
> disabled, of course) is always good.
> 
> However, because it's a semantic change, I _really_ don't want to do 
> it right now. We're maybe a week away from 2.6.19, and the "ISA irq's 
> don't work" report is one of the things that is holding things up 
> right now.
>
> So that's why I'd much rather go with Eric's patch for now - because 
> it keeps the semantics that we've always had.

ok, i'm fine with Eric's patch too, if it solves Komuro's problem:

  Acked-by: Ingo Molnar <mingo@elte.hu>

and we dont have to worry about the present ugliness of the 
delayed-disabled flag either, as it would just go away in 2.6.20.

	Ingo

[patch] genirq: do not mask interrupts by default

[Ingo Molnar]

Subject: [patch] genirq: do not mask interrupts by default
From: Ingo Molnar <mingo@elte.hu>

never mask interrupts immediately upon request. Disabling interrupts in 
high-performance codepaths is rare, and on the other hand this change 
could recover lost edges (or even other types of lost interrupts) by 
conservatively only masking interrupts after they happen. (NOTE: with 
this change the highlevel irq-disable code still soft-disables this IRQ 
line - and if such an interrupt happens then the IRQ flow handler keeps 
the IRQ masked.)

mark i8529A controllers as 'never loses an edge'.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/i386/kernel/i8259.c   |    1 +
 arch/x86_64/kernel/i8259.c |    1 +
 kernel/irq/chip.c          |   17 ++++++++++-------
 3 files changed, 12 insertions(+), 7 deletions(-)

Index: linux/arch/i386/kernel/i8259.c
===================================================================
--- linux.orig/arch/i386/kernel/i8259.c
+++ linux/arch/i386/kernel/i8259.c
@@ -41,6 +41,7 @@ static void mask_and_ack_8259A(unsigned 
 static struct irq_chip i8259A_chip = {
 	.name		= "XT-PIC",
 	.mask		= disable_8259A_irq,
+	.disable	= disable_8259A_irq,
 	.unmask		= enable_8259A_irq,
 	.mask_ack	= mask_and_ack_8259A,
 };
Index: linux/arch/x86_64/kernel/i8259.c
===================================================================
--- linux.orig/arch/x86_64/kernel/i8259.c
+++ linux/arch/x86_64/kernel/i8259.c
@@ -103,6 +103,7 @@ static void mask_and_ack_8259A(unsigned 
 static struct irq_chip i8259A_chip = {
 	.name		= "XT-PIC",
 	.mask		= disable_8259A_irq,
+	.disable	= disable_8259A_irq,
 	.unmask		= enable_8259A_irq,
 	.mask_ack	= mask_and_ack_8259A,
 };
Index: linux/kernel/irq/chip.c
===================================================================
--- linux.orig/kernel/irq/chip.c
+++ linux/kernel/irq/chip.c
@@ -202,10 +202,6 @@ static void default_enable(unsigned int 
  */
 static void default_disable(unsigned int irq)
 {
-	struct irq_desc *desc = irq_desc + irq;
-
-	if (!(desc->status & IRQ_DELAYED_DISABLE))
-		desc->chip->mask(irq);
 }
 
 /*
@@ -270,13 +266,18 @@ handle_simple_irq(unsigned int irq, stru
 
 	if (unlikely(desc->status & IRQ_INPROGRESS))
 		goto out_unlock;
-	desc->status &= ~(IRQ_REPLAY | IRQ_WAITING);
 	kstat_cpu(cpu).irqs[irq]++;
 
 	action = desc->action;
-	if (unlikely(!action || (desc->status & IRQ_DISABLED)))
+	if (unlikely(!action || (desc->status & IRQ_DISABLED))) {
+		if (desc->chip->mask)
+			desc->chip->mask(irq);
+		desc->status &= ~(IRQ_REPLAY | IRQ_WAITING);
+		desc->status |= IRQ_PENDING;
 		goto out_unlock;
+	}
 
+	desc->status &= ~(IRQ_REPLAY | IRQ_WAITING | IRQ_PENDING);
 	desc->status |= IRQ_INPROGRESS;
 	spin_unlock(&desc->lock);
 
@@ -368,11 +369,13 @@ handle_fasteoi_irq(unsigned int irq, str
 
 	/*
 	 * If its disabled or no action available
-	 * keep it masked and get out of here
+	 * then mask it and get out of here:
 	 */
 	action = desc->action;
 	if (unlikely(!action || (desc->status & IRQ_DISABLED))) {
 		desc->status |= IRQ_PENDING;
+		if (desc->chip->mask)
+			desc->chip->mask(irq);
 		goto out;
 	}

----- End forwarded message -----

Re: [PATCH] genirq: do not mask interrupts by default

[Benjamin Herrenschmidt]

On Fri, 2007-02-16 at 17:59 +0000, Linux Kernel Mailing List wrote:
> Gitweb:     http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=76d2160147f43f982dfe881404cfde9fd0a9da21
> Commit:     76d2160147f43f982dfe881404cfde9fd0a9da21
> Parent:     1f2ea0837dbc263ce2a2512c4e73c83df68a6a55
> Author:     Ingo Molnar <mingo@elte.hu>
> AuthorDate: Fri Feb 16 01:28:24 2007 -0800
> Committer:  Linus Torvalds <torvalds@woody.linux-foundation.org>
> CommitDate: Fri Feb 16 08:14:00 2007 -0800
> 
>     [PATCH] genirq: do not mask interrupts by default
>     
>     Never mask interrupts immediately upon request.  Disabling interrupts in
>     high-performance codepaths is rare, and on the other hand this change could
>     recover lost edges (or even other types of lost interrupts) by conservatively
>     only masking interrupts after they happen.  (NOTE: with this change the
>     highlevel irq-disable code still soft-disables this IRQ line - and if such an
>     interrupt happens then the IRQ flow handler keeps the IRQ masked.)
>     
>     Mark i8529A controllers as 'never loses an edge'.

I missed that one ... thanks for not telling/CC'ing me and not fixing
powerpc :-( (I know, everybody is supposed to have the bandwidth to read
all of lkml... I don't).

We need to audit all of our PICs to make sure they can deal with
disabling an already ack'ed interrupt, which isn't clear.. in fact, that
might cause some to lose it when re-enabling later on. This is a major
semantic change and now I just discover it while we are at -rc5 !

I'm concerned that quite a few PICs will not re-emit the interrupt once
re-enabled.

Ben.

Re: [PATCH] genirq: do not mask interrupts by default

[Benjamin Herrenschmidt]

> I missed that one ... thanks for not telling/CC'ing me and not fixing
> powerpc :-( (I know, everybody is supposed to have the bandwidth to read
> all of lkml... I don't).
> 
> We need to audit all of our PICs to make sure they can deal with
> disabling an already ack'ed interrupt, which isn't clear.. in fact, that
> might cause some to lose it when re-enabling later on. This is a major
> semantic change and now I just discover it while we are at -rc5 !
> 
> I'm concerned that quite a few PICs will not re-emit the interrupt once
> re-enabled.

Note that I'm not opposed to the change at all, I think it's a good
idea, I'm just worried I'm discovering it a bit late and I've seen PICs
broken in some many colorful ways that I'm a bit worried... Oh well...

Ben.

Re: [PATCH] genirq: do not mask interrupts by default

[Ingo Molnar]

* Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> Note that I'm not opposed to the change at all, I think it's a good 
> idea, I'm just worried I'm discovering it a bit late and I've seen 
> PICs broken in some many colorful ways that I'm a bit worried... Oh 
> well...

This change does not really change irq-flow semantics, what it does is 
that disable_irq()'s effect is delayed. The irq controller does not have 
to re-assert the irq, we've got the soft-resend mechanism. What am i 
missing? Are you worried about this change causing actual breakage? (and 
i'm sorry about not having Cc:-ed you explicitly, i could have sworn you 
were included in that discussion but apparently not!)

	Ingo

Re: [PATCH] genirq: do not mask interrupts by default

[Benjamin Herrenschmidt]

On Tue, 2007-03-27 at 09:32 +0200, Ingo Molnar wrote:
> * Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> 
> > Note that I'm not opposed to the change at all, I think it's a good 
> > idea, I'm just worried I'm discovering it a bit late and I've seen 
> > PICs broken in some many colorful ways that I'm a bit worried... Oh 
> > well...
> 
> This change does not really change irq-flow semantics, what it does is 
> that disable_irq()'s effect is delayed. The irq controller does not have 
> to re-assert the irq, we've got the soft-resend mechanism. What am i 
> missing? Are you worried about this change causing actual breakage? (and 
> i'm sorry about not having Cc:-ed you explicitly, i could have sworn you 
> were included in that discussion but apparently not!)

I'm worried about some broken controllers I know of that might indeed
swallow the interrupt if it occurs, we ack it, then disable it, and
later on re-enable it...

I think the main case I have in mind (pmac-pic) has the necessary
retrigger all over the place but there is definitely a change in the
flow of disabling/enabling here.

Anyway, I'll run some tests tomorrow and make noise if I find something
broken, though I can't test the various embedded thingies in
arch/powerpc.

Ben.