[PATCH 00/12] soc: fsl: qe: QE PIC improvement and add support of IRQs to QUICC ENGINE GPIOs

[PATCH 00/12] soc: fsl: qe: QE PIC improvement and add support of IRQs to QUICC ENGINE GPIOs

[Paul Louvel]

This series modernizes the QUICC Engine Port Interrupt Controller (QE
PIC) driver and adds the ability for QE GPIO pins to generate interrupts
through the QE PIC, completing Christophe Leroy's prior work [1].

Christophe's series was partially merged; patches 4, 6 and 7 did not
make it to mainline.

The series is organized in three parts:

1) Add missing chained_irq_{enter,exit}() calls

   - In a chained handler, the parent controller need to mask and ack
     the interrupt source.

2) DT binding updates

   - Update #interrupt-cells from 1 to 2 in the QE PIC binding so
     consumers can encode the interrupt type (falling-edge or
     both-edges).

   - Convert the QE GPIO binding from freeform text to DT schema.

   - Extend the QE GPIO binding with an interrupt-map (nexus node) that
     maps GPIO lines to parent QE PIC interrupts.  This approach was
     suggested by Rob Herring [2] as an alternative to using compatible
     strings and driver data to specify which pins support interrupts in
     a given bank.

3) QE PIC driver refactoring

   - The QE PIC is a perfect fit to use the generic irq framework
     instead. Perform the necessary changes to the driver to convert it.

   - Minor cleanups.

4) QE GPIO interrupt support

   - Add a to_irq() method to the QE GPIO driver that perform the
     mapping of the GPIO pin to the parent interrupt domain, allowing
     GPIO pins to be used as interrupt sources through the QE PIC via
     gpio_to_irq().

[1] https://lore.kernel.org/all/cover.1758212309.git.christophe.leroy@csgroup.eu/
[2] https://lore.kernel.org/all/20250919152414.GB852815-robh@kernel.org/

Signed-off-by: Paul Louvel <paul.louvel@bootlin.com>

---
Christophe Leroy (1):
      dt-bindings: soc: fsl: qe: Convert QE GPIO to DT schema

Paul Louvel (11):
      soc: fsl: qe: Add chained_irq_{enter,exit}() calls in cascade handler
      dt-bindings: soc: fsl: qe: Set #interrupt-cells to 2 to support interrupt type encoding
      dt-bindings: soc: fsl: qe: Add support of IRQ in QE GPIO
      soc: fsl: qe: Use generic_handle_domain_irq()
      soc: fsl: qe: Iterate over all pending interrupts in cascade handler
      soc: fsl: qe: Handle spurious interrupts
      soc: fsl: qe: Convert to generic IRQ chip
      soc: fsl: qe: Rename irq variable to parent_irq
      soc: fsl: qe: Rename host member to domain in struct qepic_data
      soc: fsl: qe: Remove useless struct member
      soc: fsl: qe: Add support of IRQs in QE GPIO

 .../bindings/gpio/fsl,mpc8323-qe-pario-bank.yaml   | 108 ++++++++++++++++
 .../interrupt-controller/fsl,qe-ports-ic.yaml      |   4 +-
 .../bindings/soc/fsl/cpm_qe/qe/par_io.txt          |  26 +---
 arch/powerpc/platforms/Kconfig                     |   1 +
 drivers/soc/fsl/qe/gpio.c                          |  28 +++-
 drivers/soc/fsl/qe/qe_ports_ic.c                   | 144 +++++++++++++--------
 6 files changed, 232 insertions(+), 79 deletions(-)
---
base-commit: f1b7f9c6e1a7fc549f37cafb48d233c9f7c26adc
change-id: 20260513-qe-pic-gpios-073e284615a3

Best regards,
--  
Paul Louvel, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH 01/12] soc: fsl: qe: Add chained_irq_{enter,exit}() calls in cascade handler

[Paul Louvel]

Wrap the cascade handler body with chained_irq_{enter,exit}() to
properly inform the parent IRQ chip that a chained interrupt is being
serviced.

Fixes: f0bcd784e1b76 ("soc: fsl: qe: Add an interrupt controller for QUICC Engine Ports")
Signed-off-by: Paul Louvel <paul.louvel@bootlin.com>
Cc: stable@kernel.org
---
 drivers/soc/fsl/qe/qe_ports_ic.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/soc/fsl/qe/qe_ports_ic.c b/drivers/soc/fsl/qe/qe_ports_ic.c
index 9b0bba64e91e..73a77763210d 100644
--- a/drivers/soc/fsl/qe/qe_ports_ic.c
+++ b/drivers/soc/fsl/qe/qe_ports_ic.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/irq.h>
+#include <linux/irqchip/chained_irq.h>
 #include <linux/irqdomain.h>
 #include <linux/platform_device.h>
 
@@ -79,7 +80,13 @@ static int qepic_get_irq(struct irq_desc *desc)
 
 static void qepic_cascade(struct irq_desc *desc)
 {
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+
+	chained_irq_enter(chip, desc);
+
 	generic_handle_irq(qepic_get_irq(desc));
+
+	chained_irq_exit(chip, desc);
 }
 
 static int qepic_host_map(struct irq_domain *h, unsigned int virq, irq_hw_number_t hw)

-- 
2.55.0

[PATCH 02/12] dt-bindings: soc: fsl: qe: Set #interrupt-cells to 2 to support interrupt type encoding

[Paul Louvel]

The QUICC Engine port interrupt controller can be configured to generate
an interrupt on either a high-to-low transition or any change in the
signal state on the related GPIOs.

Update the #interrupt-cells property to 2 so consumers can encode
interrupt level information.

Signed-off-by: Paul Louvel <paul.louvel@bootlin.com>
---
 .../devicetree/bindings/interrupt-controller/fsl,qe-ports-ic.yaml     | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,qe-ports-ic.yaml b/Documentation/devicetree/bindings/interrupt-controller/fsl,qe-ports-ic.yaml
index 2b8e7b9c6d7a..2b7c6b4f0389 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/fsl,qe-ports-ic.yaml
+++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,qe-ports-ic.yaml
@@ -23,7 +23,7 @@ properties:
     const: 0
 
   '#interrupt-cells':
-    const: 1
+    const: 2
 
   interrupts:
     maxItems: 1
@@ -45,7 +45,7 @@ examples:
       reg = <0xc00 0x18>;
       interrupt-controller;
       #address-cells = <0>;
-      #interrupt-cells = <1>;
+      #interrupt-cells = <2>;
       interrupts = <74 0x8>;
       interrupt-parent = <&ipic>;
     };

-- 
2.55.0

[PATCH 03/12] dt-bindings: soc: fsl: qe: Convert QE GPIO to DT schema

[Paul Louvel]

From: Christophe Leroy <christophe.leroy@csgroup.eu>

Convert QE GPIO devicetree binding to DT schema.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Signed-off-by: Paul Louvel <paul.louvel@bootlin.com>
---
 .../bindings/gpio/fsl,mpc8323-qe-pario-bank.yaml   | 45 ++++++++++++++++++++++
 .../bindings/soc/fsl/cpm_qe/qe/par_io.txt          | 26 +------------
 2 files changed, 46 insertions(+), 25 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/fsl,mpc8323-qe-pario-bank.yaml b/Documentation/devicetree/bindings/gpio/fsl,mpc8323-qe-pario-bank.yaml
