Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1397154

Re: [PATCH V3 06/17] irqdomain: Don't set type when mapping an IRQ

Show all headers | View raw

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 :-)

>> 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...

Indeed.

Jon

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH V3 00/17] Add support for Tegra210 AGIC Jon Hunter <jonathanh@nvidia.com> - 2016-05-04 18:30 +0200
  [PATCH V3 11/17] irqchip/gic: Return an error if GIC initialisation fails Jon Hunter <jonathanh@nvidia.com> - 2016-05-04 18:30 +0200
  [PATCH V3 06/17] irqdomain: Don't set type when mapping an IRQ Jon Hunter <jonathanh@nvidia.com> - 2016-05-04 18:30 +0200
    Re: [PATCH V3 06/17] irqdomain: Don't set type when mapping an IRQ Marc Zyngier <marc.zyngier@arm.com> - 2016-05-09 14:30 +0200
      Re: [PATCH V3 06/17] irqdomain: Don't set type when mapping an IRQ Jon Hunter <jonathanh@nvidia.com> - 2016-05-09 15:20 +0200
        Re: [PATCH V3 06/17] irqdomain: Don't set type when mapping an IRQ Marc Zyngier <marc.zyngier@arm.com> - 2016-05-09 17:20 +0200
          Re: [PATCH V3 06/17] irqdomain: Don't set type when mapping an IRQ Jon Hunter <jonathanh@nvidia.com> - 2016-05-09 17:50 +0200
            Re: [PATCH V3 06/17] irqdomain: Don't set type when mapping an IRQ Marc Zyngier <marc.zyngier@arm.com> - 2016-05-10 14:30 +0200
  [PATCH V3 15/17] irqchip/gic: Split GIC init in preparation for platform driver Jon Hunter <jonathanh@nvidia.com> - 2016-05-04 18:30 +0200
  [PATCH V3 14/17] irqchip/gic: Add helper function for configuring a GIC via device-tree Jon Hunter <jonathanh@nvidia.com> - 2016-05-04 18:30 +0200
  [PATCH V3 10/17] irqchip/gic: Remove static irq_chip definition for eoimode1 Jon Hunter <jonathanh@nvidia.com> - 2016-05-04 18:30 +0200
  [PATCH V3 07/17] genirq: Ensure IRQ descriptor is valid when setting-up the IRQ Jon Hunter <jonathanh@nvidia.com> - 2016-05-04 18:40 +0200
  [PATCH V3 04/17] irqdomain: Fix handling of type settings for existing mappings Jon Hunter <jonathanh@nvidia.com> - 2016-05-04 18:40 +0200
  [PATCH V3 09/17] irqchip/gic: Don't initialise chip if mapping IO space fails Jon Hunter <jonathanh@nvidia.com> - 2016-05-04 18:40 +0200

csiph-web