From: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
To: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>,
Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>,
Jason Cooper <jason-NLaQJdtUoK4Be96aLqz0jA@public.gmane.org>,
Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>,
Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>,
Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>,
Ian Campbell
<ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>,
Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>,
Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>,
Thierry Reding
<thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Kevin Hilman <khilman-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>,
Geert Uytterhoeven
<geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>,
Grygorii Strashko
<grygorii.strashko-l0cyMroinI0@public.gmane.org>,
Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>,
Linus Walleij
<linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: Re: [PATCH V3 06/17] irqdomain: Don't set type when mapping an IRQ
Date: Mon, 9 May 2016 16:44:56 +0100 [thread overview]
Message-ID: <5730B078.8090908@nvidia.com> (raw)
In-Reply-To: <5730A867.9070504-5wv7dgnIgG8@public.gmane.org>
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
next prev parent reply other threads:[~2016-05-09 15:44 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-04 16:25 [PATCH V3 00/17] Add support for Tegra210 AGIC Jon Hunter
2016-05-04 16:25 ` [PATCH V3 01/17] irqchip/gic: Don't unnecessarily write the IRQ configuration Jon Hunter
2016-05-04 16:25 ` [PATCH V3 02/17] irqchip/gic: WARN if setting the interrupt type for a PPI fails Jon Hunter
[not found] ` <1462379130-11742-3-git-send-email-jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2016-05-05 12:06 ` Marc Zyngier
2016-05-05 13:22 ` Jon Hunter
2016-05-05 13:40 ` Marc Zyngier
2016-05-05 14:41 ` Jon Hunter
2016-05-04 16:25 ` [PATCH V3 04/17] irqdomain: Fix handling of type settings for existing mappings Jon Hunter
2016-05-04 16:25 ` [PATCH V3 05/17] genirq: Look-up trigger type if not specified by caller Jon Hunter
2016-05-04 16:25 ` [PATCH V3 06/17] irqdomain: Don't set type when mapping an IRQ Jon Hunter
2016-05-09 12:23 ` Marc Zyngier
[not found] ` <5730813B.7060206-5wv7dgnIgG8@public.gmane.org>
2016-05-09 13:13 ` Jon Hunter
[not found] ` <57308D0D.4080800-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2016-05-09 15:10 ` Marc Zyngier
[not found] ` <5730A867.9070504-5wv7dgnIgG8@public.gmane.org>
2016-05-09 15:44 ` Jon Hunter [this message]
[not found] ` <5730B078.8090908-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2016-05-10 12:20 ` Marc Zyngier
2016-05-04 16:25 ` [PATCH V3 07/17] genirq: Ensure IRQ descriptor is valid when setting-up the IRQ Jon Hunter
2016-05-04 16:25 ` [PATCH V3 08/17] genirq: Add runtime power management support for IRQ chips Jon Hunter
2016-05-04 16:25 ` [PATCH V3 09/17] irqchip/gic: Don't initialise chip if mapping IO space fails Jon Hunter
2016-05-04 16:25 ` [PATCH V3 10/17] irqchip/gic: Remove static irq_chip definition for eoimode1 Jon Hunter
2016-05-04 16:25 ` [PATCH V3 11/17] irqchip/gic: Return an error if GIC initialisation fails Jon Hunter
2016-05-04 16:25 ` [PATCH V3 12/17] irqchip/gic: Pass GIC pointer to save/restore functions Jon Hunter
2016-05-04 16:25 ` [PATCH V3 13/17] irqchip/gic: Don't allow early initialisation if GIC requires RPM Jon Hunter
2016-05-04 16:25 ` [PATCH V3 14/17] irqchip/gic: Add helper function for configuring a GIC via device-tree Jon Hunter
2016-05-04 16:25 ` [PATCH V3 15/17] irqchip/gic: Split GIC init in preparation for platform driver Jon Hunter
[not found] ` <1462379130-11742-1-git-send-email-jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2016-05-04 16:25 ` [PATCH V3 03/17] irqchip: Mask the non-type/sense bits when translating an IRQ Jon Hunter
2016-05-04 16:25 ` [PATCH V3 16/17] irqchip/gic: Prepare for adding platform driver Jon Hunter
[not found] ` <1462379130-11742-17-git-send-email-jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2016-05-05 14:13 ` Marc Zyngier
[not found] ` <572B54F4.2080103-5wv7dgnIgG8@public.gmane.org>
2016-05-06 14:09 ` Jon Hunter
[not found] ` <572CA5AF.7080504-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2016-05-06 14:27 ` Marc Zyngier
2016-05-04 16:25 ` [PATCH V3 17/17] irqchip/gic: Add platform driver for non-root GICs that require RPM Jon Hunter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5730B078.8090908@nvidia.com \
--to=jonathanh-ddmlm1+adcrqt0dzr+alfa@public.gmane.org \
--cc=devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org \
--cc=geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org \
--cc=grygorii.strashko-l0cyMroinI0@public.gmane.org \
--cc=ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org \
--cc=jason-NLaQJdtUoK4Be96aLqz0jA@public.gmane.org \
--cc=khilman-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org \
--cc=lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org \
--cc=linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org \
--cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=marc.zyngier-5wv7dgnIgG8@public.gmane.org \
--cc=mark.rutland-5wv7dgnIgG8@public.gmane.org \
--cc=pawel.moll-5wv7dgnIgG8@public.gmane.org \
--cc=robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org \
--cc=swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org \
--cc=tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org \
--cc=thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).