[PATCH] irq: handle irq0 special only on x86

[PATCH] irq: handle irq0 special only on x86

[Uwe Kleine-König]

I just noticed this when digging in the irq handling.  At least for arm
this doesn't make sense.  Not sure if x86 is the only arch this test
is valid for, but probably it is.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
---
 kernel/irq/spurious.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/kernel/irq/spurious.c b/kernel/irq/spurious.c
index 22b0a6e..4996b66 100644
--- a/kernel/irq/spurious.c
+++ b/kernel/irq/spurious.c
@@ -199,8 +199,10 @@ try_misrouted_irq(unsigned int irq, struct irq_desc *desc,
 	if (irqfixup < 2)
 		return 0;
 
+#if defined(CONFIG_ARCH_X86)
 	if (!irq)
 		return 1;
+#endif
 
 	/*
 	 * Since we don't get the descriptor lock, "action" can
-- 
1.6.5.2

Re: [PATCH] irq: handle irq0 special only on x86

[Américo Wang]

2009/12/9 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> I just noticed this when digging in the irq handling.  At least for arm
> this doesn't make sense.  Not sure if x86 is the only arch this test
> is valid for, but probably it is.

No, it is not.

Try grep -Inr 'irq0' arch/*/kernel.


>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: H. Peter Anvin <hpa@zytor.com>
> ---
>  kernel/irq/spurious.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/kernel/irq/spurious.c b/kernel/irq/spurious.c
> index 22b0a6e..4996b66 100644
> --- a/kernel/irq/spurious.c
> +++ b/kernel/irq/spurious.c
> @@ -199,8 +199,10 @@ try_misrouted_irq(unsigned int irq, struct irq_desc *desc,
>        if (irqfixup < 2)
>                return 0;
>
> +#if defined(CONFIG_ARCH_X86)
>        if (!irq)
>                return 1;
> +#endif
>
>        /*
>         * Since we don't get the descriptor lock, "action" can

Re: [PATCH] irq: handle irq0 special only on x86

[Uwe Kleine-König]

Hello,

On Wed, Dec 09, 2009 at 05:28:09PM +0800, Américo Wang wrote:
> 2009/12/9 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> > I just noticed this when digging in the irq handling.  At least for arm
> > this doesn't make sense.  Not sure if x86 is the only arch this test
> > is valid for, but probably it is.
> 
> No, it is not.
> 
> Try grep -Inr 'irq0' arch/*/kernel.
Some hits are unrelated, some others are usage of an irq0.  AFAIK irq0
is reported on x86 for misrouted irqs[1].  I think on other archs it's just
an ordinary irq.  If you still think I missed something in the output of
your grep, please be a bit more specific.

Thanks
Uwe

[1] I don't really know x86, so this might be complete non-sense.

> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: H. Peter Anvin <hpa@zytor.com>
> > ---
> >  kernel/irq/spurious.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> >
> > diff --git a/kernel/irq/spurious.c b/kernel/irq/spurious.c
> > index 22b0a6e..4996b66 100644
> > --- a/kernel/irq/spurious.c
> > +++ b/kernel/irq/spurious.c
> > @@ -199,8 +199,10 @@ try_misrouted_irq(unsigned int irq, struct irq_desc *desc,
> >        if (irqfixup < 2)
> >                return 0;
> >
> > +#if defined(CONFIG_ARCH_X86)
> >        if (!irq)
> >                return 1;
> > +#endif
> >
> >        /*
> >         * Since we don't get the descriptor lock, "action" can
> 

-- 
Pengutronix e.K.                              | Uwe Kleine-König            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

Re: [PATCH] irq: handle irq0 special only on x86

[David Miller]

From: Am^[$(D+1^[(Brico Wang <xiyou.wangcong@gmail.com>
Date: Wed, 9 Dec 2009 17:28:09 +0800

> 2009/12/9 Uwe Kleine-K^[$(D+S^[(Bnig <u.kleine-koenig@pengutronix.de>:
>> I just noticed this when digging in the irq handling. ^[,A ^[(BAt least for arm
>> this doesn't make sense. ^[,A ^[(BNot sure if x86 is the only arch this test
>> is valid for, but probably it is.
> 
> No, it is not.
> 
> Try grep -Inr 'irq0' arch/*/kernel.

The edict was sent down long ago that IRQ number zero is
special across the entire kernel tree.

If IRQ zero can happen, you should offset the IRQ values you publish
to the rest of the kernel, and translate them back when you process
them internally.

Re: [PATCH] irq: handle irq0 special only on x86

[Uwe Kleine-König]

Hi David,

On Wed, Dec 09, 2009 at 01:41:51AM -0800, David Miller wrote:
> From: Américo Wang <xiyou.wangcong@gmail.com>
> Date: Wed, 9 Dec 2009 17:28:09 +0800
> 
> > 2009/12/9 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> >> I just noticed this when digging in the irq handling. ^[,A At least for arm
> >> this doesn't make sense. ^[,A Not sure if x86 is the only arch this test
> >> is valid for, but probably it is.
> > 
> > No, it is not.
> > 
> > Try grep -Inr 'irq0' arch/*/kernel.
> 
> The edict was sent down long ago that IRQ number zero is
> special across the entire kernel tree.
This doesn't help here.  x86 uses irq0 for timing and AFAIK it's some
kind of catch-it-all interrupt---I don't know the exact details though.

> If IRQ zero can happen, you should offset the IRQ values you publish
> to the rest of the kernel, and translate them back when you process
> them internally.
Even if x86 will do that the problem remains.  After it does an offset
of 1 (say) the code will read as follows after adaption:

	/*
	 * But for 'irqfixup == 2' we also do it for handled interrupts if
	 * they are marked as IRQF_IRQPOLL (or for irq zero, which is the
	 * traditional PC timer interrupt.. Legacy)
	 */
	if (irqfixup < 2)
		return 0;

	if (irq == 1)
		return 1;

So try_misrouted_irq hard codes the value of the timer irq (be it zero
or one) and this is wrong for other archs like ARM where irq0 is used on
some machines, too.  There it's just an ordinary irq.

BTW I don't see a reason to forbid irq0 if you don't need to doubt of
it's validity even in driver code.

So if the resources of a platform device specify:

	struct resource mydevicesresources[] = {
		...
		{
			.start = 0,
			.end = 0,
			.flags = IORESOURCE_IRQ,
		},
		...
	};

I'd like to use the returned value by platform_get_irq for this device.

Note, I fully agree to use 0 for NO_IRQ if you have an int-sized value
that holds either NO_IRQ or a valid irq number.  But in practise I'd not
recommend to use this idiom.

Best regards
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-König            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

Re: [PATCH] irq: handle irq0 special only on x86

[H. Peter Anvin]

On 12/10/2009 12:24 AM, Uwe Kleine-König wrote:
> 
> Note, I fully agree to use 0 for NO_IRQ if you have an int-sized value
> that holds either NO_IRQ or a valid irq number.  But in practise I'd not
> recommend to use this idiom.
> 

You're tilting at windmills about something that was settled long ago,
like it or not.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH] irq: handle irq0 special only on x86

[Maciej W. Rozycki]

On Thu, 10 Dec 2009, H. Peter Anvin wrote:

> > Note, I fully agree to use 0 for NO_IRQ if you have an int-sized value
> > that holds either NO_IRQ or a valid irq number.  But in practise I'd not
> > recommend to use this idiom.
> > 
> 
> You're tilting at windmills about something that was settled long ago,
> like it or not.

 This is nothing that couldn't be changed; my personal preference would be 
quite the obvious choice of something along the lines of:

#define NO_IRQ -1U

that would also have the benefit of forcing all the relevant places to use 
the macro rather than an expression like !irq, improving code readability 
and greppability.

 My motivation to push any such change at the moment is rather weak though 
-- it's been a while since I was last pissed off by the weirdness of 
assumptions about IRQ 0 in Linux.

  Maciej

Re: [PATCH] irq: handle irq0 special only on x86

[Uwe Kleine-König]

On Thu, Dec 10, 2009 at 08:40:11AM -0800, H. Peter Anvin wrote:
> On 12/10/2009 12:24 AM, Uwe Kleine-König wrote:
> > 
> > Note, I fully agree to use 0 for NO_IRQ if you have an int-sized value
> > that holds either NO_IRQ or a valid irq number.  But in practise I'd not
> > recommend to use this idiom.
> > 
> 
> You're tilting at windmills about something that was settled long ago,
> like it or not.
And what about the patch, not judging my comments about irq0 in general?

AFAICT the check in try_misrouted_irq for irq being not zero does only
make sense on x86, doesn't it?

The comment a few lines above the check reads:

	But for 'irqfixup == 2' we also do it for handled interrupts if
	they are marked as IRQF_IRQPOLL (or for irq zero, which is the
	traditional PC timer interrupt.. Legacy)

So I think the patch is justified.

Best regards
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-König            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

Re: [PATCH] irq: handle irq0 special only on x86

[Uwe Kleine-König]

Hello,

On Wed, Dec 09, 2009 at 10:20:01AM +0100, Uwe Kleine-König wrote:
> I just noticed this when digging in the irq handling.  At least for arm
> this doesn't make sense.  Not sure if x86 is the only arch this test
> is valid for, but probably it is.
... so I added linux-arch@vger.kernel.org
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: H. Peter Anvin <hpa@zytor.com>
> ---
>  kernel/irq/spurious.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/irq/spurious.c b/kernel/irq/spurious.c
> index 22b0a6e..4996b66 100644
> --- a/kernel/irq/spurious.c
> +++ b/kernel/irq/spurious.c
> @@ -199,8 +199,10 @@ try_misrouted_irq(unsigned int irq, struct irq_desc *desc,
>  	if (irqfixup < 2)
>  		return 0;
>  
> +#if defined(CONFIG_ARCH_X86)
>  	if (!irq)
>  		return 1;
> +#endif
>  
>  	/*
>  	 * Since we don't get the descriptor lock, "action" can
the feed-back I have got up to now wasn't helpfull.  (Only some "irq0 is
evil---no it's not" discussion.) So what do you think?  I admit the
#ifdef isn't nice, but if the semantic is OK I'm willing to rework it
into something more pretty.

Best regards
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-König            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

Re: [PATCH] irq: handle irq0 special only on x86

[H. Peter Anvin]

On 01/12/2010 07:59 AM, Uwe Kleine-König wrote:
> the feed-back I have got up to now wasn't helpfull.  (Only some "irq0 is
> evil---no it's not" discussion.) So what do you think?  I admit the
> #ifdef isn't nice, but if the semantic is OK I'm willing to rework it
> into something more pretty.

There was a debate on this a long time ago, and the outcome was that IRQ
0 is invalid, across the kernel, and that it is up to each architecture
to carry exceptions (like IRQ 0 for the timer interrupt in x86.)  Hinc
dictat Linus, so you would have to convince him before any of the arch
maintainer could realistically even consider this change.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.