[PATCH v3] gpio: ml-ioh: Convert to use managed functions pcim* and devm_*

[PATCH v3] gpio: ml-ioh: Convert to use managed functions pcim* and devm_*

[Zheyu Ma]

 When removing the module, we will get the following flaw:

[   14.204955] remove_proc_entry: removing non-empty directory 'irq/21', leaking at least 'gpio_ml_ioh'
[   14.205827] WARNING: CPU: 0 PID: 305 at fs/proc/generic.c:717 remove_proc_entry+0x389/0x3f0
...
[   14.220613]  ioh_gpio_remove+0xc5/0xe0 [gpio_ml_ioh]
[   14.221075]  pci_device_remove+0x92/0x240

Fix this by using managed functions, this makes the error handling more
simpler.

Fixes: e971ac9a564a ("gpio: ml-ioh: use resource management for irqs")
Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
---
 drivers/gpio/gpio-ml-ioh.c | 76 ++++++++------------------------------
 1 file changed, 16 insertions(+), 60 deletions(-)

diff --git a/drivers/gpio/gpio-ml-ioh.c b/drivers/gpio/gpio-ml-ioh.c
index b060c4773698..48e3768a830e 100644
--- a/drivers/gpio/gpio-ml-ioh.c
+++ b/drivers/gpio/gpio-ml-ioh.c
@@ -409,29 +409,27 @@ static int ioh_gpio_probe(struct pci_dev *pdev,
 	void *chip_save;
 	int irq_base;
 
-	ret = pci_enable_device(pdev);
+	ret = pcim_enable_device(pdev);
 	if (ret) {
-		dev_err(dev, "%s : pci_enable_device failed", __func__);
-		goto err_pci_enable;
+		dev_err(dev, "%s : pcim_enable_device failed", __func__);
+		return ret;
 	}
 
-	ret = pci_request_regions(pdev, KBUILD_MODNAME);
+	ret = pcim_iomap_regions(pdev, BIT(1), KBUILD_MODNAME);
 	if (ret) {
-		dev_err(dev, "pci_request_regions failed-%d", ret);
-		goto err_request_regions;
+		dev_err(dev, "pcim_iomap_regions failed-%d", ret);
+		return ret;
 	}
 
-	base = pci_iomap(pdev, 1, 0);
+	base = pcim_iomap_table(pdev)[1];
 	if (!base) {
-		dev_err(dev, "%s : pci_iomap failed", __func__);
-		ret = -ENOMEM;
-		goto err_iomap;
+		dev_err(dev, "%s : pcim_iomap_table failed", __func__);
+		return -ENOMEM;
 	}
 
-	chip_save = kcalloc(8, sizeof(*chip), GFP_KERNEL);
+	chip_save = devm_kcalloc(dev, 8, sizeof(*chip), GFP_KERNEL);
 	if (chip_save == NULL) {
-		ret = -ENOMEM;
-		goto err_kzalloc;
+		return -ENOMEM;
 	}
 
 	chip = chip_save;
@@ -442,10 +440,10 @@ static int ioh_gpio_probe(struct pci_dev *pdev,
 		chip->ch = i;
 		spin_lock_init(&chip->spinlock);
 		ioh_gpio_setup(chip, num_ports[i]);
-		ret = gpiochip_add_data(&chip->gpio, chip);
+		ret = devm_gpiochip_add_data(dev, &chip->gpio, chip);
 		if (ret) {
 			dev_err(dev, "IOH gpio: Failed to register GPIO\n");
-			goto err_gpiochip_add;
+			return ret;
 		}
 	}
 
@@ -456,15 +454,14 @@ static int ioh_gpio_probe(struct pci_dev *pdev,
 		if (irq_base < 0) {
 			dev_warn(dev,
 				"ml_ioh_gpio: Failed to get IRQ base num\n");
-			ret = irq_base;
-			goto err_gpiochip_add;
+			return irq_base;
 		}
 		chip->irq_base = irq_base;
 
 		ret = ioh_gpio_alloc_generic_chip(chip,
 						  irq_base, num_ports[j]);
 		if (ret)
-			goto err_gpiochip_add;
+			return ret;
 	}
 
 	chip = chip_save;
@@ -472,52 +469,12 @@ static int ioh_gpio_probe(struct pci_dev *pdev,
 			       IRQF_SHARED, KBUILD_MODNAME, chip);
 	if (ret != 0) {
 		dev_err(dev, "%s request_irq failed\n", __func__);
-		goto err_gpiochip_add;
+		return ret;
 	}
 
 	pci_set_drvdata(pdev, chip);
 
 	return 0;
-
-err_gpiochip_add:
-	chip = chip_save;
-	while (--i >= 0) {
-		gpiochip_remove(&chip->gpio);
-		chip++;
-	}
-	kfree(chip_save);
-
-err_kzalloc:
-	pci_iounmap(pdev, base);
-
-err_iomap:
-	pci_release_regions(pdev);
-
-err_request_regions:
-	pci_disable_device(pdev);
-
-err_pci_enable:
-
-	dev_err(dev, "%s Failed returns %d\n", __func__, ret);
-	return ret;
-}
-
-static void ioh_gpio_remove(struct pci_dev *pdev)
-{
-	int i;
-	struct ioh_gpio *chip = pci_get_drvdata(pdev);
-	void *chip_save;
-
-	chip_save = chip;
-
-	for (i = 0; i < 8; i++, chip++)
-		gpiochip_remove(&chip->gpio);
-
-	chip = chip_save;
-	pci_iounmap(pdev, chip->base);
-	pci_release_regions(pdev);
-	pci_disable_device(pdev);
-	kfree(chip);
 }
 
 static int __maybe_unused ioh_gpio_suspend(struct device *dev)
