Received-SPF: pass (google.com: domain of linux-kernel+bounces-4970-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20231219004158.12405-1-warthog618@gmail.com> <20231219004158.12405-4-warthog618@gmail.com>
 <CAMRc=McPQnLzPCJz6AKV44=qp6z=V9sCAcYkDTQA6FVjWj6E3A@mail.gmail.com>
In-Reply-To: <CAMRc=McPQnLzPCJz6AKV44=qp6z=V9sCAcYkDTQA6FVjWj6E3A@mail.gmail.com>
From: Bartosz Golaszewski <brgl@bgdev.pl>
Date: Tue, 19 Dec 2023 10:32:42 +0100
Message-ID: <CAMRc=McirXisM34GTDQbbs7pEzAsLMNHZRQy_vS34HfEFxdu+w@mail.gmail.com>
Subject: Re: [PATCH v5 3/5] gpiolib: cdev: reduce locking in gpio_desc_to_lineinfo()
To: Kent Gibson <warthog618@gmail.com>
Cc: linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org, 
	linus.walleij@linaro.org, andy@kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Dec 19, 2023 at 10:30=E2=80=AFAM Bartosz Golaszewski <brgl@bgdev.pl=
> wrote:
>
> On Tue, Dec 19, 2023 at 1:42=E2=80=AFAM Kent Gibson <warthog618@gmail.com=
> wrote:
> >
> > Reduce the time holding the gpio_lock by snapshotting the desc flags,
> > rather than testing them individually while holding the lock.
> >
> > Accept that the calculation of the used field is inherently racy, and
> > only check the availability of the line from pinctrl if other checks
> > pass, so avoiding the check for lines that are otherwise in use.
> >
> > Signed-off-by: Kent Gibson <warthog618@gmail.com>
> > Reviewed-by: Andy Shevchenko <andy@kernel.org>
> > ---
> >  drivers/gpio/gpiolib-cdev.c | 74 ++++++++++++++++++-------------------
> >  1 file changed, 36 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> > index aecc4241b6c8..9f5104d7532f 100644
> > --- a/drivers/gpio/gpiolib-cdev.c
> > +++ b/drivers/gpio/gpiolib-cdev.c
> > @@ -2399,74 +2399,72 @@ static void gpio_desc_to_lineinfo(struct gpio_d=
esc *desc,
> >                                   struct gpio_v2_line_info *info)
> >  {
> >         struct gpio_chip *gc =3D desc->gdev->chip;
> > -       bool ok_for_pinctrl;
> > -       unsigned long flags;
> > +       unsigned long dflags;
> >
> >         memset(info, 0, sizeof(*info));
> >         info->offset =3D gpio_chip_hwgpio(desc);
> >
> > -       /*
> > -        * This function takes a mutex so we must check this before tak=
ing
> > -        * the spinlock.
> > -        *
> > -        * FIXME: find a non-racy way to retrieve this information. May=
be a
> > -        * lock common to both frameworks?
> > -        */
> > -       ok_for_pinctrl =3D pinctrl_gpio_can_use_line(gc, info->offset);
> > +       scoped_guard(spinlock_irqsave, &gpio_lock) {
> > +               if (desc->name)
> > +                       strscpy(info->name, desc->name, sizeof(info->na=
me));
> >
> > -       spin_lock_irqsave(&gpio_lock, flags);
> > +               if (desc->label)
> > +                       strscpy(info->consumer, desc->label,
> > +                               sizeof(info->consumer));
> >
> > -       if (desc->name)
> > -               strscpy(info->name, desc->name, sizeof(info->name));
> > -
> > -       if (desc->label)
> > -               strscpy(info->consumer, desc->label, sizeof(info->consu=
mer));
> > +               dflags =3D READ_ONCE(desc->flags);
> > +       }
> >
> >         /*
> > -        * Userspace only need to know that the kernel is using this GP=
IO so
> > -        * it can't use it.
> > +        * Userspace only need know that the kernel is using this GPIO =
so it
> > +        * can't use it.
> > +        * The calculation of the used flag is slightly racy, as it may=
 read
> > +        * desc, gc and pinctrl state without a lock covering all three=
 at
> > +        * once.  Worst case if the line is in transition and the calcu=
lation
> > +        * is inconsistent then it looks to the user like they performe=
d the
> > +        * read on the other side of the transition - but that can alwa=
ys
> > +        * happen.
> > +        * The definitive test that a line is available to userspace is=
 to
> > +        * request it.
> >          */
> > -       info->flags =3D 0;
> > -       if (test_bit(FLAG_REQUESTED, &desc->flags) ||
> > -           test_bit(FLAG_IS_HOGGED, &desc->flags) ||
> > -           test_bit(FLAG_USED_AS_IRQ, &desc->flags) ||
> > -           test_bit(FLAG_EXPORT, &desc->flags) ||
> > -           test_bit(FLAG_SYSFS, &desc->flags) ||
> > +       if (test_bit(FLAG_REQUESTED, &dflags) ||
> > +           test_bit(FLAG_IS_HOGGED, &dflags) ||
> > +           test_bit(FLAG_USED_AS_IRQ, &dflags) ||
> > +           test_bit(FLAG_EXPORT, &dflags) ||
> > +           test_bit(FLAG_SYSFS, &dflags) ||
> >             !gpiochip_line_is_valid(gc, info->offset) ||
> > -           !ok_for_pinctrl)
> > +           !pinctrl_gpio_can_use_line(gc, info->offset))
> >                 info->flags |=3D GPIO_V2_LINE_FLAG_USED;
> >
> > -       if (test_bit(FLAG_IS_OUT, &desc->flags))
> > +       if (test_bit(FLAG_IS_OUT, &dflags))
> >                 info->flags |=3D GPIO_V2_LINE_FLAG_OUTPUT;
> >         else
> >                 info->flags |=3D GPIO_V2_LINE_FLAG_INPUT;
> >
> > -       if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
> > +       if (test_bit(FLAG_ACTIVE_LOW, &dflags))
>
> One more nit: you no longer have to use atomic bitops here, you can
> switch to the ones without guarantees for better performance.

-ENEVERMIND, there's no non-atomic test_bit(). I'll go ahead and apply
this one too.

Bart

>
> Bart
>
> >                 info->flags |=3D GPIO_V2_LINE_FLAG_ACTIVE_LOW;
> >
> > -       if (test_bit(FLAG_OPEN_DRAIN, &desc->flags))
> > +       if (test_bit(FLAG_OPEN_DRAIN, &dflags))
> >                 info->flags |=3D GPIO_V2_LINE_FLAG_OPEN_DRAIN;
> > -       if (test_bit(FLAG_OPEN_SOURCE, &desc->flags))
> > +       if (test_bit(FLAG_OPEN_SOURCE, &dflags))
> >                 info->flags |=3D GPIO_V2_LINE_FLAG_OPEN_SOURCE;
> >
> > -       if (test_bit(FLAG_BIAS_DISABLE, &desc->flags))
> > +       if (test_bit(FLAG_BIAS_DISABLE, &dflags))
> >                 info->flags |=3D GPIO_V2_LINE_FLAG_BIAS_DISABLED;
> > -       if (test_bit(FLAG_PULL_DOWN, &desc->flags))
> > +       if (test_bit(FLAG_PULL_DOWN, &dflags))
> >                 info->flags |=3D GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN;
> > -       if (test_bit(FLAG_PULL_UP, &desc->flags))
> > +       if (test_bit(FLAG_PULL_UP, &dflags))
> >                 info->flags |=3D GPIO_V2_LINE_FLAG_BIAS_PULL_UP;
> >
> > -       if (test_bit(FLAG_EDGE_RISING, &desc->flags))
> > +       if (test_bit(FLAG_EDGE_RISING, &dflags))
> >                 info->flags |=3D GPIO_V2_LINE_FLAG_EDGE_RISING;
> > -       if (test_bit(FLAG_EDGE_FALLING, &desc->flags))
> > +       if (test_bit(FLAG_EDGE_FALLING, &dflags))
> >                 info->flags |=3D GPIO_V2_LINE_FLAG_EDGE_FALLING;
> >
> > -       if (test_bit(FLAG_EVENT_CLOCK_REALTIME, &desc->flags))
> > +       if (test_bit(FLAG_EVENT_CLOCK_REALTIME, &dflags))
> >                 info->flags |=3D GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME=
;
> > -       else if (test_bit(FLAG_EVENT_CLOCK_HTE, &desc->flags))
> > +       else if (test_bit(FLAG_EVENT_CLOCK_HTE, &dflags))
> >                 info->flags |=3D GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE;
> > -
> > -       spin_unlock_irqrestore(&gpio_lock, flags);
> >  }
> >
> >  struct gpio_chardev_data {
> > --
> > 2.39.2
> >