[PATCH 1/1] irq: add IRQF_TRIGGER_MASK on PPI by default

[PATCH 1/1] irq: add IRQF_TRIGGER_MASK on PPI by default

[Aniruddha Banerjee]

add IRQF_TRIGGER_MASK on PPI by default so that the PPIs are
not configured as edge-triggered, which may be wrong for certain GIC
implementations such as the GIC-400

Signed-off-by: Aniruddha Banerjee <aniruddhab@nvidia.com>
---
 kernel/irq/manage.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 6b669593e7eb..9b2983cf9fd3 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1982,7 +1982,7 @@ int request_percpu_irq(unsigned int irq, irq_handler_t handler,
 		return -ENOMEM;
 
 	action->handler = handler;
-	action->flags = IRQF_PERCPU | IRQF_NO_SUSPEND;
+	action->flags = IRQF_PERCPU | IRQF_NO_SUSPEND | IRQF_TRIGGER_MASK;
 	action->name = devname;
 	action->percpu_dev_id = dev_id;
 
-- 
2.11.0

Re: [PATCH 1/1] irq: add IRQF_TRIGGER_MASK on PPI by default

[Thomas Gleixner]

On Thu, 30 Mar 2017, Aniruddha Banerjee wrote:

> add IRQF_TRIGGER_MASK on PPI by default so that the PPIs are
> not configured as edge-triggered, which may be wrong for certain GIC
> implementations such as the GIC-400

The above is just useless blurb.

I can't figure out at all WHY a generic interface has anything to do with
edge trigger configuration.

I assume this is (Nvidia) GIC specific nonsense, so why are you inflicting
this on every caller of this interface unconditionally w/o explaining what
the impact of this change might be and why it does not cause havoc for any
existing caller?

This is function is implemented in kernel/irq/ not in foo/gic/ so you
better come up with some coherent explanation.

Thanks,

	tglx

Re: [PATCH 1/1] irq: add IRQF_TRIGGER_MASK on PPI by default

[Jon Hunter]

On 31/03/17 09:01, Thomas Gleixner wrote:
> On Thu, 30 Mar 2017, Aniruddha Banerjee wrote:
> 
>> add IRQF_TRIGGER_MASK on PPI by default so that the PPIs are
>> not configured as edge-triggered, which may be wrong for certain GIC
>> implementations such as the GIC-400
> 
> The above is just useless blurb.
> 
> I can't figure out at all WHY a generic interface has anything to do with
> edge trigger configuration.

I have to agree, it does not make sense in the context of the patch. The
only thing I can think of that this is trying to circumvent the lookup
of the trigger type in __setup_irq() ...

 /*
  * If the trigger type is not specified by the caller,
  * then use the default for this interrupt.
  */
 if (!(new->flags & IRQF_TRIGGER_MASK))
 	new->flags |= irqd_get_trigger_type(&desc->irq_data);

If that is the case, then this does not look correct to me and will most
likely breaking percpu interrupts that do need to lookup the type.

> I assume this is (Nvidia) GIC specific nonsense, so why are you inflicting
> this on every caller of this interface unconditionally w/o explaining what
> the impact of this change might be and why it does not cause havoc for any
> existing caller?

Yes, however, some new nonsense I am not aware of :-(

Aniruddha, why can we not just set the type correctly for the PPI in the
device-tree file and avoid this?

Cheers
Jon

-- 
nvpublic

Re: [PATCH 1/1] irq: add IRQF_TRIGGER_MASK on PPI by default

[Marc Zyngier]

On 31/03/17 09:01, Thomas Gleixner wrote:
> On Thu, 30 Mar 2017, Aniruddha Banerjee wrote:
> 
>> add IRQF_TRIGGER_MASK on PPI by default so that the PPIs are
>> not configured as edge-triggered, which may be wrong for certain GIC
>> implementations such as the GIC-400
> 
> The above is just useless blurb.
> 
> I can't figure out at all WHY a generic interface has anything to do with
> edge trigger configuration.
> 
> I assume this is (Nvidia) GIC specific nonsense, so why are you inflicting
> this on every caller of this interface unconditionally w/o explaining what
> the impact of this change might be and why it does not cause havoc for any
> existing caller?
> 
> This is function is implemented in kernel/irq/ not in foo/gic/ so you
> better come up with some coherent explanation.

Indeed. I'm not aware of anything wrong so far with GIC400, so this is
most likely referring to an integration issue.

Furthermore, PPI triggers are usually not configurable on GIC400. My bet
is that this is only a DT issue, but in the absence of any coherent
justification, it is hard to make an educated guess...

Aniruddha: please state your problem clearly so that we can understand
what exactly is going wrong.

Thanks,

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

RE: [PATCH 1/1] irq: add IRQF_TRIGGER_MASK on PPI by default

[Aniruddha Banerjee]

> On 31/03/17 , Marc Zyngier wrote:
> On 31/03/17 09:01, Thomas Gleixner wrote:
> > On Thu, 30 Mar 2017, Aniruddha Banerjee wrote:
> >
> >> add IRQF_TRIGGER_MASK on PPI by default so that the PPIs are not
> >> configured as edge-triggered, which may be wrong for certain GIC
> >> implementations such as the GIC-400
> >
> > The above is just useless blurb.
> >
> > I can't figure out at all WHY a generic interface has anything to do
> > with edge trigger configuration.
> >
> > I assume this is (Nvidia) GIC specific nonsense, so why are you
> > inflicting this on every caller of this interface unconditionally w/o
> > explaining what the impact of this change might be and why it does not
> > cause havoc for any existing caller?
> >
> > This is function is implemented in kernel/irq/ not in foo/gic/ so you
> > better come up with some coherent explanation.
> 
> Indeed. I'm not aware of anything wrong so far with GIC400, so this is most likely
> referring to an integration issue.
> 
> Furthermore, PPI triggers are usually not configurable on GIC400. My bet is that this is
> only a DT issue, but in the absence of any coherent justification, it is hard to make an
> educated guess...

That was an awesome guess and we were in fact doing something very wrong in the DT. 
In the GIC-400 implementation, the PPI triggers are read-only. I was trying to configure
the PPI as edge-triggered, and the writes were dropped in the process.
A big thank you to Jon Hunter and Marc for pointing this out.

Regards,
Aniruddha.