linux-kernel - 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]

Date:   Wed, 13 Sep 2023 17:27:31 +0200
From:   Bartosz Golaszewski <brgl@...ev.pl>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
        Linus Walleij <linus.walleij@...aro.org>,
        Andy Shevchenko <andy@...nel.org>
Subject: Re: [PATCH v1 05/10] gpio: pca953x: Simplify code with cleanup helpers

On Wed, Sep 13, 2023 at 4:35 PM Geert Uytterhoeven <geert@...ux-m68k.org> wrote:
>
>         Hi Andy,
>
> 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.

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?

Bart

>
> 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