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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Fri, May 20, 2022 at 4:56 AM Zheyu Ma <zheyuma97@xxxxxxxxx> 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