| Message ID | 20190710090852.9239-1-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] gpio: em: remove the gpiochip before removing the irq domain | expand |
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, >
ś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 >
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
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,