irqchip/gic*: Complain about the use of IRQ_TYPE_NONE

Grepping through the dts files, the documentation, and reviewing patches, one can only notice the use of IRQ_TYPE_NONE in interrupt specifiers. At least for the GIC, this doesn't mean anything. The unsuspecting driver will end-up with whatever was there before, and there is a 50% probability that it is not what it wants.

I'd love to fix it myself, but I also have a 50% probability of getting it wrong. In order to make the user aware they are walking on thin ice, let's add some warnings. Hopefully, they'll be annoying enough that people will fix their firmware. Croudsourcing debugging...

If nobody complains louder than the warnings, I plan to get this into 4.17.

Marc Zyngier (2):
irqchip/gic: Loudly complain about the use of IRQ_TYPE_NONE irqchip/gic-v3: Loudly complain about the use of IRQ_TYPE_NONE

 drivers/irqchip/irq-gic-v3.c | 5 +++++
 drivers/irqchip/irq-gic.c    | 5 +++++
 2 files changed, 10 insertions(+)

2.14.2

There is a huge number of broken device trees out there. Just grepping through the tree for the use of IRQ_TYPE_NONE in conjunction with the GIC is scary.

People just don't realise that IRQ_TYPE_NONE just doesn't exist, and you just get whatever junk was there before. So let's make them aware of the issue.

---
 drivers/irqchip/irq-gic.c | 5 +++++
 1 file changed, 5 insertions(+)

index 79801c24800b..ac2e62d613d1 100644
@@ -1011,6 +1011,9 @@ static int gic_irq_domain_translate(struct irq_domain *d,
 			*hwirq += 16;
 
 		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
+
+		/* Make it clear that broken DTs are... broken */
+		WARN_ON(*type == IRQ_TYPE_NONE);
 		return 0;
 	}
 

@@ -1020,6 +1023,8 @@ static int gic_irq_domain_translate(struct irq_domain *d,
 
 		*hwirq = fwspec->param[0];
 		*type = fwspec->param[1];
+
+		WARN_ON(*type == IRQ_TYPE_NONE);
 		return 0;
 	}
 

2.14.2

There is a huge number of broken device trees out there. Just grepping through the tree for the use of IRQ_TYPE_NONE in conjunction with the GIC is scary.

People just don't realise that IRQ_TYPE_NONE just doesn't exist, and you just get whatever junk was there before. So let's make them aware of the issue.

---
 drivers/irqchip/irq-gic-v3.c | 5 +++++
 1 file changed, 5 insertions(+)

index 3e9eeb6cb294..5bb7bb22f1c1 100644
@@ -916,6 +916,9 @@ static int gic_irq_domain_translate(struct irq_domain *d,
 		}
 
 		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
+
+		/* Make it clear that broken DTs are... broken */
+		WARN_ON(*type == IRQ_TYPE_NONE);
 		return 0;
 	}
 

@@ -925,6 +928,8 @@ static int gic_irq_domain_translate(struct irq_domain *d,
 
 		*hwirq = fwspec->param[0];
 		*type = fwspec->param[1];
+
+		WARN_ON(*type == IRQ_TYPE_NONE);
 		return 0;
 	}
 

2.14.2

Hi,

I'm late to the party here and this patch has already landed.

I'm a bit curious. You say "you just get whatever junk was there before", I think you're implying that if you specify "IRQ_TYPE_NONE" in the device tree then you'll be left with whatever the default trigger was: either however the hardware was initted or how the bootloader left it.

...but from my recollection that's not the case and someone from stack overflow seems to have nicely documented how I think this works:

https://stackoverflow.com/questions/40011799/mapping-device-tree-interrupt-flags-to-devm-request-irq

Specifically it's my understanding that the irq_flags in the device tree rarely have any impact on reality because most drivers specify how they want the interrupt configured anyway. The device tree flags will only ever be used if the client code didn't specify.

Assuming my understanding is correct, then I guess it calls into question a bit the requirement that the device tree _must_ specify the IRQ trigger type. For the most part the code know (and needs to know) what the IRQ trigger type is. In cases where the code is really generic (like an i2c device) and needs to run on lots of platforms where not all triggers might be available, it's nice if the trigger type _can_ be specified in the device tree, but does it need to be mandatory?

Downsides of forcing everyone to start specifying their trigger type in the device tree:

* There's no double-checking. You can totally put something bogus there and it will have no impact on your system whatsoever. AKA I just changed a whole pile of defines in my device tree from IRQ_TYPE_LEVEL_HIGH to IRQ_TYPE_LEVEL_LOW and it had no impact on my system. That's because the drivers all passed "IRQF_TRIGGER_HIGH" to request_irq.

* Developers will think that they can change the value in the device tree and it will have an effect. It won't.

-Doug

I guess there's also the alternative nuclear option of breaking their build ;)

Robin.

----->8-----

b/include/dt-bindings/interrupt-controller/irq.h
index a8b310555f14..de79af80d01e 100644

@@ -10,7 +10,7 @@
  #ifndef _DT_BINDINGS_INTERRUPT_CONTROLLER_IRQ_H
  #define _DT_BINDINGS_INTERRUPT_CONTROLLER_IRQ_H
 
-#define IRQ_TYPE_NONE		0
+#define IRQ_TYPE_NONE		"This is nonsense and needs fixing"
  #define IRQ_TYPE_EDGE_RISING	1
  #define IRQ_TYPE_EDGE_FALLING	2
  #define IRQ_TYPE_EDGE_BOTH	(IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING)

On a more serious note, though, it dawns on me that this might be something DTC could realistically scream about for us, although I guess not all irqchip bindings include a type specifier so it would probably need to special-case known ones.

Robin.