Date:   Mon, 29 Nov 2021 19:31:55 +0000
Message-ID: <87mtlmn4gk.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Rob Herring <robh@kernel.org>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        devicetree@vger.kernel.org,
        Android Kernel Team <kernel-team@android.com>,
        John Crispin <john@phrozen.org>, Biwen Li <biwen.li@nxp.com>,
        Chris Brandt <chris.brandt@renesas.com>,
        Geert Uytterhoeven <geert+renesas@glider.be>
Subject: Re: [PATCH] of/irq: Add a quirk for controllers with their own definition of interrupt-map
In-Reply-To: <CAL_Jsq+kLK-277FOSvOCVDdw8_j-6xL-SnHs15Zj4MddsxZqNw@mail.gmail.com>
References: <20211122103032.517923-1-maz@kernel.org>
        <CAL_Jsq+kLK-277FOSvOCVDdw8_j-6xL-SnHs15Zj4MddsxZqNw@mail.gmail.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Mon, 29 Nov 2021 19:15:27 +0000,
Rob Herring <robh@kernel.org> wrote:
> 
> On Mon, Nov 22, 2021 at 4:30 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > Since 041284181226 ("of/irq: Allow matching of an interrupt-map local
> > to an interrupt controller"), a handful of interrupt controllers have
> > stopped working correctly. This is due to the DT exposing a non-sensical
> > interrupt-map property, and their drivers relying on the kernel ignoring
> > this property.
> >
> > Since we cannot realistically fix this terrible behaviour, add a quirk
> > for the limited set of devices that have implemented this monster,
> > and document that this is a pretty bad practice.
> >
> > Cc: Rob Herring <robh@kernel.org>
> > Cc: John Crispin <john@phrozen.org>
> > Cc: Biwen Li <biwen.li@nxp.com>
> > Cc: Chris Brandt <chris.brandt@renesas.com>
> > Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  drivers/of/irq.c | 37 +++++++++++++++++++++++++++++++++++--
> >  1 file changed, 35 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> > index b10f015b2e37..27a5173c813c 100644
> > --- a/drivers/of/irq.c
> > +++ b/drivers/of/irq.c
> > @@ -76,6 +76,36 @@ struct device_node *of_irq_find_parent(struct device_node *child)
> >  }
> >  EXPORT_SYMBOL_GPL(of_irq_find_parent);
> >
> > +/*
> > + * These interrupt controllers abuse interrupt-map for unspeakable
> > + * reasons and rely on the core code to *ignore* it (the drivers do
> > + * their own parsing of the property).
> > + *
> > + * If you think of adding to the list for something *new*, think
> > + * again. There is a high chance that you will be sent back to the
> > + * drawing board.
> > + */
> > +static const char * const of_irq_imap_abusers[] = {
> > +       "CBEA,platform-spider-pic",
> > +       "sti,platform-spider-pic",
> > +       "realtek,rtl-intc",
> > +       "fsl,ls1021a-extirq",
> > +       "fsl,ls1043a-extirq",
> > +       "fsl,ls1088a-extirq",
> > +       "renesas,rza1-irqc",
> > +};
> 
> I guess this list was obtained by with a: git grep '"interrupt-map"'

Yes. Anyone having its own interrupt-map parser is likely to have the
same problem.

> I suppose that should be sufficient to find all the cases. I'd like to
> be able to identify this case just from a DT file, but it's not really
> clear

Indeed. Not to mention that the PPC stuff doesn't has its DT hidden in
some firmware.

> Perhaps a simpler solution to all this is only handle interrupt-map
> with interrupt-controller if it points to its own node. That works for
> Apple and I don't see a need beyond that case.

The problem is that interrupt-map can point to more than a single
controller. What if the map points to a both a local interrupt and a a
remote one?

It feels weird to standardise on a behaviour that seems to contradict
the spec and to single out the one that (IMO) matches the expected
behaviour. At the end of the day, I'll implement whichever solution
you prefer.

> > +static bool of_irq_abuses_interrupt_map(struct device_node *np)
> > +{
> > +       int i;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(of_irq_imap_abusers); i++)
> > +               if (of_device_is_compatible(np, of_irq_imap_abusers[i]))
> > +                       return true;
> > +
> > +       return false;
> 
> With a NULL terminated list, you can use of_device_compatible_match() instead .

Ah, neat.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.