gpio: mvebu: fix devres LIFO ordering between GPIO chip and IRQ domain

PATCH Superseded Rosen Penev 3 participants 5 messages Jul 6 – Jul 12 GPIO

Share & tools

Rosen Penev

author

2026-07-06 23:22 UTC #0 lore raw

PATCH v2 1/1 gpio: mvebu: fix devres LIFO ordering between GPIO chip and IRQ domain

During driver removal, devres cleans up in LIFO order. The IRQ domain was created and its devm cleanup action registered after devm_gpiochip_add_data(), so the domain was destroyed before the GPIO chip was deregistered. If gpiod_to_irq() is called on a pin during this window, mvebu_gpio_to_irq() passes the freed mvchip->domain to irq_create_mapping().

Fix by moving the IRQ domain creation, devm cleanup action registration, generic chip allocation, and chip type setup before devm_gpiochip_add_data(). This ensures the GPIO chip is torn down first (preventing new IRQ mappings), then the IRQ domain is removed, and finally mvchip is freed.

Fixes 644ee70267a9 ("gpio: mvebu: fix irq domain leak")

Assisted-by opencode:big-pickle

Signed-off-by Rosen Penev <[email protected]>

---
v2: added Fixes tag

 drivers/gpio/gpio-mvebu.c | 133 +++++++++++++++++++-------------------
 1 file changed, 68 insertions(+), 65 deletions(-)

drivers/gpio/gpio-mvebu.c +68 ~~-65~~

index 5b4408fcc10b..d8105a05d261 100644
@@ -1250,6 +1250,59 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
 		BUG();
 	}
 
+	/* Some gpio controllers do not provide irq support */
+	if (have_irqs) {
+		mvchip->domain = irq_domain_create_linear(dev_fwnode(&pdev->dev),
+							  ngpios,
+							  &irq_generic_chip_ops,
+							  NULL);
+		if (!mvchip->domain)
+			return dev_err_probe(&pdev->dev, -ENODEV,
+					     "couldn't allocate irq domain %s (DT).\n",
+					     mvchip->chip.label);
+
+		err = devm_add_action_or_reset(&pdev->dev,
+					       mvebu_gpio_remove_irq_domain,
+					       mvchip->domain);
+		if (err)
+			return err;
+
+		err = irq_alloc_domain_generic_chips(
+		    mvchip->domain, ngpios, 2, np->name, handle_level_irq,
+		    IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_LEVEL, 0,
+		    IRQ_GC_INIT_NESTED_LOCK);
+		if (err)
+			return dev_err_probe(&pdev->dev, err,
+					     "couldn't allocate irq chips %s (DT).\n",
+					     mvchip->chip.label);
+
+		/*
+		 * NOTE: The common accessors cannot be used because of the
+		 * percpu access to the mask registers
+		 */
+		gc = irq_get_domain_generic_chip(mvchip->domain, 0);
+		gc->private = mvchip;
+		ct = &gc->chip_types[0];
+		ct->type = IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW;
+		ct->chip.irq_mask = mvebu_gpio_level_irq_mask;
+		ct->chip.irq_unmask = mvebu_gpio_level_irq_unmask;
+		ct->chip.irq_set_type = mvebu_gpio_irq_set_type;
+		ct->chip.irq_set_wake = mvebu_gpio_set_wake_irq;
+		ct->chip.flags = IRQCHIP_SET_TYPE_MASKED | IRQCHIP_MASK_ON_SUSPEND;
+		ct->chip.name = mvchip->chip.label;
+
+		ct = &gc->chip_types[1];
+		ct->type = IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING;
+		ct->chip.irq_ack = mvebu_gpio_irq_ack;
+		ct->chip.irq_mask = mvebu_gpio_edge_irq_mask;
+		ct->chip.irq_unmask = mvebu_gpio_edge_irq_unmask;
+		ct->chip.irq_set_type = mvebu_gpio_irq_set_type;
+		ct->chip.irq_set_wake = mvebu_gpio_set_wake_irq;
+		ct->chip.flags = IRQCHIP_SET_TYPE_MASKED | IRQCHIP_MASK_ON_SUSPEND;
+		ct->handler = handle_edge_irq;
+		ct->chip.name = mvchip->chip.label;
+	}
+
 	err = devm_gpiochip_add_data(&pdev->dev, &mvchip->chip, mvchip);
 	if (err)
 		return dev_err_probe(&pdev->dev, err,

@@ -1262,71 +1315,21 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
 			return err;
 	}
 
