linux-kernel - Re: [PATCH v1 1/1] gpiolib: cdev: Split line_get_debounce

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: <ZYWHjq_7PnwO27ro@rigel>
Date: Fri, 22 Dec 2023 20:56:46 +0800
From: Kent Gibson <warthog618@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>,
	linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
	Linus Walleij <linus.walleij@...aro.org>,
	Bartosz Golaszewski <brgl@...ev.pl>
Subject: Re: [PATCH v1 1/1] gpiolib: cdev: Split line_get_debounce_period()
 and use

On Fri, Dec 22, 2023 at 02:39:38PM +0200, Andy Shevchenko wrote:
> On Fri, Dec 22, 2023 at 09:12:37AM +0800, Kent Gibson wrote:
> > On Thu, Dec 21, 2023 at 07:55:27PM +0200, Andy Shevchenko wrote:
> > > Instead of repeating the same code and reduce possible miss
> > > of READ_ONCE(), split line_get_debounce_period() heler out
> > > and use in the existing cases.
> > >
> >
> > helper
> >
> > Not a fan of this change.
> >
> > So using READ_ONCE() is repeating code??
>
> Yes. Because one may forget about it.

Just as one may forget to use your wrapper.
This argument is a NULL - so I'll just forget about it.

>
> > Doesn't providing a wrapper around READ_ONCE() just rename that repitition?
> > What of all the other uses of READ_ONCE() in cdev (and there are a lot) -
> > why pick on debounce_period?
>
> Because you have a setter, but getter. Inconsistency.
>

But then "for consistency" ALL the struct line fields require accessors
and mutators.  That path is insanity.

The setter is there as setting the value now has side effects - none of
which are visible to the caller, hence the usage of the standard
setter name.
You are siggesting every function name describe everything the function
does?

And, in case you've forgotten, YOU REVIEWED THIS.

> > The line_set_debounce_period() is necessary as the set is now a
> > multi-step process as it can impact whether the line is contained
> > in the supinfo_tree.  The get is just a get.
> >
> > And you could've included me in the Cc so I didn't just find it by
> > accident.
>
> Maybe it's time to add you to the MAINTAINERS for this file as a designated
> reviewer?
>

You are patching my recent change that you yourself reviewed only days
ago. I would think that you would Cc me whether I were a maintainer or
not as I'm very likely to have relevant feedback.

Cheers,
Kent.