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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75Vc5MERshhsqfiG5XXREJYkWO03V0a=6v4TUKG-fPdy+sw@mail.gmail.com>
Date:   Fri, 20 May 2022 15:48:52 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Zheyu Ma <zheyuma97@...il.com>
Cc:     Andy Shevchenko <andy@...nel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <brgl@...ev.pl>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] gpio: ml-ioh: Convert to use managed functions pcim*
 and devm_*

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