| Message ID | 20210516061425.8757-1-aardelean@deviqon.com |
|---|---|
| State | Accepted |
| Commit | 2a9a2ccaab99d6f8bbe92f01839dfd1cd4a33ddf |
| Headers | show |
| Series | gpio: gpio-stmpe: fully use convert probe to device-managed | expand |
On Sun, May 16, 2021 at 8:14 AM Alexandru Ardelean <aardelean@deviqon.com> wrote: > The driver doesn't look like it can be built as a kmod, so leaks cannot > happen via a rmmod mechanism. > The remove hook was removed via commit 3b52bb960ec6 ("gpio: stmpe: make > it explicitly non-modular"). > > The IRQ is registered via devm_request_threaded_irq(), making the driver > only partially device-managed. > > In any case all resources should be made device-managed, mostly as a good > practice. That way at least the unwinding on error is happening in reverse > order (as the probe). > > This change also removes platform_set_drvdata() since the information is > never retrieved to be used in the driver. > > Signed-off-by: Alexandru Ardelean <aardelean@deviqon.com> Nice! Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Thu, 20 May 2021 at 09:40, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > On Sunday, May 16, 2021, Alexandru Ardelean <aardelean@deviqon.com> wrote: >> >> The driver doesn't look like it can be built as a kmod, so leaks cannot >> happen via a rmmod mechanism. >> The remove hook was removed via commit 3b52bb960ec6 ("gpio: stmpe: make >> it explicitly non-modular"). >> >> The IRQ is registered via devm_request_threaded_irq(), making the driver >> only partially device-managed. >> >> In any case all resources should be made device-managed, mostly as a good >> practice. That way at least the unwinding on error is happening in reverse >> order (as the probe). >> >> This change also removes platform_set_drvdata() since the information is >> never retrieved to be used in the driver. > > > Any driver can be unbind from device thru sysfs. The exception is when they (device drivers) specifically disable that. Oh, I see. Thanks for the info :) > >> >> Signed-off-by: Alexandru Ardelean <aardelean@deviqon.com> >> --- >> >> I'm not sure if this should be marked with a Fixes tag. >> But if so, it should probably be for commit 3b52bb960ec6 (also mentioned in >> the comment above). >> >> drivers/gpio/gpio-stmpe.c | 32 +++++++++++++------------------- >> 1 file changed, 13 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c >> index b94ef8181428..dd4d58b4ae49 100644 >> --- a/drivers/gpio/gpio-stmpe.c >> +++ b/drivers/gpio/gpio-stmpe.c >> @@ -449,6 +449,11 @@ static void stmpe_init_irq_valid_mask(struct gpio_chip *gc, >> } >> } >> >> +static void stmpe_gpio_disable(void *stmpe) >> +{ >> + stmpe_disable(stmpe, STMPE_BLOCK_GPIO); >> +} >> + >> static int stmpe_gpio_probe(struct platform_device *pdev) >> { >> struct stmpe *stmpe = dev_get_drvdata(pdev->dev.parent); >> @@ -461,7 +466,7 @@ static int stmpe_gpio_probe(struct platform_device *pdev) >> return -EINVAL; >> } >> >> - stmpe_gpio = kzalloc(sizeof(*stmpe_gpio), GFP_KERNEL); >> + stmpe_gpio = devm_kzalloc(&pdev->dev, sizeof(*stmpe_gpio), GFP_KERNEL); >> if (!stmpe_gpio) >> return -ENOMEM; >> >> @@ -489,7 +494,11 @@ static int stmpe_gpio_probe(struct platform_device *pdev) >> >> ret = stmpe_enable(stmpe, STMPE_BLOCK_GPIO); >> if (ret) >> - goto out_free; >> + return ret; >> + >> + ret = devm_add_action_or_reset(&pdev->dev, stmpe_gpio_disable, stmpe); >> + if (ret) >> + return ret; >> >> if (irq > 0) { >> struct gpio_irq_chip *girq; >> @@ -499,7 +508,7 @@ static int stmpe_gpio_probe(struct platform_device *pdev) >> "stmpe-gpio", stmpe_gpio); >> if (ret) { >> dev_err(&pdev->dev, "unable to get irq: %d\n", ret); >> - goto out_disable; >> + return ret; >> } >> >> girq = &stmpe_gpio->chip.irq; >> @@ -514,22 +523,7 @@ static int stmpe_gpio_probe(struct platform_device *pdev) >> girq->init_valid_mask = stmpe_init_irq_valid_mask; >> } >> >> - ret = gpiochip_add_data(&stmpe_gpio->chip, stmpe_gpio); >> - if (ret) { >> - dev_err(&pdev->dev, "unable to add gpiochip: %d\n", ret); >> - goto out_disable; >> - } >> - >> - platform_set_drvdata(pdev, stmpe_gpio); >> - >> - return 0; >> - >> -out_disable: >> - stmpe_disable(stmpe, STMPE_BLOCK_GPIO); >> - gpiochip_remove(&stmpe_gpio->chip); >> -out_free: >> - kfree(stmpe_gpio); >> - return ret; >> + return devm_gpiochip_add_data(&pdev->dev, &stmpe_gpio->chip, stmpe_gpio); >> } >> >> static struct platform_driver stmpe_gpio_driver = { >> -- >> 2.31.1 >> > > > -- > With Best Regards, > Andy Shevchenko > >
On Sun, May 16, 2021 at 8:14 AM Alexandru Ardelean <aardelean@deviqon.com> wrote: > > The driver doesn't look like it can be built as a kmod, so leaks cannot > happen via a rmmod mechanism. > The remove hook was removed via commit 3b52bb960ec6 ("gpio: stmpe: make > it explicitly non-modular"). > > The IRQ is registered via devm_request_threaded_irq(), making the driver > only partially device-managed. > > In any case all resources should be made device-managed, mostly as a good > practice. That way at least the unwinding on error is happening in reverse > order (as the probe). > > This change also removes platform_set_drvdata() since the information is > never retrieved to be used in the driver. > > Signed-off-by: Alexandru Ardelean <aardelean@deviqon.com> I applied the patch and tweaked the commit message because as Andy pointed out - this driver can be unbound over sysfs. Bart
diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c index b94ef8181428..dd4d58b4ae49 100644 --- a/drivers/gpio/gpio-stmpe.c +++ b/drivers/gpio/gpio-stmpe.c @@ -449,6 +449,11 @@ static void stmpe_init_irq_valid_mask(struct gpio_chip *gc, } } +static void stmpe_gpio_disable(void *stmpe) +{ + stmpe_disable(stmpe, STMPE_BLOCK_GPIO); +} + static int stmpe_gpio_probe(struct platform_device *pdev) { struct stmpe *stmpe = dev_get_drvdata(pdev->dev.parent); @@ -461,7 +466,7 @@ static int stmpe_gpio_probe(struct platform_device *pdev) return -EINVAL; } - stmpe_gpio = kzalloc(sizeof(*stmpe_gpio), GFP_KERNEL); + stmpe_gpio = devm_kzalloc(&pdev->dev, sizeof(*stmpe_gpio), GFP_KERNEL); if (!stmpe_gpio) return -ENOMEM; @@ -489,7 +494,11 @@ static int stmpe_gpio_probe(struct platform_device *pdev) ret = stmpe_enable(stmpe, STMPE_BLOCK_GPIO); if (ret) - goto out_free; + return ret; + + ret = devm_add_action_or_reset(&pdev->dev, stmpe_gpio_disable, stmpe); + if (ret) + return ret; if (irq > 0) { struct gpio_irq_chip *girq; @@ -499,7 +508,7 @@ static int stmpe_gpio_probe(struct platform_device *pdev) "stmpe-gpio", stmpe_gpio); if (ret) { dev_err(&pdev->dev, "unable to get irq: %d\n", ret); - goto out_disable; + return ret; } girq = &stmpe_gpio->chip.irq; @@ -514,22 +523,7 @@ static int stmpe_gpio_probe(struct platform_device *pdev) girq->init_valid_mask = stmpe_init_irq_valid_mask; } - ret = gpiochip_add_data(&stmpe_gpio->chip, stmpe_gpio); - if (ret) { - dev_err(&pdev->dev, "unable to add gpiochip: %d\n", ret); - goto out_disable; - } - - platform_set_drvdata(pdev, stmpe_gpio); - - return 0; - -out_disable: - stmpe_disable(stmpe, STMPE_BLOCK_GPIO); - gpiochip_remove(&stmpe_gpio->chip); -out_free: - kfree(stmpe_gpio); - return ret; + return devm_gpiochip_add_data(&pdev->dev, &stmpe_gpio->chip, stmpe_gpio); } static struct platform_driver stmpe_gpio_driver = {
The driver doesn't look like it can be built as a kmod, so leaks cannot happen via a rmmod mechanism. The remove hook was removed via commit 3b52bb960ec6 ("gpio: stmpe: make it explicitly non-modular"). The IRQ is registered via devm_request_threaded_irq(), making the driver only partially device-managed. In any case all resources should be made device-managed, mostly as a good practice. That way at least the unwinding on error is happening in reverse order (as the probe). This change also removes platform_set_drvdata() since the information is never retrieved to be used in the driver. Signed-off-by: Alexandru Ardelean <aardelean@deviqon.com> --- I'm not sure if this should be marked with a Fixes tag. But if so, it should probably be for commit 3b52bb960ec6 (also mentioned in the comment above). drivers/gpio/gpio-stmpe.c | 32 +++++++++++++------------------- 1 file changed, 13 insertions(+), 19 deletions(-)