[PATCH] gpio: mvebu: fix irq domain leak

[PATCH] gpio: mvebu: fix irq domain leak

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

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 <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpio-mvebu.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index a35958e7adf6..67497116ce27 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -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 = {
-- 
2.39.2

Re: [PATCH] gpio: mvebu: fix irq domain leak

[Andy Shevchenko]

On Wed, Jul 19, 2023 at 2:41 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> 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 <andy.shevchenko@gmail.com>

> Fixes: 812d47889a8e ("gpio/mvebu: Use irq_domain_add_linear")
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  drivers/gpio/gpio-mvebu.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
> index a35958e7adf6..67497116ce27 100644
> --- a/drivers/gpio/gpio-mvebu.c
> +++ b/drivers/gpio/gpio-mvebu.c
> @@ -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);
}

> +}
> +
>  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;
>  }


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpio: mvebu: fix irq domain leak

[Bartosz Golaszewski]

On Wed, Jul 19, 2023 at 3:03 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Jul 19, 2023 at 2:41 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > 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 <andy.shevchenko@gmail.com>
>
> > Fixes: 812d47889a8e ("gpio/mvebu: Use irq_domain_add_linear")
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> >  drivers/gpio/gpio-mvebu.c | 18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
> > index a35958e7adf6..67497116ce27 100644
> > --- a/drivers/gpio/gpio-mvebu.c
> > +++ b/drivers/gpio/gpio-mvebu.c
> > @@ -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
>

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

Bart

> static void mvebu_gpio_remove_irq_domain(void *domain)
> {
>       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;
> >  }
>
>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH] gpio: mvebu: fix irq domain leak

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1849 bytes --]

On Wed, Jul 19, 2023 at 04:02:39PM +0300, Andy Shevchenko wrote:
> On Wed, Jul 19, 2023 at 2:41 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > 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 <andy.shevchenko@gmail.com>
> 
> > Fixes: 812d47889a8e ("gpio/mvebu: Use irq_domain_add_linear")
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> >  drivers/gpio/gpio-mvebu.c | 18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
> > index a35958e7adf6..67497116ce27 100644
> > --- a/drivers/gpio/gpio-mvebu.c
> > +++ b/drivers/gpio/gpio-mvebu.c
> > @@ -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);
> }

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

Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

for whatever variant you pick.

Best regards and thanks for acting on my report,
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]