linux-kernel - Re: [lm-sensors] [PATCH] hwmon: (ina2xx) Change register cache to signed

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] [day] [month] [year] [list]

Date:	Sun, 8 Jun 2014 21:54:07 +0100
From:	Fabio Baltieri <fabio.baltieri@...il.com>
To:	Guenter Roeck <linux@...ck-us.net>
Cc:	linux-kernel@...r.kernel.org, Lothar Felten <l-felten@...com>,
	lm-sensors@...sensors.org
Subject: Re: [lm-sensors] [PATCH] hwmon: (ina2xx) Change register cache to signed

On Sun, Jun 8, 2014 at 9:30 PM, Guenter Roeck <linux@...ck-us.net> wrote:
> On Sun, Jun 08, 2014 at 01:16:00PM -0700, Guenter Roeck wrote:
>> On Sat, Jun 07, 2014 at 09:47:01PM +0100, Fabio Baltieri wrote:
>> > All devices supported by the ina2xx driver are bidirectional and reports
>> > the measured value as a signed 16 bit, but the current driver
>> > implementation caches the number as an u16, leading to an incorrect sign
>> > extension when reporting to the userspace in ina2xx_get_value().
>> >
>> > This patch fixes the problem by using a s16 instead, and has been tested
>> > on an INA219.
>> >
>> > Signed-off-by: Fabio Baltieri <fabio.baltieri@...il.com>
>>
>> Applied.
>>
> Actually, no, this won't work. The statement above is only correct for current
> and shunt voltage measurements, but not for power measurements and not for bus
> voltage measurements. Changing the register to s16 won't help; conversion needs
> to be done in ina2xx_get_value() for shunt voltage and current measurement only.
> Otherwise we just move the bug from current/shunt voltage measurements to
> power / bus voltage measurements.
>
> Even more interesting, the power is supposed to be the product of Bus voltage
> and current, and the latter can be negative. However, the power register
> description does not suggest that the upper bit would be a sign bit. So there
> is some discrepancy in the datasheet, and we'll need some real-world data to
> understand if the upper power bit is signed or not.

Hi Guenter,

looks like you're right here, I wasn't paying too attention to the
power register and it actually always reads positive, even when the
current is flowing in the reverse direction, real data from the
ina219:

[55694.263502] shunt_voltage=fb46
[55694.263691] bus_voltage=20c2
[55694.263847] power=00fe
[55694.263954] current=fb46

I'll send a v2 to only cast the two signed register then.

Many thanks for spotting this!

Fabio

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