On 09/05/16 14:13, Jon Hunter wrote:
>
> On 09/05/16 13:23, Marc Zyngier wrote:
>> On 04/05/16 17:25, Jon Hunter wrote:
>>> Some IRQ chips, such as GPIO controllers or secondary level interrupt
>>> controllers, may require require additional runtime power management
>>> control to ensure they are accessible. For such IRQ chips, it makes sense
>>> to enable the IRQ chip when interrupts are requested and disabled them
>>> again once all interrupts have been freed.
>>>
>>> When mapping an IRQ, the IRQ type settings are read and then programmed.
>>> The mapping of the IRQ happens before the IRQ is requested and so the
>>> programming of the type settings occurs before the IRQ is requested. This
>>> is a problem for IRQ chips that require additional power management
>>> control because they may not be accessible yet. Therefore, when mapping
>>> the IRQ, don't program the type settings, just save them and then program
>>> these saved settings when the IRQ is requested (so long as if they are not
>>> overridden via the call to request the IRQ).
>>>
>>> Add a stub function for irq_domain_free_irqs() to avoid any compilation
>>> errors when CONFIG_IRQ_DOMAIN_HIERARCHY is not selected.
>>>
>>> Signed-off-by: Jon Hunter <jonathanh@xxxxxxxxxx>
>>> ---
>>> include/linux/irqdomain.h | 3 +++
>>> kernel/irq/irqdomain.c | 23 ++++++++++++++++++-----
>>> 2 files changed, 21 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
>>> index 2aed04396210..fc66876d1965 100644
>>> --- a/include/linux/irqdomain.h
>>> +++ b/include/linux/irqdomain.h
>>> @@ -440,6 +440,9 @@ static inline int irq_domain_alloc_irqs(struct irq_domain *domain,
>>> return -1;
>>> }
>>>
>>> +static inline void irq_domain_free_irqs(unsigned int virq,
>>> + unsigned int nr_irqs) { }
>>> +
>>> static inline bool irq_domain_is_hierarchy(struct irq_domain *domain)
>>> {
>>> return false;
>>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
>>> index d68371213fc9..bbf5b9b8ac3d 100644
>>> --- a/kernel/irq/irqdomain.c
>>> +++ b/kernel/irq/irqdomain.c
>>> @@ -564,6 +564,7 @@ static void of_phandle_args_to_fwspec(struct of_phandle_args *irq_data,
>>> unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>>> {
>>> struct irq_domain *domain;
>>> + struct irq_data *irq_data;
>>> irq_hw_number_t hwirq;
>>> unsigned int type = IRQ_TYPE_NONE;
>>> int virq;
>>> @@ -613,7 +614,11 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>>> * it now and return the interrupt number.
>>> */
>>> if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
>>> - irq_set_irq_type(virq, type);
>>> + irq_data = irq_get_irq_data(virq);
>>> + if (!irq_data)
>>> + return 0;
>>> +
>>> + irqd_set_trigger_type(irq_data, type);
>>> return virq;
>>> }
>>>
>>> @@ -633,10 +638,18 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>>> return virq;
>>> }
>>>
>>> - /* Set type if specified and different than the current one */
>>> - if (type != IRQ_TYPE_NONE &&
>>> - type != irq_get_trigger_type(virq))
>>> - irq_set_irq_type(virq, type);
>>> + irq_data = irq_get_irq_data(virq);
>>> + if (!irq_data) {
>>> + if (irq_domain_is_hierarchy(domain))
>>> + irq_domain_free_irqs(virq, 1);
>>> + else
>>> + irq_dispose_mapping(virq);
>>> + return 0;
>>> + }
>>> +
>>> + /* Store trigger type */
>>> + irqd_set_trigger_type(irq_data, type);
>>> +
>>> return virq;
>>> }
>>> EXPORT_SYMBOL_GPL(irq_create_fwspec_mapping);
>>>
>>
>> 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.
> We could add a WARN to the existing irq_set_irq_type() or may be just a
> pr_warn() if a WARN is too verbose so people can fix up any issues.
>
> I am also wondering if patch 4/17 "iqdomain: Fix handling of type
> settings for existing mappings" could generate a lot of reports
> interrupts failing due to bad firmware? I wonder if I should tone this
> patch down to a warning message as well as opposed to a complete failure.
We'll see. We can always tone it down a notch, should it prove to be too noisy...
So far, I haven't seen it firing. On the other hand, I get the following stuff
on my APM board:
[ 0.000000] GIC: PPI0 is either secure or misconfigured
[ 0.000000] GIC: PPI13 is either secure or misconfigured
[ 0.000000] arm_arch_timer: WARNING: Invalid trigger for IRQ1, assuming level low
[ 0.000000] arm_arch_timer: WARNING: Please fix your firmware
[ 0.000000] arm_arch_timer: WARNING: Invalid trigger for IRQ2, assuming level low
[ 0.000000] arm_arch_timer: WARNING: Please fix your firmware
Pretty awesome...
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
