IRQF_VALID

IRQF_VALID

[Magnus Damm]

Hi everyone,

I'm trying to understand the idea behind IRQF_VALID. Ideally I'd like
to submit a patch to remove IRQF_VALID to make it easier to share
interrupt code between architectures. Perhaps IRQF_VALID is a left
over from good old times, or maybe it has some hidden use that only
more experienced hackers are aware or. Can anyone please shed some
light?

The ARM default state for interrupts seems to be IRQ_NOREQUEST, so
mach code is required to call the ARM-specific set_irq_flags() even
though most of the interrupt handling code is fairly generic using
CONFIG_GENERIC_HARDIRQS=y. Why this special IRQF_VALID case for ARM?

IRQF_VALID

[Russell King - ARM Linux]

On Thu, Jan 28, 2010 at 06:34:24PM +0900, Magnus Damm wrote:
> I'm trying to understand the idea behind IRQF_VALID. Ideally I'd like
> to submit a patch to remove IRQF_VALID to make it easier to share
> interrupt code between architectures. Perhaps IRQF_VALID is a left
> over from good old times, or maybe it has some hidden use that only
> more experienced hackers are aware or. Can anyone please shed some
> light?

On ARM, there are no IRQ tables, there is no hardware IRQ demultiplexing.
It's all done by software.  ARM systems tend to have chained interrupt
controllers, and as such NR_IRQS is normally greater than the actual
number of IRQs in the system.

We also have some situations where IRQs are supported by the controller,
but must never be requested (eg, there are some controllers where the
IRQ is permanently asserted and unmasking it will result in a screaming
interrupt.)

So, in order to make things sane, we need to tell the IRQ layer which
IRQs are valid for each platform.

Asking each platform to tell the core which IRQs are not valid is totally
the wrong thing to do - expecting platform X to deal with N additional
IRQs because platform Y is also configured is just insane.

> The ARM default state for interrupts seems to be IRQ_NOREQUEST, so
> mach code is required to call the ARM-specific set_irq_flags() even
> though most of the interrupt handling code is fairly generic using
> CONFIG_GENERIC_HARDIRQS=y. Why this special IRQF_VALID case for ARM?
> >From my point of view defaulting to IRQ_NOREQUEST unset like other
> platforms would make more sense.

genirq came from ARM originally; I've no idea why it was only partially
ported over...

> Today IRQ_NOREQUEST seems to be used for chained interrupt handlers.
> The check in request_threaded_irq() returns -EINVAL if IRQ_NOREQUEST
> is set. This covers request_irq(), but setup_irq() is lacking a check.
> Maybe this is done intentionally, or perhaps it's something I should
> fix?
> 
> If the purpose with IRQF_VALID is to guard against
> request_irq()/setup_irq() on irqs lacking chip backing, then good news
> is that the CONFIG_GENERIC_HARDIRQ already has code to handle this for
> us. Basically, the default in irq_desc_init makes use of &no_irq_chip.
> And the __setup_irq() code is already checking against irqs mapped to
> &no_irq_chip, those are disallowed with a -ENOSYS.
> 
> Is there any point in keeping IRQF_VALID on ARM, or shall I submit a
> patch to clean things up?

I have no real idea - in this respect, the forced conversion of ARM to
this genirq stuff was a half-botched job.  I really can't comment, and
from my point of view what we have we _know_ works for us, and I really
really really do not want to go and change it.

In order to make any change, there's a _huge_ platform base that would
need to be tested.

IRQF_VALID

[Magnus Damm]

Hi Russell,

On Fri, Jan 29, 2010 at 2:44 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Jan 28, 2010 at 06:34:24PM +0900, Magnus Damm wrote:
>> I'm trying to understand the idea behind IRQF_VALID. Ideally I'd like
>> to submit a patch to remove IRQF_VALID to make it easier to share
>> interrupt code between architectures. Perhaps IRQF_VALID is a left
>> over from good old times, or maybe it has some hidden use that only
>> more experienced hackers are aware or. Can anyone please shed some
>> light?
>
> On ARM, there are no IRQ tables, there is no hardware IRQ demultiplexing.
> It's all done by software. ?ARM systems tend to have chained interrupt
> controllers, and as such NR_IRQS is normally greater than the actual
> number of IRQs in the system.

Most SuperH boards only make use of the INTC controller built-in the
SoC, and in that case NR_IRQS map to the number of INTC vectors. Some
boards also hook up chained interrupt handlers and in that case the
NR_IRQS may be larger than the number of INTC vectors. The INTC
vectors are often pretty sparsely populated though, so on SuperH there
are quite a few IRQs without any chip backing which means that they
make use of &no_irq_chip and handle_bad_irq. Same thing goes for the
SuperH Mobile ARM implementation.

> We also have some situations where IRQs are supported by the controller,
> but must never be requested (eg, there are some controllers where the
> IRQ is permanently asserted and unmasking it will result in a screaming
> interrupt.)
>
> So, in order to make things sane, we need to tell the IRQ layer which
> IRQs are valid for each platform.

