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

[Hugo Villeneuve <hugo@hugovil.com>]

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>