linux-kernel - Re: [RFC PATCH v2 4/5] iio: accel: Support Kionix/ROHM KX022A accelerometer

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:   Fri, 14 Oct 2022 14:34:23 +0100
From:   Jonathan Cameron <Jonathan.Cameron@...wei.com>
To:     "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
CC:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Matti Vaittinen <mazziesaccount@...il.com>,
        Jonathan Cameron <jic23@...nel.org>,
        "Lars-Peter Clausen" <lars@...afoo.de>,
        Rob Herring <robh+dt@...nel.org>,
        "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@...aro.org>,
        Jagath Jog J <jagathjog1996@...il.com>,
        Nikita Yushchenko <nikita.yoush@...entembedded.com>,
        Cosmin Tanislav <demonsingur@...il.com>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH v2 4/5] iio: accel: Support Kionix/ROHM KX022A
 accelerometer


...

> >   
> >>>>>> +	if (en)
> >>>>>> +		return regmap_set_bits(data->regmap, KX022A_REG_CNTL,
> >>>>>> +				       KX022A_MASK_DRDY);  
> >>>>>
> >>>>> I would put redundant 'else' here to have them on the same column, but
> >>>>> it's pity we don't have regmap_assign_bits() API (or whatever name you
> >>>>> can come up with) to hide this kind of code.  
> >>>>
> >>>> Eh, you mean you would put else here even though we return from the if? And
> >>>> then put another return in else, and no return outside the if/else?
> >>>>
> >>>> I definitely don't like the idea.
> >>>>
> >>>> We could probably use regmap_update_bits and ternary but in my opinion that
> >>>> would be just much less obvious. I do like the use of set/clear bits which
> >>>> also makes the meaning / "polarity" of bits really clear.  
> >>>
> >>> The idea behind is simple (and note that I'm usually on the side of _removing_
> >>> redundancy)
> >>>
> >>> 	if (en)
> >>> 		return regmap_set_bits(data->regmap, KX022A_REG_CNTL,
> >>> 				       KX022A_MASK_DRDY);
> >>> 	else
> >>> 		return regmap_clear_bits(data->regmap, KX022A_REG_CNTL,
> >>> 					 ...
> >>>
> >>> Because the branches are semantically tighten to each other. But it's not
> >>> a big deal to me.  
> >>
> >> What I do not really like in above example is that we never reach the
> >> end of function body.  
> > 
> > What do you mean by that? Compiler does warn or what?  
> 
> I don't know if compiler warns about it as I didn't test it. Now that 
> you mentioned it, I think I have seen such warnings from a compiler or 
> some static analyzer (klocwork?) in the past though. What I mean is that:
> 
> int foo()
> {
> 	if () {
> 		...
> 		return 1;
> 	} else {
> 		...
> 		return 2;
> 	}
> }
> 

For reference, this is the one I'd write if both options are good
(or both are bad) and we don't need to worry about reducing indent
for readability.

However, I long since decided this was trivial enough not to
comment on it in the code of others!

> construct makes mistakes like:
> 
> int foo()
> {
> 	if () {
> 		...
> 		return 1;
> 	} else {
> 		...
> 		return 2;
> 	}
> 
> 	...
> 
> 	return 0;
> }

That should given unreachable code warning unless you've really managed
to confuse the compiler  / static analysis tooling.

> 
> easy to make. When one uses:
> 
> int foo()
> {
> 	if () {
> 		...
> 		return 1;
> 	}
> 
> 	...
> 	return 2;
> }
> 
> to begin with there is zero room for such confusion.
> 
> >   
> >> It is against the expectation - and adding the
> >> else has no real meaning when if returns. I actually think that
> >> checkpatch could even warn about that construct.  
> > 
> > No way we ever accept such a thing for checkpatch because it's subjective  
> 
> Eh. Are you telling me that there is no subjective stuff in checkpatch? ;)
> 
> > (very dependant on the code piece). OTOH the proposed pattern is used in
> > many places and left like that in places where I cleaned up the 'else',
> > to leave the semantic tights with the above lines).
> >   
> >>>>>> +	return regmap_clear_bits(data->regmap, KX022A_REG_CNTL,
> >>>>>> +				 KX022A_MASK_DRDY);  
> > 
> > I see that we have a strong disagreement here. I leave it to maintainers.  
> 
> 
> Okay, let's hear what others have to say here.

Non answer above ;)

Time for the old "Don't let perfect be the enemy of good!"


> 
> Thanks for all the input this far.
> 
> Best Regards
> 	-- Matti
>