gpiolib: fix efficiency regression when using gpio_chip_get_multiple()

[PATCH] gpiolib: fix efficiency regression when using gpio_chip_get_multiple()

Posted by Hugo Villeneuve 3 months ago

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

commit 74abd086d2ee ("gpiolib: sanitize the return value of
gpio_chip::get_multiple()") altered the value returned by
gc->get_multiple() in case it is positive (> 0), but failed to
return for other cases (<= 0).

This may result in the "if (gc->get)" block being executed and thus
negates the performance gain that is normally obtained by using
gc->get_multiple().

Fix by returning the result of gc->get_multiple() if it is <= 0.

Also move the "ret" variable to the scope where it is used, which as an
added bonus fixes an indentation error introduced by the aforementioned
commit.

Fixes: 74abd086d2ee ("gpiolib: sanitize the return value of gpio_chip::get_multiple()")
Cc: stable@vger.kernel.org
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 drivers/gpio/gpiolib.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index fdafa0df1b43..3a3eca5b4c40 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -3297,14 +3297,15 @@ static int gpiod_get_raw_value_commit(const struct gpio_desc *desc)
 static int gpio_chip_get_multiple(struct gpio_chip *gc,
 				  unsigned long *mask, unsigned long *bits)
 {
-	int ret;
-	
 	lockdep_assert_held(&gc->gpiodev->srcu);
 
 	if (gc->get_multiple) {
+		int ret;
+
 		ret = gc->get_multiple(gc, mask, bits);
 		if (ret > 0)
 			return -EBADE;
+		return ret;
 	}
 
 	if (gc->get) {

base-commit: b4911fb0b060899e4eebca0151eb56deb86921ec
-- 
2.39.5

Re: [PATCH] gpiolib: fix efficiency regression when using gpio_chip_get_multiple()

Posted by Bartosz Golaszewski 3 months ago

On Thu, Jul 3, 2025 at 9:18 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
>
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
>
> commit 74abd086d2ee ("gpiolib: sanitize the return value of
> gpio_chip::get_multiple()") altered the value returned by
> gc->get_multiple() in case it is positive (> 0), but failed to
> return for other cases (<= 0).
>
> This may result in the "if (gc->get)" block being executed and thus
> negates the performance gain that is normally obtained by using
> gc->get_multiple().
>
> Fix by returning the result of gc->get_multiple() if it is <= 0.
>
> Also move the "ret" variable to the scope where it is used, which as an
> added bonus fixes an indentation error introduced by the aforementioned
> commit.

Thanks, I queued it for fixes. I typically keep local variables at the
top of the function (just a personal readability preference) but since
this function already has scope-local variables, let's do it. What is
the indentation error you're mentioning exactly?

Bart

>
> Fixes: 74abd086d2ee ("gpiolib: sanitize the return value of gpio_chip::get_multiple()")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> ---
>  drivers/gpio/gpiolib.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index fdafa0df1b43..3a3eca5b4c40 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -3297,14 +3297,15 @@ static int gpiod_get_raw_value_commit(const struct gpio_desc *desc)
>  static int gpio_chip_get_multiple(struct gpio_chip *gc,
>                                   unsigned long *mask, unsigned long *bits)
>  {
> -       int ret;
> -
>         lockdep_assert_held(&gc->gpiodev->srcu);
>
>         if (gc->get_multiple) {
> +               int ret;
> +
>                 ret = gc->get_multiple(gc, mask, bits);
>                 if (ret > 0)
>                         return -EBADE;
> +               return ret;
>         }
>
>         if (gc->get) {
>
> base-commit: b4911fb0b060899e4eebca0151eb56deb86921ec
> --
> 2.39.5
>

Re: [PATCH] gpiolib: fix efficiency regression when using gpio_chip_get_multiple()

Posted by Hugo Villeneuve 3 months ago

Hi Bartosz,

On Fri, 4 Jul 2025 10:26:58 +0200
Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> On Thu, Jul 3, 2025 at 9:18 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> >
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> >
> > commit 74abd086d2ee ("gpiolib: sanitize the return value of
> > gpio_chip::get_multiple()") altered the value returned by
> > gc->get_multiple() in case it is positive (> 0), but failed to
> > return for other cases (<= 0).
> >
> > This may result in the "if (gc->get)" block being executed and thus
> > negates the performance gain that is normally obtained by using
> > gc->get_multiple().
> >
> > Fix by returning the result of gc->get_multiple() if it is <= 0.
> >
> > Also move the "ret" variable to the scope where it is used, which as an
> > added bonus fixes an indentation error introduced by the aforementioned
> > commit.
> 
> Thanks, I queued it for fixes. I typically keep local variables at the
> top of the function (just a personal readability preference) but since
> this function already has scope-local variables, let's do it. What is
> the indentation error you're mentioning exactly?

Ok, I was under the assumption that having local-scope variables was the preferred approach in the kernel, as I sometimes see patches just for fixing variables scope. If it is something specific to the GPIO subsystem, no problem.

The ret variable was indented ok, but an empty line with spaces was introduced just after it.

Thanks for applying the patch.

Hugo.


> > Fixes: 74abd086d2ee ("gpiolib: sanitize the return value of gpio_chip::get_multiple()")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > ---
> >  drivers/gpio/gpiolib.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index fdafa0df1b43..3a3eca5b4c40 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -3297,14 +3297,15 @@ static int gpiod_get_raw_value_commit(const struct gpio_desc *desc)
> >  static int gpio_chip_get_multiple(struct gpio_chip *gc,
> >                                   unsigned long *mask, unsigned long *bits)
> >  {
> > -       int ret;
> > -
> >         lockdep_assert_held(&gc->gpiodev->srcu);
> >
> >         if (gc->get_multiple) {
> > +               int ret;
> > +
> >                 ret = gc->get_multiple(gc, mask, bits);
> >                 if (ret > 0)
> >                         return -EBADE;
> > +               return ret;
> >         }
> >
> >         if (gc->get) {
> >
> > base-commit: b4911fb0b060899e4eebca0151eb56deb86921ec
> > --
> > 2.39.5
> >
> 


-- 
Hugo Villeneuve <hugo@hugovil.com>

Re: [PATCH] gpiolib: fix efficiency regression when using gpio_chip_get_multiple()

Posted by Bartosz Golaszewski 3 months ago

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>


On Thu, 03 Jul 2025 15:18:29 -0400, Hugo Villeneuve wrote:
> commit 74abd086d2ee ("gpiolib: sanitize the return value of
> gpio_chip::get_multiple()") altered the value returned by
> gc->get_multiple() in case it is positive (> 0), but failed to
> return for other cases (<= 0).
> 
> This may result in the "if (gc->get)" block being executed and thus
> negates the performance gain that is normally obtained by using
> gc->get_multiple().
> 
> [...]

Applied, thanks!

[1/1] gpiolib: fix efficiency regression when using gpio_chip_get_multiple()
      https://git.kernel.org/brgl/linux/c/30e0fd3c0273dc106320081793793a424f1f1950

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>