| Message ID | 20171122220843.18309-1-rlippert@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Nov 22, 2017 at 02:08:43PM -0800, Robert Lippert wrote: > Power values in the 100s of watt range can easily blow past > 32bit math limits when processing everything in microwatts. > > Use 64bit math instead to avoid these issues on common 32bit ARM > BMC platforms. > > Signed-off-by: Robert Lippert <rlippert@google.com> > --- > drivers/hwmon/pmbus/pmbus_core.c | 21 ++++++++++++--------- > 1 file changed, 12 insertions(+), 9 deletions(-) > > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c > index 52a58b8b6e1b..f4efea9f282e 100644 > --- a/drivers/hwmon/pmbus/pmbus_core.c > +++ b/drivers/hwmon/pmbus/pmbus_core.c > @@ -499,8 +499,8 @@ static long pmbus_reg2data_linear(struct pmbus_data *data, > static long pmbus_reg2data_direct(struct pmbus_data *data, > struct pmbus_sensor *sensor) > { > - long val = (s16) sensor->data; > - long m, b, R; > + s64 val = (s16) sensor->data; > + s64 m, b, R; > > m = data->info->m[sensor->class]; > b = data->info->b[sensor->class]; > @@ -528,11 +528,13 @@ static long pmbus_reg2data_direct(struct pmbus_data *data, > R--; > } > while (R < 0) { > - val = DIV_ROUND_CLOSEST(val, 10); > + do_div(val, 10); Any reason to not use DIV_ROUND_CLOSEST_ULL ? > R++; > } > > - return (val - b) / m; > + val = val - b; > + do_div(val, m); > + return val; Can this overflow ? > } > > /* > @@ -656,7 +658,8 @@ static u16 pmbus_data2reg_linear(struct pmbus_data *data, > static u16 pmbus_data2reg_direct(struct pmbus_data *data, > struct pmbus_sensor *sensor, long val) > { > - long m, b, R; > + s64 m, b, R; > + s64 val64 = val; > > m = data->info->m[sensor->class]; > b = data->info->b[sensor->class]; > @@ -673,18 +676,18 @@ static u16 pmbus_data2reg_direct(struct pmbus_data *data, > R -= 3; /* Adjust R and b for data in milli-units */ > b *= 1000; > } > - val = val * m + b; > + val64 = val64 * m + b; > > while (R > 0) { > - val *= 10; > + val64 *= 10; > R--; > } > while (R < 0) { > - val = DIV_ROUND_CLOSEST(val, 10); > + do_div(val64, 10); Same here. > R++; > } > > - return val; > + return (u16) val64; Can this overflow ? > } > > static u16 pmbus_data2reg_vid(struct pmbus_data *data, > -- > 2.15.0.448.gf294e3d99a-goog > -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Robert,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on hwmon/hwmon-next]
[also build test WARNING on v4.14 next-20171124]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Robert-Lippert/hwmon-pmbus-Use-64bit-math-for-DIRECT-format-values/20171126-031040
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: xtensa-allmodconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 4.9.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=xtensa
All warnings (new ones prefixed by >>):
In file included from ./arch/xtensa/include/generated/asm/div64.h:1:0,
from include/linux/kernel.h:173,
from include/linux/list.h:9,
from include/linux/wait.h:7,
from include/linux/wait_bit.h:8,
from include/linux/fs.h:6,
from include/linux/debugfs.h:18,
from drivers/hwmon//pmbus/pmbus_core.c:22:
drivers/hwmon//pmbus/pmbus_core.c: In function 'pmbus_reg2data_direct':
include/asm-generic/div64.h:222:28: warning: comparison of distinct pointer types lacks a cast
(void)(((typeof((n)) *)0) == ((uint64_t *)0)); \
^
>> drivers/hwmon//pmbus/pmbus_core.c:531:3: note: in expansion of macro 'do_div'
do_div(val, 10);
^
include/asm-generic/div64.h:222:28: warning: comparison of distinct pointer types lacks a cast
(void)(((typeof((n)) *)0) == ((uint64_t *)0)); \
^
drivers/hwmon//pmbus/pmbus_core.c:536:2: note: in expansion of macro 'do_div'
do_div(val, m);
^
drivers/hwmon//pmbus/pmbus_core.c: In function 'pmbus_data2reg_direct':
include/asm-generic/div64.h:222:28: warning: comparison of distinct pointer types lacks a cast
(void)(((typeof((n)) *)0) == ((uint64_t *)0)); \
^
drivers/hwmon//pmbus/pmbus_core.c:686:3: note: in expansion of macro 'do_div'
do_div(val64, 10);
^
vim +/do_div +531 drivers/hwmon//pmbus/pmbus_core.c
494
495 /*
496 * Convert direct sensor values to milli- or micro-units
497 * depending on sensor type.
498 */
499 static long pmbus_reg2data_direct(struct pmbus_data *data,
500 struct pmbus_sensor *sensor)
501 {
502 s64 val = (s16) sensor->data;
503 s64 m, b, R;
504
505 m = data->info->m[sensor->class];
506 b = data->info->b[sensor->class];
507 R = data->info->R[sensor->class];
508
509 if (m == 0)
510 return 0;
511
512 /* X = 1/m * (Y * 10^-R - b) */
513 R = -R;
514 /* scale result to milli-units for everything but fans */
515 if (sensor->class != PSC_FAN) {
516 R += 3;
517 b *= 1000;
518 }
519
520 /* scale result to micro-units for power sensors */
521 if (sensor->class == PSC_POWER) {
522 R += 3;
523 b *= 1000;
524 }
525
526 while (R > 0) {
527 val *= 10;
528 R--;
529 }
530 while (R < 0) {
> 531 do_div(val, 10);
532 R++;
533 }
534
535 val = val - b;
536 do_div(val, m);
537 return val;
538 }
539
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Wed, Nov 22, 2017 at 2:28 PM, Guenter Roeck <linux@roeck-us.net> wrote: > On Wed, Nov 22, 2017 at 02:08:43PM -0800, Robert Lippert wrote: >> Power values in the 100s of watt range can easily blow past >> 32bit math limits when processing everything in microwatts. >> >> Use 64bit math instead to avoid these issues on common 32bit ARM >> BMC platforms. >> >> Signed-off-by: Robert Lippert <rlippert@google.com> >> --- >> drivers/hwmon/pmbus/pmbus_core.c | 21 ++++++++++++--------- >> 1 file changed, 12 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c >> index 52a58b8b6e1b..f4efea9f282e 100644 >> --- a/drivers/hwmon/pmbus/pmbus_core.c >> +++ b/drivers/hwmon/pmbus/pmbus_core.c >> @@ -499,8 +499,8 @@ static long pmbus_reg2data_linear(struct pmbus_data *data, >> static long pmbus_reg2data_direct(struct pmbus_data *data, >> struct pmbus_sensor *sensor) >> { >> - long val = (s16) sensor->data; >> - long m, b, R; >> + s64 val = (s16) sensor->data; >> + s64 m, b, R; >> >> m = data->info->m[sensor->class]; >> b = data->info->b[sensor->class]; >> @@ -528,11 +528,13 @@ static long pmbus_reg2data_direct(struct pmbus_data *data, >> R--; >> } >> while (R < 0) { >> - val = DIV_ROUND_CLOSEST(val, 10); >> + do_div(val, 10); > > Any reason to not use DIV_ROUND_CLOSEST_ULL ? That macro doesn't quite work for signed division. v2 changes this to use the signed 64bit division functions and I emulated the "round closest" by just adding 5 before dividing. > >> R++; >> } >> >> - return (val - b) / m; >> + val = val - b; >> + do_div(val, m); >> + return val; > > Can this overflow ? Added clamp() to the return values in v2 to prevent overflow when returning a narrower type. > >> } >> >> /* >> @@ -656,7 +658,8 @@ static u16 pmbus_data2reg_linear(struct pmbus_data *data, >> static u16 pmbus_data2reg_direct(struct pmbus_data *data, >> struct pmbus_sensor *sensor, long val) >> { >> - long m, b, R; >> + s64 m, b, R; >> + s64 val64 = val; >> >> m = data->info->m[sensor->class]; >> b = data->info->b[sensor->class]; >> @@ -673,18 +676,18 @@ static u16 pmbus_data2reg_direct(struct pmbus_data *data, >> R -= 3; /* Adjust R and b for data in milli-units */ >> b *= 1000; >> } >> - val = val * m + b; >> + val64 = val64 * m + b; >> >> while (R > 0) { >> - val *= 10; >> + val64 *= 10; >> R--; >> } >> while (R < 0) { >> - val = DIV_ROUND_CLOSEST(val, 10); >> + do_div(val64, 10); > > Same here. > >> R++; >> } >> >> - return val; >> + return (u16) val64; > > Can this overflow ? > >> } >> >> static u16 pmbus_data2reg_vid(struct pmbus_data *data, >> -- >> 2.15.0.448.gf294e3d99a-goog >> -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c index 52a58b8b6e1b..f4efea9f282e 100644 --- a/drivers/hwmon/pmbus/pmbus_core.c +++ b/drivers/hwmon/pmbus/pmbus_core.c @@ -499,8 +499,8 @@ static long pmbus_reg2data_linear(struct pmbus_data *data, static long pmbus_reg2data_direct(struct pmbus_data *data, struct pmbus_sensor *sensor) { - long val = (s16) sensor->data; - long m, b, R; + s64 val = (s16) sensor->data; + s64 m, b, R; m = data->info->m[sensor->class]; b = data->info->b[sensor->class]; @@ -528,11 +528,13 @@ static long pmbus_reg2data_direct(struct pmbus_data *data, R--; } while (R < 0) { - val = DIV_ROUND_CLOSEST(val, 10); + do_div(val, 10); R++; } - return (val - b) / m; + val = val - b; + do_div(val, m); + return val; } /* @@ -656,7 +658,8 @@ static u16 pmbus_data2reg_linear(struct pmbus_data *data, static u16 pmbus_data2reg_direct(struct pmbus_data *data, struct pmbus_sensor *sensor, long val) { - long m, b, R; + s64 m, b, R; + s64 val64 = val; m = data->info->m[sensor->class]; b = data->info->b[sensor->class]; @@ -673,18 +676,18 @@ static u16 pmbus_data2reg_direct(struct pmbus_data *data, R -= 3; /* Adjust R and b for data in milli-units */ b *= 1000; } - val = val * m + b; + val64 = val64 * m + b; while (R > 0) { - val *= 10; + val64 *= 10; R--; } while (R < 0) { - val = DIV_ROUND_CLOSEST(val, 10); + do_div(val64, 10); R++; } - return val; + return (u16) val64; } static u16 pmbus_data2reg_vid(struct pmbus_data *data,
Power values in the 100s of watt range can easily blow past 32bit math limits when processing everything in microwatts. Use 64bit math instead to avoid these issues on common 32bit ARM BMC platforms. Signed-off-by: Robert Lippert <rlippert@google.com> --- drivers/hwmon/pmbus/pmbus_core.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)