Date:   Mon, 14 Mar 2022 09:15:15 -0600
From:   Rob Herring <robh+dt@...nel.org>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     devicetree@...r.kernel.org,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        Marc Zyngier <maz@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
        Li Yang <leoyang.li@....com>, Biwen Li <biwen.li@....com>,
        Zhiqiang Hou <Zhiqiang.Hou@....com>,
        Kurt Kanzenbach <kurt@...utronix.de>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH v2 devicetree] Revert "arm64: dts: freescale: Fix
 'interrupt-map' parent address cells"

On Tue, Dec 14, 2021 at 6:59 AM Vladimir Oltean <vladimir.oltean@....com> wrote:
>
> This reverts commit 869f0ec048dc8fd88c0b2003373bd985795179fb. That
> updated the expected device tree binding format for the ls-extirq
> driver, without also updating the parsing code (ls_extirq_parse_map)
> to the new format.

Sorry, I missed this until mentioned on IRC.

>
> The context is that the ls-extirq driver uses the standard
> "interrupt-map" OF property in a non-standard way, as suggested by
> Rob Herring during review:
> https://lore.kernel.org/lkml/20190927161118.GA19333@bogus/
>
> This has turned out to be problematic, as Marc Zyngier discovered
> through commit 041284181226 ("of/irq: Allow matching of an interrupt-map
> local to an interrupt controller"), later fixed through commit
> de4adddcbcc2 ("of/irq: Add a quirk for controllers with their own
> definition of interrupt-map"). Marc's position, expressed on multiple
> opportunities, is that:
>
> (a) [ making private use of the reserved "interrupt-map" name in a
>     driver ] "is wrong, by the very letter of what an interrupt-map
>     means. If the interrupt map points to an interrupt controller,
>     that's the target for the interrupt."
> https://lore.kernel.org/lkml/87k0g8jlmg.wl-maz@kernel.org/
>
> (b) [ updating the driver's bindings to accept a non-reserved name for
>     this property, as an alternative, is ] "is totally pointless. These
>     machines have been in the wild for years, and existing DTs will be
>     there *forever*."
> https://lore.kernel.org/lkml/87ilvrk1r0.wl-maz@kernel.org/
>
> Considering the above, the Linux kernel has quirks in place to deal with
> the ls-extirq's non-standard use of the "interrupt-map". These quirks
> may be needed in other operating systems that consume this device tree,
> yet this is seen as the only viable solution.
>
> Therefore, the premise of the patch being reverted here is invalid.
> It doesn't matter whether the driver, in its non-standard use of the
> property, complies to the standard format or not, since this property
> isn't expected to be used for interrupt translation by the core.

I disagree. The non-standard part is that 'interrupt-map' translation
is not transparent. 'interrupt-map' that can't be parsed in the
standard way is just wrong, and I imagine was never the intention
here. We simply cannot have platforms defining their own format for
standard properties.

Reverting this will cause dtc warnings now (IIRC) and just kicks the
can down the road. Reverting is fine for now (I gave Arnd the okay on
IRC), but I think the parsing will need to be updated to honor
#address-cells and detect an old DT (probably by looking at the total
size of 'interrupt-map') and mark that change for stable. That would
only leave a new dt with an old kernel without stable updates broken.
Seems unlikely a device is getting firmware updates, but not OS
updates.

Rob

Powered by blists - more mailing lists