Re: [RFC PATCH 08/17] gpio: Drop uses of pci_read_config_*() return value

Bartosz Golaszewski
2020-08-18T20:00:07Z

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