new file mode 100644
index 000000000000..1af99339ff40
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/fsl,mpc8323-qe-pario-bank.yaml
@@ -0,0 +1,45 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/fsl,mpc8323-qe-pario-bank.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Freescale QUICC Engine Parallel I/O (QE PARIO) GPIO Bank
+
+maintainers:
+  - Christophe Leroy <christophe.leroy@csgroup.eu>
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+          - enum:
+              - fsl,mpc8360-qe-pario-bank
+              - fsl,mpc8569-qe-pario-bank
+          - const: fsl,mpc8323-qe-pario-bank
+      - const: fsl,mpc8323-qe-pario-bank
+
+  reg:
+    maxItems: 1
+
+  gpio-controller: true
+
+  "#gpio-cells":
+    const: 2
+
+required:
+  - compatible
+  - reg
+  - gpio-controller
+  - "#gpio-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    gpio-controller@1400 {
+        compatible = "fsl,mpc8360-qe-pario-bank", "fsl,mpc8323-qe-pario-bank";
+        reg = <0x1400 0x18>;
+        gpio-controller;
+        #gpio-cells = <2>;
+    };
diff --git a/Documentation/devicetree/bindings/soc/fsl/cpm_qe/qe/par_io.txt b/Documentation/devicetree/bindings/soc/fsl/cpm_qe/qe/par_io.txt
index 09b1b05fa677..782699c14567 100644
--- a/Documentation/devicetree/bindings/soc/fsl/cpm_qe/qe/par_io.txt
+++ b/Documentation/devicetree/bindings/soc/fsl/cpm_qe/qe/par_io.txt
@@ -24,28 +24,4 @@ par_io@1400 {
 
 Note that "par_io" nodes are obsolete, and should not be used for
 the new device trees. Instead, each Par I/O bank should be represented
-via its own gpio-controller node:
-
-Required properties:
-- #gpio-cells : should be "2".
-- compatible : should be "fsl,<chip>-qe-pario-bank",
-  "fsl,mpc8323-qe-pario-bank".
-- reg : offset to the register set and its length.
-- gpio-controller : node to identify gpio controllers.
-
-Example:
-	qe_pio_a: gpio-controller@1400 {
-		#gpio-cells = <2>;
-		compatible = "fsl,mpc8360-qe-pario-bank",
-		"fsl,mpc8323-qe-pario-bank";
-		reg = <0x1400 0x18>;
-		gpio-controller;
-	  };
-
-	qe_pio_e: gpio-controller@1460 {
-		#gpio-cells = <2>;
-		compatible = "fsl,mpc8360-qe-pario-bank",
-			     "fsl,mpc8323-qe-pario-bank";
-		reg = <0x1460 0x18>;
-		gpio-controller;
-	  };
+via its own gpio-controller node.

-- 
2.55.0

[PATCH 04/12] dt-bindings: soc: fsl: qe: Add support of IRQ in QE GPIO

[Paul Louvel]

Some QE GPIO pins have an associated interrupt line in the QE PIC to
signal state changes on the pin.  Add the corresponding
interrupt-controller / nexus properties to the QE GPIO binding.

Because the GPIO controller does not perform any interrupt handling
itself, a nexus node (interrupt-map) is used to map each GPIO line
supporting IRQ to the parent QE PIC interrupt domain.

As the QE PIC can be configured to generate an interrupt on either a
high-to-low transition or any change in signal state, three
interrupt-map entries are needed per GPIO pin that can yield an
interrupt (falling, both, and the "none" case which defaults to both in
QE PIC).  This overhead is necessary because the interrupt-map-pass-thru
property is not part of the DT specification.

The interrupt-map property is optional: it is not required for GPIO
banks that have no interrupt capable GPIO line (e.g. port D on MPC8323),
or when interrupt functionality is not used.

Update the example to show a scenario where each bank supports a
different numbers of IRQs, or no IRQs at all.

Signed-off-by: Paul Louvel <paul.louvel@bootlin.com>
---
 .../bindings/gpio/fsl,mpc8323-qe-pario-bank.yaml   | 69 +++++++++++++++++++++-
 1 file changed, 66 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/fsl,mpc8323-qe-pario-bank.yaml b/Documentation/devicetree/bindings/gpio/fsl,mpc8323-qe-pario-bank.yaml
index 1af99339ff40..0c849a5698f4 100644
--- a/Documentation/devicetree/bindings/gpio/fsl,mpc8323-qe-pario-bank.yaml
+++ b/Documentation/devicetree/bindings/gpio/fsl,mpc8323-qe-pario-bank.yaml
@@ -27,6 +27,17 @@ properties:
   "#gpio-cells":
     const: 2
 
+  "#address-cells":
+    const: 0
+
+  "#interrupt-cells":
+    const: 2
+
+  interrupt-map:
+    description: |
+      Specifies the mapping of GPIO lines to the parent interrupt controller, as the
+      GPIO controller does not do interrupt handling itself.
+
 required:
   - compatible
   - reg
@@ -37,9 +48,61 @@ additionalProperties: false
 
 examples:
   - |
-    gpio-controller@1400 {
-        compatible = "fsl,mpc8360-qe-pario-bank", "fsl,mpc8323-qe-pario-bank";
-        reg = <0x1400 0x18>;
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    pic: interrupt-controller {
+      interrupt-controller;
+      #address-cells = <0>;
+      #interrupt-cells = <2>;
+    };
+
+    gpio-controller@1418 {
+        #gpio-cells = <2>;
+        #address-cells = <0>;
+        #interrupt-cells = <2>;
+        compatible = "fsl,mpc8323-qe-pario-bank";
+        reg = <0x1418 0x18>;
         gpio-controller;
+        interrupt-map = <
+          7 IRQ_TYPE_EDGE_FALLING  &pic 4 IRQ_TYPE_EDGE_FALLING
+          7 IRQ_TYPE_EDGE_BOTH     &pic 4 IRQ_TYPE_EDGE_BOTH
+          7 0                      &pic 4 IRQ_TYPE_NONE
+
+          9 IRQ_TYPE_EDGE_FALLING  &pic 5 IRQ_TYPE_EDGE_FALLING
+          9 IRQ_TYPE_EDGE_BOTH     &pic 5 IRQ_TYPE_EDGE_BOTH
+          9 0                      &pic 5 IRQ_TYPE_NONE
+
+          25 IRQ_TYPE_EDGE_FALLING &pic 6 IRQ_TYPE_EDGE_FALLING
+          25 IRQ_TYPE_EDGE_BOTH    &pic 6 IRQ_TYPE_EDGE_BOTH
+          25 0                     &pic 6 IRQ_TYPE_NONE
+
+          27 IRQ_TYPE_EDGE_FALLING &pic 7 IRQ_TYPE_EDGE_FALLING
+          27 IRQ_TYPE_EDGE_BOTH    &pic 7 IRQ_TYPE_EDGE_BOTH
+          27 0                     &pic 7 IRQ_TYPE_NONE
+        >;
+    };
+
+    gpio-controller@1430 {
         #gpio-cells = <2>;
+        #address-cells = <0>;
+        #interrupt-cells = <2>;
+        compatible = "fsl,mpc8323-qe-pario-bank";
+        reg = <0x1430 0x18>;
+        gpio-controller;
+        interrupt-map = <
+          24 IRQ_TYPE_EDGE_FALLING &pic 8 IRQ_TYPE_EDGE_FALLING
+          24 IRQ_TYPE_EDGE_BOTH    &pic 8 IRQ_TYPE_EDGE_BOTH
+          24 0                     &pic 8 IRQ_TYPE_NONE
+
+          29 IRQ_TYPE_EDGE_FALLING &pic 9 IRQ_TYPE_EDGE_FALLING
+          29 IRQ_TYPE_EDGE_BOTH    &pic 9 IRQ_TYPE_EDGE_BOTH
+          29 0                     &pic 9 IRQ_TYPE_NONE
+        >;
+    };
+
+    gpio-controller@1448 {
+        #gpio-cells = <2>;
+        compatible = "fsl,mpc8323-qe-pario-bank";
+        reg = <0x1448 0x18>;
+        gpio-controller;
     };

-- 
2.55.0

[PATCH 05/12] soc: fsl: qe: Use generic_handle_domain_irq()

[Paul Louvel]

Replace the irq_find_mapping() + generic_handle_irq() pattern with
generic_handle_domain_irq(), which handles the IRQ domain lookup
internally. This is less error-prone and more idiomatic.

Remove the now-unused irq_find_mapping() call from qepic_get_irq().

Signed-off-by: Paul Louvel <paul.louvel@bootlin.com>
---
 drivers/soc/fsl/qe/qe_ports_ic.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/fsl/qe/qe_ports_ic.c b/drivers/soc/fsl/qe/qe_ports_ic.c
index 73a77763210d..3bca116fd2f5 100644
--- a/drivers/soc/fsl/qe/qe_ports_ic.c
+++ b/drivers/soc/fsl/qe/qe_ports_ic.c
@@ -75,16 +75,17 @@ static int qepic_get_irq(struct irq_desc *desc)
 	if (!event)
 		return -1;
 
-	return irq_find_mapping(data->host, 32 - ffs(event));
+	return 32 - ffs(event);
 }
 
 static void qepic_cascade(struct irq_desc *desc)
 {
+	struct qepic_data *data = irq_desc_get_handler_data(desc);
 	struct irq_chip *chip = irq_desc_get_chip(desc);
 
 	chained_irq_enter(chip, desc);
 
-	generic_handle_irq(qepic_get_irq(desc));
+	generic_handle_domain_irq(data->host, qepic_get_irq(desc));
 
 	chained_irq_exit(chip, desc);
 }

-- 
2.55.0

[PATCH 06/12] soc: fsl: qe: Iterate over all pending interrupts in cascade handler

[Paul Louvel]

Instead of only servicing a single interrupt, the chained handler can
handle all IRQs that have their bit set in the event register.
This avoid multiple parent IRQ handler being serviced if more than one
interrupt are pending on the QE PIC.

Remove unused code.

Signed-off-by: Paul Louvel <paul.louvel@bootlin.com>
---
 drivers/soc/fsl/qe/qe_ports_ic.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/soc/fsl/qe/qe_ports_ic.c b/drivers/soc/fsl/qe/qe_ports_ic.c
index 3bca116fd2f5..27b62f4e3afb 100644
--- a/drivers/soc/fsl/qe/qe_ports_ic.c
+++ b/drivers/soc/fsl/qe/qe_ports_ic.c
@@ -5,6 +5,7 @@
  * Copyright (c) 2025 Christophe Leroy CS GROUP France (christophe.leroy@csgroup.eu)
  */
 
+#include <linux/bitops.h>
 #include <linux/irq.h>
 #include <linux/irqchip/chained_irq.h>
 #include <linux/irqdomain.h>
@@ -67,25 +68,17 @@ static struct irq_chip qepic = {
 	.irq_set_type = qepic_set_type,
 };
 
-static int qepic_get_irq(struct irq_desc *desc)
-{
-	struct qepic_data *data = irq_desc_get_handler_data(desc);
-	u32 event = in_be32(data->reg + CEPIER);
-
-	if (!event)
-		return -1;
-
-	return 32 - ffs(event);
-}
-
 static void qepic_cascade(struct irq_desc *desc)
 {
 	struct qepic_data *data = irq_desc_get_handler_data(desc);
 	struct irq_chip *chip = irq_desc_get_chip(desc);
+	unsigned long event, bit;
 
 	chained_irq_enter(chip, desc);
 
-	generic_handle_domain_irq(data->host, qepic_get_irq(desc));
+	event = in_be32(data->reg + CEPIER);
+	for_each_set_bit(bit, &event, 32)
+		generic_handle_domain_irq(data->host, 32 - bit);
 
 	chained_irq_exit(chip, desc);
 }

-- 
2.55.0

[PATCH 07/12] soc: fsl: qe: Handle spurious interrupts

[Paul Louvel]

When no interrupt bits are set in the event register, call
handle_bad_irq() to account for the spurious interrupt before
exiting the cascade handler.

Signed-off-by: Paul Louvel <paul.louvel@bootlin.com>
---
 drivers/soc/fsl/qe/qe_ports_ic.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/soc/fsl/qe/qe_ports_ic.c b/drivers/soc/fsl/qe/qe_ports_ic.c
index 27b62f4e3afb..c8b73b0aa233 100644
--- a/drivers/soc/fsl/qe/qe_ports_ic.c
+++ b/drivers/soc/fsl/qe/qe_ports_ic.c
@@ -77,9 +77,15 @@ static void qepic_cascade(struct irq_desc *desc)
 	chained_irq_enter(chip, desc);
 
 	event = in_be32(data->reg + CEPIER);
+	if (!event) {
+		handle_bad_irq(desc);
+		goto out;
+	}
+
 	for_each_set_bit(bit, &event, 32)
 		generic_handle_domain_irq(data->host, 32 - bit);
 
+out:
 	chained_irq_exit(chip, desc);
 }
 

