[PATCH v2] of/irq: Add a quirk for controllers with their own definition of interrupt-map

[PATCH v2] of/irq: Add a quirk for controllers with their own definition of interrupt-map

[Marc Zyngier]

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>
Cc: Sander Vanheule <sander@svanheule.net>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---

Notes:
    v2: Switched over to of_device_compatible_match() as per Rob's
        request.

 drivers/of/irq.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index b10f015b2e37..65a325aad984 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -76,6 +76,26 @@ 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",
+	NULL,
+};
+
 /**
  * of_irq_parse_raw - Low level interrupt tree parsing
  * @addr:	address specifier (start of "reg" property of the device) in be32 format
@@ -159,12 +179,16 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
 		/*
 		 * Now check if cursor is an interrupt-controller and
 		 * if it is then we are done, unless there is an
-		 * interrupt-map which takes precedence.
+		 * interrupt-map which takes precedence if we're not
+		 * in presence of once of these broken platform that
+		 * want to parse interrupt-map themselves for $reason.
 		 */
 		bool intc = of_property_read_bool(ipar, "interrupt-controller");
+		bool imap_abuse;
 
 		imap = of_get_property(ipar, "interrupt-map", &imaplen);
-		if (imap == NULL && intc) {
+		imap_abuse = imap && of_device_compatible_match(ipar, of_irq_imap_abusers);
+		if (intc && (imap == NULL || imap_abuse)) {
 			pr_debug(" -> got it !\n");
 			return 0;
 		}
-- 
2.30.2

Re: [PATCH v2] of/irq: Add a quirk for controllers with their own definition of interrupt-map

[Geert Uytterhoeven]

Hi Marc,

On Wed, Dec 1, 2021 at 12:41 PM 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>
> Cc: Sander Vanheule <sander@svanheule.net>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>
> Notes:
>     v2: Switched over to of_device_compatible_match() as per Rob's
>         request.

Thanks for the update!

> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c

> @@ -159,12 +179,16 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
>                 /*
>                  * Now check if cursor is an interrupt-controller and
>                  * if it is then we are done, unless there is an
> -                * interrupt-map which takes precedence.
> +                * interrupt-map which takes precedence if we're not
> +                * in presence of once of these broken platform that

one

> +                * want to parse interrupt-map themselves for $reason.
>                  */
>                 bool intc = of_property_read_bool(ipar, "interrupt-controller");
> +               bool imap_abuse;
>
>                 imap = of_get_property(ipar, "interrupt-map", &imaplen);
> -               if (imap == NULL && intc) {
> +               imap_abuse = imap && of_device_compatible_match(ipar, of_irq_imap_abusers);

... = intc && imap && of_device_compatible_match(...)

> +               if (intc && (imap == NULL || imap_abuse)) {
>                         pr_debug(" -> got it !\n");
>                         return 0;
>                 }

Still working fine on RZ/A1, so
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2] of/irq: Add a quirk for controllers with their own definition of interrupt-map

[Rob Herring]

On Thu, Dec 02, 2021 at 04:06:21PM +0100, Geert Uytterhoeven wrote:
> Hi Marc,
> 
> On Wed, Dec 1, 2021 at 12:41 PM 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>
> > Cc: Sander Vanheule <sander@svanheule.net>
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >
> > Notes:
> >     v2: Switched over to of_device_compatible_match() as per Rob's
> >         request.
> 
> Thanks for the update!
> 
> > --- a/drivers/of/irq.c
> > +++ b/drivers/of/irq.c
> 
> > @@ -159,12 +179,16 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
> >                 /*
> >                  * Now check if cursor is an interrupt-controller and
> >                  * if it is then we are done, unless there is an
> > -                * interrupt-map which takes precedence.
> > +                * interrupt-map which takes precedence if we're not
> > +                * in presence of once of these broken platform that
> 
> one

and 'platforms'. Will fixup.

> 
> > +                * want to parse interrupt-map themselves for $reason.
> >                  */
> >                 bool intc = of_property_read_bool(ipar, "interrupt-controller");
> > +               bool imap_abuse;
> >
> >                 imap = of_get_property(ipar, "interrupt-map", &imaplen);
> > -               if (imap == NULL && intc) {
> > +               imap_abuse = imap && of_device_compatible_match(ipar, of_irq_imap_abusers);
> 
> ... = intc && imap && of_device_compatible_match(...)

Why? Then we are comparing intc twice because we still need it for the 
intc && !imap case.
> 
> > +               if (intc && (imap == NULL || imap_abuse)) {
> >                         pr_debug(" -> got it !\n");
> >                         return 0;
> >                 }
> 
> Still working fine on RZ/A1, so
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> 

Re: [PATCH v2] of/irq: Add a quirk for controllers with their own definition of interrupt-map

[Rob Herring]

On Wed, 01 Dec 2021 11:41:02 +0000, Marc Zyngier 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>
> Cc: Sander Vanheule <sander@svanheule.net>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> 
> Notes:
>     v2: Switched over to of_device_compatible_match() as per Rob's
>         request.
> 
>  drivers/of/irq.c | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 

Applied, thanks!

Re: [PATCH v2] of/irq: Add a quirk for controllers with their own definition of interrupt-map

[Rob Herring]

On Fri, Dec 3, 2021 at 11:17 AM Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Dec 02, 2021 at 04:06:21PM +0100, Geert Uytterhoeven wrote:
> > Hi Marc,
> >
> > On Wed, Dec 1, 2021 at 12:41 PM 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>
> > > Cc: Sander Vanheule <sander@svanheule.net>
> > > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > > ---
> > >
> > > Notes:
> > >     v2: Switched over to of_device_compatible_match() as per Rob's
> > >         request.
> >
> > Thanks for the update!
> >
> > > --- a/drivers/of/irq.c
> > > +++ b/drivers/of/irq.c
> >
> > > @@ -159,12 +179,16 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
> > >                 /*
> > >                  * Now check if cursor is an interrupt-controller and
> > >                  * if it is then we are done, unless there is an
> > > -                * interrupt-map which takes precedence.
> > > +                * interrupt-map which takes precedence if we're not
> > > +                * in presence of once of these broken platform that
> >
> > one
>
> and 'platforms'. Will fixup.
>
> >
> > > +                * want to parse interrupt-map themselves for $reason.
> > >                  */
> > >                 bool intc = of_property_read_bool(ipar, "interrupt-controller");
> > > +               bool imap_abuse;
> > >
> > >                 imap = of_get_property(ipar, "interrupt-map", &imaplen);
> > > -               if (imap == NULL && intc) {
> > > +               imap_abuse = imap && of_device_compatible_match(ipar, of_irq_imap_abusers);
> >
> > ... = intc && imap && of_device_compatible_match(...)
>
> Why? Then we are comparing intc twice because we still need it for the
> intc && !imap case.

I ended up rewriting it like this:

-               if (imap == NULL && intc) {
+               if (intc &&
+                   (!imap || of_device_compatible_match(ipar,
of_irq_imap_abusers))) {

Rob

Re: [PATCH v2] of/irq: Add a quirk for controllers with their own definition of interrupt-map

[Vladimir Oltean]

Hello Marc,

On Wed, Dec 01, 2021 at 11:41:02AM +0000, Marc Zyngier 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>
> Cc: Sander Vanheule <sander@svanheule.net>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> 
> Notes:
>     v2: Switched over to of_device_compatible_match() as per Rob's
>         request.
> 
>  drivers/of/irq.c | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index b10f015b2e37..65a325aad984 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -76,6 +76,26 @@ 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",
> +	NULL,
> +};
> +
>  /**
>   * of_irq_parse_raw - Low level interrupt tree parsing
>   * @addr:	address specifier (start of "reg" property of the device) in be32 format
> @@ -159,12 +179,16 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
>  		/*
>  		 * Now check if cursor is an interrupt-controller and
>  		 * if it is then we are done, unless there is an
> -		 * interrupt-map which takes precedence.
> +		 * interrupt-map which takes precedence if we're not
> +		 * in presence of once of these broken platform that
> +		 * want to parse interrupt-map themselves for $reason.
>  		 */
>  		bool intc = of_property_read_bool(ipar, "interrupt-controller");
> +		bool imap_abuse;
>  
>  		imap = of_get_property(ipar, "interrupt-map", &imaplen);
> -		if (imap == NULL && intc) {
> +		imap_abuse = imap && of_device_compatible_match(ipar, of_irq_imap_abusers);
> +		if (intc && (imap == NULL || imap_abuse)) {
>  			pr_debug(" -> got it !\n");
>  			return 0;
>  		}
> -- 
> 2.30.2
> 

I am a user of the ls-extirq driver which is responsible for 3 of the 7
compatible strings mentioned by you here. I have close to zero knowledge
of the irq subsystem, although I am looking forward to learn.

Could you please spend a few minutes to detail what you see as a possible
path forward for this driver? I am getting mixed impressions about what
it's doing wrong.

On one hand, it was requested by Rob during review that what used to be
called "fsl,extirq-map" should be named "interrupt-map" instead:
https://lore.kernel.org/lkml/20190928092331.GB1894@linutronix.de/
Then, you seem to suggest something's wrong with drivers privately using
that name and parsing a property which used to be ignored by the core,
due to your "silly-interrupt-map" comment:
https://lore.kernel.org/all/9c169aad-3c7b-2ffb-90a2-1ca791a3f411@phrozen.org/T/#ebae8f9231296dc936cb7c9791218fc6785a03390
Then, Rob breaks the ls-extirq driver for platforms that have a GIC ITS*
defined in the device tree via commit 869f0ec048dc ("arm64: dts:
freescale: Fix 'interrupt-map' parent address cells") - this is also,
incidentally, the reason why I'm here.
* because the driver doesn't parse the "standard" format where the
  interrupt parent has a non-zero #address-cells - which the "arm,gic-v3"
  may have when there's a "arm,gic-v3-its" under it (although I don't
  necessarily see the relevance of the ITS being there to the needs of
  the ls-extirq - which are just a bijective mapping of IRQs - this
  driver simply drives a multi-channel logical inverter).

So if I understand correctly, we keep ignoring the non-standard use of
the "interrupt-map" property in these abuser drivers, yet we patch their
device trees to have a more standard format in their non-standard use? :)

Since some breakage has already been introduced, for good or bad, I
think we can start discussing how things should have been done from the
beginning, and see if we can make those changes now.

Re: [PATCH v2] of/irq: Add a quirk for controllers with their own definition of interrupt-map

[Marc Zyngier]

Hi Vladimir,

On Mon, 13 Dec 2021 19:59:58 +0000,
Vladimir Oltean <olteanv@gmail.com> wrote:

[...]

>
> I am a user of the ls-extirq driver which is responsible for 3 of the 7
> compatible strings mentioned by you here. I have close to zero knowledge
> of the irq subsystem, although I am looking forward to learn.

Unfortunately, this has nothing to do with the IRQ subsystem, which
doesn't really care about the firmware interfaces.

> Could you please spend a few minutes to detail what you see as a possible
> path forward for this driver?

Define "path forward". My preference would be to travel back in time
so that this driver doesn't make it into the tree, but it is an
unlikely outcome. The only other solution is to leave it as is, but
not to allow any further occurrence of the issue.

> I am getting mixed impressions about what it's doing wrong.
>
> On one hand, it was requested by Rob during review that what used to be
> called "fsl,extirq-map" should be named "interrupt-map" instead:
> https://lore.kernel.org/lkml/20190928092331.GB1894@linutronix.de/

I stand by my analysis that this 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. No ifs, no buts.

> Then, you seem to suggest something's wrong with drivers privately using
> that name and parsing a property which used to be ignored by the core,
> due to your "silly-interrupt-map" comment:
> https://lore.kernel.org/all/9c169aad-3c7b-2ffb-90a2-1ca791a3f411@phrozen.org/T/#ebae8f9231296dc936cb7c9791218fc6785a03390

And I stand by this comment.

> Then, Rob breaks the ls-extirq driver for platforms that have a GIC ITS*
> defined in the device tree via commit 869f0ec048dc ("arm64: dts:
> freescale: Fix 'interrupt-map' parent address cells") - this is also,
> incidentally, the reason why I'm here.
> * because the driver doesn't parse the "standard" format where the
>   interrupt parent has a non-zero #address-cells - which the "arm,gic-v3"
>   may have when there's a "arm,gic-v3-its" under it (although I don't
>   necessarily see the relevance of the ITS being there to the needs of
>   the ls-extirq - which are just a bijective mapping of IRQs - this
>   driver simply drives a multi-channel logical inverter).

And that's another reason why using interrupt-map is totally
bonkers. You can't have your cake and eat it (in this case: use a
standard property and yet attribute it some other semantics) -- at
some point, these things break. And when they break, we're left with
these stupid quirks to paper over the breakage.

> So if I understand correctly, we keep ignoring the non-standard use of
> the "interrupt-map" property in these abuser drivers, yet we patch their
> device trees to have a more standard format in their non-standard use? :)

I'm happy to drop support for these FSL/NXP machines immediately. Say
the word, and I will merge the patch!

Now, when it comes to Rob's patch, I think this was the logic thing to
do, and that nobody realised how badly broken the whole thing was. I'm
just as guilty to have merged some of these drivers without really
checking what they were doing in their DT parsing (I tend to focus on
the correctness of the runtime behaviour). Expect a lot more scrutiny
for any new patch.

> Since some breakage has already been introduced, for good or bad, I
> think we can start discussing how things should have been done from the
> beginning, and see if we can make those changes now.

If using standard properties, this should have never been an
interrupt-map. A whole collection of 'interrupts', maybe (we have some
other issues with that, but nothing that cannot be fixed without
changing the DT). Or the initially proposed fsl,blah. But if you are
using a standard property, it is handled by the core code, and you
have no business messing with it.

Thanks,

	M.

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