Re: [PATCH v3 3/3] hwmon: Driver for Texas Instruments INA238

On 10/31/21 10:55 PM, Nathan Rossi wrote:

On Mon, 1 Nov 2021 at 13:48, Guenter Roeck [off-list ref] wrote:

quoted

On 10/31/21 7:20 PM, Nathan Rossi wrote:
[ ... ]

quoted

quoted

quoted

+
+     if (attr != hwmon_in_max && attr != hwmon_in_min)
+             return -EOPNOTSUPP;
+
+     /* convert decimal to register value */
+     switch (channel) {
+     case 0:
+             /* signed value, clamp to max range +/-163 mV */
+             regval = clamp_val(val, -163, 163);
+             regval = (regval * 1000L * (4 - (int)data->gain + 1)) /

nit: The typecast "(int)" is not needed here.

Due to the unsigned type of gain, it causes promotion of regval (and
the rest of the numerator) to unsigned long which causes issues with
negative numbers on the divide. It makes more sense for gain to be an
int to begin with, I will change it to int to avoid the need for type
casting.

Are you sure ? I initially thought that as well and wrote a little test
program with that expression, but it didn't do the promotion to unsigned.

It definitely calculates incorrectly at run time (on an arm 32-bit
platform), looking at the gcc output from -fdump-tree-original reveals
some more insight. Which is that the promotion to long overrides the
unsigned (from the 1000L) on long=64 but not on long=32.

Where regval is int, and gain is unsigned int (u32).

regval = (regval * 1000L * (4 - gain + 1)) / 5;
-> armv7-a (invalid)
regval = (int) ((((long unsigned int) regval * (long unsigned int) (5
- gain)) * 1000) / 5);
-> x86-64 (valid result)
regval = (int) ((unsigned int) (gain * 4294967096 + 1000) * (unsigned
int) regval);

note: 4294967096 is -800, 1000 * (4 - gain + 1) => (-800 * gain) + 1000

Slight variation without the 1000 being long.

regval = (regval * 1000 * (4 - gain + 1)) / 5;
-> armv7-a (invalid)
regval = (int) ((((unsigned int) regval * (5 - gain)) * 1000) / 5);
-> x86-64 (invalid)
regval = (int) ((((unsigned int) regval * (5 - gain)) * 1000) / 5);

regval = (regval * 1000LL * (4 - gain + 1)) / 5;
-> armv7-a (valid)
regval = (int) ((unsigned int) (gain * 4294967096 + 1000) * (unsigned
int) regval);
-> x86-64 (valid)
regval = (int) ((unsigned int) (gain * 4294967096 + 1000) * (unsigned
int) regval);

I think it still makes sense to change gain to be int, and avoid the
unsigned type issues.

Thanks for the details. I agree, changing gain to int makes sense.

Thanks,
Guenter