-- 
2.55.0

[PATCH 08/12] soc: fsl: qe: Convert to generic IRQ chip

[Paul Louvel]

The generic IRQ chip framework is available to handle IRQ chips. Using
this framework for the QE interrupt controller allows to simplify the
driver. Indeed, the framework internally handles operations coded
directly in the driver.

Add a select dependency to GENERIC_IRQ_CHIP in the PPC platform Kconfig.

Signed-off-by: Paul Louvel <paul.louvel@bootlin.com>
---
 arch/powerpc/platforms/Kconfig   |   1 +
 drivers/soc/fsl/qe/qe_ports_ic.c | 103 ++++++++++++++++++++++++++-------------
 2 files changed, 70 insertions(+), 34 deletions(-)

diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/Kconfig
index c4e61843d9d9..b0b3a80f8cde 100644
--- a/arch/powerpc/platforms/Kconfig
+++ b/arch/powerpc/platforms/Kconfig
@@ -232,6 +232,7 @@ config QE_GPIO
 	bool "QE GPIO support"
 	depends on QUICC_ENGINE
 	select GPIOLIB
+	select GENERIC_IRQ_CHIP
 	help
 	  Say Y here if you're going to use hardware that connects to the
 	  QE GPIOs.
diff --git a/drivers/soc/fsl/qe/qe_ports_ic.c b/drivers/soc/fsl/qe/qe_ports_ic.c
index c8b73b0aa233..d022aa224f6d 100644
--- a/drivers/soc/fsl/qe/qe_ports_ic.c
+++ b/drivers/soc/fsl/qe/qe_ports_ic.c
@@ -20,63 +20,65 @@ struct qepic_data {
 	void __iomem *reg;
 	struct irq_domain *host;
 	int irq;
+	struct irq_chip_generic *gc;
 };
 
 static void qepic_mask(struct irq_data *d)
 {
-	struct qepic_data *data = irq_data_get_irq_chip_data(d);
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct irq_chip_type *ct = irq_data_get_chip_type(d);
 
-	clrbits32(data->reg + CEPIMR, 1 << (31 - irqd_to_hwirq(d)));
+	clrbits32(gc->reg_base + ct->regs.mask, d->mask);
 }
 
 static void qepic_unmask(struct irq_data *d)
 {
-	struct qepic_data *data = irq_data_get_irq_chip_data(d);
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct irq_chip_type *ct = irq_data_get_chip_type(d);
 
-	setbits32(data->reg + CEPIMR, 1 << (31 - irqd_to_hwirq(d)));
+	setbits32(gc->reg_base + ct->regs.mask, d->mask);
 }
 
 static void qepic_end(struct irq_data *d)
 {
-	struct qepic_data *data = irq_data_get_irq_chip_data(d);
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct irq_chip_type *ct = irq_data_get_chip_type(d);
 
-	out_be32(data->reg + CEPIER, 1 << (31 - irqd_to_hwirq(d)));
+	out_be32(gc->reg_base + ct->regs.eoi, d->mask);
+}
+
+static void qepic_calc_mask(struct irq_data *d)
+{
+	d->mask = 1 << (31 - irqd_to_hwirq(d));
 }
 
 static int qepic_set_type(struct irq_data *d, unsigned int flow_type)
 {
-	struct qepic_data *data = irq_data_get_irq_chip_data(d);
-	unsigned int vec = (unsigned int)irqd_to_hwirq(d);
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct irq_chip_type *ct = irq_data_get_chip_type(d);
 
 	switch (flow_type & IRQ_TYPE_SENSE_MASK) {
 	case IRQ_TYPE_EDGE_FALLING:
-		setbits32(data->reg + CEPICR, 1 << (31 - vec));
+		setbits32(gc->reg_base + ct->regs.type, d->mask);
 		return 0;
 	case IRQ_TYPE_EDGE_BOTH:
 	case IRQ_TYPE_NONE:
-		clrbits32(data->reg + CEPICR, 1 << (31 - vec));
+		clrbits32(gc->reg_base + ct->regs.type, d->mask);
 		return 0;
 	}
 	return -EINVAL;
 }
 
