On 09/05/16 16:44, Jon Hunter wrote: > > On 09/05/16 16:10, Marc Zyngier wrote: >> On 09/05/16 14:13, Jon Hunter wrote: >>> On 09/05/16 13:23, Marc Zyngier wrote: > > [snip] > >>>> This patch have the effect of making misconfigured PPIs absolutely >>>> obvious. I still need to wrap my head around the root cause, but here's >>>> the findings I have so far: >>>> >>>> - kvmtool generates a DT with the wrong trigger information (edge >>>> instead of level) for the timer. >>>> - with this patch applied, "cyclictest -S" reliably locks up when run in >>>> a guest (missing a timer interrupt, goodbye CPU). >>>> - Either fixing kvmtool or reverting that patch makes it work reliably >>>> again. >>>> >>>> My gut feeling is that until that patch, the failing irq_set_irq_type() >>>> wasn't affecting the kernel's view of the trigger (it was still treated >>>> as level). With this patch, the kernel now trusts whatever is coming >>>> from the firmware, and the misconfiguration becomes obvious. And just >>>> grepping through the DT files for arm and arm64 sends makes me thing >>>> "Holly effin' crap!". >>>> >>>> I'm not saying that we shouldn't perform this change though. But it is >>>> quite obvious that it is going to break an awful lot of existing code >>>> and platforms. I'm also cooking a small patch for the arch timer (which >>>> seems to be described in DT with a fairly high level of brokenness), so >>>> that we can mop-up most of the brain damage. >>> >>> Hmmm ... yes I see. I wonder if we should make the setting of the type >>> here dependent upon PM being enabled for an irqchip? We could check to >>> see if the .parent_device is populated and if so only then save the type >>> and otherwise just set it as we do today. >> >> I don't really like the idea of having multiple code paths for the same thing. >> This is very error prone, and likely to bitrot pretty quickly. > > True. However, we really need this change for irqchips and runtime-pm. > So to confirm what are you suggesting we do? We could add a WARN around > irq_set_irq_type() in irq_create_fwspec_mapping() for v4.7 and see how > many complaints we get :-) Let's add a pr_warn(), and see how noisy this becomes. We may have to remove it if it becomes too loud though. Thanks, M. -- Jazz is not dead. It just smells funny... -- 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