@@ -558,7 +515,6 @@ static struct pci_driver ioh_gpio_driver = {
 	.name = "ml_ioh_gpio",
 	.id_table = ioh_gpio_pcidev_id,
 	.probe = ioh_gpio_probe,
-	.remove = ioh_gpio_remove,
 	.driver = {
 		.pm = &ioh_gpio_pm_ops,
 	},
-- 
2.36.1

Re: [PATCH v3] gpio: ml-ioh: Convert to use managed functions pcim* and devm_*

[Bartosz Golaszewski]

On Fri, May 20, 2022 at 4:56 AM Zheyu Ma <zheyuma97@gmail.com> wrote:
>
>  When removing the module, we will get the following flaw:
>
> [   14.204955] remove_proc_entry: removing non-empty directory 'irq/21', leaking at least 'gpio_ml_ioh'
> [   14.205827] WARNING: CPU: 0 PID: 305 at fs/proc/generic.c:717 remove_proc_entry+0x389/0x3f0
> ...
> [   14.220613]  ioh_gpio_remove+0xc5/0xe0 [gpio_ml_ioh]
> [   14.221075]  pci_device_remove+0x92/0x240
>
> Fix this by using managed functions, this makes the error handling more
> simpler.
>
> Fixes: e971ac9a564a ("gpio: ml-ioh: use resource management for irqs")
> Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
> ---
>  drivers/gpio/gpio-ml-ioh.c | 76 ++++++++------------------------------
>  1 file changed, 16 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/gpio/gpio-ml-ioh.c b/drivers/gpio/gpio-ml-ioh.c
> index b060c4773698..48e3768a830e 100644
> --- a/drivers/gpio/gpio-ml-ioh.c
> +++ b/drivers/gpio/gpio-ml-ioh.c
> @@ -409,29 +409,27 @@ static int ioh_gpio_probe(struct pci_dev *pdev,
>         void *chip_save;
>         int irq_base;
>
> -       ret = pci_enable_device(pdev);
> +       ret = pcim_enable_device(pdev);
>         if (ret) {
> -               dev_err(dev, "%s : pci_enable_device failed", __func__);
> -               goto err_pci_enable;
> +               dev_err(dev, "%s : pcim_enable_device failed", __func__);
> +               return ret;
>         }
>
> -       ret = pci_request_regions(pdev, KBUILD_MODNAME);
> +       ret = pcim_iomap_regions(pdev, BIT(1), KBUILD_MODNAME);
>         if (ret) {
> -               dev_err(dev, "pci_request_regions failed-%d", ret);
> -               goto err_request_regions;
> +               dev_err(dev, "pcim_iomap_regions failed-%d", ret);
> +               return ret;
>         }
>
> -       base = pci_iomap(pdev, 1, 0);
> +       base = pcim_iomap_table(pdev)[1];
>         if (!base) {
> -               dev_err(dev, "%s : pci_iomap failed", __func__);
> -               ret = -ENOMEM;
> -               goto err_iomap;
> +               dev_err(dev, "%s : pcim_iomap_table failed", __func__);
> +               return -ENOMEM;
>         }
>
> -       chip_save = kcalloc(8, sizeof(*chip), GFP_KERNEL);
> +       chip_save = devm_kcalloc(dev, 8, sizeof(*chip), GFP_KERNEL);
>         if (chip_save == NULL) {
> -               ret = -ENOMEM;
> -               goto err_kzalloc;
> +               return -ENOMEM;
>         }
>
>         chip = chip_save;
> @@ -442,10 +440,10 @@ static int ioh_gpio_probe(struct pci_dev *pdev,
>                 chip->ch = i;
>                 spin_lock_init(&chip->spinlock);
>                 ioh_gpio_setup(chip, num_ports[i]);
> -               ret = gpiochip_add_data(&chip->gpio, chip);
> +               ret = devm_gpiochip_add_data(dev, &chip->gpio, chip);
>                 if (ret) {
>                         dev_err(dev, "IOH gpio: Failed to register GPIO\n");
> -                       goto err_gpiochip_add;
> +                       return ret;
>                 }
>         }
>
> @@ -456,15 +454,14 @@ static int ioh_gpio_probe(struct pci_dev *pdev,
>                 if (irq_base < 0) {
>                         dev_warn(dev,
>                                 "ml_ioh_gpio: Failed to get IRQ base num\n");
> -                       ret = irq_base;
> -                       goto err_gpiochip_add;
> +                       return irq_base;
>                 }
>                 chip->irq_base = irq_base;
>
>                 ret = ioh_gpio_alloc_generic_chip(chip,
>                                                   irq_base, num_ports[j]);
>                 if (ret)
> -                       goto err_gpiochip_add;
> +                       return ret;
>         }
>
>         chip = chip_save;
> @@ -472,52 +469,12 @@ static int ioh_gpio_probe(struct pci_dev *pdev,
>                                IRQF_SHARED, KBUILD_MODNAME, chip);
>         if (ret != 0) {
>                 dev_err(dev, "%s request_irq failed\n", __func__);
> -               goto err_gpiochip_add;
> +               return ret;
>         }
>
>         pci_set_drvdata(pdev, chip);
>
>         return 0;
> -
> -err_gpiochip_add:
> -       chip = chip_save;
> -       while (--i >= 0) {
> -               gpiochip_remove(&chip->gpio);
> -               chip++;
> -       }
> -       kfree(chip_save);
> -
> -err_kzalloc:
> -       pci_iounmap(pdev, base);
> -
> -err_iomap:
> -       pci_release_regions(pdev);
> -
> -err_request_regions:
> -       pci_disable_device(pdev);
> -
> -err_pci_enable:
> -
> -       dev_err(dev, "%s Failed returns %d\n", __func__, ret);
> -       return ret;
> -}
> -
> -static void ioh_gpio_remove(struct pci_dev *pdev)
> -{
> -       int i;
> -       struct ioh_gpio *chip = pci_get_drvdata(pdev);
> -       void *chip_save;
> -
> -       chip_save = chip;
> -
> -       for (i = 0; i < 8; i++, chip++)
> -               gpiochip_remove(&chip->gpio);
> -
> -       chip = chip_save;
> -       pci_iounmap(pdev, chip->base);
> -       pci_release_regions(pdev);
> -       pci_disable_device(pdev);
> -       kfree(chip);
>  }
>
>  static int __maybe_unused ioh_gpio_suspend(struct device *dev)
> @@ -558,7 +515,6 @@ static struct pci_driver ioh_gpio_driver = {
>         .name = "ml_ioh_gpio",
>         .id_table = ioh_gpio_pcidev_id,
>         .probe = ioh_gpio_probe,
> -       .remove = ioh_gpio_remove,
>         .driver = {
>                 .pm = &ioh_gpio_pm_ops,
>         },
> --
> 2.36.1
>

Looks so much better now, gotta love devres. :)

If Andy takes it through his tree:

Reviewed-by: Bartosz Golaszewski <brgl@bgdev.pl>

Otherwise let me know, I can pick it up myself too.

Bart

Re: [PATCH v3] gpio: ml-ioh: Convert to use managed functions pcim* and devm_*

[Andy Shevchenko]

On Fri, May 20, 2022 at 1:33 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Fri, May 20, 2022 at 4:56 AM Zheyu Ma <zheyuma97@gmail.com> wrote:
> >
> >  When removing the module, we will get the following flaw:
> >
> > [   14.204955] remove_proc_entry: removing non-empty directory 'irq/21', leaking at least 'gpio_ml_ioh'
> > [   14.205827] WARNING: CPU: 0 PID: 305 at fs/proc/generic.c:717 remove_proc_entry+0x389/0x3f0
> > ...
> > [   14.220613]  ioh_gpio_remove+0xc5/0xe0 [gpio_ml_ioh]
> > [   14.221075]  pci_device_remove+0x92/0x240
> >
> > Fix this by using managed functions, this makes the error handling more
> > simpler.
> >
> > Fixes: e971ac9a564a ("gpio: ml-ioh: use resource management for irqs")


> Looks so much better now, gotta love devres. :)
>
> If Andy takes it through his tree:
>
> Reviewed-by: Bartosz Golaszewski <brgl@bgdev.pl>
>
> Otherwise let me know, I can pick it up myself too.

