Messages in this thread | | | Date | Mon, 8 Feb 2016 11:02:40 +0000 | From | Brian Starkey <> | Subject | Re: [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 >
| |