linux-kernel - Re: guard coding style (was: Re: [PATCH v1 05/10] gpio: pca953x: Simplify code with cleanup helpers)

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: <CAMJwLczd7oZ3JPqKNW-qOiB0S2WRsqV7TVFWGD=yysK0nmZrSQ@mail.gmail.com>
Date:   Thu, 14 Sep 2023 13:51:01 -0700
From:   Mitchell Levy <levymitchell0@...il.com>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     Bartosz Golaszewski <brgl@...ev.pl>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Andy Shevchenko <andy@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: guard coding style (was: Re: [PATCH v1 05/10] gpio: pca953x:
 Simplify code with cleanup helpers)

Hey all,
A brief disclaimer, I'm a fairly new kernel contributor, but since I
was cc'd directly, I figured I might as well drop into the
conversation.

On Thu, Sep 14, 2023 at 12:47 AM Geert Uytterhoeven
<geert@...ux-m68k.org> wrote:
>
> Hi Bartosz,
>
> On Wed, Sep 13, 2023 at 5:27 PM Bartosz Golaszewski <brgl@...ev.pl> wrote:
> > On Wed, Sep 13, 2023 at 4:35 PM Geert Uytterhoeven <geert@...ux-m68k.org> wrote:
> > > On Fri, 1 Sep 2023, Andy Shevchenko wrote:
> > > > Use macros defined in linux/cleanup.h to automate resource lifetime
> > > > control in gpio-pca953x.
> > > >
> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> > >
> > > Thanks for your patch, which is now commit 8e471b784a720f6f
> > > ("gpio: pca953x: Simplify code with cleanup helpers") in
> > > gpio/gpio/for-next.
> > >
> > > > --- a/drivers/gpio/gpio-pca953x.c
> > > > +++ b/drivers/gpio/gpio-pca953x.c
> > > > @@ -557,9 +554,8 @@ static int pca953x_gpio_get_value(struct gpio_chip *gc, unsigned off)
> > > >       u32 reg_val;
> > > >       int ret;
> > > >
> > > > -     mutex_lock(&chip->i2c_lock);
> > > > -     ret = regmap_read(chip->regmap, inreg, &reg_val);
> > > > -     mutex_unlock(&chip->i2c_lock);
> > > > +     scoped_guard(mutex, &chip->i2c_lock)
> > > > +             ret = regmap_read(chip->regmap, inreg, &reg_val);
> > >
> > > I can't say I'm thrilled about the lack of curly braces.  I was also
> > > surprised to discover that checkpatch nor gcc W=1 complain about the
> > > indentation change.
> > > I know we don't use curly braces in single-statement for_each_*() loops,
> > > but at least these have the familiar "for"-prefix.  And having the scope
> > > is very important here, so using braces, this would stand out more.
> > >
> > > Hence can we please get curly braces, like
> > >
> > >      scoped_guard(mutex, &chip->i2c_lock) {
> > >             ret = regmap_read(chip->regmap, inreg, &reg_val);
> > >      }
> > >
> > > ?
> > >
> > > Thanks! ;-)
> >
> > I strongly disagree. The scope here is very clear - just like it is in
> > a for loop, in a while loop or in an if block:
> >
> > if (foo)
> >     bar()
> >
> > if (foo) {
> >     bar();
> >     baz();
> > }
> >
> > Only compound statements need curly braces in the kernel and it has
> > been like this forever. I don't really see a need to make it an
> > exception.

The more I think on this issue, the more I go back and forth. If we
only had guard(...), the only way to approximate scoped guard would be
to either just do what the macro does (i.e., a dummy for loop that
only runs once) or use an anonymous scope, e.g.,
{
    guard(...);
    my_one_statement();
}
Since this is how I've previously used std::lock_guard in C++, this
pattern feels very familiar to me, and the scoped_guard feels almost
like syntax sugar for this. As such, I feel like including the braces
is most natural because, as Geert mentioned, it emphasizes the scope
that "should" (in my brain, at least) be there.

Thanks,
Mitchell

> > That being said - I don't think the coding style for guard has ever
> > been addressed yet, so maybe bring it up with Peter Zijlstra?
>
> That's a good idea!
>
> I see Peter always used curly braces (but he didn't have any
> single-statement blocks, except for one with an "if", and we do tend
> to use curly braces in "for"-statements containing a single "if", too),
> but he does put a space after the "scoped_guard", as is also
> shown in the template in include/linux/cleanup.h:
>
>     scoped_guard (name, args...) { }:
>
> Then, "guard" does not get a space (but it is funny syntax
> anyway, with the double set of parentheses ;-).  The template in
> include/linux/cleanup.h doesn't match actual usage as it lacks the
> second set of parentheses:
>
>     guard(name):
>
> Peter: care to comment?
> Or do you have a different bikeshed to paint today? ;-)
>
> Thanks!
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds