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

Groups > linux.kernel > #1328926

Re: [PATCH] genirq: fix trigger flags check for shared irqs

Show all headers | View raw

Hi Thomas,

Any further thoughts on this? (some comments below)

On Thu, Jan 28, 2016 at 02:37:24PM +0100, Thomas Gleixner wrote:
>In principle I agree. The issue is that it really depends on the 
>particular
>hardware situation.
>
>If there is an explicit requirement for one driver - expressed by a trigger
>flag - and the other driver relies on the default configuration, then this
>might cause malfunction.
>
>The hassle is, that IRQF_TRIGGER_NONE has unclear semantics. It can mean "I
>don't care" or "I rely on the hw configuration". The latter is what worries
>me.
>
>first driver:
>
>      creates the mapping and sets the trigger type according to the DT
>      setting.
>
>      driver calls request_irq() with IRQF_TRIGGER_NONE. It relies on the DT
>      setting to be correct.
>
>second driver:
>
>      Finds an existing mapping. Now we have two cases:
>
>      1) flat irqdomains:
>
>      	 The DT setting is applied to the trigger type unconditionally.
>
>	 So if that setting is contrary to first drivers DT setting then we
>	 are already in trouble.
>
>	 If the driver uses IRQF_TRIGGER_NONE, bad luck as nothing will notice
>	 the issue.
>
>      2) hierarchical irqdomains
>
>      	 That code path ignores the type setting of the second driver and
>      	 leaves the irq line in the existing state.
>
>	 If the driver uses IRQF_TRIGGER_NONE, bad luck as nothing will notice
>	 the issue.
>      	
>So we have two problems here.
>
>1) We should detect the mismatch already in the mapping function.
>
>   But, that's hard for legacy reasons. Interrupts can be mapped at early boot
>   with hardware default settings and we currently have no way to distinguish
>   that. It shouldn't be hard to fix that.
>

Would you agree that this is a separate issue that should be fixed
separately? Even with this fixed, my problem would still exist.

>2) How to deal with the mismatch in request_irq()
>
>   Relaxing the check is not really a good decision. So what we could do is:
>
>   if ((new_action->flags & IRQF_TRIGGER_MASK) == IRQF_TRIGGER_NONE)
>      	new_action->flags |= irqd_get_trigger_type(irqdata);
>
>   Now that has an issue as well. If the driver requests with
>   IRQF_TRIGGER_NONE and does an explicit type setting afterwards, the
>   action->flags still do not reflect it.
>

Yes, an explicit type-setting afterwards would make action->flags get
out-of-sync, but isn't that already the case, regardless of the relaxed
check?

My patch fixes a bogus error for a real use-case, and as far as I can
see doesn't make any of the existing problems worse - so I feel like
that's a net win.

>The whole trigger handling versus shared interrupts needs some deep thoughts
>and I really want to understand what that of commit 4a43d686fe336 before
>making any decisions.
>

If you'd rather see a patch something like your 2) above, I can do that,
let me know what you think.

Many thanks,

Brian

>Thanks,
>
>	tglx
>

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

Re: [PATCH] genirq: fix trigger flags check for shared irqs Brian Starkey <brian.starkey@arm.com> - 2016-01-28 11:10 +0100
  Re: [PATCH] genirq: fix trigger flags check for shared irqs Thomas Gleixner <tglx@linutronix.de> - 2016-01-28 11:40 +0100
    Re: [PATCH] genirq: fix trigger flags check for shared irqs Brian Starkey <brian.starkey@arm.com> - 2016-01-28 12:20 +0100
      Re: [PATCH] genirq: fix trigger flags check for shared irqs Thomas Gleixner <tglx@linutronix.de> - 2016-01-28 13:00 +0100
        Re: [PATCH] genirq: fix trigger flags check for shared irqs Brian Starkey <brian.starkey@arm.com> - 2016-01-28 13:30 +0100
          Re: [PATCH] genirq: fix trigger flags check for shared irqs Thomas Gleixner <tglx@linutronix.de> - 2016-01-28 14:40 +0100
            Re: [PATCH] genirq: fix trigger flags check for shared irqs Brian Starkey <brian.starkey@arm.com> - 2016-02-08 12:10 +0100
        Re: [PATCH] genirq: fix trigger flags check for shared irqs Rob Herring <robh+dt@kernel.org> - 2016-01-28 17:00 +0100
          Re: [PATCH] genirq: fix trigger flags check for shared irqs Thomas Gleixner <tglx@linutronix.de> - 2016-01-28 21:10 +0100
            Re: [PATCH] genirq: fix trigger flags check for shared irqs Tomasz Figa <tomasz.figa@gmail.com> - 2016-01-29 05:20 +0100

csiph-web