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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
