linux-kernel - Re: [PATCH v5 05/10] iio: pressure: bmp280: Make return values consistent

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: Sun, 5 May 2024 20:08:18 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Vasileios Amoiridis <vassilisamir@...il.com>
Cc: lars@...afoo.de, andriy.shevchenko@...ux.intel.com,
 ang.iglesiasg@...il.com, mazziesaccount@...il.com, ak@...klinger.de,
 petre.rodan@...dimension.ro, phil@...pberrypi.com, 579lpy@...il.com,
 linus.walleij@...aro.org, semen.protsenko@...aro.org,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 05/10] iio: pressure: bmp280: Make return values
 consistent

On Mon, 29 Apr 2024 21:00:41 +0200
Vasileios Amoiridis <vassilisamir@...il.com> wrote:

> Throughout the driver there are quite a few places were return
> values are treated as errors if they are negative or not-zero.
> This commit tries to make the return values of those functions
> consistent and treat them as errors in case there is a negative
> value since the vast majority of the functions are returning
> erorrs coming from regmap_*() functions.

The changes are fine, but that argument isn't correct.
regmap_*() functions never (that I can recall) return positive
values, so if (ret) would be valid for those and I'd have expected
the exact opposite outcome if you are looking at regmap*() return
values to make the decision.

The if (ret) pattern is sometimes used throughout because it
makes
	return function()

consistent without needing to do

	ret = function();
	if (ret < 0)
		return ret;

	return 0;

That pattern isn't particularly common in this driver (there are few cases).
We also tend not to worry too much about that slight inconsistency though
in a few cases it has lead to compilers failing to detect that some paths
are not possible and reporting false warnings.

However, all arguments about which is 'better' aside, key is that consistency
(either choice) is better than a mix.  So I'm fine with ret < 0 on basis
it's the most common in this driver being your justification. Just don't
blame regmap*() return values!

> 
> While at it, add error messages that were not implemented before.
> 
> Finally, remove any extra error checks that are dead code.

Ideally this would be broken up a little more as, whilst all error
code related, these aren't all the same thing.

I'd have preferred:
1) Dead code removal.
2) Message updates.
3) Switch to consistent ret handling.

However it isn't that bad as a single patch, so just address the question
above and I think this will be fine as one patch.

> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@...il.com>

Jonathan