MIME-Version: 1.0
References: <20211122103032.517923-1-maz@kernel.org> <CAL_Jsq+kLK-277FOSvOCVDdw8_j-6xL-SnHs15Zj4MddsxZqNw@mail.gmail.com>
 <87mtlmn4gk.wl-maz@kernel.org>
In-Reply-To: <87mtlmn4gk.wl-maz@kernel.org>
From:   Rob Herring <robh@kernel.org>
Date:   Mon, 29 Nov 2021 13:40:21 -0600
Message-ID: <CAL_JsqJLzwM-k+h3-DpQ5soOU9Anj3C-itnRFU13-TtCvjpn1g@mail.gmail.com>
Subject: Re: [PATCH] of/irq: Add a quirk for controllers with their own
 definition of interrupt-map
To:     Marc Zyngier <maz@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>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Nov 29, 2021 at 1:31 PM Marc Zyngier <maz@kernel.org> wrote:
>
> 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?

Seems like a theoretical problem...

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

Let's keep the public shaming list I guess. If it grows I may change my mind...

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