-	/* Some gpio controllers do not provide irq support */
-	if (!have_irqs)
-		return 0;
-
-	mvchip->domain = irq_domain_create_linear(dev_fwnode(&pdev->dev), ngpios,
-						  &irq_generic_chip_ops, NULL);
-	if (!mvchip->domain) {
-		dev_err(&pdev->dev, "couldn't allocate irq domain %s (DT).\n",
-			mvchip->chip.label);
-		return -ENODEV;
-	}
-
-	err = devm_add_action_or_reset(&pdev->dev, mvebu_gpio_remove_irq_domain,
-				       mvchip->domain);
-	if (err)
-		return err;
-
-	err = irq_alloc_domain_generic_chips(
-	    mvchip->domain, ngpios, 2, np->name, handle_level_irq,
-	    IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_LEVEL, 0, IRQ_GC_INIT_NESTED_LOCK);
-	if (err) {
-		dev_err(&pdev->dev, "couldn't allocate irq chips %s (DT).\n",
-			mvchip->chip.label);
-		return err;
-	}
-
-	/*
-	 * NOTE: The common accessors cannot be used because of the percpu
-	 * access to the mask registers
-	 */
-	gc = irq_get_domain_generic_chip(mvchip->domain, 0);
-	gc->private = mvchip;
-	ct = &gc->chip_types[0];
-	ct->type = IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW;
-	ct->chip.irq_mask = mvebu_gpio_level_irq_mask;
-	ct->chip.irq_unmask = mvebu_gpio_level_irq_unmask;
-	ct->chip.irq_set_type = mvebu_gpio_irq_set_type;
-	ct->chip.irq_set_wake = mvebu_gpio_set_wake_irq;
-	ct->chip.flags = IRQCHIP_SET_TYPE_MASKED | IRQCHIP_MASK_ON_SUSPEND;
-	ct->chip.name = mvchip->chip.label;
-
-	ct = &gc->chip_types[1];
-	ct->type = IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING;
-	ct->chip.irq_ack = mvebu_gpio_irq_ack;
-	ct->chip.irq_mask = mvebu_gpio_edge_irq_mask;
-	ct->chip.irq_unmask = mvebu_gpio_edge_irq_unmask;
-	ct->chip.irq_set_type = mvebu_gpio_irq_set_type;
-	ct->chip.irq_set_wake = mvebu_gpio_set_wake_irq;
-	ct->chip.flags = IRQCHIP_SET_TYPE_MASKED | IRQCHIP_MASK_ON_SUSPEND;
-	ct->handler = handle_edge_irq;
-	ct->chip.name = mvchip->chip.label;
-
-	/*
-	 * Setup the interrupt handlers. Each chip can have up to 4
-	 * interrupt handlers, with each handler dealing with 8 GPIO
-	 * pins.
-	 */
-	for (i = 0; i < ARRAY_SIZE(mvchip->bank_irq); i++) {
-		int irq = platform_get_irq_optional(pdev, i);
-
-		if (irq < 0)
-			continue;
-		irq_set_chained_handler_and_data(irq, mvebu_gpio_irq_handler,
-						 mvchip);
-		mvchip->bank_irq[i] = irq;
+	if (have_irqs) {
+		/*
+		 * Setup the interrupt handlers. Each chip can have up to 4
+		 * interrupt handlers, with each handler dealing with 8 GPIO
+		 * pins.
+		 */
+		for (i = 0; i < ARRAY_SIZE(mvchip->bank_irq); i++) {
+			int irq = platform_get_irq_optional(pdev, i);
+
+			if (irq < 0)
+				continue;
+			irq_set_chained_handler_and_data(irq,
+					mvebu_gpio_irq_handler, mvchip);
+			mvchip->bank_irq[i] = irq;
+		}
 	}
 
 	return 0;

Signature · 1 line

2.55.0

Uwe Kleine-König

2026-07-07 07:13 UTC #1 lore raw

Quoted 6 lines · Rosen Penev

During driver removal, devres cleans up in LIFO order. The IRQ domain was created and its devm cleanup action registered after devm_gpiochip_add_data(), so the domain was destroyed before the GPIO chip was deregistered. If gpiod_to_irq() is called on a pin during this window, mvebu_gpio_to_irq() passes the freed mvchip->domain to irq_create_mapping().

Without having looked at your patch, I would expect that there is a similar window during probe where calling gpiod_to_irq() between devm_gpiochip_add_data() and IRQ domain creation results in an invalid mvchip->domain being passed to irq_create_mapping(), right?

Quoted 7 lines

Fix by moving the IRQ domain creation, devm cleanup action registration, generic chip allocation, and chip type setup before devm_gpiochip_add_data(). This ensures the GPIO chip is torn down first (preventing new IRQ mappings), then the IRQ domain is removed, and finally mvchip is freed.
Fixes 644ee70267a9 ("gpio: mvebu: fix irq domain leak")

If the above is right, the referenced commit only creates the issue at driver detach. The issue at probe time exists since fefe7b092345 ("gpio: introduce gpio-mvebu driver for Marvell SoCs") which has gpiochip_add(&mvchip->chip); before coping for irqs.

Best regards
Uwe

Rosen Penev

author

2026-07-08 00:18 UTC #2 lore raw

Quoted 9 lines · Uwe Kleine-König

Quoted 6 lines · Rosen Penev
During driver removal, devres cleans up in LIFO order. The IRQ domain was created and its devm cleanup action registered after devm_gpiochip_add_data(), so the domain was destroyed before the GPIO chip was deregistered. If gpiod_to_irq() is called on a pin during this window, mvebu_gpio_to_irq() passes the freed mvchip->domain to irq_create_mapping().

Quoted 4 lines

Without having looked at your patch, I would expect that there is a similar window during probe where calling gpiod_to_irq() between devm_gpiochip_add_data() and IRQ domain creation results in an invalid mvchip->domain being passed to irq_create_mapping(), right?

This patch fixes that.

Quoted 7 lines

Quoted 7 lines
Fix by moving the IRQ domain creation, devm cleanup action registration, generic chip allocation, and chip type setup before devm_gpiochip_add_data(). This ensures the GPIO chip is torn down first (preventing new IRQ mappings), then the IRQ domain is removed, and finally mvchip is freed.

Quoted 6 lines

Quoted 1 line
Fixes 644ee70267a9 ("gpio: mvebu: fix irq domain leak")
If the above is right, the referenced commit only creates the issue at driver detach. The issue at probe time exists since fefe7b092345 ("gpio: introduce gpio-mvebu driver for Marvell SoCs") which has gpiochip_add(&mvchip->chip); before coping for irqs.

Will fix.

Quoted 3 lines

Best regards
Uwe

Linus Walleij

maintainer

2026-07-10 19:33 UTC #3 lore raw

In reply to Rosen Penev

Quoted 13 lines · Rosen Penev

During driver removal, devres cleans up in LIFO order. The IRQ domain was created and its devm cleanup action registered after devm_gpiochip_add_data(), so the domain was destroyed before the GPIO chip was deregistered. If gpiod_to_irq() is called on a pin during this window, mvebu_gpio_to_irq() passes the freed mvchip->domain to irq_create_mapping().
Fix by moving the IRQ domain creation, devm cleanup action registration, generic chip allocation, and chip type setup before devm_gpiochip_add_data(). This ensures the GPIO chip is torn down first (preventing new IRQ mappings), then the IRQ domain is removed, and finally mvchip is freed.

Quoted 3 lines

Fixes 644ee70267a9 ("gpio: mvebu: fix irq domain leak")
Assisted-by opencode:big-pickle
Signed-off-by Rosen Penev <[email protected]>

Nice work on this driver!

Despite it's a pre-existing issue, if you have time (and already have the hardware set up and all) can you look into converting this driver to IRQCHIP_IMMUTABLE?

Yours,
Linus Walleij

Rosen Penev

author

2026-07-12 21:22 UTC #4 lore raw

Quoted 20 lines · Linus Walleij

Quoted 13 lines · Rosen Penev
During driver removal, devres cleans up in LIFO order. The IRQ domain was created and its devm cleanup action registered after devm_gpiochip_add_data(), so the domain was destroyed before the GPIO chip was deregistered. If gpiod_to_irq() is called on a pin during this window, mvebu_gpio_to_irq() passes the freed mvchip->domain to irq_create_mapping().
Fix by moving the IRQ domain creation, devm cleanup action registration, generic chip allocation, and chip type setup before devm_gpiochip_add_data(). This ensures the GPIO chip is torn down first (preventing new IRQ mappings), then the IRQ domain is removed, and finally mvchip is freed.
Quoted 3 lines
Fixes 644ee70267a9 ("gpio: mvebu: fix irq domain leak")
Assisted-by opencode:big-pickle
Signed-off-by Rosen Penev <[email protected]>

Quoted 5 lines

Nice work on this driver!
Despite it's a pre-existing issue, if you have time (and already have the hardware set up and all) can you look into converting this driver to IRQCHIP_IMMUTABLE?

Not a simple task looks like. This thing generated

 1 file changed, 105 insertions(+), 149 deletions(-)

which means heavy testing is needed. There's also a huge chance of regressions as the driver touches multiple pieces of hardware. I have 388 and 3720 that can be tested.

Quoted 3 lines

Yours,
Linus Walleij