Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200801112446.149549-1-refactormyself@gmail.com> <20200801112446.149549-9-refactormyself@gmail.com>
In-Reply-To: <20200801112446.149549-9-refactormyself@gmail.com>
From:   Bartosz Golaszewski <bgolaszewski@baylibre.com>
Date:   Tue, 18 Aug 2020 21:59:50 +0200
Message-ID: <CAMpxmJX8SV6RTgy4vKNRPzKvnVaJZpZKQmOf1pX1wGd+H2zaeA@mail.gmail.com>
Subject: Re: [RFC PATCH 08/17] gpio: Drop uses of pci_read_config_*() return value
To:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     Linus Walleij <linus.walleij@linaro.org>,
        "Saheed O. Bolarinwa" <refactormyself@gmail.com>,
        bjorn@helgaas.com, Shuah Khan <skhan@linuxfoundation.org>,
        linux-kernel-mentees@lists.linuxfoundation.org,
        linux-pci@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>,
        linux-gpio <linux-gpio@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sat, Aug 1, 2020 at 2:24 PM Saheed O. Bolarinwa
<refactormyself@gmail.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@helgaas.com>
> Signed-off-by: Saheed O. Bolarinwa <refactormyself@gmail.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