linux-kernel - Re: [PATCH v4 1/5] gpiolib: cdev: relocate debounce_period_us from struct gpio

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZYBoA25z76uutBBI@rigel>
Date: Mon, 18 Dec 2023 23:40:51 +0800
From: Kent Gibson <warthog618@...il.com>
To: Bartosz Golaszewski <brgl@...ev.pl>
Cc: linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
	linus.walleij@...aro.org, andy@...nel.org
Subject: Re: [PATCH v4 1/5] gpiolib: cdev: relocate debounce_period_us from
 struct gpio_desc

On Mon, Dec 18, 2023 at 04:24:50PM +0100, Bartosz Golaszewski wrote:
> On Sat, Dec 16, 2023 at 1:17 AM Kent Gibson <warthog618@...il.com> wrote:
> >
> > Store the debounce period for a requested line locally, rather than in
> > the debounce_period_us field in the gpiolib struct gpio_desc.
> >
> > Add a global tree of lines containing supplemental line information
> > to make the debounce period available to be reported by the
> > GPIO_V2_GET_LINEINFO_IOCTL and the line change notifier.
> >
> > Signed-off-by: Kent Gibson <warthog618@...il.com>
> > Reviewed-by: Andy Shevchenko <andy@...nel.org>
> > ---
> >  drivers/gpio/gpiolib-cdev.c | 154 ++++++++++++++++++++++++++++++------
> >  1 file changed, 132 insertions(+), 22 deletions(-)
> >
> > +static inline bool line_is_supplemental(struct line *line)
>
> Under v2 I suggested naming this line_has_suppinfo(). Any reason not
> to do it? I think it's more logical than saying "line is
> supplemental". The latter makes it seem as if certain line objects
> would "supplement" some third party with something. What this really
> checks is: does this line contain additional information.
>


My bad - responded to your first comment and then missed the rest.

Agreed - the naming could be better. Will fix for v5.

> > +{
> > +       return READ_ONCE(line->debounce_period_us);
> > +}
> > +
> > +static void line_set_debounce_period(struct line *line,
> > +                                    unsigned int debounce_period_us)
> > +{
> > +       bool was_suppl = line_is_supplemental(line);
> > +
> > +       WRITE_ONCE(line->debounce_period_us, debounce_period_us);
> > +
> > +       if (line_is_supplemental(line) == was_suppl)
> > +               return;
> > +
> > +       if (was_suppl)
> > +               supinfo_erase(line);
> > +       else
> > +               supinfo_insert(line);
>
> Could you add a comment here saying it's called with the config mutex
> taken as at first glance it looks racy but actually isn't?
>

Sure.  Though it is also covered by the gdev->sem you added, right?
So the config_mutex is now redundant?
Should I document it is covered by both?
Or drop the config_mutex entirely?

And you wanted some comments to explain the logic?
I thought this is a common "has it changed" pattern, and so didn't
require additional explanation, but I guess not as common as I thought.

Cheers,
Kent.