[PATCH 0/2] irqchip/gic*: Complain about the use of IRQ_TYPE_NONE

[PATCH 0/2] irqchip/gic*: Complain about the use of IRQ_TYPE_NONE

[Marc Zyngier]

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

[PATCH 1/2] irqchip/gic: Loudly complain about the use of IRQ_TYPE_NONE

[Marc Zyngier]

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.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/irqchip/irq-gic.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 79801c24800b..ac2e62d613d1 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -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

[PATCH 2/2] irqchip/gic-v3: Loudly complain about the use of IRQ_TYPE_NONE

[Marc Zyngier]

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.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/irqchip/irq-gic-v3.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 3e9eeb6cb294..5bb7bb22f1c1 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -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

[PATCH 2/2] irqchip/gic-v3: Loudly complain about the use of IRQ_TYPE_NONE

[Doug Anderson]

Hi,

On Fri, 16 Mar 2018 at 11:02 AM Marc Zyngier <marc.zyngier@arm.com> wrote:
>
> 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.

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

[PATCH 0/2] irqchip/gic*: Complain about the use of IRQ_TYPE_NONE

[Robin Murphy]

On 16/03/18 14:55, Marc Zyngier wrote:
> 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...

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

Robin.

----->8-----
diff --git a/include/dt-bindings/interrupt-controller/irq.h 
b/include/dt-bindings/interrupt-controller/irq.h
index a8b310555f14..de79af80d01e 100644
--- a/include/dt-bindings/interrupt-controller/irq.h
+++ b/include/dt-bindings/interrupt-controller/irq.h
@@ -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)

[PATCH 0/2] irqchip/gic*: Complain about the use of IRQ_TYPE_NONE

[Marc Zyngier]

On 16/03/18 16:19, Robin Murphy wrote:
> On 16/03/18 14:55, Marc Zyngier wrote:
>> 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...
> 
> I guess there's also the alternative nuclear option of breaking their 
> build ;)
> 
> Robin.
> 
> ----->8-----
> diff --git a/include/dt-bindings/interrupt-controller/irq.h 
> b/include/dt-bindings/interrupt-controller/irq.h
> index a8b310555f14..de79af80d01e 100644
> --- a/include/dt-bindings/interrupt-controller/irq.h
> +++ b/include/dt-bindings/interrupt-controller/irq.h
> @@ -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)
> 

What really annoys me with this patch is that you haven't put a SoB on it...

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 0/2] irqchip/gic*: Complain about the use of IRQ_TYPE_NONE

[Robin Murphy]

On 16/03/18 16:39, Marc Zyngier wrote:
> On 16/03/18 16:19, Robin Murphy wrote:
>> On 16/03/18 14:55, Marc Zyngier wrote:
>>> 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...
>>
>> I guess there's also the alternative nuclear option of breaking their
>> build ;)
>>
>> Robin.
>>
>> ----->8-----
>> diff --git a/include/dt-bindings/interrupt-controller/irq.h
>> b/include/dt-bindings/interrupt-controller/irq.h
>> index a8b310555f14..de79af80d01e 100644
>> --- a/include/dt-bindings/interrupt-controller/irq.h
>> +++ b/include/dt-bindings/interrupt-controller/irq.h
>> @@ -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)
>>
> 
> What really annoys me with this patch is that you haven't put a SoB on it...

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.