Re: IRQF_DISABLED problem

Re: IRQF_DISABLED problem

[Robert Hancock]

David Miller wrote:
> From: Arjan van de Ven <arjan@infradead.org>
> Date: Fri, 27 Jul 2007 13:11:56 -0700
> 
>> On Thu, 2007-07-26 at 16:17 -0700, Linus Torvalds wrote:
>>> On Thu, 26 Jul 2007, Linus Torvalds wrote:
>>>>  (c) "one IRQF_DISABLED means that everything runs disabled". This is 
>>>>      quite possibly buggy.
>>> (Side note: I'm not claiming this (or it's mirror image (d)) is really any 
>>> better/worse than the current behaviour from a theoretical standpoint, but 
>>> at least the current behaviour is _tested_, which makes it better in 
>>> practice. So if we want to change this, I think we want to change it to 
>>> something that is _obviously_ better).
>> my personal preference would actually be to just never enable
>> interrupts. It's the fastest solution obviously, the most friendly on
>> stack and.. well simplest. Drivers no longer need to play some of the
>> games that they do today. And while there is an argument that this may
>> introduce a bit of latency... I'm not really convinced.
> 
> If you have a "chirpy" serial controller with only a 1 byte
> fifo, even a quite reasonable interrupt handler can cause
> receive characters to get lost if you disable interrupts during
> the entirety of it's execution.
> 
> It really is needed.
> 
> And it's just plain rude to disable interrupts when it isn't
> absolutely necessary.

Does anyone really use those serial controllers with no FIFO anymore? 
They've never been reliable for remotely high speeds..

-- 
Robert Hancock      Saskatoon, SK, Canada
To email, remove "nospam" from hancockr@nospamshaw.ca
Home Page: http://www.roberthancock.com/

Re: IRQF_DISABLED problem

[Mark Lord]

Robert Hancock wrote:
> David Miller wrote:
>> From: Arjan van de Ven <arjan@infradead.org>
>> Date: Fri, 27 Jul 2007 13:11:56 -0700
>>
>>> On Thu, 2007-07-26 at 16:17 -0700, Linus Torvalds wrote:
>>>> On Thu, 26 Jul 2007, Linus Torvalds wrote:
>>>>>  (c) "one IRQF_DISABLED means that everything runs disabled". This 
>>>>> is      quite possibly buggy.
>>>> (Side note: I'm not claiming this (or it's mirror image (d)) is 
>>>> really any better/worse than the current behaviour from a 
>>>> theoretical standpoint, but at least the current behaviour is 
>>>> _tested_, which makes it better in practice. So if we want to change 
>>>> this, I think we want to change it to something that is _obviously_ 
>>>> better).
>>> my personal preference would actually be to just never enable
>>> interrupts. It's the fastest solution obviously, the most friendly on
>>> stack and.. well simplest. Drivers no longer need to play some of the
>>> games that they do today. And while there is an argument that this may
>>> introduce a bit of latency... I'm not really convinced.
>>
>> If you have a "chirpy" serial controller with only a 1 byte
>> fifo, even a quite reasonable interrupt handler can cause
>> receive characters to get lost if you disable interrupts during
>> the entirety of it's execution.
>>
>> It really is needed.
>>
>> And it's just plain rude to disable interrupts when it isn't
>> absolutely necessary.
> 
> Does anyone really use those serial controllers with no FIFO anymore? 
> They've never been reliable for remotely high speeds..

There's another thread on lkml currently about this exact issue,
whereby the UART "fifo nearly full" interrupt isn't getting in quickly
enough for the kernel to de-assert RTS in time..

IRQF_DISABLED problem

[Matthew Wilcox]

I noticed that we only look at the first action in the chain when
determining whether to re-enable local interrupts during handle_IRQ_event.
But we don't try to exclude sharing interrupts with mixtures of
IRQF_DISABLED set and clear.  I just tried to do that locally, and one
of my USB ports disappears, because it shares an interrupt with qla2xxx
which sets IRQF_DISABLED, and UHCI doesn't.

Another possibility is to force it if *any* of the handlers want
IRQF_DISABLED.  This seems to work:

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 203a518..d804a0b 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -308,6 +308,15 @@ int setup_irq(unsigned int irq, struct irqaction *new)
 			goto mismatch;
 #endif
 
