Thread (7 messages) flat view 7 messages, 4 authors, 2021-12-13

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

From: Vladimir Oltean <olteanv@gmail.com>
Date: 2021-12-13 20:00:04
Also in: lkml

Hello Marc,

On Wed, Dec 01, 2021 at 11:41:02AM +0000, Marc Zyngier wrote:
quoted hunk ↗ jump to hunk
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 <redacted>
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/ (local)
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 (local)
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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help