linux-kernel - Re: [RFC PATCH 08/17] gpio: Drop uses of pci_read_config

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: <CAMpxmJX8SV6RTgy4vKNRPzKvnVaJZpZKQmOf1pX1wGd+H2zaeA@mail.gmail.com>
Date:   Tue, 18 Aug 2020 21:59:50 +0200
From:   Bartosz Golaszewski <bgolaszewski@...libre.com>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        "Saheed O. Bolarinwa" <refactormyself@...il.com>,
        bjorn@...gaas.com, Shuah Khan <skhan@...uxfoundation.org>,
        linux-kernel-mentees@...ts.linuxfoundation.org,
        linux-pci@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
        linux-gpio <linux-gpio@...r.kernel.org>
Subject: Re: [RFC PATCH 08/17] gpio: Drop uses of pci_read_config_*() return value

On Sat, Aug 1, 2020 at 2:24 PM Saheed O. Bolarinwa
<refactormyself@...il.com> wrote:
>
> The return value of pci_read_config_*() may not indicate a device error.
> However, the value read by these functions is more likely to indicate
> this kind of error. This presents two overlapping ways of reporting
> errors and complicates error checking.
>
> It is possible to move to one single way of checking for error if the
> dependency on the return value of these functions is removed, then it
> can later be made to return void.
>
> Remove all uses of the return value of pci_read_config_*().
> Check the actual value read for ~0. In this case, ~0 is an invalid
> value thus it indicates some kind of error.
>
> Suggested-by: Bjorn Helgaas <bjorn@...gaas.com>
> Signed-off-by: Saheed O. Bolarinwa <refactormyself@...il.com>
> ---
>  drivers/gpio/gpio-amd8111.c |  7 +++++--
>  drivers/gpio/gpio-rdc321x.c | 21 ++++++++++++---------
>  2 files changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpio/gpio-amd8111.c b/drivers/gpio/gpio-amd8111.c
> index fdcebe59510d..7b9882380cbc 100644
> --- a/drivers/gpio/gpio-amd8111.c
> +++ b/drivers/gpio/gpio-amd8111.c
> @@ -198,9 +198,12 @@ static int __init amd_gpio_init(void)
>         goto out;
>
>  found:
> -       err = pci_read_config_dword(pdev, 0x58, &gp.pmbase);
> -       if (err)
> +       pci_read_config_dword(pdev, 0x58, &gp.pmbase);
> +       if (gp.pmbase == (u32)~0) {
> +               err = -ENODEV;
>                 goto out;
> +       }
> +
>         err = -EIO;
>         gp.pmbase &= 0x0000FF00;
>         if (gp.pmbase == 0)
> diff --git a/drivers/gpio/gpio-rdc321x.c b/drivers/gpio/gpio-rdc321x.c
> index 01ed2517e9fd..03f1ff07b844 100644
> --- a/drivers/gpio/gpio-rdc321x.c
> +++ b/drivers/gpio/gpio-rdc321x.c
> @@ -85,10 +85,13 @@ static int rdc_gpio_config(struct gpio_chip *chip,
>         gpch = gpiochip_get_data(chip);
>
>         spin_lock(&gpch->lock);
> -       err = pci_read_config_dword(gpch->sb_pdev, gpio < 32 ?
> -                       gpch->reg1_ctrl_base : gpch->reg2_ctrl_base, &reg);
> -       if (err)
> +       pci_read_config_dword(gpch->sb_pdev,
> +                               (gpio < 32) ? gpch->reg1_ctrl_base
> +                                       : gpch->reg2_ctrl_base, &reg);
> +       if (reg == (u32)~0) {
> +               err = -ENODEV;
>                 goto unlock;
> +       }
>
>         reg |= 1 << (gpio & 0x1f);
>
> @@ -166,17 +169,17 @@ static int rdc321x_gpio_probe(struct platform_device *pdev)
>         /* This might not be, what others (BIOS, bootloader, etc.)
>            wrote to these registers before, but it's a good guess. Still
>            better than just using 0xffffffff. */
> -       err = pci_read_config_dword(rdc321x_gpio_dev->sb_pdev,
> +       pci_read_config_dword(rdc321x_gpio_dev->sb_pdev,
>                                         rdc321x_gpio_dev->reg1_data_base,
>                                         &rdc321x_gpio_dev->data_reg[0]);
> -       if (err)
> -               return err;
> +       if (rdc321x_gpio_dev->data_reg[0] == (u32)~0)
> +               return -ENODEV;
>
> -       err = pci_read_config_dword(rdc321x_gpio_dev->sb_pdev,
> +       pci_read_config_dword(rdc321x_gpio_dev->sb_pdev,
>                                         rdc321x_gpio_dev->reg2_data_base,
>                                         &rdc321x_gpio_dev->data_reg[1]);
> -       if (err)
> -               return err;
> +       if (rdc321x_gpio_dev->data_reg[1] == (u32)~0)
> +               return -ENODEV;
>
>         dev_info(&pdev->dev, "registering %d GPIOs\n",
>                                         rdc321x_gpio_dev->chip.ngpio);
> --
> 2.18.4
>

Bjorn,

I don't know the pci sub-system at all. Does this look good to you?

Bartosz