+		/* If one handler wants interrupts to be disabled,
+		 * they must all be disabled.  Rather than walk the list of
+		 * handlers twice at interrupt time, just make sure the
+		 * head handler has its flag set
+		 */
+		if ((new->flags & IRQF_DISABLED) &&
+		    !(old->flags & IRQF_DISABLED)
+			old->flags |= IRQF_DISABLED);
+
 		/* add new interrupt at end of irq queue */
 		do {
 			p = &old->next;


This patch makes interrupts requested with IRQF_DISABLED non-matching
fail, in case you think it's a better solution:

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 203a518..2c99b88 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -293,10 +293,12 @@ int setup_irq(unsigned int irq, struct irqaction *new)
 		 * Can't share interrupts unless both agree to and are
 		 * the same type (level, edge, polarity). So both flag
 		 * fields must have IRQF_SHARED set and the bits which
-		 * set the trigger type must match.
+		 * set the trigger type must match and the disabled bit
+		 * must match.
 		 */
 		if (!((old->flags & new->flags) & IRQF_SHARED) ||
-		    ((old->flags ^ new->flags) & IRQF_TRIGGER_MASK)) {
+		    ((old->flags ^ new->flags) &
+		     (IRQF_TRIGGER_MASK | IRQF_DISABLED))) {
 			old_name = old->name;
 			goto mismatch;
 		}

-- 
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: IRQF_DISABLED problem

[Linus Torvalds]

On Thu, 26 Jul 2007, Matthew Wilcox wrote:
> 
> I noticed that we only look at the first action in the chain when
> determining whether to re-enable local interrupts during handle_IRQ_event.

You can't really share an interrupt handler that wants to run with 
interrupts on with one that wants to run with them off.

That said, I think the whole IRQF_DISABLED thing should go away. It is 
total legacy crud, methinks - it used to be SA_INTERRUPT, and it's always 
worked the way IRQF_DISABLED works now: it only looks at the first one in 
the chain.

> But we don't try to exclude sharing interrupts with mixtures of
> IRQF_DISABLED set and clear.

I think you should just consider it to be a "if you mix them, you get 
randomr results".

> I just tried to do that locally, and one
> of my USB ports disappears, because it shares an interrupt with qla2xxx
> which sets IRQF_DISABLED, and UHCI doesn't.

There really is no excuse for using IRQF_DISABLED unless you're something 
like a system device (like the timer interrupt or similar) and you have an 
exclusive irq handler. A SCSI driver almost certainly has no business 
doing it.

Generally, I would say that "IRQF_DISABLED | IRQF_SHARED" is an insane 
combination, but a quick grep shows that it's distressingly common.

The real fix is to just leave it as it is. It's always worked that way. 
IRQF_DISABLED basically cannot have any sane behaviour in the presense of 
mixing.

		Linus

Re: IRQF_DISABLED problem

[David Miller]

From: Matthew Wilcox <matthew@wil.cx>
Date: Thu, 26 Jul 2007 14:13:56 -0600

> 
> I noticed that we only look at the first action in the chain when
> determining whether to re-enable local interrupts during handle_IRQ_event.
> But we don't try to exclude sharing interrupts with mixtures of
> IRQF_DISABLED set and clear.  I just tried to do that locally, and one
> of my USB ports disappears, because it shares an interrupt with qla2xxx
> which sets IRQF_DISABLED, and UHCI doesn't.
> 
> Another possibility is to force it if *any* of the handlers want
> IRQF_DISABLED.  This seems to work:

Yes, this is consistent with how we handle sharing, we should
enforce that all the flags on the chain are compatible.

Re: IRQF_DISABLED problem

[Linus Torvalds]

On Thu, 26 Jul 2007, David Miller wrote:
> > 
> > Another possibility is to force it if *any* of the handlers want
> > IRQF_DISABLED.  This seems to work:
> 
> Yes, this is consistent with how we handle sharing, we should
> enforce that all the flags on the chain are compatible.

No. It's no better than the current situation.

There are really a few choices:

 (a) Keep it like we always have.  What's the downside, really? It's what 
     we've got, it's what is tested.

 (b) Enforce that flags match. This may sound logical, but it will 
     actually *break* existing setups, because tons of drivers set 
     IRQF_DISABLED for no good reason (probably all totally historical)

 (c) "one IRQF_DISABLED means that everything runs disabled". This is 
     quite possibly buggy. There have been SCSI drivers with timeout 
     behaviour where they actually wait for timers to happen while in 
     their irq handlers. Bad form, and I *hope* we've fixed them all, but 
     I distinctly remember it being important that the timer had higher 
     priority than some SCSI drivers on some architecture.

 (d) "one !IRFQ_DISABLED means that everything runs with irq's on". I 
     don't think it's any better than the other behaviour.

 (e) Just ignore IRQF_DISABLED entirely when mixed with IRQF_SHARED, 
     because they're all pretty much guaranteed to be legacy and buggy and 
     pointless.

 (f) Disable and re-enable interrupts per handler.

Quite frankly, my preference would be (a) followed by (e) or (f), and 
(b)-(d) are in my opinion the worst of the lot with no upsides at all (and 
(b) in particular is pretty much _guaranteed_ to break existing setups).

			Linus

Re: IRQF_DISABLED problem

[David Miller]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Thu, 26 Jul 2007 16:04:42 -0700 (PDT)

> Quite frankly, my preference would be (a) followed by (e) or (f), and 
> (b)-(d) are in my opinion the worst of the lot with no upsides at all (and 
> (b) in particular is pretty much _guaranteed_ to break existing setups).

I look at some cases, such as EHEA on powerpc which is a pretty
modern driver written by not clueless folks, and wonder if they
do it because they know the IRQ is non-shared, they know their
interrupt handler is insanely simple and short, and just want to
avoid the overhead of that sti()/cli() in the caller.

All the EHEA interrupt handler does is unconditionally set a state bit
and schedule a softirq, then return.

The powerpc folks do delayed IRQ enable/disable using software state,
but perhaps these drivers were written before that.

Re: IRQF_DISABLED problem

[Linus Torvalds]

On Thu, 26 Jul 2007, Linus Torvalds wrote:
> 
>  (c) "one IRQF_DISABLED means that everything runs disabled". This is 
>      quite possibly buggy.

(Side note: I'm not claiming this (or it's mirror image (d)) is really any 
better/worse than the current behaviour from a theoretical standpoint, but 
at least the current behaviour is _tested_, which makes it better in 
practice. So if we want to change this, I think we want to change it to 
something that is _obviously_ better).

		Linus

