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]

Date:	Tue, 25 Nov 2014 09:59:08 -0800
From:	Guenter Roeck <linux@...ck-us.net>
To:	Bartosz Golaszewski <bgolaszewski@...libre.com>
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

On 11/25/2014 09:50 AM, Bartosz Golaszewski wrote:
> 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():
>

It seems to me this is a problem of your architecture. That doesn't
make it a generic problem. Your architecture should detect that a
probe was disconnected and de-instantiate the device automatically
if so, and re-instantiate it if it is re-inserted. Ultimately this
should be done with, for example, devicetree overlays.

Even worse, the remove/reinsertion sequence results in a non-initialized
chip.

This makes me wonder: Is the shunt resistor value set by software,
or by replacing one probe with another ?

Guenter

> 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/