It's too late in the cycle, I don't believe I have time to pick this
up. Go ahead to proceed it!


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] gpio: ml-ioh: Convert to use managed functions pcim* and devm_*

[Andy Shevchenko]

On Fri, May 20, 2022 at 4:56 AM Zheyu Ma <zheyuma97@gmail.com> wrote:
>
>  When removing the module, we will get the following flaw:
>
> [   14.204955] remove_proc_entry: removing non-empty directory 'irq/21', leaking at least 'gpio_ml_ioh'
> [   14.205827] WARNING: CPU: 0 PID: 305 at fs/proc/generic.c:717 remove_proc_entry+0x389/0x3f0
> ...
> [   14.220613]  ioh_gpio_remove+0xc5/0xe0 [gpio_ml_ioh]
> [   14.221075]  pci_device_remove+0x92/0x240
>
> Fix this by using managed functions, this makes the error handling more
> simpler.

Thanks!

I have a few comments, but they are not critical, so either a followup
or new version depends on Bart's preferences.

...

> -       ret = pci_enable_device(pdev);
> +       ret = pcim_enable_device(pdev);
>         if (ret) {
> -               dev_err(dev, "%s : pci_enable_device failed", __func__);
> -               goto err_pci_enable;
> +               dev_err(dev, "%s : pcim_enable_device failed", __func__);
> +               return ret;

Since you touch them both, we may convert to `return
dev_err_probe(...);` pattern here and elsewhere. But it might be
better to have in the followup as logically different change.

>         }

