linux-kernel - Re: [PATCH v4 4/6] iio: light: stk3310: use dev_err

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: <6jwurbs27slfpsredvpxfgwjkurkqvfmzccaxnfgtuh4aks3c6@ciapprv3wsex>
Date: Tue, 12 Nov 2024 18:11:37 -0500
From: Aren <aren@...cevolution.org>
To: Uwe Kleine-König <u.kleine-koenig@...libre.com>
Cc: Andy Shevchenko <andy.shevchenko@...il.com>, 
	Jonathan Cameron <jic23@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, Chen-Yu Tsai <wens@...e.org>, 
	Jernej Skrabec <jernej.skrabec@...il.com>, Samuel Holland <samuel@...lland.org>, 
	Kaustabh Chakraborty <kauschluss@...root.org>, Barnabás Czémán <trabarni@...il.com>, 
	Ondrej Jirman <megi@....cz>, linux-iio@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	linux-sunxi@...ts.linux.dev, Dragan Simic <dsimic@...jaro.org>, phone-devel@...r.kernel.org
Subject: Re: [PATCH v4 4/6] iio: light: stk3310: use dev_err_probe where
 possible

On Tue, Nov 12, 2024 at 11:15:54AM +0100, Uwe Kleine-König wrote:
> Hello Andy, hello Aren,
> 
> On Mon, Nov 11, 2024 at 11:44:51AM +0200, Andy Shevchenko wrote:
> > On Sun, Nov 10, 2024 at 04:34:30PM -0500, Aren wrote:
> > > On Sun, Nov 10, 2024 at 09:52:32PM +0200, Andy Shevchenko wrote:
> > > > Sun, Nov 10, 2024 at 02:14:24PM -0500, Aren kirjoitti:
> > 
> > You can do it differently
> > 
> > #define STK3310_REGFIELD(name)							\
> > do {										\
> > 	data->reg_##name =							\
> > 		devm_regmap_field_alloc(dev, regmap, stk3310_reg_field_##name);	\
> > 	if (IS_ERR(data->reg_##name))						\
> > 		return dev_err_probe(dev, PTR_ERR(data->reg_##name),		\
> > 				     "reg field alloc failed.\n");		\
> > } while (0)
> > 
> > > #define STK3310_REGFIELD(name) ({						\
> > > 	data->reg_##name = devm_regmap_field_alloc(dev, regmap,			\
> > > 						   stk3310_reg_field_##name);   \
> > > 	if (IS_ERR(data->reg_##name))						\
> > > 		return dev_err_probe(dev, PTR_ERR(data->reg_##name),		\
> > > 				     "reg field alloc failed\n");		\
> > > })
> > 
> > I am against unneeded use of GNU extensions.
> > 
> > > > > replacing "do { } while (0)" with "({ })" and deindenting could make
> > > > > enough room to clean this up the formatting of this macro though.
> > > > 
> > > > do {} while (0) is C standard, ({}) is not.
> > > 
> > > ({ }) is used throughout the kernel, and is documented as such[1]. I
> > > don't see a reason to avoid it, if it helps readability.
> > 
> > I don't see how it makes things better here, and not everybody is familiar with
> > the concept even if it's used in the kernel here and there. Also if a tool is
> > being used in one case it doesn't mean it's suitable for another.
> 
> Just to throw in my subjective view here: I don't expect anyone with
> some base level knowledge of C will have doubts about the semantics of
> ({ ... }) and compared to that I find do { ... } while (0) less optimal,
> because it's more verbose and when spotting the "do {" part, the
> semantic only gets clear when you also see the "while (0)". Having said
> that I also dislike the "do" starting on column 0, IMHO the RHS of the
> #define should be intended.

Thank you, this sums up my opinion on this better than I could have (and
some bits I hadn't considered).

> So if you ask me, this is not an unneeded use of an extension. The
> extension is used to improve readabilty and I blame the C standard to
> not support this syntax.
> 
> While I'm in critics mode: I consider hiding a return in a macro bad
> style.

Yeah... probably worse than any of the formatting options here. I guess
the proper way would be to use devm_regmap_field_bulk_alloc, but that's
well outside the scope of this series. Perhaps it would make sense to
move the macro definition to just before the function it's used in so
it's at least a little easier to spot?

 - Aren