[v1] gpio: it87: balance superio enter/exit calls in error path

[PATCH] gpio: it87: balance superio enter/exit calls in error path

Posted by Bartosz Golaszewski 1 week, 3 days ago

We always call superio_enter() in it87_gpio_direction_out() but only
call superio_exit() if the call to it87_gpio_set() succeeds. Move the
label to balance the calls in error path as well.

Fixes: ef877a159072 ("gpio: it87: use new line value setter callbacks")
Reported-by: Daniel Gibson <daniel@gibson.sh>
Closes: https://lore.kernel.org/all/bd0a00e3-9b8c-43e8-8772-e67b91f4c71e@gibson.sh/
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
 drivers/gpio/gpio-it87.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-it87.c b/drivers/gpio/gpio-it87.c
index 5d677bcfccf2..528ac1890613 100644
--- a/drivers/gpio/gpio-it87.c
+++ b/drivers/gpio/gpio-it87.c
@@ -254,9 +254,8 @@ static int it87_gpio_direction_out(struct gpio_chip *chip,
 	if (rc)
 		goto exit;
 
-	superio_exit();
-
 exit:
+	superio_exit();
 	spin_unlock(&it87_gpio->lock);
 	return rc;
 }
-- 
2.51.0

Re: [PATCH] gpio: it87: balance superio enter/exit calls in error path

Posted by Daniel Gibson 1 week, 2 days ago

On 12/9/25 07:12, Bartosz Golaszewski wrote:
> We always call superio_enter() in it87_gpio_direction_out() but only
> call superio_exit() if the call to it87_gpio_set() succeeds. Move the
> label to balance the calls in error path as well.
> 
> Fixes: ef877a159072 ("gpio: it87: use new line value setter callbacks")
> Reported-by: Daniel Gibson <daniel@gibson.sh>
> Closes: https://lore.kernel.org/all/bd0a00e3-9b8c-43e8-8772-e67b91f4c71e@gibson.sh/
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> ---
>  drivers/gpio/gpio-it87.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-it87.c b/drivers/gpio/gpio-it87.c
> index 5d677bcfccf2..528ac1890613 100644
> --- a/drivers/gpio/gpio-it87.c
> +++ b/drivers/gpio/gpio-it87.c
> @@ -254,9 +254,8 @@ static int it87_gpio_direction_out(struct gpio_chip *chip,
>  	if (rc)
>  		goto exit;
>  
> -	superio_exit();
> -
>  exit:
> +	superio_exit();
>  	spin_unlock(&it87_gpio->lock);
>  	return rc;
>  }

Moving this after exit: means that superio_exit() is also called if
superio_enter() failed, a few lines above this patch:

	rc = superio_enter();
	if (rc)
		goto exit;

I don't know if this is really wrong, but it looks fishy and this code
behaved differently for years - in contrast to the change to skip
superio_exit() if it87_gpio_set() failed, which is very recent,
introduced when the return type of it87_gpio_set() was changed from void
to int.

Probably (again, I don't know if this is actually wrong, I'm not that
familiar with how this chip works) superio_exit() should remain above
`exit:` and `if(rc) goto exit;` after `rc = it87_gpio_set(chip,
gpio_num, val);` should be removed, so:
- if superio_enter() fails, its error code is returned,
  but superio_exit() is *not* called
- the it87_gpio_set(chip, gpio_num, val); return code is returned by
  it87_gpio_direction_out() no matter if it succeeds or fails,
  but in either case superio_exit() is called


Cheers,
Daniel

Re: [PATCH] gpio: it87: balance superio enter/exit calls in error path

Posted by Bartosz Golaszewski 1 week, 2 days ago

On Tue, Dec 9, 2025 at 3:42 PM Daniel Gibson <daniel@gibson.sh> wrote:
>
> On 12/9/25 07:12, Bartosz Golaszewski wrote:
> > We always call superio_enter() in it87_gpio_direction_out() but only
> > call superio_exit() if the call to it87_gpio_set() succeeds. Move the
> > label to balance the calls in error path as well.
> >
> > Fixes: ef877a159072 ("gpio: it87: use new line value setter callbacks")
> > Reported-by: Daniel Gibson <daniel@gibson.sh>
> > Closes: https://lore.kernel.org/all/bd0a00e3-9b8c-43e8-8772-e67b91f4c71e@gibson.sh/
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> > ---
> >  drivers/gpio/gpio-it87.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-it87.c b/drivers/gpio/gpio-it87.c
> > index 5d677bcfccf2..528ac1890613 100644
> > --- a/drivers/gpio/gpio-it87.c
> > +++ b/drivers/gpio/gpio-it87.c
> > @@ -254,9 +254,8 @@ static int it87_gpio_direction_out(struct gpio_chip *chip,
> >       if (rc)
> >               goto exit;
> >
> > -     superio_exit();
> > -
> >  exit:
> > +     superio_exit();
> >       spin_unlock(&it87_gpio->lock);
> >       return rc;
> >  }
>
> Moving this after exit: means that superio_exit() is also called if
> superio_enter() failed, a few lines above this patch:
>
>         rc = superio_enter();
>         if (rc)
>                 goto exit;
>
> I don't know if this is really wrong, but it looks fishy and this code
> behaved differently for years - in contrast to the change to skip
> superio_exit() if it87_gpio_set() failed, which is very recent,
> introduced when the return type of it87_gpio_set() was changed from void
> to int.
>
> Probably (again, I don't know if this is actually wrong, I'm not that
> familiar with how this chip works) superio_exit() should remain above
> `exit:` and `if(rc) goto exit;` after `rc = it87_gpio_set(chip,
> gpio_num, val);` should be removed, so:
> - if superio_enter() fails, its error code is returned,
>   but superio_exit() is *not* called
> - the it87_gpio_set(chip, gpio_num, val); return code is returned by
>   it87_gpio_direction_out() no matter if it succeeds or fails,
>   but in either case superio_exit() is called
>

You're probably right, I don't really know this chip either. I will
send a better version.

Bart