...

> -       base = pci_iomap(pdev, 1, 0);
> +       base = pcim_iomap_table(pdev)[1];

>         if (!base) {
> -               dev_err(dev, "%s : pci_iomap failed", __func__);
> -               ret = -ENOMEM;
> -               goto err_iomap;
> +               dev_err(dev, "%s : pcim_iomap_table failed", __func__);
> +               return -ENOMEM;
>         }

These lines are dead code since you already checked
pcim_ioremap_regions(). If it doesn't fail, this one never fails.

...

> -       chip_save = kcalloc(8, sizeof(*chip), GFP_KERNEL);
> +       chip_save = devm_kcalloc(dev, 8, sizeof(*chip), GFP_KERNEL);

>         if (chip_save == NULL) {
> -               ret = -ENOMEM;
> -               goto err_kzalloc;
> +               return -ENOMEM;
>         }

The {} are redundant now and the ' == NULL' part can be replaced by '!'.

...

>                 if (irq_base < 0) {

>                         dev_warn(dev,
>                                 "ml_ioh_gpio: Failed to get IRQ base num\n");

This should be dev_err(), but you may convert it altogether to `return
dev_err_probe(...);` in the respective patch.

> -                       ret = irq_base;
> -                       goto err_gpiochip_add;
> +                       return irq_base;
>                 }
>                 chip->irq_base = irq_base;
>
>                 ret = ioh_gpio_alloc_generic_chip(chip,
>                                                   irq_base, num_ports[j]);
>                 if (ret)
> -                       goto err_gpiochip_add;
> +                       return ret;
>         }

...

>         if (ret != 0) {

Also in a separate patch you may replace all this kind of lines;

if (chip == NULL) ==> if (!chip)
if (ret != 0) ==> if (ret)

>                 dev_err(dev, "%s request_irq failed\n", __func__);
> -               goto err_gpiochip_add;
> +               return ret;

return dev_err_probe(...);
But here it's definitely in a separate patch.

>         }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] gpio: ml-ioh: Convert to use managed functions pcim* and devm_*

[Bartosz Golaszewski]

On Fri, May 20, 2022 at 4:56 AM Zheyu Ma <zheyuma97@gmail.com> wrote:
>
>  When removing the module, we will get the following flaw:
>
> [   14.204955] remove_proc_entry: removing non-empty directory 'irq/21', leaking at least 'gpio_ml_ioh'
> [   14.205827] WARNING: CPU: 0 PID: 305 at fs/proc/generic.c:717 remove_proc_entry+0x389/0x3f0
> ...
> [   14.220613]  ioh_gpio_remove+0xc5/0xe0 [gpio_ml_ioh]
> [   14.221075]  pci_device_remove+0x92/0x240
>
> Fix this by using managed functions, this makes the error handling more
> simpler.
>
> Fixes: e971ac9a564a ("gpio: ml-ioh: use resource management for irqs")
> Signed-off-by: Zheyu Ma <zheyuma97@gmail.com>
> ---

Applied, thanks!

Bart