linux-kernel - Re: [PATCH v3 08/14] iio: bmg160_core: Simplify using devm_regulator_*get

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: <CAHp75VdMA5mkxkMrtiRTGn5F-5GWjxKyuD5iBuj3HKWqZZMxkg@mail.gmail.com>
Date:   Sat, 20 Aug 2022 10:18:14 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
Cc:     Matti Vaittinen <mazziesaccount@...il.com>,
        Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Miaoqian Lin <linmq006@...il.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Xiang wangx <wangxiang@...rlc.com>,
        linux-iio <linux-iio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 08/14] iio: bmg160_core: Simplify using devm_regulator_*get_enable()

On Sat, Aug 20, 2022 at 9:48 AM Vaittinen, Matti
<Matti.Vaittinen@...rohmeurope.com> wrote:
> On 8/20/22 09:25, Andy Shevchenko wrote:
> > On Sat, Aug 20, 2022 at 9:19 AM Vaittinen, Matti
> > <Matti.Vaittinen@...rohmeurope.com> wrote:
> >> On 8/20/22 02:30, Andy Shevchenko wrote:
> >>> On Fri, Aug 19, 2022 at 10:21 PM Matti Vaittinen
> >>> <mazziesaccount@...il.com> wrote:
> >
> > What did I miss?
>
>  >>>>           struct bmg160_data *data;
>  >>>>           struct iio_dev *indio_dev;
>
> This does already violate the rule.

Indeed, I am reading this with an MTA that has True Type fonts, and I
can't see it at the first glance. But this breaks that rule slightly
while your added line breaks it significantly.

> >>> this case you even can move it out of the function, so we will see
> >>> clearly that this is (not a hidden) global variable.
> >>
> >> Here I do disagree with you. Moving the array out of the function makes
> >> it _much_ less obvious it is not used outside this function. Reason for
> >> making is "static const" is to allow the data be placed in read-only
> >> area (thanks to Guenter who originally gave me this tip).
> >
> > "static" in C language means two things (that's what come to my mind):
> > - for functions this tells that a function is not used outside of the module;
> > - for variables that it is a _global_ variable.
> >
> > Hiding static inside functions is not a good coding practice since it
> > hides scope of the variable.
>
> For const arrays the static in function does make sense. Being able to
> place the data in read-only areas do help with the memory on limited
> systems.

I'm not sure we are on the same page. I do not object to the "const"
part and we are _not_ talking about that.

> > And if you look into the kernel code, I
> > believe the use you are proposing is in minority.
>
> I don't know about the statistics. What I know is that we do have a
> technical benefits when we use static const arrays instead of non static
> ones in the functions. I do also believe placing the variables in blocks
> is a good practice.

Yes, and global variables are better to be seen as global variables.

> I tend to agree with you that using local, non const statics has
> pitfalls - but the pitfalls do not really apply with const ones. You
> know the value and have no races. Benefit is that just by seeing that no
> pointer is returned you can be sure that no "sane code" uses the data
> outside the function it resides.

Putting a global variable (const or non-const) to the function will
hide its scope and it is prone to getting two variables with the same
or very similar names with quite different semantics. That's why it's
really not good practice. I would rather see it outside of the
function _esp_ because it's static const.

-- 
With Best Regards,
Andy Shevchenko