linux-kernel - Re: [PATCH v8 2/3] regulator: Add devres version of of_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: <ZvZ_idJsmuzNhFMc@smile.fi.intel.com>
Date: Fri, 27 Sep 2024 12:48:57 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Chen-Yu Tsai <wenst@...omium.org>
Cc: Ulf Hansson <ulf.hansson@...aro.org>,
	Matthias Brugger <matthias.bgg@...il.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	Mark Brown <broonie@...nel.org>, devicetree@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linux-pm@...r.kernel.org, Douglas Anderson <dianders@...omium.org>,
	Johan Hovold <johan@...nel.org>, Pablo Sun <pablo.sun@...iatek.com>,
	Macpaul Lin <macpaul.lin@...iatek.com>,
	Sebastian Reichel <sebastian.reichel@...labora.com>
Subject: Re: [PATCH v8 2/3] regulator: Add devres version of
 of_regulator_get_optional()

On Fri, Sep 27, 2024 at 12:38:35PM +0800, Chen-Yu Tsai wrote:
> On Thu, Sep 26, 2024 at 8:26 PM Andy Shevchenko
> <andriy.shevchenko@...ux.intel.com> wrote:
> > On Thu, Sep 26, 2024 at 04:43:52PM +0800, Chen-Yu Tsai wrote:
> > > On Wed, Sep 25, 2024 at 6:56 PM Andy Shevchenko
> > > <andriy.shevchenko@...ux.intel.com> wrote:
> > > > On Wed, Sep 25, 2024 at 05:38:05PM +0800, Chen-Yu Tsai wrote:

...

> > > > > +#if IS_ENABLED(CONFIG_OF)
> > > >
> > > > Do we really need this?
> > >
> > > What's the point of going through devres_* stuff if we already know
> > > _of_regulator_get() is going to fail anyway?
> >
> > With devm_add_action*() this will be other way around and there are plenty of
> > APIs done this way. The ifdeffery is simply ugly in the code.
> 
> It's still extra code that doesn't get compiled out.

Are you sure? In case of the stub the compiler should go with a "dead code
elimination" optimisation and get rid of most of it (yes, I admit that it might
be the overhead for the exporting a function which returns a constant).

> > > Also, _of_regulator_get() does not have a stub version for !CONFIG_OF.
> >
> > So, what prevents us from adding it?
> 
> Because there's no need if the only internal user isn't using it.
> 
> I could also move them over to of_regulator.c.
> 
> _of_regulator_get() stays internal to that file, and devm_regulator_release()
> gets exposed instead.
> 
> Does that sound better?

This sounds good to me!

...

> > > > > +static inline struct regulator *__must_check devm_of_regulator_get_optional(struct device *dev,
> > > > > +                                                                         struct device_node *node,
> > > > > +                                                                         const char *id)
> > > >
> > > > I don't know the conventions here, but I find better to have it as
> > > >
> > > > static inline __must_check struct regulator *
> > > > devm_of_regulator_get_optional(struct device *dev, struct device_node *node, const char *id)
> > > >
> > > > Similar to other stubs and declarations.
> > >
> > > I don't think there are any conventions. This file already has three types:
> > >
> > > 1. Wrap the line with the function name on the second line
> > > 2. Wrap the arguments; wrapped arguments aligned to the left parenthesis.
> > > 3. Wrap the arguments; wrapped arguments aligned with aribtrary number of
> > >    tabs.
> > >
> > > I prefer the way I have put them.
> >
> > The way you put it despite relaxed limit is slightly harder to read.
> > I don't remember many headers that do so-o indented parameters. Besides
> > your way defers the burden of resplit to the future in case one more parameter
> > needs to be added which will excess the 100 limit.
> >
> > Also __must_check is somehow misplaced in my opinion (talking from my
> > experience and this can be simply checked by grepping other headers).
> 
> Seems correct to me. It's between the return type and the function name.
> From the coding style doc:
> 
>  __init void * __must_check action(enum magic value, size_t size, u8 count,
>                                    char *fmt, ...) __printf(4, 5) __malloc;
> 
> The preferred order of elements for a function prototype is:
> 
> - storage class (below, ``static __always_inline``, noting that
> ``__always_inline``
>   is technically an attribute but is treated like ``inline``)
> - storage class attributes (here, ``__init`` -- i.e. section
> declarations, but also
>   things like ``__cold``)
> - return type (here, ``void *``)
> - return type attributes (here, ``__must_check``)
> - function name (here, ``action``)
> - function parameters (here, ``(enum magic value, size_t size, u8
> count, char *fmt, ...)``,
>   noting that parameter names should always be included)
> - function parameter attributes (here, ``__printf(4, 5)``)
> - function behavior attributes (here, ``__malloc``)
> 
> > That said, I prefer the way I suggested or something alike.
> 
> Two people arguing over style that is not clearly specified in the coding
> style doc is probably wasting time. I'll use what `clang-format` gave:
> 
> static inline struct regulator *__must_check of_regulator_get_optional(
>        struct device *dev, struct device_node *node, const char *id)
> static inline struct regulator *__must_check devm_of_regulator_get_optional(
>        struct device *dev, struct device_node *node, const char *id)

With all my hatred towards this clang-format "feature", i.e. open ended
parenthesis, this looks better than your original variant.

-- 
With Best Regards,
Andy Shevchenko