Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1613410 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/1] irq: add IRQF_TRIGGER_MASK on PPI by default Aniruddha Banerjee <aniruddhab@nvidia.com> - 2017-03-30 21:50 +0200
    Re: [PATCH 1/1] irq: add IRQF_TRIGGER_MASK on PPI by default Thomas Gleixner <tglx@linutronix.de> - 2017-03-31 10:10 +0200
      Re: [PATCH 1/1] irq: add IRQF_TRIGGER_MASK on PPI by default Marc Zyngier <marc.zyngier@arm.com> - 2017-03-31 10:20 +0200
        RE: [PATCH 1/1] irq: add IRQF_TRIGGER_MASK on PPI by default Aniruddha Banerjee <aniruddhab@nvidia.com> - 2017-03-31 14:10 +0200
      Re: [PATCH 1/1] irq: add IRQF_TRIGGER_MASK on PPI by default Jon Hunter <jonathanh@nvidia.com> - 2017-03-31 10:20 +0200

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

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

[toc] | [next] | [standalone]

#1613724

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

[toc] | [prev] | [next] | [standalone]

#1613727

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...

[toc] | [prev] | [next] | [standalone]

#1613944

> 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.

[toc] | [prev] | [next] | [standalone]

#1613730

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web