Message ID | 20230719114101.55051-1-brgl@bgdev.pl |
---|---|
State | Handled Elsewhere |
Headers | show |
Series | gpio: mvebu: fix irq domain leak | expand |
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; > }
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
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
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 = {