Message ID | 20220520025624.3803776-1-zheyuma97@gmail.com |
---|---|
State | New |
Headers | show |
Series | [v3] gpio: ml-ioh: Convert to use managed functions pcim* and devm_* | expand |
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
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!
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. > }
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
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, },
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(-)