Message ID | 20190508170035.19671-3-f.fainelli@gmail.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | hwmon: scmi: Scale values to target desired HWMON units | expand |
Hi Florian, On Wed, May 08, 2019 at 10:00:35AM -0700, Florian Fainelli wrote: > If the SCMI firmware implementation is reporting values in a scale that > is different from the HWMON units, we need to scale up or down the value > according to how far appart they are. > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > drivers/hwmon/scmi-hwmon.c | 46 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 46 insertions(+) > > diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c > index a80183a488c5..4399372e2131 100644 > --- a/drivers/hwmon/scmi-hwmon.c > +++ b/drivers/hwmon/scmi-hwmon.c > @@ -7,6 +7,7 @@ > */ > > #include <linux/hwmon.h> > +#include <linux/limits.h> > #include <linux/module.h> > #include <linux/scmi_protocol.h> > #include <linux/slab.h> > @@ -18,6 +19,47 @@ struct scmi_sensors { > const struct scmi_sensor_info **info[hwmon_max]; > }; > > +static inline u64 __pow10(u8 x) > +{ > + u64 r = 1; > + > + while (x--) > + r *= 10; > + > + return r; > +} > + > +static int scmi_hwmon_scale(const struct scmi_sensor_info *sensor, u64 *value) > +{ > + s8 scale = sensor->scale; > + u64 f; > + > + switch (sensor->type) { > + case TEMPERATURE_C: > + case VOLTAGE: > + case CURRENT: > + scale += 3; > + break; > + case POWER: > + case ENERGY: > + scale += 6; > + break; > + default: > + break; > + } > + > + f = __pow10(abs(scale)); > + if (f == U64_MAX) > + return -E2BIG; Unfortunately that is not how integer overflows work. A test program with increasing values of scale reports: 0: 1 ... 18: 1000000000000000000 19: 10000000000000000000 20: 7766279631452241920 21: 3875820019684212736 22: 1864712049423024128 23: 200376420520689664 24: 2003764205206896640 ... 61: 11529215046068469760 62: 4611686018427387904 63: 9223372036854775808 64: 0 ... You'll have to check for abs(scale) > 19 if you want to report overflows. Guenter > + > + if (scale > 0) > + *value *= f; > + else > + *value = div64_u64(*value, f); > + > + return 0; > +} > + > static int scmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type, > u32 attr, int channel, long *val) > { > @@ -29,6 +71,10 @@ static int scmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type, > > sensor = *(scmi_sensors->info[type] + channel); > ret = h->sensor_ops->reading_get(h, sensor->id, false, &value); > + if (ret) > + return ret; > + > + ret = scmi_hwmon_scale(sensor, value); > if (!ret) > *val = value; > > -- > 2.17.1 >
On 5/8/19 11:32 AM, Guenter Roeck wrote: > Hi Florian, > > On Wed, May 08, 2019 at 10:00:35AM -0700, Florian Fainelli wrote: >> If the SCMI firmware implementation is reporting values in a scale that >> is different from the HWMON units, we need to scale up or down the value >> according to how far appart they are. >> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >> --- >> drivers/hwmon/scmi-hwmon.c | 46 ++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 46 insertions(+) >> >> diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c >> index a80183a488c5..4399372e2131 100644 >> --- a/drivers/hwmon/scmi-hwmon.c >> +++ b/drivers/hwmon/scmi-hwmon.c >> @@ -7,6 +7,7 @@ >> */ >> >> #include <linux/hwmon.h> >> +#include <linux/limits.h> >> #include <linux/module.h> >> #include <linux/scmi_protocol.h> >> #include <linux/slab.h> >> @@ -18,6 +19,47 @@ struct scmi_sensors { >> const struct scmi_sensor_info **info[hwmon_max]; >> }; >> >> +static inline u64 __pow10(u8 x) >> +{ >> + u64 r = 1; >> + >> + while (x--) >> + r *= 10; >> + >> + return r; >> +} >> + >> +static int scmi_hwmon_scale(const struct scmi_sensor_info *sensor, u64 *value) >> +{ >> + s8 scale = sensor->scale; >> + u64 f; >> + >> + switch (sensor->type) { >> + case TEMPERATURE_C: >> + case VOLTAGE: >> + case CURRENT: >> + scale += 3; >> + break; >> + case POWER: >> + case ENERGY: >> + scale += 6; >> + break; >> + default: >> + break; >> + } >> + >> + f = __pow10(abs(scale)); >> + if (f == U64_MAX) >> + return -E2BIG; > > Unfortunately that is not how integer overflows work. > > A test program with increasing values of scale reports: > > 0: 1 > ... > 18: 1000000000000000000 > 19: 10000000000000000000 > 20: 7766279631452241920 > 21: 3875820019684212736 > 22: 1864712049423024128 > 23: 200376420520689664 > 24: 2003764205206896640 > ... > 61: 11529215046068469760 > 62: 4611686018427387904 > 63: 9223372036854775808 > 64: 0 > ... > > You'll have to check for abs(scale) > 19 if you want to report overflows. Yes silly me, my test program was flawed, thanks for pointing out that. You are okay with returning E2BIG when we overflow?
On Wed, May 08, 2019 at 11:34:50AM -0700, Florian Fainelli wrote: > On 5/8/19 11:32 AM, Guenter Roeck wrote: > > Hi Florian, > > > > On Wed, May 08, 2019 at 10:00:35AM -0700, Florian Fainelli wrote: > >> If the SCMI firmware implementation is reporting values in a scale that > >> is different from the HWMON units, we need to scale up or down the value > >> according to how far appart they are. > >> > >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > >> --- > >> drivers/hwmon/scmi-hwmon.c | 46 ++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 46 insertions(+) > >> > >> diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c > >> index a80183a488c5..4399372e2131 100644 > >> --- a/drivers/hwmon/scmi-hwmon.c > >> +++ b/drivers/hwmon/scmi-hwmon.c > >> @@ -7,6 +7,7 @@ > >> */ > >> > >> #include <linux/hwmon.h> > >> +#include <linux/limits.h> > >> #include <linux/module.h> > >> #include <linux/scmi_protocol.h> > >> #include <linux/slab.h> > >> @@ -18,6 +19,47 @@ struct scmi_sensors { > >> const struct scmi_sensor_info **info[hwmon_max]; > >> }; > >> > >> +static inline u64 __pow10(u8 x) > >> +{ > >> + u64 r = 1; > >> + > >> + while (x--) > >> + r *= 10; > >> + > >> + return r; > >> +} > >> + > >> +static int scmi_hwmon_scale(const struct scmi_sensor_info *sensor, u64 *value) > >> +{ > >> + s8 scale = sensor->scale; > >> + u64 f; > >> + > >> + switch (sensor->type) { > >> + case TEMPERATURE_C: > >> + case VOLTAGE: > >> + case CURRENT: > >> + scale += 3; > >> + break; > >> + case POWER: > >> + case ENERGY: > >> + scale += 6; > >> + break; > >> + default: > >> + break; > >> + } > >> + > >> + f = __pow10(abs(scale)); > >> + if (f == U64_MAX) > >> + return -E2BIG; > > > > Unfortunately that is not how integer overflows work. > > > > A test program with increasing values of scale reports: > > > > 0: 1 > > ... > > 18: 1000000000000000000 > > 19: 10000000000000000000 > > 20: 7766279631452241920 > > 21: 3875820019684212736 > > 22: 1864712049423024128 > > 23: 200376420520689664 > > 24: 2003764205206896640 > > ... > > 61: 11529215046068469760 > > 62: 4611686018427387904 > > 63: 9223372036854775808 > > 64: 0 > > ... > > > > You'll have to check for abs(scale) > 19 if you want to report overflows. > > Yes silly me, my test program was flawed, thanks for pointing out that. > You are okay with returning E2BIG when we overflow? Yes. Thanks, Guenter
diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c index a80183a488c5..4399372e2131 100644 --- a/drivers/hwmon/scmi-hwmon.c +++ b/drivers/hwmon/scmi-hwmon.c @@ -7,6 +7,7 @@ */ #include <linux/hwmon.h> +#include <linux/limits.h> #include <linux/module.h> #include <linux/scmi_protocol.h> #include <linux/slab.h> @@ -18,6 +19,47 @@ struct scmi_sensors { const struct scmi_sensor_info **info[hwmon_max]; }; +static inline u64 __pow10(u8 x) +{ + u64 r = 1; + + while (x--) + r *= 10; + + return r; +} + +static int scmi_hwmon_scale(const struct scmi_sensor_info *sensor, u64 *value) +{ + s8 scale = sensor->scale; + u64 f; + + switch (sensor->type) { + case TEMPERATURE_C: + case VOLTAGE: + case CURRENT: + scale += 3; + break; + case POWER: + case ENERGY: + scale += 6; + break; + default: + break; + } + + f = __pow10(abs(scale)); + if (f == U64_MAX) + return -E2BIG; + + if (scale > 0) + *value *= f; + else + *value = div64_u64(*value, f); + + return 0; +} + static int scmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel, long *val) { @@ -29,6 +71,10 @@ static int scmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type, sensor = *(scmi_sensors->info[type] + channel); ret = h->sensor_ops->reading_get(h, sensor->id, false, &value); + if (ret) + return ret; + + ret = scmi_hwmon_scale(sensor, value); if (!ret) *val = value;
If the SCMI firmware implementation is reporting values in a scale that is different from the HWMON units, we need to scale up or down the value according to how far appart they are. Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- drivers/hwmon/scmi-hwmon.c | 46 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+)