Re: IRQF_DISABLED problem

[Arjan van de Ven]

On Thu, 2007-07-26 at 16:17 -0700, Linus Torvalds wrote:
> 
> On Thu, 26 Jul 2007, Linus Torvalds wrote:
> > 
> >  (c) "one IRQF_DISABLED means that everything runs disabled". This is 
> >      quite possibly buggy.
> 
> (Side note: I'm not claiming this (or it's mirror image (d)) is really any 
> better/worse than the current behaviour from a theoretical standpoint, but 
> at least the current behaviour is _tested_, which makes it better in 
> practice. So if we want to change this, I think we want to change it to 
> something that is _obviously_ better).

my personal preference would actually be to just never enable
interrupts. It's the fastest solution obviously, the most friendly on
stack and.. well simplest. Drivers no longer need to play some of the
games that they do today. And while there is an argument that this may
introduce a bit of latency... I'm not really convinced. The real work if
it's really heavy is supposed to happen in not-hard-irq context anyway,
and for all other cases interrupting the original handler is very likely
the most expensive thing in terms of adding latency to the system.


If I remember correctly there have been a few fedora releases which did
this, with no bad behavior.. I don't know if Fedora still does this.

-- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
Test the interaction between Linux and your BIOS via http://www.linuxfirmwarekit.org

Re: IRQF_DISABLED problem

[David Miller]

From: Arjan van de Ven <arjan@infradead.org>
Date: Fri, 27 Jul 2007 13:11:56 -0700

> On Thu, 2007-07-26 at 16:17 -0700, Linus Torvalds wrote:
> > 
> > On Thu, 26 Jul 2007, Linus Torvalds wrote:
> > > 
> > >  (c) "one IRQF_DISABLED means that everything runs disabled". This is 
> > >      quite possibly buggy.
> > 
> > (Side note: I'm not claiming this (or it's mirror image (d)) is really any 
> > better/worse than the current behaviour from a theoretical standpoint, but 
> > at least the current behaviour is _tested_, which makes it better in 
> > practice. So if we want to change this, I think we want to change it to 
> > something that is _obviously_ better).
> 
> my personal preference would actually be to just never enable
> interrupts. It's the fastest solution obviously, the most friendly on
> stack and.. well simplest. Drivers no longer need to play some of the
> games that they do today. And while there is an argument that this may
> introduce a bit of latency... I'm not really convinced.

If you have a "chirpy" serial controller with only a 1 byte
fifo, even a quite reasonable interrupt handler can cause
receive characters to get lost if you disable interrupts during
the entirety of it's execution.

It really is needed.

And it's just plain rude to disable interrupts when it isn't
absolutely necessary.