-static struct irq_chip qepic = {
-	.name = "QEPIC",
-	.irq_mask = qepic_mask,
-	.irq_unmask = qepic_unmask,
-	.irq_eoi = qepic_end,
-	.irq_set_type = qepic_set_type,
-};
-
 static void qepic_cascade(struct irq_desc *desc)
 {
 	struct qepic_data *data = irq_desc_get_handler_data(desc);
+	struct irq_chip_type *ct = data->gc->chip_types;
 	struct irq_chip *chip = irq_desc_get_chip(desc);
 	unsigned long event, bit;
 
 	chained_irq_enter(chip, desc);
 
-	event = in_be32(data->reg + CEPIER);
+	event = in_be32(data->gc->reg_base + ct->regs.eoi);
 	if (!event) {
 		handle_bad_irq(desc);
 		goto out;
@@ -89,33 +91,64 @@ static void qepic_cascade(struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
-static int qepic_host_map(struct irq_domain *h, unsigned int virq, irq_hw_number_t hw)
+static int qepic_chip_init(struct irq_chip_generic *gc)
 {
-	irq_set_chip_data(virq, h->host_data);
-	irq_set_chip_and_handler(virq, &qepic, handle_fasteoi_irq);
+	struct irq_chip_type *ct = gc->chip_types;
+
+	ct->regs.mask = CEPIMR;
+	ct->chip.irq_mask = qepic_mask;
+	ct->chip.irq_unmask = qepic_unmask;
+	ct->regs.eoi = CEPIER;
+	ct->chip.irq_eoi = qepic_end;
+	ct->regs.type = CEPICR;
+	ct->chip.irq_set_type = qepic_set_type;
+	ct->chip.irq_calc_mask = qepic_calc_mask;
+
 	return 0;
 }
 
-static const struct irq_domain_ops qepic_host_ops = {
-	.map = qepic_host_map,
-};
+static int qepic_domain_init(struct irq_domain *d)
+{
+	struct qepic_data *data = d->host_data;
 
-static void qepic_remove(void *res)
+	irq_set_chained_handler_and_data(data->irq, qepic_cascade, data);
+
+	return 0;
+}
+
+static void qepic_domain_exit(struct irq_domain *d)
 {
-	struct qepic_data *data = res;
+	struct qepic_data *data = d->host_data;
 
 	irq_set_chained_handler_and_data(data->irq, NULL, NULL);
-	irq_domain_remove(data->host);
 }
 
 static int qepic_probe(struct platform_device *pdev)
 {
+	struct irq_domain_chip_generic_info dgc_info = {
+		.name		= "QEPIC",
+		.handler	= handle_fasteoi_irq,
+		.irqs_per_chip	= 32,
+		.num_ct		= 1,
+		.init		= qepic_chip_init,
+	};
+	struct irq_domain_info d_info = {
+		.fwnode		= of_fwnode_handle(pdev->dev.of_node),
+		.domain_flags	= IRQ_DOMAIN_FLAG_DESTROY_GC,
+		.size		= 32,
+		.hwirq_max	= 32,
+		.ops		= &irq_generic_chip_ops,
+		.dgc_info	= &dgc_info,
+		.init		= qepic_domain_init,
+		.exit		= qepic_domain_exit,
+	};
 	struct device *dev = &pdev->dev;
 	struct qepic_data *data;
 
 	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
+	d_info.host_data = data;
 
 	data->reg = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(data->reg))
@@ -125,14 +158,16 @@ static int qepic_probe(struct platform_device *pdev)
 	if (data->irq < 0)
 		return data->irq;
 
-	data->host = irq_domain_create_linear(dev_fwnode(dev), 32, &qepic_host_ops, data);
-	if (!data->host)
-		return -ENODEV;
+	data->host = devm_irq_domain_instantiate(dev, &d_info);
+	if (IS_ERR(data->host))
+		return PTR_ERR(data->host);
 
-	irq_set_chained_handler_and_data(data->irq, qepic_cascade, data);
-
-	return devm_add_action_or_reset(dev, qepic_remove, data);
+	data->gc = irq_get_domain_generic_chip(data->host, 0);
+	if (!data->gc)
+		return -ENODEV;
+	data->gc->reg_base = data->reg;
 
+	return 0;
 }
 
 static const struct of_device_id qepic_match[] = {

-- 
2.55.0

[PATCH 09/12] soc: fsl: qe: Rename irq variable to parent_irq

[Paul Louvel]

Rename the local variable holding the platform IRQ to parent_irq, which
better describes its role as the upstream/chained interrupt in the
hierarchy.

Signed-off-by: Paul Louvel <paul.louvel@bootlin.com>
---
 drivers/soc/fsl/qe/qe_ports_ic.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/soc/fsl/qe/qe_ports_ic.c b/drivers/soc/fsl/qe/qe_ports_ic.c
index d022aa224f6d..718703dab024 100644
--- a/drivers/soc/fsl/qe/qe_ports_ic.c
+++ b/drivers/soc/fsl/qe/qe_ports_ic.c
@@ -19,7 +19,7 @@
 struct qepic_data {
 	void __iomem *reg;
 	struct irq_domain *host;
-	int irq;
+	int parent_irq;
 	struct irq_chip_generic *gc;
 };
 
@@ -111,7 +111,7 @@ static int qepic_domain_init(struct irq_domain *d)
 {
 	struct qepic_data *data = d->host_data;
 
-	irq_set_chained_handler_and_data(data->irq, qepic_cascade, data);
+	irq_set_chained_handler_and_data(data->parent_irq, qepic_cascade, data);
 
 	return 0;
 }
@@ -120,7 +120,7 @@ static void qepic_domain_exit(struct irq_domain *d)
 {
 	struct qepic_data *data = d->host_data;
 
-	irq_set_chained_handler_and_data(data->irq, NULL, NULL);
+	irq_set_chained_handler_and_data(data->parent_irq, NULL, NULL);
 }
 
 static int qepic_probe(struct platform_device *pdev)
@@ -154,9 +154,9 @@ static int qepic_probe(struct platform_device *pdev)
 	if (IS_ERR(data->reg))
 		return PTR_ERR(data->reg);
 
-	data->irq = platform_get_irq(pdev, 0);
-	if (data->irq < 0)
-		return data->irq;
+	data->parent_irq = platform_get_irq(pdev, 0);
+	if (data->parent_irq < 0)
+		return data->parent_irq;
 
 	data->host = devm_irq_domain_instantiate(dev, &d_info);
 	if (IS_ERR(data->host))

-- 
2.55.0

[PATCH 10/12] soc: fsl: qe: Rename host member to domain in struct qepic_data

[Paul Louvel]

Rename the host field to domain to match the common kernel naming
convention for irq_domain pointers.

Signed-off-by: Paul Louvel <paul.louvel@bootlin.com>
---
 drivers/soc/fsl/qe/qe_ports_ic.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/soc/fsl/qe/qe_ports_ic.c b/drivers/soc/fsl/qe/qe_ports_ic.c
index 718703dab024..1bf2af087b78 100644
--- a/drivers/soc/fsl/qe/qe_ports_ic.c
+++ b/drivers/soc/fsl/qe/qe_ports_ic.c
@@ -18,8 +18,8 @@
 
 struct qepic_data {
 	void __iomem *reg;
-	struct irq_domain *host;
 	int parent_irq;
+	struct irq_domain *domain;
 	struct irq_chip_generic *gc;
 };
 
@@ -85,7 +85,7 @@ static void qepic_cascade(struct irq_desc *desc)
 	}
 
 	for_each_set_bit(bit, &event, 32)
-		generic_handle_domain_irq(data->host, 32 - bit);
+		generic_handle_domain_irq(data->domain, 32 - bit);
 
 out:
 	chained_irq_exit(chip, desc);
@@ -158,11 +158,11 @@ static int qepic_probe(struct platform_device *pdev)
 	if (data->parent_irq < 0)
 		return data->parent_irq;
 
-	data->host = devm_irq_domain_instantiate(dev, &d_info);
-	if (IS_ERR(data->host))
-		return PTR_ERR(data->host);
+	data->domain = devm_irq_domain_instantiate(dev, &d_info);
+	if (IS_ERR(data->domain))
+		return PTR_ERR(data->domain);
 
-	data->gc = irq_get_domain_generic_chip(data->host, 0);
+	data->gc = irq_get_domain_generic_chip(data->domain, 0);
 	if (!data->gc)
 		return -ENODEV;
 	data->gc->reg_base = data->reg;

-- 
2.55.0

[PATCH 11/12] soc: fsl: qe: Remove useless struct member

[Paul Louvel]

The reg member from the struct qepic_data is no longer needed, since it
is now embedded in reg_base of struct irq_chip_generic. Remove it.

Signed-off-by: Paul Louvel <paul.louvel@bootlin.com>
---
 drivers/soc/fsl/qe/qe_ports_ic.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/soc/fsl/qe/qe_ports_ic.c b/drivers/soc/fsl/qe/qe_ports_ic.c
index 1bf2af087b78..6cf23a721ff3 100644
--- a/drivers/soc/fsl/qe/qe_ports_ic.c
+++ b/drivers/soc/fsl/qe/qe_ports_ic.c
@@ -17,7 +17,6 @@
 #define CEPICR		0x14
 
 struct qepic_data {
-	void __iomem *reg;
 	int parent_irq;
 	struct irq_domain *domain;
 	struct irq_chip_generic *gc;
@@ -144,15 +143,16 @@ static int qepic_probe(struct platform_device *pdev)
 	};
 	struct device *dev = &pdev->dev;
 	struct qepic_data *data;
+	void __iomem *reg;
 
 	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 	d_info.host_data = data;
 
-	data->reg = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(data->reg))
-		return PTR_ERR(data->reg);
+	reg = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(reg))
+		return PTR_ERR(reg);
 
 	data->parent_irq = platform_get_irq(pdev, 0);
 	if (data->parent_irq < 0)
@@ -165,7 +165,7 @@ static int qepic_probe(struct platform_device *pdev)
 	data->gc = irq_get_domain_generic_chip(data->domain, 0);
 	if (!data->gc)
 		return -ENODEV;
-	data->gc->reg_base = data->reg;
+	data->gc->reg_base = reg;
 
 	return 0;
 }

-- 
2.55.0

[PATCH 12/12] soc: fsl: qe: Add support of IRQs in QE GPIO

[Paul Louvel]

Some QE GPIO pins have an associated interrupt line in the QE PIC to
signal state changes on the pin.

Because the GPIO controller does not perform any interrupt handling
itself, a nexus node (interrupt-map) is used to map each GPIO line
supporting IRQ to the parent QE PIC interrupt domain.

Add the to_irq() method in the corresponding GPIO controller driver,
that uses the nexus node to perform the translation.

Signed-off-by: Paul Louvel <paul.louvel@bootlin.com>
---
 drivers/soc/fsl/qe/gpio.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/fsl/qe/gpio.c b/drivers/soc/fsl/qe/gpio.c
index 66828f2a3577..f8919642f40d 100644
--- a/drivers/soc/fsl/qe/gpio.c
+++ b/drivers/soc/fsl/qe/gpio.c
@@ -16,6 +16,7 @@
 #include <linux/gpio/driver.h>
 #include <linux/slab.h>
 #include <linux/export.h>
+#include <linux/of_irq.h>
 #include <linux/platform_device.h>
 
 #include <soc/fsl/qe/qe.h>
@@ -23,6 +24,7 @@
 #define PIN_MASK(gpio) (1UL << (QE_PIO_PINS - 1 - (gpio)))
 
 struct qe_gpio_chip {
+	struct device_node *np;
 	struct gpio_chip gc;
 	void __iomem *regs;
 	spinlock_t lock;
@@ -135,6 +137,29 @@ static int qe_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
 	return 0;
 }
 