But isn't the call to set_irq_chip_and_handler_name() just that?

Is there any reason why we want a chip handler installed but marked
with IRQ_NOREQUEST?

Maybe you explained just that and I'm being slow. =)

Perhaps this feature is for some shared interrupt code that installs a
bunch of contiguous IRQs and then each board/cpu can select which that
are valid? If so, wouldn't it make more sense to tell the shared
interrupt code which IRQs that should be installed and which that
should be masked and left out?

> Asking each platform to tell the core which IRQs are not valid is totally
> the wrong thing to do - expecting platform X to deal with N additional
> IRQs because platform Y is also configured is just insane.

Yeah, that's no good.

I wonder if the issue that IRQF_VALID tries to solve is something
unique to ARM, and if not, perhaps it's already there in the generic
code. I believe that the state of an unused IRQ is quite similar to
the non-valid ARM IRQ.

Or maybe ARM platforms so far have a "chip" installed to be able to
mask and unmask, but other platforms just mask the interrupt and keep
it as an unused IRQ by not doing set_irq_chip_and_handler_name()?

>> Is there any point in keeping IRQF_VALID on ARM, or shall I submit a
>> patch to clean things up?
>
> I have no real idea - in this respect, the forced conversion of ARM to
> this genirq stuff was a half-botched job. ?I really can't comment, and
> from my point of view what we have we _know_ works for us, and I really
> really really do not want to go and change it.
>
> In order to make any change, there's a _huge_ platform base that would
> need to be tested.

Thanks for your comments. It sounds like you prefer to keep things as-is. =)

Either way is fine with me, I'd be happy to drop this right now, but I
also don't mind spending a bit of time and cleaning up things if you
guys think it's the right way forward.

How about something small to begin with and per-mach incremental
changes? The default would be to keep things as-is but also let each
mach implement their own set_irq_flags(). SuperH Mobile ARM will just
have a nop version.

ARM-specific prototype code and a hack for mach-pxa below:

 arch/arm/include/asm/hw_irq.h         |    6 ------
 arch/arm/include/asm/irq.h            |    8 ++++++++
 arch/arm/kernel/irq.c                 |    4 +++-
 arch/arm/mach-pxa/include/mach/irqs.h |    3 +++
 4 files changed, 14 insertions(+), 7 deletions(-)

--- 0001/arch/arm/include/asm/hw_irq.h
+++ work/arch/arm/include/asm/hw_irq.h	2010-01-29 15:16:30.000000000 +0900
@@ -18,10 +18,4 @@ static inline void desc_handle_irq(unsig
 	desc->handle_irq(irq, desc);
 }

-void set_irq_flags(unsigned int irq, unsigned int flags);
-
-#define IRQF_VALID	(1 << 0)
-#define IRQF_PROBE	(1 << 1)
-#define IRQF_NOAUTOEN	(1 << 2)
-
 #endif
--- 0001/arch/arm/include/asm/irq.h
+++ work/arch/arm/include/asm/irq.h	2010-01-29 15:17:16.000000000 +0900
@@ -22,6 +22,14 @@ extern void migrate_irqs(void);
 extern void asm_do_IRQ(unsigned int, struct pt_regs *);
 void init_IRQ(void);

+#ifndef __arch_set_irq_flags
+void set_irq_flags(unsigned int irq, unsigned int flags);
+
+#define IRQF_VALID	(1 << 0)
+#define IRQF_PROBE	(1 << 1)
+#define IRQF_NOAUTOEN	(1 << 2)
+#endif
+
 #endif

 #endif
--- 0001/arch/arm/kernel/irq.c
+++ work/arch/arm/kernel/irq.c	2010-01-29 14:10:02.000000000 +0900
@@ -128,6 +128,7 @@ asmlinkage void __exception asm_do_IRQ(u
 	set_irq_regs(old_regs);
 }

+#ifndef __arch_set_irq_flags
 void set_irq_flags(unsigned int irq, unsigned int iflags)
 {
 	struct irq_desc *desc;
@@ -149,13 +150,14 @@ void set_irq_flags(unsigned int irq, uns
 		desc->status &= ~IRQ_NOAUTOEN;
 	raw_spin_unlock_irqrestore(&desc->lock, flags);
 }
+#endif

 void __init init_IRQ(void)
 {
 	int irq;

 	for (irq = 0; irq < NR_IRQS; irq++)
-		irq_desc[irq].status |= IRQ_NOREQUEST | IRQ_NOPROBE;
+		set_irq_flags(irq, IRQF_NOAUTOEN);

 	init_arch_irq();
 }
--- 0001/arch/arm/mach-pxa/include/mach/irqs.h
+++ work/arch/arm/mach-pxa/include/mach/irqs.h	2010-01-29
15:21:44.000000000 +0900
@@ -310,4 +310,7 @@

 #endif /* CONFIG_PCI_HOST_ITE8152 */

+#define __arch_set_irq_flags
+#define set_irq_flags(irq, flags) do {} while(0)
+
 #endif /* __ASM_MACH_IRQS_H */