* [PATCH 1/2] gpio: em: remove the gpiochip before removing the irq domain @ 2019-07-10 9:08 Bartosz Golaszewski 2019-07-10 9:08 ` [PATCH 2/2] gpio: em: use a helper variable for &pdev->dev Bartosz Golaszewski 2019-07-10 9:37 ` [PATCH 1/2] gpio: em: remove the gpiochip before removing the irq domain Phil Reid 0 siblings, 2 replies; 6+ messages in thread From: Bartosz Golaszewski @ 2019-07-10 9:08 UTC (permalink / raw) To: Linus Walleij, Geert Uytterhoeven Cc: linux-gpio, linux-kernel, Bartosz Golaszewski, stable From: Bartosz Golaszewski <bgolaszewski@baylibre.com> In commit 8764c4ca5049 ("gpio: em: use the managed version of gpiochip_add_data()") we implicitly altered the ordering of resource freeing: since gpiochip_remove() calls gpiochip_irqchip_remove() internally, we now can potentially use the irq_domain after it was destroyed in the remove() callback (as devm resources are freed after remove() has returned). Use devm_add_action() to keep the ordering right and entirely kill the remove() callback in the driver. Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> Fixes: 8764c4ca5049 ("gpio: em: use the managed version of gpiochip_add_data()") Cc: stable@vger.kernel.org Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> --- drivers/gpio/gpio-em.c | 35 +++++++++++++++++------------------ 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/drivers/gpio/gpio-em.c b/drivers/gpio/gpio-em.c index b6af705a4e5f..c88028ac66f2 100644 --- a/drivers/gpio/gpio-em.c +++ b/drivers/gpio/gpio-em.c @@ -259,6 +259,13 @@ static const struct irq_domain_ops em_gio_irq_domain_ops = { .xlate = irq_domain_xlate_twocell, }; +static void em_gio_irq_domain_remove(void *data) +{ + struct irq_domain *domain = data; + + irq_domain_remove(domain); +} + static int em_gio_probe(struct platform_device *pdev) { struct em_gio_priv *p; @@ -333,39 +340,32 @@ static int em_gio_probe(struct platform_device *pdev) return -ENXIO; } + ret = devm_add_action(&pdev->dev, + em_gio_irq_domain_remove, p->irq_domain); + if (ret) { + irq_domain_remove(p->irq_domain); + return ret; + } + if (devm_request_irq(&pdev->dev, irq[0]->start, em_gio_irq_handler, 0, name, p)) { dev_err(&pdev->dev, "failed to request low IRQ\n"); - ret = -ENOENT; - goto err1; + return -ENOENT; } if (devm_request_irq(&pdev->dev, irq[1]->start, em_gio_irq_handler, 0, name, p)) { dev_err(&pdev->dev, "failed to request high IRQ\n"); - ret = -ENOENT; - goto err1; + return -ENOENT; } ret = devm_gpiochip_add_data(&pdev->dev, gpio_chip, p); if (ret) { dev_err(&pdev->dev, "failed to add GPIO controller\n"); - goto err1; + return ret; } return 0; - -err1: - irq_domain_remove(p->irq_domain); - return ret; -} - -static int em_gio_remove(struct platform_device *pdev) -{ - struct em_gio_priv *p = platform_get_drvdata(pdev); - - irq_domain_remove(p->irq_domain); - return 0; } static const struct of_device_id em_gio_dt_ids[] = { @@ -376,7 +376,6 @@ MODULE_DEVICE_TABLE(of, em_gio_dt_ids); static struct platform_driver em_gio_device_driver = { .probe = em_gio_probe, - .remove = em_gio_remove, .driver = { .name = "em_gio", .of_match_table = em_gio_dt_ids, -- 2.21.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] gpio: em: use a helper variable for &pdev->dev 2019-07-10 9:08 [PATCH 1/2] gpio: em: remove the gpiochip before removing the irq domain Bartosz Golaszewski @ 2019-07-10 9:08 ` Bartosz Golaszewski 2019-07-28 23:10 ` Linus Walleij 2019-07-10 9:37 ` [PATCH 1/2] gpio: em: remove the gpiochip before removing the irq domain Phil Reid 1 sibling, 1 reply; 6+ messages in thread From: Bartosz Golaszewski @ 2019-07-10 9:08 UTC (permalink / raw) To: Linus Walleij, Geert Uytterhoeven Cc: linux-gpio, linux-kernel, Bartosz Golaszewski From: Bartosz Golaszewski <bgolaszewski@baylibre.com> Instead of always dereferencing &pdev->dev, just assign a helper local variable of type struct device * and use it where applicable. Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> --- drivers/gpio/gpio-em.c | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/drivers/gpio/gpio-em.c b/drivers/gpio/gpio-em.c index c88028ac66f2..e3aa6fe3a320 100644 --- a/drivers/gpio/gpio-em.c +++ b/drivers/gpio/gpio-em.c @@ -272,11 +272,12 @@ static int em_gio_probe(struct platform_device *pdev) struct resource *io[2], *irq[2]; struct gpio_chip *gpio_chip; struct irq_chip *irq_chip; - const char *name = dev_name(&pdev->dev); + struct device *dev = &pdev->dev; + const char *name = dev_name(dev); unsigned int ngpios; int ret; - p = devm_kzalloc(&pdev->dev, sizeof(*p), GFP_KERNEL); + p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL); if (!p) return -ENOMEM; @@ -290,27 +291,27 @@ static int em_gio_probe(struct platform_device *pdev) irq[1] = platform_get_resource(pdev, IORESOURCE_IRQ, 1); if (!io[0] || !io[1] || !irq[0] || !irq[1]) { - dev_err(&pdev->dev, "missing IRQ or IOMEM\n"); + dev_err(dev, "missing IRQ or IOMEM\n"); return -EINVAL; } - p->base0 = devm_ioremap_nocache(&pdev->dev, io[0]->start, + p->base0 = devm_ioremap_nocache(dev, io[0]->start, resource_size(io[0])); if (!p->base0) return -ENOMEM; - p->base1 = devm_ioremap_nocache(&pdev->dev, io[1]->start, + p->base1 = devm_ioremap_nocache(dev, io[1]->start, resource_size(io[1])); if (!p->base1) return -ENOMEM; - if (of_property_read_u32(pdev->dev.of_node, "ngpios", &ngpios)) { - dev_err(&pdev->dev, "Missing ngpios OF property\n"); + if (of_property_read_u32(dev->of_node, "ngpios", &ngpios)) { + dev_err(dev, "Missing ngpios OF property\n"); return -EINVAL; } gpio_chip = &p->gpio_chip; - gpio_chip->of_node = pdev->dev.of_node; + gpio_chip->of_node = dev->of_node; gpio_chip->direction_input = em_gio_direction_input; gpio_chip->get = em_gio_get; gpio_chip->direction_output = em_gio_direction_output; @@ -319,7 +320,7 @@ static int em_gio_probe(struct platform_device *pdev) gpio_chip->request = em_gio_request; gpio_chip->free = em_gio_free; gpio_chip->label = name; - gpio_chip->parent = &pdev->dev; + gpio_chip->parent = dev; gpio_chip->owner = THIS_MODULE; gpio_chip->base = -1; gpio_chip->ngpio = ngpios; @@ -333,35 +334,35 @@ static int em_gio_probe(struct platform_device *pdev) irq_chip->irq_release_resources = em_gio_irq_relres; irq_chip->flags = IRQCHIP_SKIP_SET_WAKE | IRQCHIP_MASK_ON_SUSPEND; - p->irq_domain = irq_domain_add_simple(pdev->dev.of_node, ngpios, 0, + p->irq_domain = irq_domain_add_simple(dev->of_node, ngpios, 0, &em_gio_irq_domain_ops, p); if (!p->irq_domain) { - dev_err(&pdev->dev, "cannot initialize irq domain\n"); + dev_err(dev, "cannot initialize irq domain\n"); return -ENXIO; } - ret = devm_add_action(&pdev->dev, + ret = devm_add_action(dev, em_gio_irq_domain_remove, p->irq_domain); if (ret) { irq_domain_remove(p->irq_domain); return ret; } - if (devm_request_irq(&pdev->dev, irq[0]->start, + if (devm_request_irq(dev, irq[0]->start, em_gio_irq_handler, 0, name, p)) { - dev_err(&pdev->dev, "failed to request low IRQ\n"); + dev_err(dev, "failed to request low IRQ\n"); return -ENOENT; } - if (devm_request_irq(&pdev->dev, irq[1]->start, + if (devm_request_irq(dev, irq[1]->start, em_gio_irq_handler, 0, name, p)) { - dev_err(&pdev->dev, "failed to request high IRQ\n"); + dev_err(dev, "failed to request high IRQ\n"); return -ENOENT; } - ret = devm_gpiochip_add_data(&pdev->dev, gpio_chip, p); + ret = devm_gpiochip_add_data(dev, gpio_chip, p); if (ret) { - dev_err(&pdev->dev, "failed to add GPIO controller\n"); + dev_err(dev, "failed to add GPIO controller\n"); return ret; } -- 2.21.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] gpio: em: use a helper variable for &pdev->dev 2019-07-10 9:08 ` [PATCH 2/2] gpio: em: use a helper variable for &pdev->dev Bartosz Golaszewski @ 2019-07-28 23:10 ` Linus Walleij 0 siblings, 0 replies; 6+ messages in thread From: Linus Walleij @ 2019-07-28 23:10 UTC (permalink / raw) To: Bartosz Golaszewski Cc: Geert Uytterhoeven, open list:GPIO SUBSYSTEM, linux-kernel, Bartosz Golaszewski On Wed, Jul 10, 2019 at 11:08 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Instead of always dereferencing &pdev->dev, just assign a helper local > variable of type struct device * and use it where applicable. > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> I expect I get this and other EM patches in a pull request from you. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] gpio: em: remove the gpiochip before removing the irq domain 2019-07-10 9:08 [PATCH 1/2] gpio: em: remove the gpiochip before removing the irq domain Bartosz Golaszewski 2019-07-10 9:08 ` [PATCH 2/2] gpio: em: use a helper variable for &pdev->dev Bartosz Golaszewski @ 2019-07-10 9:37 ` Phil Reid 2019-07-10 9:47 ` Bartosz Golaszewski 2019-07-10 10:13 ` Geert Uytterhoeven 1 sibling, 2 replies; 6+ messages in thread From: Phil Reid @ 2019-07-10 9:37 UTC (permalink / raw) To: Bartosz Golaszewski, Linus Walleij, Geert Uytterhoeven Cc: linux-gpio, linux-kernel, Bartosz Golaszewski, stable G'day Bartosz, One comment below On 10/07/2019 17:08, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > In commit 8764c4ca5049 ("gpio: em: use the managed version of > gpiochip_add_data()") we implicitly altered the ordering of resource > freeing: since gpiochip_remove() calls gpiochip_irqchip_remove() > internally, we now can potentially use the irq_domain after it was > destroyed in the remove() callback (as devm resources are freed after > remove() has returned). > > Use devm_add_action() to keep the ordering right and entirely kill > the remove() callback in the driver. > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > Fixes: 8764c4ca5049 ("gpio: em: use the managed version of gpiochip_add_data()") > Cc: stable@vger.kernel.org > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > --- > drivers/gpio/gpio-em.c | 35 +++++++++++++++++------------------ > 1 file changed, 17 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpio/gpio-em.c b/drivers/gpio/gpio-em.c > index b6af705a4e5f..c88028ac66f2 100644 > --- a/drivers/gpio/gpio-em.c > +++ b/drivers/gpio/gpio-em.c > @@ -259,6 +259,13 @@ static const struct irq_domain_ops em_gio_irq_domain_ops = { > .xlate = irq_domain_xlate_twocell, > }; > > +static void em_gio_irq_domain_remove(void *data) > +{ > + struct irq_domain *domain = data; > + > + irq_domain_remove(domain); > +} > + > static int em_gio_probe(struct platform_device *pdev) > { > struct em_gio_priv *p; > @@ -333,39 +340,32 @@ static int em_gio_probe(struct platform_device *pdev) > return -ENXIO; > } > > + ret = devm_add_action(&pdev->dev, > + em_gio_irq_domain_remove, p->irq_domain); Could devm_add_action_or_reset be used? > + if (ret) { > + irq_domain_remove(p->irq_domain); > + return ret; > + } > + > if (devm_request_irq(&pdev->dev, irq[0]->start, > em_gio_irq_handler, 0, name, p)) { > dev_err(&pdev->dev, "failed to request low IRQ\n"); > - ret = -ENOENT; > - goto err1; > + return -ENOENT; > } > > if (devm_request_irq(&pdev->dev, irq[1]->start, > em_gio_irq_handler, 0, name, p)) { > dev_err(&pdev->dev, "failed to request high IRQ\n"); > - ret = -ENOENT; > - goto err1; > + return -ENOENT; > } > > ret = devm_gpiochip_add_data(&pdev->dev, gpio_chip, p); > if (ret) { > dev_err(&pdev->dev, "failed to add GPIO controller\n"); > - goto err1; > + return ret; > } > > return 0; > - > -err1: > - irq_domain_remove(p->irq_domain); > - return ret; > -} > - > -static int em_gio_remove(struct platform_device *pdev) > -{ > - struct em_gio_priv *p = platform_get_drvdata(pdev); > - > - irq_domain_remove(p->irq_domain); > - return 0; > } > > static const struct of_device_id em_gio_dt_ids[] = { > @@ -376,7 +376,6 @@ MODULE_DEVICE_TABLE(of, em_gio_dt_ids); > > static struct platform_driver em_gio_device_driver = { > .probe = em_gio_probe, > - .remove = em_gio_remove, > .driver = { > .name = "em_gio", > .of_match_table = em_gio_dt_ids, > -- Regards Phil Reid ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] gpio: em: remove the gpiochip before removing the irq domain 2019-07-10 9:37 ` [PATCH 1/2] gpio: em: remove the gpiochip before removing the irq domain Phil Reid @ 2019-07-10 9:47 ` Bartosz Golaszewski 2019-07-10 10:13 ` Geert Uytterhoeven 1 sibling, 0 replies; 6+ messages in thread From: Bartosz Golaszewski @ 2019-07-10 9:47 UTC (permalink / raw) To: Phil Reid Cc: Bartosz Golaszewski, Linus Walleij, Geert Uytterhoeven, linux-gpio, LKML, Stable # 4 . 20+ śr., 10 lip 2019 o 11:37 Phil Reid <preid@electromag.com.au> napisał(a): > > G'day Bartosz, > > One comment below > > On 10/07/2019 17:08, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > In commit 8764c4ca5049 ("gpio: em: use the managed version of > > gpiochip_add_data()") we implicitly altered the ordering of resource > > freeing: since gpiochip_remove() calls gpiochip_irqchip_remove() > > internally, we now can potentially use the irq_domain after it was > > destroyed in the remove() callback (as devm resources are freed after > > remove() has returned). > > > > Use devm_add_action() to keep the ordering right and entirely kill > > the remove() callback in the driver. > > > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > > Fixes: 8764c4ca5049 ("gpio: em: use the managed version of gpiochip_add_data()") > > Cc: stable@vger.kernel.org > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > --- > > drivers/gpio/gpio-em.c | 35 +++++++++++++++++------------------ > > 1 file changed, 17 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/gpio/gpio-em.c b/drivers/gpio/gpio-em.c > > index b6af705a4e5f..c88028ac66f2 100644 > > --- a/drivers/gpio/gpio-em.c > > +++ b/drivers/gpio/gpio-em.c > > @@ -259,6 +259,13 @@ static const struct irq_domain_ops em_gio_irq_domain_ops = { > > .xlate = irq_domain_xlate_twocell, > > }; > > > > +static void em_gio_irq_domain_remove(void *data) > > +{ > > + struct irq_domain *domain = data; > > + > > + irq_domain_remove(domain); > > +} > > + > > static int em_gio_probe(struct platform_device *pdev) > > { > > struct em_gio_priv *p; > > @@ -333,39 +340,32 @@ static int em_gio_probe(struct platform_device *pdev) > > return -ENXIO; > > } > > > > + ret = devm_add_action(&pdev->dev, > > + em_gio_irq_domain_remove, p->irq_domain); > > Could devm_add_action_or_reset be used? > Of course it could and it should. :) I'll resend tomorrow to not spam the mailing list. Thanks, Bart > > + if (ret) { > > + irq_domain_remove(p->irq_domain); > > + return ret; > > + } > > + > > if (devm_request_irq(&pdev->dev, irq[0]->start, > > em_gio_irq_handler, 0, name, p)) { > > dev_err(&pdev->dev, "failed to request low IRQ\n"); > > - ret = -ENOENT; > > - goto err1; > > + return -ENOENT; > > } > > > > if (devm_request_irq(&pdev->dev, irq[1]->start, > > em_gio_irq_handler, 0, name, p)) { > > dev_err(&pdev->dev, "failed to request high IRQ\n"); > > - ret = -ENOENT; > > - goto err1; > > + return -ENOENT; > > } > > > > ret = devm_gpiochip_add_data(&pdev->dev, gpio_chip, p); > > if (ret) { > > dev_err(&pdev->dev, "failed to add GPIO controller\n"); > > - goto err1; > > + return ret; > > } > > > > return 0; > > - > > -err1: > > - irq_domain_remove(p->irq_domain); > > - return ret; > > -} > > - > > -static int em_gio_remove(struct platform_device *pdev) > > -{ > > - struct em_gio_priv *p = platform_get_drvdata(pdev); > > - > > - irq_domain_remove(p->irq_domain); > > - return 0; > > } > > > > static const struct of_device_id em_gio_dt_ids[] = { > > @@ -376,7 +376,6 @@ MODULE_DEVICE_TABLE(of, em_gio_dt_ids); > > > > static struct platform_driver em_gio_device_driver = { > > .probe = em_gio_probe, > > - .remove = em_gio_remove, > > .driver = { > > .name = "em_gio", > > .of_match_table = em_gio_dt_ids, > > > > > -- > Regards > Phil Reid > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] gpio: em: remove the gpiochip before removing the irq domain 2019-07-10 9:37 ` [PATCH 1/2] gpio: em: remove the gpiochip before removing the irq domain Phil Reid 2019-07-10 9:47 ` Bartosz Golaszewski @ 2019-07-10 10:13 ` Geert Uytterhoeven 1 sibling, 0 replies; 6+ messages in thread From: Geert Uytterhoeven @ 2019-07-10 10:13 UTC (permalink / raw) To: Phil Reid Cc: Bartosz Golaszewski, Linus Walleij, open list:GPIO SUBSYSTEM, Linux Kernel Mailing List, Bartosz Golaszewski, stable Hi Phil, On Wed, Jul 10, 2019 at 11:37 AM Phil Reid <preid@electromag.com.au> wrote: > On 10/07/2019 17:08, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > In commit 8764c4ca5049 ("gpio: em: use the managed version of > > gpiochip_add_data()") we implicitly altered the ordering of resource > > freeing: since gpiochip_remove() calls gpiochip_irqchip_remove() > > internally, we now can potentially use the irq_domain after it was > > destroyed in the remove() callback (as devm resources are freed after > > remove() has returned). > > > > Use devm_add_action() to keep the ordering right and entirely kill > > the remove() callback in the driver. > > > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > > Fixes: 8764c4ca5049 ("gpio: em: use the managed version of gpiochip_add_data()") > > Cc: stable@vger.kernel.org > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > --- a/drivers/gpio/gpio-em.c > > +++ b/drivers/gpio/gpio-em.c > > @@ -333,39 +340,32 @@ static int em_gio_probe(struct platform_device *pdev) > > return -ENXIO; > > } > > > > + ret = devm_add_action(&pdev->dev, > > + em_gio_irq_domain_remove, p->irq_domain); > > Could devm_add_action_or_reset be used? Thank you very much for bringing this function to my attention! I was just wondering if devm_add_action() should call the action on failure, as this is what most callers seem to do anyway. > > > + if (ret) { > > + irq_domain_remove(p->irq_domain); > > + return ret; > > + } Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-07-28 23:10 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-10 9:08 [PATCH 1/2] gpio: em: remove the gpiochip before removing the irq domain Bartosz Golaszewski 2019-07-10 9:08 ` [PATCH 2/2] gpio: em: use a helper variable for &pdev->dev Bartosz Golaszewski 2019-07-28 23:10 ` Linus Walleij 2019-07-10 9:37 ` [PATCH 1/2] gpio: em: remove the gpiochip before removing the irq domain Phil Reid 2019-07-10 9:47 ` Bartosz Golaszewski 2019-07-10 10:13 ` Geert Uytterhoeven
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).