Skip to contentSkip to conversation

gpio: mvebu: fix irq domain leak

PATCH In review Bartosz Golaszewski 3 participants 4 messages Jul 19, 2023

Subsystems (2)
GPIO PWM
lore
Share & tools
lore

Bartosz Golaszewski

author
#0 lore raw

PATCH 1/1 gpio: mvebu: fix irq domain leak

From: Bartosz Golaszewski <[email protected]>

Uwe Kleine-König pointed out we still have one resource leak in the mvebu driver triggered on driver detach. Let's address it with a custom devm action.

Fixes 812d47889a8e ("gpio/mvebu: Use irq_domain_add_linear")
Signed-off-by Bartosz Golaszewski <[email protected]>
---
 drivers/gpio/gpio-mvebu.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)
drivers/gpio/gpio-mvebu.c +13 -5
index a35958e7adf6..67497116ce27 100644
@@ -1112,6 +1112,13 @@ static int mvebu_gpio_probe_syscon(struct platform_device *pdev,
 	return 0;
 }
 
+static void mvebu_gpio_remove_irq_domain(void *data)
+{
+	struct irq_domain *domain = data;
+
+	irq_domain_remove(domain);
+}
+
 static int mvebu_gpio_probe(struct platform_device *pdev)
 {
 	struct mvebu_gpio_chip *mvchip;

@@ -1246,13 +1253,18 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
 		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, 0);
 	if (err) {
 		dev_err(&pdev->dev, "couldn't allocate irq chips %s (DT).\n",
 			mvchip->chip.label);
-		goto err_domain;
+		return err;
 	}
 
 	/*

@@ -1292,10 +1304,6 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
 	}
 
 	return 0;
-
-err_domain:
-	irq_domain_remove(mvchip->domain);
-	return err;
 }
 
 static struct platform_driver mvebu_gpio_driver = {
Signature · 1 line
2.39.2
Reviewed-by on v1 1/1
#1 lore raw
Quoted 6 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Uwe Kleine-König pointed out we still have one resource leak in the mvebu driver triggered on driver detach. Let's address it with a custom devm action.

One nit-pick below, in either case

Reviewed-by Andy Shevchenko <[email protected]>
Quoted 11 lines
Fixes 812d47889a8e ("gpio/mvebu: Use irq_domain_add_linear")
Signed-off-by Bartosz Golaszewski <[email protected]>
---
 drivers/gpio/gpio-mvebu.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)
drivers/gpio/gpio-mvebu.c +0 -0
index a35958e7adf6..67497116ce27 100644

drivers/gpio/gpio-mvebu.c static int mvebu_gpio_probe_syscon(struct platform_device *pdev, · line 1119

Quoted 8 lines
drivers/gpio/gpio-mvebu.c +5 -0
@@ -1112,6 +1112,13 @@ static int mvebu_gpio_probe_syscon(struct platform_device *pdev,
        return 0;
 }
 
+static void mvebu_gpio_remove_irq_domain(void *data)
+{
+       struct irq_domain *domain = data;
+
+       irq_domain_remove(domain);

The from/to void * doesn't need an explicit casting in C. This can be a one liner

static void mvebu_gpio_remove_irq_domain(void *domain)

{
      irq_domain_remove(domain);
}
Quoted 34 lines
drivers/gpio/gpio-mvebu.c +8 -5
+}
+
 static int mvebu_gpio_probe(struct platform_device *pdev)
 {
        struct mvebu_gpio_chip *mvchip;

@@ -1246,13 +1253,18 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
                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, 0);
        if (err) {
                dev_err(&pdev->dev, "couldn't allocate irq chips %s (DT).\n",
                        mvchip->chip.label);
-               goto err_domain;
+               return err;
        }
 
        /*

@@ -1292,10 +1304,6 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
        }
 
        return 0;
-
-err_domain:
-       irq_domain_remove(mvchip->domain);
-       return err;
 }
Signature · 2 lines
With Best Regards,
Andy Shevchenko
author
#2 lore raw
Quoted 26 lines · Andy Shevchenko
Quoted 6 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Uwe Kleine-König pointed out we still have one resource leak in the mvebu driver triggered on driver detach. Let's address it with a custom devm action.

One nit-pick below, in either case

Reviewed-by Andy Shevchenko <[email protected]>
Quoted 14 lines
Fixes 812d47889a8e ("gpio/mvebu: Use irq_domain_add_linear")
Signed-off-by Bartosz Golaszewski <[email protected]>
---
 drivers/gpio/gpio-mvebu.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)
drivers/gpio/gpio-mvebu.c +0 -0
index a35958e7adf6..67497116ce27 100644
@@ -1112,6 +1112,13 @@ static int mvebu_gpio_probe_syscon(struct platform_device *pdev,
        return 0;
 }
Quoted 9 lines

drivers/gpio/gpio-mvebu.c static int mvebu_gpio_probe_syscon(struct platform_device *pdev, · line 1119

Quoted 5 lines
drivers/gpio/gpio-mvebu.c +5 -0
+static void mvebu_gpio_remove_irq_domain(void *data)
+{
+       struct irq_domain *domain = data;
+
+       irq_domain_remove(domain);

The from/to void * doesn't need an explicit casting in C. This can be a one liner

I know but I prioritise readability over brevity. I prefer this version.

Bart

Quoted 44 lines

static void mvebu_gpio_remove_irq_domain(void *domain)

{
      irq_domain_remove(domain);
}
Quoted 26 lines
drivers/gpio/gpio-mvebu.c +8 -1
+}
+
 static int mvebu_gpio_probe(struct platform_device *pdev)
 {
        struct mvebu_gpio_chip *mvchip;

@@ -1246,13 +1253,18 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
                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, 0);
        if (err) {
                dev_err(&pdev->dev, "couldn't allocate irq chips %s (DT).\n",
                        mvchip->chip.label);
-               goto err_domain;
+               return err;
        }
 
        /*

drivers/gpio/gpio-mvebu.c static int mvebu_gpio_probe(struct platform_device *pdev) · line 1307

Quoted 8 lines
drivers/gpio/gpio-mvebu.c +0 -4
@@ -1292,10 +1304,6 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
        }
 
        return 0;
-
-err_domain:
-       irq_domain_remove(mvchip->domain);
-       return err;
 }

--
With Best Regards,
Andy Shevchenko

Reviewed-by
#3 lore raw
In reply to Andy Shevchenko
Quoted 34 lines · Andy Shevchenko
Quoted 6 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

Uwe Kleine-König pointed out we still have one resource leak in the mvebu driver triggered on driver detach. Let's address it with a custom devm action.

One nit-pick below, in either case

Reviewed-by Andy Shevchenko <[email protected]>
Quoted 11 lines
Fixes 812d47889a8e ("gpio/mvebu: Use irq_domain_add_linear")
Signed-off-by Bartosz Golaszewski <[email protected]>
---
 drivers/gpio/gpio-mvebu.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)
drivers/gpio/gpio-mvebu.c +0 -0
index a35958e7adf6..67497116ce27 100644

drivers/gpio/gpio-mvebu.c static int mvebu_gpio_probe_syscon(struct platform_device *pdev, · line 1119

Quoted 8 lines
drivers/gpio/gpio-mvebu.c +5 -0
@@ -1112,6 +1112,13 @@ static int mvebu_gpio_probe_syscon(struct platform_device *pdev,
        return 0;
 }
 
+static void mvebu_gpio_remove_irq_domain(void *data)
+{
+       struct irq_domain *domain = data;
+
+       irq_domain_remove(domain);

The from/to void * doesn't need an explicit casting in C. This can be a one liner

Quoted 4 lines

static void mvebu_gpio_remove_irq_domain(void *domain)

{
      irq_domain_remove(domain);
}

I slightly prefer Bartosz's version, but wouldn't get sleepless nights from that one.

Reviewed-by Uwe Kleine-König <[email protected]>

for whatever variant you pick.

Best regards and thanks for acting on my report,
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |