linux-kernel - Re: [PATCH 1/5] hwmon: ina2xx: bail-out from ina2xx

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: <CAMpxmJVGE444t3gE-=u8kuK_B9rJ=0fw9ZYVuYgQRPdswiPj7Q@mail.gmail.com>
Date:	Tue, 25 Nov 2014 18:50:07 +0100
From:	Bartosz Golaszewski <bgolaszewski@...libre.com>
To:	Guenter Roeck <linux@...ck-us.net>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Benoit Cousson <bcousson@...libre.com>,
	Patrick Titiano <ptitiano@...libre.com>
Subject: Re: [PATCH 1/5] hwmon: ina2xx: bail-out from ina2xx_probe() in case
 of configuration errors

2014-11-25 17:59 GMT+01:00 Guenter Roeck <linux@...ck-us.net>:
> The new functions _might_ make a bit more sense if you would
> implement the necessary calculations in the functions, but you are
> not doing that. Instead, in the next patch, you introduce yet
> another function to do the calibration calculation, just to add it
> as parameter to the actual calibration function whenever you call it.

This wasn't my intention, but I'll keep that in mind for the next version.

> - I don't accept unnecessary ( ).
> - I don't accept unnecessary typecasts.
> - If you don't accept negative values, use kstrtoul().
> - kstrtol can at least in theory return other errors besides -EINVAL.

I'll fix those in the next version.

> - Locking should be done in the calling code. It is not needed during
>   probe and more appropriate in set functions.

I don't use locks in probe - they're used directly in
ina2xx_set_value() or indirectly in ina226_update_avg(), but these
functions are never called from probe.

> - Any reason for selecting "rshunt" as sysfs attribute name instead
>   of, say, shunt-resistor or maybe shunt_resistor ?

I'll change it to shunt_resistor for more readability.

> - Returning -ENODEV from a set function doesn't make much sense.

It does make sense in case of ACME (http://baylibre.com/acme/) - a
probe can be disconnected at run-time, which, even without these
patches, would cause ina2xx_update_device() to error out when called
by ina2xx_show_value():

231                         int rv = i2c_smbus_read_word_swapped(client, i);
232                         if (rv < 0) {
233                                 ret = ERR_PTR(rv);
234                                 goto abort;

I just reproduced this behavior in ina2xx_set_value().

> - We don't overwrite error codes except in probe functions.

I'll fix that too.

Bart
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/