lkml.org 
[lkml]   [2016]   [Feb]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] genirq: fix trigger flags check for shared irqs
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
>

\
 
 \ /
  Last update: 2016-02-08 12:21    [W:0.062 / U:0.296 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site