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

[Brian Starkey]

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