| Message ID | 1481316062-4999-1-git-send-email-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Fri, 9 Dec 2016 12:41:02 -0800, Guenter Roeck wrote: > Fix overflows seen when writing voltage and temperature limit attributes. > > The value passed to DIV_ROUND_CLOSEST() needs to be clamped, and the > value parameter passed to nct7802_write_fan_min() is an unsigned long. > > Also, writing values larger than 2700000 into a limit attribute results > in writing 0 into the chip's limit registers. You are only talking about _fan_ limits, right? > The exact behavior when > writing this value is unspecified. For consistency, report a limit of > 1350000 if the chip register reads 0. This may be wrong, and the chip > behavior should be verified with the actual chip, but it is better than > reporting a value of 0 (which, if written, results in writing a value > of 0x1fff into the chip register). This fix is good by doesn't seem to be related with the overflows? > > Fixes: 3434f3783580 ("hwmon: Driver for Nuvoton NCT7802Y") > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > v2: When reading a register value of 0 from a limit register, > report it as speed of 1350000. > Avoid overflow due to different variable types passed to > nct7802_write_fan_min(). > Fix low limit in store_temp (-128, not -127 as in v1). > > drivers/hwmon/nct7802.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/hwmon/nct7802.c b/drivers/hwmon/nct7802.c > index 3ce33d244cc0..12b94b094c0d 100644 > --- a/drivers/hwmon/nct7802.c > +++ b/drivers/hwmon/nct7802.c > @@ -259,13 +259,15 @@ static int nct7802_read_fan_min(struct nct7802_data *data, u8 reg_fan_low, > ret = 0; > else if (ret) > ret = DIV_ROUND_CLOSEST(1350000U, ret); > + else > + ret = 1350000U; > abort: > mutex_unlock(&data->access_lock); > return ret; > } > > static int nct7802_write_fan_min(struct nct7802_data *data, u8 reg_fan_low, > - u8 reg_fan_high, unsigned int limit) > + u8 reg_fan_high, unsigned long limit) > { > int err; > > @@ -326,8 +328,8 @@ static int nct7802_write_voltage(struct nct7802_data *data, int nr, int index, > int shift = 8 - REG_VOLTAGE_LIMIT_MSB_SHIFT[index - 1][nr]; > int err; > > + voltage = clamp_val(voltage, 0, 0x3ff * nct7802_vmul[nr]); > voltage = DIV_ROUND_CLOSEST(voltage, nct7802_vmul[nr]); > - voltage = clamp_val(voltage, 0, 0x3ff); > > mutex_lock(&data->access_lock); > err = regmap_write(data->regmap, > @@ -402,7 +404,7 @@ static ssize_t store_temp(struct device *dev, struct device_attribute *attr, > if (err < 0) > return err; > > - val = clamp_val(DIV_ROUND_CLOSEST(val, 1000), -128, 127); > + val = DIV_ROUND_CLOSEST(clamp_val(val, -128000, 127000), 1000); > > err = regmap_write(data->regmap, nr, val & 0xff); > return err ? : count; Reviewed-by: Jean Delvare <jdelvare@suse.de>
Hi Jean, On 12/12/2016 02:03 AM, Jean Delvare wrote: > On Fri, 9 Dec 2016 12:41:02 -0800, Guenter Roeck wrote: >> Fix overflows seen when writing voltage and temperature limit attributes. >> >> The value passed to DIV_ROUND_CLOSEST() needs to be clamped, and the >> value parameter passed to nct7802_write_fan_min() is an unsigned long. >> >> Also, writing values larger than 2700000 into a limit attribute results >> in writing 0 into the chip's limit registers. > > You are only talking about _fan_ limits, right? > Yes. I'll clarify. >> The exact behavior when >> writing this value is unspecified. For consistency, report a limit of >> 1350000 if the chip register reads 0. This may be wrong, and the chip >> behavior should be verified with the actual chip, but it is better than >> reporting a value of 0 (which, if written, results in writing a value >> of 0x1fff into the chip register). > > This fix is good by doesn't seem to be related with the overflows? > Writing a limit larger than 2700000 results in writing 0 into the register, which is reported back as 0. 2699999 -> 1 -> 1350000, 2700000 -> 0 -> 0. I consider that an overflow situation. Thanks, Guenter >> >> Fixes: 3434f3783580 ("hwmon: Driver for Nuvoton NCT7802Y") >> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >> --- >> v2: When reading a register value of 0 from a limit register, >> report it as speed of 1350000. >> Avoid overflow due to different variable types passed to >> nct7802_write_fan_min(). >> Fix low limit in store_temp (-128, not -127 as in v1). >> >> drivers/hwmon/nct7802.c | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/hwmon/nct7802.c b/drivers/hwmon/nct7802.c >> index 3ce33d244cc0..12b94b094c0d 100644 >> --- a/drivers/hwmon/nct7802.c >> +++ b/drivers/hwmon/nct7802.c >> @@ -259,13 +259,15 @@ static int nct7802_read_fan_min(struct nct7802_data *data, u8 reg_fan_low, >> ret = 0; >> else if (ret) >> ret = DIV_ROUND_CLOSEST(1350000U, ret); >> + else >> + ret = 1350000U; >> abort: >> mutex_unlock(&data->access_lock); >> return ret; >> } >> >> static int nct7802_write_fan_min(struct nct7802_data *data, u8 reg_fan_low, >> - u8 reg_fan_high, unsigned int limit) >> + u8 reg_fan_high, unsigned long limit) >> { >> int err; >> >> @@ -326,8 +328,8 @@ static int nct7802_write_voltage(struct nct7802_data *data, int nr, int index, >> int shift = 8 - REG_VOLTAGE_LIMIT_MSB_SHIFT[index - 1][nr]; >> int err; >> >> + voltage = clamp_val(voltage, 0, 0x3ff * nct7802_vmul[nr]); >> voltage = DIV_ROUND_CLOSEST(voltage, nct7802_vmul[nr]); >> - voltage = clamp_val(voltage, 0, 0x3ff); >> >> mutex_lock(&data->access_lock); >> err = regmap_write(data->regmap, >> @@ -402,7 +404,7 @@ static ssize_t store_temp(struct device *dev, struct device_attribute *attr, >> if (err < 0) >> return err; >> >> - val = clamp_val(DIV_ROUND_CLOSEST(val, 1000), -128, 127); >> + val = DIV_ROUND_CLOSEST(clamp_val(val, -128000, 127000), 1000); >> >> err = regmap_write(data->regmap, nr, val & 0xff); >> return err ? : count; > > Reviewed-by: Jean Delvare <jdelvare@suse.de> > -- 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
On Mon, 12 Dec 2016 06:14:14 -0800, Guenter Roeck wrote: > Hi Jean, > > On 12/12/2016 02:03 AM, Jean Delvare wrote: > > On Fri, 9 Dec 2016 12:41:02 -0800, Guenter Roeck wrote: > >> Fix overflows seen when writing voltage and temperature limit attributes. > >> > >> The value passed to DIV_ROUND_CLOSEST() needs to be clamped, and the > >> value parameter passed to nct7802_write_fan_min() is an unsigned long. > >> > >> Also, writing values larger than 2700000 into a limit attribute results > >> in writing 0 into the chip's limit registers. > > > > You are only talking about _fan_ limits, right? > > > Yes. I'll clarify. > > >> The exact behavior when > >> writing this value is unspecified. For consistency, report a limit of > >> 1350000 if the chip register reads 0. This may be wrong, and the chip > >> behavior should be verified with the actual chip, but it is better than > >> reporting a value of 0 (which, if written, results in writing a value > >> of 0x1fff into the chip register). > > > > This fix is good by doesn't seem to be related with the overflows? > > > Writing a limit larger than 2700000 results in writing 0 into the register, > which is reported back as 0. 2699999 -> 1 -> 1350000, 2700000 -> 0 -> 0. > I consider that an overflow situation. OK, I understand, you are right.
diff --git a/drivers/hwmon/nct7802.c b/drivers/hwmon/nct7802.c index 3ce33d244cc0..12b94b094c0d 100644 --- a/drivers/hwmon/nct7802.c +++ b/drivers/hwmon/nct7802.c @@ -259,13 +259,15 @@ static int nct7802_read_fan_min(struct nct7802_data *data, u8 reg_fan_low, ret = 0; else if (ret) ret = DIV_ROUND_CLOSEST(1350000U, ret); + else + ret = 1350000U; abort: mutex_unlock(&data->access_lock); return ret; } static int nct7802_write_fan_min(struct nct7802_data *data, u8 reg_fan_low, - u8 reg_fan_high, unsigned int limit) + u8 reg_fan_high, unsigned long limit) { int err; @@ -326,8 +328,8 @@ static int nct7802_write_voltage(struct nct7802_data *data, int nr, int index, int shift = 8 - REG_VOLTAGE_LIMIT_MSB_SHIFT[index - 1][nr]; int err; + voltage = clamp_val(voltage, 0, 0x3ff * nct7802_vmul[nr]); voltage = DIV_ROUND_CLOSEST(voltage, nct7802_vmul[nr]); - voltage = clamp_val(voltage, 0, 0x3ff); mutex_lock(&data->access_lock); err = regmap_write(data->regmap, @@ -402,7 +404,7 @@ static ssize_t store_temp(struct device *dev, struct device_attribute *attr, if (err < 0) return err; - val = clamp_val(DIV_ROUND_CLOSEST(val, 1000), -128, 127); + val = DIV_ROUND_CLOSEST(clamp_val(val, -128000, 127000), 1000); err = regmap_write(data->regmap, nr, val & 0xff); return err ? : count;
Fix overflows seen when writing voltage and temperature limit attributes. The value passed to DIV_ROUND_CLOSEST() needs to be clamped, and the value parameter passed to nct7802_write_fan_min() is an unsigned long. Also, writing values larger than 2700000 into a limit attribute results in writing 0 into the chip's limit registers. The exact behavior when writing this value is unspecified. For consistency, report a limit of 1350000 if the chip register reads 0. This may be wrong, and the chip behavior should be verified with the actual chip, but it is better than reporting a value of 0 (which, if written, results in writing a value of 0x1fff into the chip register). Fixes: 3434f3783580 ("hwmon: Driver for Nuvoton NCT7802Y") Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- v2: When reading a register value of 0 from a limit register, report it as speed of 1350000. Avoid overflow due to different variable types passed to nct7802_write_fan_min(). Fix low limit in store_temp (-128, not -127 as in v1). drivers/hwmon/nct7802.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)