Re: [Ksummit-discuss] [CORE TOPIC] (group) maintainership models

[Arnd Bergmann <arnd@arndb.de>]

On Friday, September 2, 2016 10:25:09 AM CEST Linus Torvalds wrote:
> On Fri, Sep 2, 2016 at 3:46 AM, Vinod Koul <vinod.koul@intel.com> wrote:
> >
> > Arm seems to define NO_IRQ. This is not available in other arch and one
> > driver (moxart-dma) is using it. I don't see why this should be arm specific.
> 
> NO_IRQ is broken. We long long since agreed that 0 is "no irq", and
> that the right way to test for it is just
> 
>    if (!irq) ...
> 
> which is what all the generic drivers use.

Right, this one must have escaped review. We had NO_IRQ almost eliminated
from ARM specific drivers, and this one was probably added later.

> See for example
> 
>    git grep '(!.*irq)'
> 
> for how common that is.
> 
> (If you grep for NO_IRQ, you'll find a lot in particularly powerpc
> drivers and platforms, but there NO_IRQ is actually just defined to
> (0), so it ends up being the same thing).
> 
> The fact that ARM still has a NO_IRQ that seems to be defined to
> ((unsigned int)-1) just means that there are bugs hiding: if ARM
> actually _uses_ that value for "no IRQ", then all the generic drivers
> will be broken, and if ARM core doesn't, then the drivers that still
> use NO_IRQ are broken. Either way you can't win.
>
> So please make sure that NO_IRQ goes away entirely (or, if you insist
> on having the confusing #define for some legacy reason, make it be 0
> like powerpc, so that the real way of just testing "!irq" also works)

I'll throw this patch at my randconfig builder and see what breaks:

diff --git a/arch/arm/include/asm/irq.h b/arch/arm/include/asm/irq.h
index 1bd9510de1b9..b5b8354c1d9e 100644
--- a/arch/arm/include/asm/irq.h
+++ b/arch/arm/include/asm/irq.h
@@ -13,14 +13,6 @@
 #define irq_canonicalize(i)	(i)
 #endif
 
-/*
- * Use this value to indicate lack of interrupt
- * capability
- */
-#ifndef NO_IRQ
-#define NO_IRQ	((unsigned int)(-1))
-#endif
-
 #ifndef __ASSEMBLY__
 struct irqaction;
 struct pt_regs;


When I once looked, I thought all drivers using NO_IRQ were specific
to powerpc or one of the less common architectures.  We do have
these files however:

arch/arm/common/locomo.c:       if (lchip->irq != NO_IRQ && lchip->irq_base != NO_IRQ)
arch/arm/common/locomo.c:       if (lchip->irq != NO_IRQ) {
arch/arm/common/sa1111.c:       if (sachip->irq != NO_IRQ) {
arch/arm/common/sa1111.c:       if (sachip->irq != NO_IRQ) {
arch/arm/mach-mmp/devices.c:    if (desc->irq != NO_IRQ) {
arch/arm/mach-mv78xx0/common.c:                 NO_IRQ,
arch/arm/mach-mv78xx0/common.c:                 NO_IRQ);
arch/arm/mach-mv78xx0/common.c:                 NO_IRQ);
arch/arm/mach-orion5x/rd88f5181l-fxo-setup.c:   orion5x_eth_switch_init(&rd88f5181l_fxo_switch_plat_data, NO_IRQ);
arch/arm/mach-orion5x/rd88f6183ap-ge-setup.c:           .irq            = NO_IRQ,
arch/arm/mach-orion5x/wnr854t-setup.c:  orion5x_eth_switch_init(&wnr854t_switch_plat_data, NO_IRQ);
arch/arm/mach-orion5x/wrt350n-v2-setup.c:       orion5x_eth_switch_init(&wrt350n_v2_switch_plat_data, NO_IRQ);
arch/arm/plat-orion/common.c:   if (irq != NO_IRQ) {
arch/arm/plat-orion/common.c:                  mapbase + 0x2000, SZ_16K - 1, NO_IRQ);
arch/arm/plat-orion/common.c:                  mapbase + 0x2000, SZ_16K - 1, NO_IRQ);
arch/arm/plat-orion/common.c:                  mapbase + 0x2000, SZ_16K - 1, NO_IRQ);
arch/arm/plat-orion/common.c:                  mapbase + 0x2000, SZ_16K - 1, NO_IRQ);
arch/arm/plat-orion/common.c:   if (irq != NO_IRQ) {
arch/arm/plat-orion/common.c:                  mapbase, SZ_512 - 1, NO_IRQ);
arch/arm/plat-orion/common.c:                  mapbase, SZ_512 - 1, NO_IRQ);

I can have another look what can be done about them. IIRC, plat-orion (which
includes mach-orion5x and mv78xx0) until recently still had devices with
IRQ as a valid number, but that was finally fixed now, so we should be able to
clean up most of them.

	Arnd