+static int qe_gpio_to_irq(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct qe_gpio_chip *qe_gc = gpiochip_get_data(gc);
+	struct of_phandle_args oirq;
+	struct irq_domain *domain;
+	int ret;
+
+	oirq.np = qe_gc->np;
+	oirq.args_count = 2;
+	oirq.args[0] = gpio;
+	oirq.args[1] = 0;
+
+	ret = of_irq_parse_raw(NULL, &oirq);
+	if (ret)
+		return ret;
+
+	domain = irq_find_host(oirq.np);
+	if (!domain)
+		return -EPROBE_DEFER;
+
+	return irq_create_of_mapping(&oirq);
+}
+
 struct qe_pin {
 	/*
 	 * The qe_gpio_chip name is unfortunate, we should change that to
@@ -299,7 +324,7 @@ static int qe_gpio_probe(struct platform_device *ofdev)
 	qe_gc = devm_kzalloc(dev, sizeof(*qe_gc), GFP_KERNEL);
 	if (!qe_gc)
 		return -ENOMEM;
-
+	qe_gc->np = np;
 	spin_lock_init(&qe_gc->lock);
 
 	gc = &qe_gc->gc;
@@ -311,6 +336,7 @@ static int qe_gpio_probe(struct platform_device *ofdev)
 	gc->get = qe_gpio_get;
 	gc->set = qe_gpio_set;
 	gc->set_multiple = qe_gpio_set_multiple;
+	gc->to_irq = qe_gpio_to_irq;
 	gc->parent = dev;
 	gc->owner = THIS_MODULE;
 

-- 
2.55.0

Re: [PATCH 06/12] soc: fsl: qe: Iterate over all pending interrupts in cascade handler

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Off-by-one error in hardware IRQ calculation causes out-of-bounds domain resolution and misrouted/dropped interrupts.
--

commit 151384fe21d3660403f9ea1eecce7c7057939a64
Author: Paul Louvel <paul.louvel@bootlin.com>

soc: fsl: qe: Iterate over all pending interrupts in cascade handler

This commit updates the chained interrupt handler for the QE PIC to process
all pending interrupts in a single run. It replaces a single-interrupt read
using ffs() with a loop over all set bits in the event register.

> diff --git a/drivers/soc/fsl/qe/qe_ports_ic.c b/drivers/soc/fsl/qe/qe_ports_ic.c
> index 3bca116fd2f5e..27b62f4e3afb1 100644
> --- a/drivers/soc/fsl/qe/qe_ports_ic.c
> +++ b/drivers/soc/fsl/qe/qe_ports_ic.c
[ ... ]
> @@ -67,25 +68,17 @@ static struct irq_chip qepic = {
[ ... ]
> -static int qepic_get_irq(struct irq_desc *desc)
> -{
> -	struct qepic_data *data = irq_desc_get_handler_data(desc);
> -	u32 event = in_be32(data->reg + CEPIER);
> -
> -	if (!event)
> -		return -1;
> -
> -	return 32 - ffs(event);
> -}
> -
>  static void qepic_cascade(struct irq_desc *desc)
>  {
>  	struct qepic_data *data = irq_desc_get_handler_data(desc);
>  	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	unsigned long event, bit;
>  
>  	chained_irq_enter(chip, desc);
>  
> -	generic_handle_domain_irq(data->host, qepic_get_irq(desc));
> +	event = in_be32(data->reg + CEPIER);
> +	for_each_set_bit(bit, &event, 32)
> +		generic_handle_domain_irq(data->host, 32 - bit);

[Severity: High]
Will this calculation map to the correct hardware IRQ?

The original qepic_get_irq() code used ffs(event), which is 1-indexed and
returns a value from 1 to 32. With for_each_set_bit(), the bit variable is
0-indexed and returns a value from 0 to 31.

Because bit is 0-indexed, if bit 0 is set, the new calculation evaluates to
32 - 0 = 32, which is out of bounds for the 0-31 hwirq range. All other mapped
interrupts appear to be shifted by +1.

Could this result in misrouted interrupts and leave the actual pending hardware
interrupt unserviced? If the interrupt is not cleared, would it cause an
interrupt storm as soon as the parent IRQ handler exits and unmasks it?

Should this subtraction be 31 - bit instead?

>  
>  	chained_irq_exit(chip, desc);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-qe-pic-gpios-v1-0-6c3e706e27dc@bootlin.com?part=6

Re: [PATCH 02/12] dt-bindings: soc: fsl: qe: Set #interrupt-cells to 2 to support interrupt type encoding

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Schema unconditionally changes `#interrupt-cells` from 1 to 2, breaking backward compatibility with existing single-cell device trees.
--

commit ea9fdfe75d38976a589a83706c16b469e2657dbe
Author: Paul Louvel <paul.louvel@bootlin.com>

dt-bindings: soc: fsl: qe: Set #interrupt-cells to 2 to support interrupt type encoding

This commit updates the #interrupt-cells property constraint in the FSL QE
ports interrupt controller binding to support interrupt type encoding. It
changes the required value from 1 to 2.

> diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,qe-ports-ic.yaml b/Documentation/devicetree/bindings/interrupt-controller/fsl,qe-ports-ic.yaml
> index 2b8e7b9c6d7a4..2b7c6b4f03892 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/fsl,qe-ports-ic.yaml
> +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,qe-ports-ic.yaml
> @@ -23,7 +23,7 @@ properties:
>      const: 0
>  
>    '#interrupt-cells':
> -    const: 1
> +    const: 2

[Severity: Medium]
Does this unconditional change to const: 2 break backward compatibility with
existing device trees?

Device tree bindings are generally considered ABI. By removing const: 1,
schema validation (dtbs_check) will fail for any existing device tree blobs
that use #interrupt-cells = <1>.

Should this be updated to allow both values (e.g., enum: [1, 2]) to safely
expand the cell counts while maintaining compatibility with older device
trees?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-qe-pic-gpios-v1-0-6c3e706e27dc@bootlin.com?part=2

Re: [PATCH 09/12] soc: fsl: qe: Rename irq variable to parent_irq

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] A race condition during probe can cause a NULL pointer dereference in the chained interrupt handler `qepic_cascade` because the chained IRQ is unmasked before generic chip data is fully initialized.
- [High] An off-by-one logic error in `qepic_cascade` calculates incorrect hardware IRQ indices, rendering interrupts incorrectly routed or completely dead.

Pre-existing issues:
- [High] The mask, unmask, and set_type callbacks perform non-atomic read-modify-write operations on shared controller registers without locking.
--

commit 233494b356181bf50d667f74d4dd3ee1947142f8
Author: Paul Louvel <paul.louvel@bootlin.com>

soc: fsl: qe: Rename irq variable to parent_irq

This commit renames the irq local variable to parent_irq in struct qepic_data
to better describe its role as the upstream interrupt. It updates the
references in qepic_probe, qepic_domain_init, and qepic_domain_exit to use
the new name.

> diff --git a/drivers/soc/fsl/qe/qe_ports_ic.c b/drivers/soc/fsl/qe/qe_ports_ic.c
> index d022aa224f6d6..718703dab0240 100644
> --- a/drivers/soc/fsl/qe/qe_ports_ic.c
> +++ b/drivers/soc/fsl/qe/qe_ports_ic.c
[ ... ]
> @@ -111,7 +111,7 @@ static int qepic_domain_init(struct irq_domain *d)
>  {
>  	struct qepic_data *data = d->host_data;
>  
> -	irq_set_chained_handler_and_data(data->irq, qepic_cascade, data);
> +	irq_set_chained_handler_and_data(data->parent_irq, qepic_cascade, data);

[Severity: High]
Does this chained handler registration have an off-by-one regression when
calculating the hardware IRQ?

Looking at qepic_cascade(), the interrupt index is calculated using a bitwise
operation but retains a subtraction logic from older code:

drivers/soc/fsl/qe/qe_ports_ic.c:qepic_cascade() {
    ...
    for_each_set_bit(bit, &val, 32)
        generic_handle_domain_irq(data->domain, 32 - bit);
    ...
}

Because for_each_set_bit() yields a 0-indexed bit (0 to 31), calculating
32 - bit yields 32 down to 1. For bit 0, it yields hwirq 32, which exceeds
hwirq_max (32, indices 0-31) and gets dropped by the IRQ core. Should this be
31 - bit instead to map correctly?

[ ... ]
> @@ -154,9 +154,9 @@ static int qepic_probe(struct platform_device *pdev)
>  	if (IS_ERR(data->reg))
>  		return PTR_ERR(data->reg);
>  
> -	data->irq = platform_get_irq(pdev, 0);
> -	if (data->irq < 0)
> -		return data->irq;
> +	data->parent_irq = platform_get_irq(pdev, 0);
> +	if (data->parent_irq < 0)
> +		return data->parent_irq;
>  
>  	data->host = devm_irq_domain_instantiate(dev, &d_info);

[Severity: High]
Can this cause a NULL pointer dereference regression during probe if the
interrupt fires immediately?

devm_irq_domain_instantiate() calls qepic_domain_init(), which immediately
registers and unmasks the chained handler qepic_cascade() via
irq_set_chained_handler_and_data().

If an interrupt is already pending or fires before qepic_probe() finishes,
qepic_cascade() will execute and attempt to access data->gc:

drivers/soc/fsl/qe/qe_ports_ic.c:qepic_cascade() {
    struct irq_chip_type *ct = data->gc->chip_types;
    ...
}

However, data->gc is not populated until after devm_irq_domain_instantiate()
returns:

drivers/soc/fsl/qe/qe_ports_ic.c:qepic_probe() {
    ...
    data->domain = devm_irq_domain_instantiate(dev, &d_info);
    if (IS_ERR(data->domain))
        return PTR_ERR(data->domain);

    data->gc = irq_get_domain_generic_chip(data->domain, 0);
    ...
}

Should we defer arming the chained handler until after the generic chip is
fully initialized?

[Severity: High]
This is a pre-existing issue, but do we need to lock the shared registers
during mask/unmask operations?

qepic_mask(), qepic_unmask(), and qepic_set_type() perform non-atomic
read-modify-write operations on shared controller registers:

drivers/soc/fsl/qe/qe_ports_ic.c:qepic_mask() {
    ...
    clrbits32(gc->reg_base + ct->regs.mask, d->mask);
}

These callbacks expand to non-atomic I/O operations. Since the core kernel
only holds the per-IRQ desc->lock during these calls, concurrent interrupt
mask/unmask operations on different CPUs could silently overwrite each other,
causing interrupts to be unintentionally left enabled or disabled.

Would it be safer to acquire gc->lock during these register modifications?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-qe-pic-gpios-v1-0-6c3e706e27dc@bootlin.com?part=9

Re: [PATCH 10/12] soc: fsl: qe: Rename host member to domain in struct qepic_data

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Off-by-one mapping between hardware interrupts and virtual IRQs due to incorrect bit index arithmetic.
- [High] Uninitialized pointer dereference because the chained IRQ handler is registered before dependencies are populated.
--

commit 2f6243965deebfddabc0af7472b701df854ecb78
Author: Paul Louvel <paul.louvel@bootlin.com>

soc: fsl: qe: Rename host member to domain in struct qepic_data

This commit renames the host field to domain in struct qepic_data to match
the common kernel naming convention for irq_domain pointers.

> diff --git a/drivers/soc/fsl/qe/qe_ports_ic.c b/drivers/soc/fsl/qe/qe_ports_ic.c
> index 718703dab0240..1bf2af087b784 100644
> --- a/drivers/soc/fsl/qe/qe_ports_ic.c
> +++ b/drivers/soc/fsl/qe/qe_ports_ic.c

[ ... ]

> @@ -85,7 +85,7 @@ static void qepic_cascade(struct irq_desc *desc)
>  	}
>  
>  	for_each_set_bit(bit, &event, 32)
> -		generic_handle_domain_irq(data->host, 32 - bit);
> +		generic_handle_domain_irq(data->domain, 32 - bit);

[Severity: High]
Does this code map the hardware interrupts to the wrong virtual IRQs?

Since for_each_set_bit() provides a 0-based index for bit, mapping it
using 32 - bit shifts all interrupts by one (e.g. mapping hardware
interrupt 31 to 32, which is out of bounds). Should this be 31 - bit
instead?

>  out:
>  	chained_irq_exit(chip, desc);

[ ... ]

> @@ -158,11 +158,11 @@ static int qepic_probe(struct platform_device *pdev)
>  	if (data->parent_irq < 0)
>  		return data->parent_irq;
>  
> -	data->host = devm_irq_domain_instantiate(dev, &d_info);
> -	if (IS_ERR(data->host))
> -		return PTR_ERR(data->host);
> +	data->domain = devm_irq_domain_instantiate(dev, &d_info);
> +	if (IS_ERR(data->domain))
> +		return PTR_ERR(data->domain);
>  
> -	data->gc = irq_get_domain_generic_chip(data->host, 0);
> +	data->gc = irq_get_domain_generic_chip(data->domain, 0);

[Severity: High]
Can this result in a NULL pointer dereference if the parent interrupt
fires during probe?

Calling devm_irq_domain_instantiate() synchronously invokes the domain
init function, which registers the chained handler (qepic_cascade) and
unmasks the parent interrupt. If the interrupt triggers immediately,
qepic_cascade() will execute before data->gc is populated here:

drivers/soc/fsl/qe/qe_ports_ic.c:qepic_cascade() {
    ...
	struct irq_chip_type *ct = data->gc->chip_types;
    ...
}

Will data->gc still be NULL when the handler tries to access it?

>  	if (!data->gc)
>  		return -ENODEV;
>  	data->gc->reg_base = data->reg;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-qe-pic-gpios-v1-0-6c3e706e27dc@bootlin.com?part=10

Re: [PATCH 04/12] dt-bindings: soc: fsl: qe: Add support of IRQ in QE GPIO

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The schema introduces the optional `interrupt-map` capability but fails to conditionally require `#address-cells` and `#interrupt-cells` when `interrupt-map` is present.
--

commit 31293f4013500ab5fb7afbb5070f5cfad4b4d8a9
Author: Paul Louvel <paul.louvel@bootlin.com>

dt-bindings: soc: fsl: qe: Add support of IRQ in QE GPIO

This commit adds interrupt-controller and nexus properties to the QE GPIO
binding to support signaling state changes on GPIO pins. It uses an
interrupt-map to route these to the parent QE PIC interrupt domain.

> diff --git a/Documentation/devicetree/bindings/gpio/fsl,mpc8323-qe-pario-bank.yaml b/Documentation/devicetree/bindings/gpio/fsl,mpc8323-qe-pario-bank.yaml
> index 1af99339ff404..0c849a5698f4b 100644
> --- a/Documentation/devicetree/bindings/gpio/fsl,mpc8323-qe-pario-bank.yaml
> +++ b/Documentation/devicetree/bindings/gpio/fsl,mpc8323-qe-pario-bank.yaml
[ ... ]
> @@ -27,6 +27,17 @@ properties:
>    "#gpio-cells":
>      const: 2
>  
> +  "#address-cells":
> +    const: 0
> +
> +  "#interrupt-cells":
> +    const: 2
> +
> +  interrupt-map:
> +    description: |
> +      Specifies the mapping of GPIO lines to the parent interrupt controller, as the
> +      GPIO controller does not do interrupt handling itself.

[Severity: Medium]
Should the schema conditionally require #address-cells and #interrupt-cells 
when the interrupt-map property is present?

If a device tree includes interrupt-map to enable GPIO interrupts but omits
#address-cells, the validation will incorrectly pass. Later at runtime,
of_irq_parse_raw() will traverse up the tree and inherit #address-cells from
the parent node (typically 1 for a bus).

This misaligns the expected parsing of the interrupt-map array by reading a
child address cell instead of 0 cells, which corrupts the interrupt mapping.

Adding a dependencies definition for interrupt-map could ensure these
cell-count properties are enforced when the mapping is used.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-qe-pic-gpios-v1-0-6c3e706e27dc@bootlin.com?part=4

Re: [PATCH 07/12] soc: fsl: qe: Handle spurious interrupts

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect hardware IRQ mapping due to an off-by-one error when dispatching cascade child interrupts, causing out-of-bounds domain access and an interrupt storm.
- [High] Calling handle_bad_irq() for spurious cascaded interrupts triggers unconditional critical log spam.
--

commit d12d1fe8292a26f2c055e99fd4c7554f2334dbdf
Author: Paul Louvel <paul.louvel@bootlin.com>

soc: fsl: qe: Handle spurious interrupts

When no interrupt bits are set in the event register, this commit calls
handle_bad_irq() to account for the spurious interrupt before exiting the
cascade handler.

> diff --git a/drivers/soc/fsl/qe/qe_ports_ic.c b/drivers/soc/fsl/qe/qe_ports_ic.c
> index 27b62f4e3afb1..c8b73b0aa2334 100644
> --- a/drivers/soc/fsl/qe/qe_ports_ic.c
> +++ b/drivers/soc/fsl/qe/qe_ports_ic.c
> @@ -77,9 +77,15 @@ static void qepic_cascade(struct irq_desc *desc)
>  	chained_irq_enter(chip, desc);
>  
>  	event = in_be32(data->reg + CEPIER);
> +	if (!event) {
> +		handle_bad_irq(desc);

[Severity: High]
Does this introduction of handle_bad_irq() for spurious interrupts risk causing
a system hang?

Spurious interrupts can occur naturally under normal workloads due to timing
races or another handler clearing the event register.

When handle_bad_irq() is called, print_irq_desc() and ack_bad_irq() will run.
On PowerPC architectures, this executes an unconditional
printk(KERN_CRIT "unexpected IRQ trap..."), which could flood the console from
hard IRQ context and potentially hang the system due to serial console
spinlocks.

Should the cascade handler simply exit without error so the parent can be
safely unmasked?

> +		goto out;
> +	}
> +
>  	for_each_set_bit(bit, &event, 32)
>  		generic_handle_domain_irq(data->host, 32 - bit);

[Severity: High]
Can this hwirq calculation cause out-of-bounds domain access and an interrupt
storm?

The calculation 32 - bit maps 0-based bit indexes to a 1-to-32 hwirq range,
instead of the valid 0-to-31 range.

For example, if bit 0 is set, hwirq 32 is passed, and if bit 31 is set, hwirq 1
is passed. Since the correct underlying hardware interrupt's handler and EOI
function are never invoked, its interrupt flag would remain asserted in the
hardware.

Will this result in an unrecoverable interrupt storm on the parent IRQ?

>  
> +out:
>  	chained_irq_exit(chip, desc);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-qe-pic-gpios-v1-0-6c3e706e27dc@bootlin.com?part=7

Re: [PATCH 08/12] soc: fsl: qe: Convert to generic IRQ chip

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Incorrect hwirq calculation in `qepic_cascade()` maps bit indices [0..31] to hwirqs [32..1] instead of [31..0], causing out-of-bounds mapping and shifting all IRQs by one.
- [High] Race condition on probe: The chained interrupt handler is registered before `data->gc` is initialized, causing a NULL pointer dereference if an interrupt fires early.

Pre-existing issues:
- [High] The IRQ mask, unmask, and set_type callbacks use lockless read-modify-write operations on shared registers, leading to data races.
--

commit 39268a559b43e95ee6bcd43807086719d4997f6e
Author: Paul Louvel <paul.louvel@bootlin.com>

soc: fsl: qe: Convert to generic IRQ chip

The generic IRQ chip framework is available to handle IRQ chips. Using
this framework for the QE interrupt controller allows to simplify the
driver. Indeed, the framework internally handles operations coded
directly in the driver.

> diff --git a/drivers/soc/fsl/qe/qe_ports_ic.c b/drivers/soc/fsl/qe/qe_ports_ic.c
> index c8b73b0aa2334..d022aa224f6d6 100644
> --- a/drivers/soc/fsl/qe/qe_ports_ic.c
> +++ b/drivers/soc/fsl/qe/qe_ports_ic.c
> @@ -20,63 +20,65 @@ struct qepic_data {
>  	void __iomem *reg;
>  	struct irq_domain *host;
>  	int irq;
> +	struct irq_chip_generic *gc;
>  };
>  
>  static void qepic_mask(struct irq_data *d)
>  {
> -	struct qepic_data *data = irq_data_get_irq_chip_data(d);
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct irq_chip_type *ct = irq_data_get_chip_type(d);
>  
> -	clrbits32(data->reg + CEPIMR, 1 << (31 - irqd_to_hwirq(d)));
> +	clrbits32(gc->reg_base + ct->regs.mask, d->mask);
>  }

[Severity: High]
This is a pre-existing issue, but does this lockless read-modify-write
operation cause data races?

The qepic_mask(), qepic_unmask(), and qepic_set_type() callbacks use
clrbits32() and setbits32() on shared registers without a shared controller
lock. If multiple CPUs modify different IRQs concurrently, could updates to
the shared mask or type registers be lost?

[ ... ]

>  static void qepic_cascade(struct irq_desc *desc)
>  {
>  	struct qepic_data *data = irq_desc_get_handler_data(desc);
> +	struct irq_chip_type *ct = data->gc->chip_types;
>  	struct irq_chip *chip = irq_desc_get_chip(desc);
>  	unsigned long event, bit;
>  
>  	chained_irq_enter(chip, desc);
>  
> -	event = in_be32(data->reg + CEPIER);
> +	event = in_be32(data->gc->reg_base + ct->regs.eoi);
>  	if (!event) {
>  		handle_bad_irq(desc);
>  		goto out;
>  	}

[Severity: High]
Could the hwirq calculation in qepic_cascade() be incorrect?

When an interrupt fires, the trailing loop (using for_each_set_bit) yields a
0-based bit index, but generic_handle_domain_irq() evaluates the hwirq using
"32 - bit".

For example, if bit 0 is set, it maps to hwirq 32, which exceeds the domain's
hwirq_max of 32 and leaves all IRQs shifted out of bounds. Does this need to
be "31 - bit" to correctly map bit indices [0..31] to hwirqs [31..0]?

[ ... ]

> @@ -125,14 +158,16 @@ static int qepic_probe(struct platform_device *pdev)
>  	if (data->irq < 0)
>  		return data->irq;
>  
> -	data->host = irq_domain_create_linear(dev_fwnode(dev), 32, &qepic_host_ops, data);
> -	if (!data->host)
> -		return -ENODEV;
> +	data->host = devm_irq_domain_instantiate(dev, &d_info);
> +	if (IS_ERR(data->host))
> +		return PTR_ERR(data->host);
>  
> -	irq_set_chained_handler_and_data(data->irq, qepic_cascade, data);
> -
> -	return devm_add_action_or_reset(dev, qepic_remove, data);
> +	data->gc = irq_get_domain_generic_chip(data->host, 0);
> +	if (!data->gc)
> +		return -ENODEV;
> +	data->gc->reg_base = data->reg;
>  
> +	return 0;
>  }

[Severity: High]
Can this probe sequence trigger a NULL pointer dereference?

When devm_irq_domain_instantiate() is called, it triggers the init callback
qepic_domain_init(), which registers the chained handler and unmasks the
parent interrupt:

qepic_domain_init()
    irq_set_chained_handler_and_data(data->parent_irq, qepic_cascade, data);

If an interrupt is pending and fires immediately, qepic_cascade() will
execute and attempt to dereference data->gc->reg_base. However, data->gc is
not initialized until irq_get_domain_generic_chip() is called after the
domain instantiation returns.

Could this cause a crash if a pending interrupt is triggered during probe?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-qe-pic-gpios-v1-0-6c3e706e27dc@bootlin.com?part=8

Re: [PATCH 11/12] soc: fsl: qe: Remove useless struct member

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Critical] The chained cascade handler is registered prematurely during IRQ domain instantiation before the generic IRQ chip and its register base are populated, leading to a potential NULL pointer dereference.
- [High] Off-by-one error in calculating `hwirq` from the interrupt event bit index, leading to out-of-bounds array access and shifted IRQ mapping.

Pre-existing issues:
- [High] Missing synchronization around shared memory-mapped registers in the custom mask, unmask, and set_type operations.
--

commit 5ecb4528440dd9ca9845f840100f9f68d449b15d
Author: Paul Louvel <paul.louvel@bootlin.com>

soc: fsl: qe: Remove useless struct member
    
The reg member from the struct qepic_data is no longer needed, since it
is now embedded in reg_base of struct irq_chip_generic. Remove it.

> diff --git a/drivers/soc/fsl/qe/qe_ports_ic.c b/drivers/soc/fsl/qe/qe_ports_ic.c
> index 1bf2af087b784..6cf23a721ff36 100644
> --- a/drivers/soc/fsl/qe/qe_ports_ic.c
> +++ b/drivers/soc/fsl/qe/qe_ports_ic.c
[ ... ]
> @@ -165,7 +165,7 @@ static int qepic_probe(struct platform_device *pdev)
>  	data->gc = irq_get_domain_generic_chip(data->domain, 0);
>  	if (!data->gc)
>  		return -ENODEV;
> -	data->gc->reg_base = data->reg;
> +	data->gc->reg_base = reg;

[Severity: Critical]
Can this lead to a NULL pointer dereference?

The devm_irq_domain_instantiate() function is called earlier in qepic_probe(),
which synchronously triggers qepic_domain_init().

Inside qepic_domain_init(), irq_set_chained_handler_and_data() is called:

drivers/soc/fsl/qe/qe_ports_ic.c:qepic_domain_init() {
    ...
    irq_set_chained_handler_and_data(data->parent_irq, qepic_cascade, data);
    ...
}

This immediately registers and unmasks the parent interrupt. If a pending
interrupt fires right after this, qepic_cascade() will execute and
dereference data->gc->chip_types:

drivers/soc/fsl/qe/qe_ports_ic.c:qepic_cascade() {
    ...
    struct irq_chip_type *ct = data->gc->chip_types;
    ...
}

However, data->gc is only allocated and initialized here, after
devm_irq_domain_instantiate() returns. Could data->gc be NULL if the hardware
interrupt is asserted early?


[Severity: High]
Is there an off-by-one error when calculating the hardware IRQ?

Looking at qepic_cascade(), when an interrupt fires, it seems to pass an
incorrect hwirq to the subsystem:

drivers/soc/fsl/qe/qe_ports_ic.c:qepic_cascade() {
    ...
    generic_handle_domain_irq(data->domain, 32 - bit);
    ...
}

If bit is 0, this calculates a hwirq of 32. Since the domain size is 32 (max
hwirq 31), will calling generic_handle_domain_irq() with 32 fail?

Does this also shift the IRQ mapping, making the 0th bit interrupt
unserviceable?


[Severity: High]
This is a pre-existing issue, but is there missing synchronization around the
shared memory-mapped registers?

The custom mask, unmask, and set_type functions perform non-atomic
read-modify-write operations on shared hardware registers without holding
any locks:

drivers/soc/fsl/qe/qe_ports_ic.c:qepic_mask() {
    ...
    clrbits32(gc->reg_base + ct->regs.mask, d->mask);
    ...
}

drivers/soc/fsl/qe/qe_ports_ic.c:qepic_unmask() {
    ...
    setbits32(gc->reg_base + ct->regs.mask, d->mask);
    ...
}

On SMP systems, could concurrent masking or unmasking of different interrupts
on different CPUs race and silently clobber the mask state?

>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-qe-pic-gpios-v1-0-6c3e706e27dc@bootlin.com?part=11

Re: [PATCH 12/12] soc: fsl: qe: Add support of IRQs in QE GPIO

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The device node reference returned by `of_irq_parse_raw()` is leaked in both the error and success paths of `qe_gpio_to_irq()`.
--

commit 2a534a80a5cfc86c1f2f49ad1709de185cee4a4b
Author: Paul Louvel <paul.louvel@bootlin.com>

soc: fsl: qe: Add support of IRQs in QE GPIO

This commit adds interrupt support to the QE GPIO controller driver by
implementing the to_irq() callback. It uses the interrupt-map nexus node
to map GPIO lines supporting IRQs to the parent QE PIC interrupt domain.

> diff --git a/drivers/soc/fsl/qe/gpio.c b/drivers/soc/fsl/qe/gpio.c
> index 66828f2a35774..f8919642f40df 100644
> --- a/drivers/soc/fsl/qe/gpio.c
> +++ b/drivers/soc/fsl/qe/gpio.c

[ ... ]

> @@ -135,6 +137,29 @@ static int qe_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
>  	return 0;
>  }
>  
> +static int qe_gpio_to_irq(struct gpio_chip *gc, unsigned int gpio)
> +{
> +	struct qe_gpio_chip *qe_gc = gpiochip_get_data(gc);
> +	struct of_phandle_args oirq;
> +	struct irq_domain *domain;
> +	int ret;
> +
> +	oirq.np = qe_gc->np;
> +	oirq.args_count = 2;
> +	oirq.args[0] = gpio;
> +	oirq.args[1] = 0;
> +
> +	ret = of_irq_parse_raw(NULL, &oirq);
> +	if (ret)
> +		return ret;
> +
> +	domain = irq_find_host(oirq.np);
> +	if (!domain)
> +		return -EPROBE_DEFER;
> +
> +	return irq_create_of_mapping(&oirq);
> +}

[Severity: Medium]
Does this leak the device node reference stored in oirq.np?

When of_irq_parse_raw() successfully parses the interrupt, it increments the
reference count of the returned device node in oirq.np. It looks like
of_node_put(oirq.np) needs to be called before returning to avoid leaking the
reference on both the -EPROBE_DEFER error path and the success path.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-qe-pic-gpios-v1-0-6c3e706e27dc@bootlin.com?part=12

Re: [PATCH 01/12] soc: fsl: qe: Add chained_irq_{enter,exit}() calls in cascade handler

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Unrecoverable infinite IRQ storm and spurious processing due to unmasked and unmapped interrupts.
- [High] Race condition in shared interrupt controller registers due to missing spinlock protection.
- [High] Missing synchronize_irq() after clearing the chained handler allows use-after-free of the qepic_data structure.
--

commit c013ba9109c1d81cc36eb17c872ac35aa3b57e9f
Author: Paul Louvel <paul.louvel@bootlin.com>

soc: fsl: qe: Add chained_irq_{enter,exit}() calls in cascade handler

This commit wraps the cascade handler body with chained_irq_enter() and
chained_irq_exit(). This properly informs the parent IRQ chip that a chained
interrupt is being serviced.

> diff --git a/drivers/soc/fsl/qe/qe_ports_ic.c b/drivers/soc/fsl/qe/qe_ports_ic.c
> index 9b0bba64e91e4..73a77763210d7 100644
> --- a/drivers/soc/fsl/qe/qe_ports_ic.c
> +++ b/drivers/soc/fsl/qe/qe_ports_ic.c
[ ... ]
> @@ -79,7 +80,13 @@ static int qepic_get_irq(struct irq_desc *desc)
>  
>  static void qepic_cascade(struct irq_desc *desc)
>  {
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +
> +	chained_irq_enter(chip, desc);
> +
>  	generic_handle_irq(qepic_get_irq(desc));
[Severity: High]
This isn't a bug introduced by this patch, but can this lead to an unrecoverable
infinite IRQ storm?

In qepic_get_irq(), the code reads the raw CEPIER event register without masking
it against the CEPIMR mask register:

static int qepic_get_irq(struct irq_desc *desc)
{
	struct qepic_data *data = irq_desc_get_handler_data(desc);
	u32 event = in_be32(data->reg + CEPIER);

	if (!event)
		return -1;

	return irq_find_mapping(data->host, 32 - ffs(event));
}

If an unmapped interrupt fires (e.g., left unmasked by the bootloader),
irq_find_mapping() returns 0. Then, generic_handle_irq(0) is called, which
typically returns -EINVAL without clearing the hardware event bit in CEPIER.

When chained_irq_exit() unmasks the parent, will the uncleared event immediately
re-trigger the parent interrupt and cause a storm?

> +
> +	chained_irq_exit(chip, desc);
>  }

[Severity: High]
This is a pre-existing issue, but is there a race condition in the shared
interrupt controller registers due to a missing spinlock?

The mask and unmask functions modify the shared CEPIMR register using non-atomic
macros:

static void qepic_mask(struct irq_data *d)
{
	struct qepic_data *data = irq_data_get_irq_chip_data(d);

	clrbits32(data->reg + CEPIMR, 1 << (31 - irqd_to_hwirq(d)));
}

Since the Linux IRQ core invokes these callbacks holding only the per-child
desc->lock (and not a chip-level lock), concurrent mask or unmask operations on
different child IRQs belonging to the same controller could race on SMP systems.

Could this lead to corrupted mask or type registers, resulting in lost or
stuck interrupts?

[Severity: High]
This isn't a bug introduced by this patch, but does the devres cleanup path risk
a use-after-free by missing synchronize_irq()?

During driver removal or probe failure, qepic_remove() is called:

static void qepic_remove(void *res)
{
	struct qepic_data *data = res;

	irq_set_chained_handler_and_data(data->irq, NULL, NULL);
	irq_domain_remove(data->host);
}

This removes the chained handler, but fails to call synchronize_irq() to ensure
concurrent hardware interrupt execution has terminated. Immediately after this
returns, the devres framework will free the qepic_data structure.

If qepic_cascade() is concurrently executing on another CPU, won't it dereference
the freed data pointer, causing a use-after-free crash?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-qe-pic-gpios-v1-0-6c3e706e27dc@bootlin.com?part=1

Re: [PATCH 08/12] soc: fsl: qe: Convert to generic IRQ chip

[Paul Louvel]

On Fri Jul 3, 2026 at 3:45 PM CEST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>
> New issues:
> - [High] Incorrect hwirq calculation in `qepic_cascade()` maps bit indices [0..31] to hwirqs [32..1] instead of [31..0], causing out-of-bounds mapping and shifting all IRQs by one.
> - [High] Race condition on probe: The chained interrupt handler is registered before `data->gc` is initialized, causing a NULL pointer dereference if an interrupt fires early.
>
> Pre-existing issues:
> - [High] The IRQ mask, unmask, and set_type callbacks use lockless read-modify-write operations on shared registers, leading to data races.
> --
>
> commit 39268a559b43e95ee6bcd43807086719d4997f6e
> Author: Paul Louvel <paul.louvel@bootlin.com>
>
> soc: fsl: qe: Convert to generic IRQ chip
>
> The generic IRQ chip framework is available to handle IRQ chips. Using
> this framework for the QE interrupt controller allows to simplify the
> driver. Indeed, the framework internally handles operations coded
> directly in the driver.
>
>> diff --git a/drivers/soc/fsl/qe/qe_ports_ic.c b/drivers/soc/fsl/qe/qe_ports_ic.c
>> index c8b73b0aa2334..d022aa224f6d6 100644
>> --- a/drivers/soc/fsl/qe/qe_ports_ic.c
>> +++ b/drivers/soc/fsl/qe/qe_ports_ic.c
>> @@ -20,63 +20,65 @@ struct qepic_data {
>>  	void __iomem *reg;
>>  	struct irq_domain *host;
>>  	int irq;
>> +	struct irq_chip_generic *gc;
>>  };
>>  
>>  static void qepic_mask(struct irq_data *d)
>>  {
>> -	struct qepic_data *data = irq_data_get_irq_chip_data(d);
>> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>> +	struct irq_chip_type *ct = irq_data_get_chip_type(d);
>>  
>> -	clrbits32(data->reg + CEPIMR, 1 << (31 - irqd_to_hwirq(d)));
>> +	clrbits32(gc->reg_base + ct->regs.mask, d->mask);
>>  }
>
> [Severity: High]
> This is a pre-existing issue, but does this lockless read-modify-write
> operation cause data races?
>
> The qepic_mask(), qepic_unmask(), and qepic_set_type() callbacks use
> clrbits32() and setbits32() on shared registers without a shared controller
> lock. If multiple CPUs modify different IRQs concurrently, could updates to
> the shared mask or type registers be lost?
>
> [ ... ]
>
>>  static void qepic_cascade(struct irq_desc *desc)
>>  {
>>  	struct qepic_data *data = irq_desc_get_handler_data(desc);
>> +	struct irq_chip_type *ct = data->gc->chip_types;
>>  	struct irq_chip *chip = irq_desc_get_chip(desc);
>>  	unsigned long event, bit;
>>  
>>  	chained_irq_enter(chip, desc);
>>  
>> -	event = in_be32(data->reg + CEPIER);
>> +	event = in_be32(data->gc->reg_base + ct->regs.eoi);
>>  	if (!event) {
>>  		handle_bad_irq(desc);
>>  		goto out;
>>  	}
>
> [Severity: High]
> Could the hwirq calculation in qepic_cascade() be incorrect?
>
> When an interrupt fires, the trailing loop (using for_each_set_bit) yields a
> 0-based bit index, but generic_handle_domain_irq() evaluates the hwirq using
> "32 - bit".
>
> For example, if bit 0 is set, it maps to hwirq 32, which exceeds the domain's
> hwirq_max of 32 and leaves all IRQs shifted out of bounds. Does this need to
> be "31 - bit" to correctly map bit indices [0..31] to hwirqs [31..0]?
>
> [ ... ]
>

Valid point.

>> @@ -125,14 +158,16 @@ static int qepic_probe(struct platform_device *pdev)
>>  	if (data->irq < 0)
>>  		return data->irq;
>>  
>> -	data->host = irq_domain_create_linear(dev_fwnode(dev), 32, &qepic_host_ops, data);
>> -	if (!data->host)
>> -		return -ENODEV;
>> +	data->host = devm_irq_domain_instantiate(dev, &d_info);
>> +	if (IS_ERR(data->host))
>> +		return PTR_ERR(data->host);
>>  
>> -	irq_set_chained_handler_and_data(data->irq, qepic_cascade, data);
>> -
>> -	return devm_add_action_or_reset(dev, qepic_remove, data);
>> +	data->gc = irq_get_domain_generic_chip(data->host, 0);
>> +	if (!data->gc)
>> +		return -ENODEV;
>> +	data->gc->reg_base = data->reg;
>>  
>> +	return 0;
>>  }
>
> [Severity: High]
> Can this probe sequence trigger a NULL pointer dereference?
>
> When devm_irq_domain_instantiate() is called, it triggers the init callback
> qepic_domain_init(), which registers the chained handler and unmasks the
> parent interrupt:
>
> qepic_domain_init()
>     irq_set_chained_handler_and_data(data->parent_irq, qepic_cascade, data);
>
> If an interrupt is pending and fires immediately, qepic_cascade() will
> execute and attempt to dereference data->gc->reg_base. However, data->gc is
> not initialized until irq_get_domain_generic_chip() is called after the
> domain instantiation returns.
>
> Could this cause a crash if a pending interrupt is triggered during probe?

Valid point.




-- 
Paul Louvel, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com