* Re: [lm-sensors] PATCH: fix various sysfs callback function issues
2008-12-11 22:21 [lm-sensors] PATCH: fix various sysfs callback function issues in Hans de Goede
@ 2008-12-13 22:18 ` Jean Delvare
2008-12-14 14:32 ` Hans de Goede
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Jean Delvare @ 2008-12-13 22:18 UTC (permalink / raw)
To: lm-sensors
Hi Hans,
On Thu, 11 Dec 2008 23:21:30 +0100, Hans de Goede wrote:
> Again see attachment.
Review:
> While working on adding f8000 support I noticed that various of the
> store sysfs functions (and a few of the show also) had issues.
>
> This patch fixes the following issues in these functions:
> * store: storing the result of strto[u]l in an int, resulting in a possible
> overflow before boundary checking
> * store: use of f71882fg_update_device(), we don't want to read the whole
> device in store functions, just the registers we need
> * store: use of cached register values instead of reading the needed regs
> in the store function, including cases where f71882fg_update_device() was
> not used, this could cause real isues
> * show: shown value is a calculation of 2 or more cached register reads,
> without locking the data struct.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> --- linux/drivers/hwmon/f71882fg.c.07-applied 2008-12-11 21:38:44.000000000 +0100
> +++ linux/drivers/hwmon/f71882fg.c 2008-12-11 22:54:21.000000000 +0100
> @@ -832,6 +832,7 @@ static ssize_t store_fan_full_speed(stru
> val = fan_to_reg(val);
>
> mutex_lock(&data->update_lock);
> + data->pwm_enable = f71882fg_read8(data, F71882FG_REG_PWM_ENABLE);
> if (data->pwm_enable & (1 << (2 * nr)))
> /* PWM mode */
> count = -EINVAL;
> @@ -862,9 +863,10 @@ static ssize_t store_fan_beep(struct dev
> {
> struct f71882fg_data *data = dev_get_drvdata(dev);
> int nr = to_sensor_dev_attr_2(devattr)->index;
> - int val = simple_strtoul(buf, NULL, 10);
> + unsigned long val = simple_strtoul(buf, NULL, 10);
>
> mutex_lock(&data->update_lock);
> + data->fan_beep = f71882fg_read8(data, F71882FG_REG_FAN_BEEP);
> if (val)
> data->fan_beep |= 1 << nr;
> else
> @@ -909,10 +911,8 @@ static ssize_t store_in_max(struct devic
> *devattr, const char *buf, size_t count)
> {
> struct f71882fg_data *data = dev_get_drvdata(dev);
> - int val = simple_strtoul(buf, NULL, 10) / 8;
> -
> - if (val > 255)
> - val = 255;
> + long val = simple_strtol(buf, NULL, 10) / 8;
> + val = SENSORS_LIMIT(val, 0, 255);
>
> mutex_lock(&data->update_lock);
> f71882fg_write8(data, F71882FG_REG_IN1_HIGH, val);
> @@ -939,9 +939,10 @@ static ssize_t store_in_beep(struct devi
> {
> struct f71882fg_data *data = dev_get_drvdata(dev);
> int nr = to_sensor_dev_attr_2(devattr)->index;
> - int val = simple_strtoul(buf, NULL, 10);
> + unsigned long val = simple_strtoul(buf, NULL, 10);
>
> mutex_lock(&data->update_lock);
> + data->in_beep = f71882fg_read8(data, F71882FG_REG_IN_BEEP);
> if (val)
> data->in_beep |= 1 << nr;
> else
> @@ -988,10 +989,8 @@ static ssize_t store_temp_max(struct dev
> {
> struct f71882fg_data *data = dev_get_drvdata(dev);
> int nr = to_sensor_dev_attr_2(devattr)->index;
> - int val = simple_strtoul(buf, NULL, 10) / 1000;
> -
> - if (val > 255)
> - val = 255;
> + long val = simple_strtol(buf, NULL, 10) / 1000;
> + val = SENSORS_LIMIT(val, 0, 255);
>
> mutex_lock(&data->update_lock);
> f71882fg_write8(data, F71882FG_REG_TEMP_HIGH(nr), val);
> @@ -1006,9 +1005,13 @@ static ssize_t show_temp_max_hyst(struct
> {
> struct f71882fg_data *data = f71882fg_update_device(dev);
> int nr = to_sensor_dev_attr_2(devattr)->index;
> + int temp_max_hyst;
> +
> + mutex_lock(&data->update_lock);
> + temp_max_hyst = (data->temp_high[nr] - data->temp_hyst[nr]) * 1000;
> + mutex_unlock(&data->update_lock);
>
> - return sprintf(buf, "%d\n",
> - (data->temp_high[nr] - data->temp_hyst[nr]) * 1000);
> + return sprintf(buf, "%d\n", temp_max_hyst);
> }
>
> static ssize_t store_temp_max_hyst(struct device *dev, struct device_attribute
> @@ -1016,37 +1019,37 @@ static ssize_t store_temp_max_hyst(struc
> {
> struct f71882fg_data *data = dev_get_drvdata(dev);
> int nr = to_sensor_dev_attr_2(devattr)->index;
> - int val = simple_strtoul(buf, NULL, 10) / 1000;
> + long val = simple_strtol(buf, NULL, 10) / 1000;
> ssize_t ret = count;
> + u8 reg;
>
> mutex_lock(&data->update_lock);
>
> /* convert abs to relative and check */
> - val = data->temp_high[nr] - val;
> - if (val < 0 || val > 15) {
> - ret = -EINVAL;
> - goto store_temp_max_hyst_exit;
> - }
> -
> + reg = f71882fg_read8(data, F71882FG_REG_TEMP_HIGH(nr));
> + val = SENSORS_LIMIT(val, reg - 15, reg);
> + val = reg - val;
> data->temp_hyst[nr] = val;
> + data->temp_high[nr] = reg;
This would gain in efficiency and, I suspect, clarity by using
data->temp_high[nr] directly instead of aliasing it to reg.
>
> /* convert value to register contents */
> switch (nr) {
> case 1:
> - val = val << 4;
> + reg = val << 4;
> break;
This is overwriting 4 bits of F71882FG_REG_TEMP_HYST1 with 0s. I guess
these are reserved/unused bits, but I still consider this bad practice.
> case 2:
> - val = val | (data->temp_hyst[3] << 4);
> + reg = f71882fg_read8(data, F71882FG_REG_TEMP_HYST23);
> + reg = (reg & 0xf0) | val;
> break;
> case 3:
> - val = data->temp_hyst[2] | (val << 4);
> + reg = f71882fg_read8(data, F71882FG_REG_TEMP_HYST23);
> + reg = (reg & 0x0f) | (val << 4);
> break;
> }
>
> f71882fg_write8(data, (nr = 1) ? F71882FG_REG_TEMP_HYST1 :
> - F71882FG_REG_TEMP_HYST23, val);
> + F71882FG_REG_TEMP_HYST23, reg);
>
> -store_temp_max_hyst_exit:
> mutex_unlock(&data->update_lock);
> return ret;
> }
> @@ -1065,10 +1068,8 @@ static ssize_t store_temp_crit(struct de
> {
> struct f71882fg_data *data = dev_get_drvdata(dev);
> int nr = to_sensor_dev_attr_2(devattr)->index;
> - int val = simple_strtoul(buf, NULL, 10) / 1000;
> -
> - if (val > 255)
> - val = 255;
> + long val = simple_strtol(buf, NULL, 10) / 1000;
> + val = SENSORS_LIMIT(val, 0, 255);
>
> mutex_lock(&data->update_lock);
> f71882fg_write8(data, F71882FG_REG_TEMP_OVT(nr), val);
> @@ -1083,9 +1084,13 @@ static ssize_t show_temp_crit_hyst(struc
> {
> struct f71882fg_data *data = f71882fg_update_device(dev);
> int nr = to_sensor_dev_attr_2(devattr)->index;
> + int temp_crit_hyst;
> +
> + mutex_lock(&data->update_lock);
> + temp_crit_hyst = (data->temp_ovt[nr] - data->temp_hyst[nr]) * 1000;
> + mutex_unlock(&data->update_lock);
>
> - return sprintf(buf, "%d\n",
> - (data->temp_ovt[nr] - data->temp_hyst[nr]) * 1000);
> + return sprintf(buf, "%d\n", temp_crit_hyst);
> }
>
> static ssize_t show_temp_type(struct device *dev, struct device_attribute
> @@ -1114,9 +1119,10 @@ static ssize_t store_temp_beep(struct de
> {
> struct f71882fg_data *data = dev_get_drvdata(dev);
> int nr = to_sensor_dev_attr_2(devattr)->index;
> - int val = simple_strtoul(buf, NULL, 10);
> + unsigned long val = simple_strtoul(buf, NULL, 10);
>
> mutex_lock(&data->update_lock);
> + data->temp_beep = f71882fg_read8(data, F71882FG_REG_TEMP_BEEP);
> if (val)
> data->temp_beep |= 1 << nr;
> else
> @@ -1157,16 +1163,16 @@ static ssize_t show_pwm(struct device *d
> {
> struct f71882fg_data *data = f71882fg_update_device(dev);
> int val, nr = to_sensor_dev_attr_2(devattr)->index;
> + mutex_lock(&data->update_lock);
> if (data->pwm_enable & (1 << (2 * nr)))
> /* PWM mode */
> val = data->pwm[nr];
> else {
> /* RPM mode */
> - mutex_lock(&data->update_lock);
> val = 255 * fan_from_reg(data->fan_target[nr])
> / fan_from_reg(data->fan_full_speed[nr]);
> - mutex_unlock(&data->update_lock);
> }
> + mutex_unlock(&data->update_lock);
> return sprintf(buf, "%d\n", val);
> }
>
> @@ -1174,23 +1180,26 @@ static ssize_t store_pwm(struct device *
> struct device_attribute *devattr, const char *buf,
> size_t count)
> {
> - /* struct f71882fg_data *data = dev_get_drvdata(dev); */
> - struct f71882fg_data *data = f71882fg_update_device(dev);
> + struct f71882fg_data *data = dev_get_drvdata(dev);
> int nr = to_sensor_dev_attr_2(devattr)->index;
> long val = simple_strtol(buf, NULL, 10);
> val = SENSORS_LIMIT(val, 0, 255);
>
> mutex_lock(&data->update_lock);
> + data->pwm_enable = f71882fg_read8(data, F71882FG_REG_PWM_ENABLE);
> if (data->pwm_enable & (1 << (2 * nr))) {
> /* PWM mode */
> f71882fg_write8(data, F71882FG_REG_PWM(nr), val);
> data->pwm[nr] = val;
> } else {
> /* RPM mode */
> - int target = val * fan_from_reg(data->fan_full_speed[nr]) / 255;
> - f71882fg_write16(data, F71882FG_REG_FAN_TARGET(nr),
> - fan_to_reg(target));
> - data->fan_target[nr] = fan_to_reg(target);
> + int target, full_speed;
> + full_speed = f71882fg_read16(data,
> + F71882FG_REG_FAN_FULL_SPEED(nr));
> + target = fan_to_reg(val * fan_from_reg(full_speed) / 255);
> + f71882fg_write16(data, F71882FG_REG_FAN_TARGET(nr), target);
> + data->fan_target[nr] = target;
> + data->fan_full_speed[nr] = full_speed;
> }
> mutex_unlock(&data->update_lock);
>
> @@ -1222,6 +1231,7 @@ static ssize_t store_pwm_enable(struct d
> return -EINVAL;
>
> mutex_lock(&data->update_lock);
> + data->pwm_enable = f71882fg_read8(data, F71882FG_REG_PWM_ENABLE);
> switch (val) {
> case 1:
> data->pwm_enable |= 2 << (2 * nr);
> @@ -1255,6 +1265,7 @@ static ssize_t show_pwm_auto_point_pwm(s
> int pwm = to_sensor_dev_attr_2(devattr)->index;
> int point = to_sensor_dev_attr_2(devattr)->nr;
>
> + mutex_lock(&data->update_lock);
> if (data->pwm_enable & (1 << (2 * pwm))) {
> /* PWM mode */
> result = data->pwm_auto_point_pwm[pwm][point];
> @@ -1262,6 +1273,7 @@ static ssize_t show_pwm_auto_point_pwm(s
> /* RPM mode */
> result = 32 * 255 / (32 + data->pwm_auto_point_pwm[pwm][point]);
> }
> + mutex_unlock(&data->update_lock);
>
> return sprintf(buf, "%d\n", result);
> }
> @@ -1270,14 +1282,14 @@ static ssize_t store_pwm_auto_point_pwm(
> struct device_attribute *devattr,
> const char *buf, size_t count)
> {
> - /* struct f71882fg_data *data = dev_get_drvdata(dev); */
> - struct f71882fg_data *data = f71882fg_update_device(dev);
> + struct f71882fg_data *data = dev_get_drvdata(dev);
> int pwm = to_sensor_dev_attr_2(devattr)->index;
> int point = to_sensor_dev_attr_2(devattr)->nr;
> - int val = simple_strtoul(buf, NULL, 10);
> + long val = simple_strtoul(buf, NULL, 10);
simple_strtol
> val = SENSORS_LIMIT(val, 0, 255);
>
> mutex_lock(&data->update_lock);
> + data->pwm_enable = f71882fg_read8(data, F71882FG_REG_PWM_ENABLE);
> if (data->pwm_enable & (1 << (2 * pwm))) {
> /* PWM mode */
> } else {
> @@ -1328,37 +1340,29 @@ static ssize_t store_pwm_auto_point_temp
> struct device_attribute *devattr,
> const char *buf, size_t count)
> {
> - struct f71882fg_data *data = f71882fg_update_device(dev);
> + struct f71882fg_data *data = dev_get_drvdata(dev);
> int nr = to_sensor_dev_attr_2(devattr)->index;
> int point = to_sensor_dev_attr_2(devattr)->nr;
> long val = simple_strtol(buf, NULL, 10) / 1000;
> + int hyst_reg_nr;
> + u8 hyst_reg;
>
> mutex_lock(&data->update_lock);
> + data->pwm_auto_point_temp[nr][point] > + f71882fg_read8(data, F71882FG_REG_POINT_TEMP(nr, point));
Trailing whitespace.
> val = SENSORS_LIMIT(val, data->pwm_auto_point_temp[nr][point] - 15,
> data->pwm_auto_point_temp[nr][point]);
> val = data->pwm_auto_point_temp[nr][point] - val;
>
> - switch (nr) {
> - case 0:
> - val = (data->pwm_auto_point_hyst[0] & 0xf0) | val;
> - break;
> - case 1:
> - val = (data->pwm_auto_point_hyst[0] & 0x0f) | (val << 4);
> - break;
> - case 2:
> - val = (data->pwm_auto_point_hyst[1] & 0xf0) | val;
> - break;
> - case 3:
> - val = (data->pwm_auto_point_hyst[1] & 0x0f) | (val << 4);
> - break;
> - }
> - if (nr = 0 || nr = 1) {
> - f71882fg_write8(data, F71882FG_REG_FAN_HYST0, val);
> - data->pwm_auto_point_hyst[0] = val;
> - } else {
> - f71882fg_write8(data, F71882FG_REG_FAN_HYST1, val);
> - data->pwm_auto_point_hyst[1] = val;
> - }
> + hyst_reg_nr = (nr >= 2) ? 1 : 0;
> + hyst_reg = f71882fg_read8(data, F71882FG_REG_FAN_HYST0 + hyst_reg_nr);
> + if (nr & 1)
> + hyst_reg = (hyst_reg & 0x0f) | (val << 4);
> + else
> + hyst_reg = (hyst_reg & 0xf0) | val;
> +
> + f71882fg_write8(data, F71882FG_REG_FAN_HYST0 + hyst_reg_nr, hyst_reg);
> + data->pwm_auto_point_hyst[hyst_reg_nr] = hyst_reg;
Hmm, that's a nice cleanup, but it doesn't really belong to this patch
as far as I can see. Also, if you are going to use
F71882FG_REG_FAN_HYST0 that way, you should make it a macro. So I'd
suggest moving all this to a separate patch.
> mutex_unlock(&data->update_lock);
>
> return count;
> @@ -1380,11 +1384,13 @@ static ssize_t store_pwm_interpolate(str
> struct device_attribute *devattr,
> const char *buf, size_t count)
> {
> - /* struct f71882fg_data *data = dev_get_drvdata(dev); */
> - struct f71882fg_data *data = f71882fg_update_device(dev);
> + struct f71882fg_data *data = dev_get_drvdata(dev);
> int nr = to_sensor_dev_attr_2(devattr)->index;
> - int val = simple_strtoul(buf, NULL, 10);
> + unsigned long val = simple_strtoul(buf, NULL, 10);
> +
> mutex_lock(&data->update_lock);
> + data->pwm_auto_point_mapping[nr] > + f71882fg_read8(data, F71882FG_REG_POINT_MAPPING(nr));
> if (val)
> val = data->pwm_auto_point_mapping[nr] | (1 << 4);
> else
> @@ -1413,8 +1419,7 @@ static ssize_t store_pwm_auto_point_chan
> struct device_attribute *devattr,
> const char *buf, size_t count)
> {
> - /* struct f71882fg_data *data = dev_get_drvdata(dev); */
> - struct f71882fg_data *data = f71882fg_update_device(dev);
> + struct f71882fg_data *data = dev_get_drvdata(dev);
> int nr = to_sensor_dev_attr_2(devattr)->index;
> long val = simple_strtol(buf, NULL, 10);
> switch (val) {
> @@ -1431,6 +1436,8 @@ static ssize_t store_pwm_auto_point_chan
> return -EINVAL;
> }
> mutex_lock(&data->update_lock);
> + data->pwm_auto_point_mapping[nr] > + f71882fg_read8(data, F71882FG_REG_POINT_MAPPING(nr));
> val = (data->pwm_auto_point_mapping[nr] & 0xfc) | val;
> f71882fg_write8(data, F71882FG_REG_POINT_MAPPING(nr), val);
> data->pwm_auto_point_mapping[nr] = val;
> @@ -1456,8 +1463,7 @@ static ssize_t store_pwm_auto_point_temp
> struct device_attribute *devattr,
> const char *buf, size_t count)
> {
> - /* struct f71882fg_data *data = dev_get_drvdata(dev); */
> - struct f71882fg_data *data = f71882fg_update_device(dev);
> + struct f71882fg_data *data = dev_get_drvdata(dev);
> int pwm = to_sensor_dev_attr_2(devattr)->index;
> int point = to_sensor_dev_attr_2(devattr)->nr;
> long val = simple_strtol(buf, NULL, 10) / 1000;
The rest looks good and is actually very welcome.
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [lm-sensors] PATCH: fix various sysfs callback function issues
2008-12-11 22:21 [lm-sensors] PATCH: fix various sysfs callback function issues in Hans de Goede
2008-12-13 22:18 ` [lm-sensors] PATCH: fix various sysfs callback function issues Jean Delvare
@ 2008-12-14 14:32 ` Hans de Goede
2008-12-14 18:04 ` Jean Delvare
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Hans de Goede @ 2008-12-14 14:32 UTC (permalink / raw)
To: lm-sensors
[-- Attachment #1: Type: text/plain, Size: 309 bytes --]
Jean Delvare wrote:
> Hi Hans,
>
> On Thu, 11 Dec 2008 23:21:30 +0100, Hans de Goede wrote:
>> Again see attachment.
>
> Review:
>
Thanks,
Attached is a new version with all mentioned issues fixed.
I'll send a separate patch with the fan hyst code cleanup as requested in the
review.
Regards,
Hans
[-- Attachment #2: hwmon-f71882fg-08-fix-store-functions-v2.patch --]
[-- Type: text/plain, Size: 11708 bytes --]
While working on adding f8000 support I noticed that various of the
store sysfs functions (and a few of the show also) had issues.
This patch fixes the following issues in these functions:
* store: storing the result of strto[u]l in an int, resulting in a possible
overflow before boundary checking
* store: use of f71882fg_update_device(), we don't want to read the whole
device in store functions, just the registers we need
* store: use of cached register values instead of reading the needed regs
in the store function, including cases where f71882fg_update_device() was
not used, this could cause real isues
* show: shown value is a calculation of 2 or more cached register reads,
without locking the data struct.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
--- linux/drivers/hwmon/f71882fg.c.07-applied 2008-12-14 14:06:02.000000000 +0100
+++ linux/drivers/hwmon/f71882fg.c 2008-12-14 14:19:47.000000000 +0100
@@ -835,6 +835,7 @@
val = fan_to_reg(val);
mutex_lock(&data->update_lock);
+ data->pwm_enable = f71882fg_read8(data, F71882FG_REG_PWM_ENABLE);
if (data->pwm_enable & (1 << (2 * nr)))
/* PWM mode */
count = -EINVAL;
@@ -865,9 +866,10 @@
{
struct f71882fg_data *data = dev_get_drvdata(dev);
int nr = to_sensor_dev_attr_2(devattr)->index;
- int val = simple_strtoul(buf, NULL, 10);
+ unsigned long val = simple_strtoul(buf, NULL, 10);
mutex_lock(&data->update_lock);
+ data->fan_beep = f71882fg_read8(data, F71882FG_REG_FAN_BEEP);
if (val)
data->fan_beep |= 1 << nr;
else
@@ -912,10 +914,8 @@
*devattr, const char *buf, size_t count)
{
struct f71882fg_data *data = dev_get_drvdata(dev);
- int val = simple_strtoul(buf, NULL, 10) / 8;
-
- if (val > 255)
- val = 255;
+ long val = simple_strtol(buf, NULL, 10) / 8;
+ val = SENSORS_LIMIT(val, 0, 255);
mutex_lock(&data->update_lock);
f71882fg_write8(data, F71882FG_REG_IN1_HIGH, val);
@@ -942,9 +942,10 @@
{
struct f71882fg_data *data = dev_get_drvdata(dev);
int nr = to_sensor_dev_attr_2(devattr)->index;
- int val = simple_strtoul(buf, NULL, 10);
+ unsigned long val = simple_strtoul(buf, NULL, 10);
mutex_lock(&data->update_lock);
+ data->in_beep = f71882fg_read8(data, F71882FG_REG_IN_BEEP);
if (val)
data->in_beep |= 1 << nr;
else
@@ -991,10 +992,8 @@
{
struct f71882fg_data *data = dev_get_drvdata(dev);
int nr = to_sensor_dev_attr_2(devattr)->index;
- int val = simple_strtoul(buf, NULL, 10) / 1000;
-
- if (val > 255)
- val = 255;
+ long val = simple_strtol(buf, NULL, 10) / 1000;
+ val = SENSORS_LIMIT(val, 0, 255);
mutex_lock(&data->update_lock);
f71882fg_write8(data, F71882FG_REG_TEMP_HIGH(nr), val);
@@ -1009,9 +1008,13 @@
{
struct f71882fg_data *data = f71882fg_update_device(dev);
int nr = to_sensor_dev_attr_2(devattr)->index;
+ int temp_max_hyst;
- return sprintf(buf, "%d\n",
- (data->temp_high[nr] - data->temp_hyst[nr]) * 1000);
+ mutex_lock(&data->update_lock);
+ temp_max_hyst = (data->temp_high[nr] - data->temp_hyst[nr]) * 1000;
+ mutex_unlock(&data->update_lock);
+
+ return sprintf(buf, "%d\n", temp_max_hyst);
}
static ssize_t store_temp_max_hyst(struct device *dev, struct device_attribute
@@ -1019,37 +1022,38 @@
{
struct f71882fg_data *data = dev_get_drvdata(dev);
int nr = to_sensor_dev_attr_2(devattr)->index;
- int val = simple_strtoul(buf, NULL, 10) / 1000;
+ long val = simple_strtol(buf, NULL, 10) / 1000;
ssize_t ret = count;
+ u8 reg;
mutex_lock(&data->update_lock);
/* convert abs to relative and check */
+ data->temp_high[nr] = f71882fg_read8(data, F71882FG_REG_TEMP_HIGH(nr));
+ val = SENSORS_LIMIT(val, data->temp_high[nr] - 15,
+ data->temp_high[nr]);
val = data->temp_high[nr] - val;
- if (val < 0 || val > 15) {
- ret = -EINVAL;
- goto store_temp_max_hyst_exit;
- }
-
data->temp_hyst[nr] = val;
/* convert value to register contents */
switch (nr) {
case 1:
- val = val << 4;
+ reg = f71882fg_read8(data, F71882FG_REG_TEMP_HYST1);
+ reg = (reg & 0x0f) | (val << 4);
break;
case 2:
- val = val | (data->temp_hyst[3] << 4);
+ reg = f71882fg_read8(data, F71882FG_REG_TEMP_HYST23);
+ reg = (reg & 0xf0) | val;
break;
case 3:
- val = data->temp_hyst[2] | (val << 4);
+ reg = f71882fg_read8(data, F71882FG_REG_TEMP_HYST23);
+ reg = (reg & 0x0f) | (val << 4);
break;
}
f71882fg_write8(data, (nr <= 1) ? F71882FG_REG_TEMP_HYST1 :
- F71882FG_REG_TEMP_HYST23, val);
+ F71882FG_REG_TEMP_HYST23, reg);
-store_temp_max_hyst_exit:
mutex_unlock(&data->update_lock);
return ret;
}
@@ -1068,10 +1072,8 @@
{
struct f71882fg_data *data = dev_get_drvdata(dev);
int nr = to_sensor_dev_attr_2(devattr)->index;
- int val = simple_strtoul(buf, NULL, 10) / 1000;
-
- if (val > 255)
- val = 255;
+ long val = simple_strtol(buf, NULL, 10) / 1000;
+ val = SENSORS_LIMIT(val, 0, 255);
mutex_lock(&data->update_lock);
f71882fg_write8(data, F71882FG_REG_TEMP_OVT(nr), val);
@@ -1086,9 +1088,13 @@
{
struct f71882fg_data *data = f71882fg_update_device(dev);
int nr = to_sensor_dev_attr_2(devattr)->index;
+ int temp_crit_hyst;
- return sprintf(buf, "%d\n",
- (data->temp_ovt[nr] - data->temp_hyst[nr]) * 1000);
+ mutex_lock(&data->update_lock);
+ temp_crit_hyst = (data->temp_ovt[nr] - data->temp_hyst[nr]) * 1000;
+ mutex_unlock(&data->update_lock);
+
+ return sprintf(buf, "%d\n", temp_crit_hyst);
}
static ssize_t show_temp_type(struct device *dev, struct device_attribute
@@ -1117,9 +1123,10 @@
{
struct f71882fg_data *data = dev_get_drvdata(dev);
int nr = to_sensor_dev_attr_2(devattr)->index;
- int val = simple_strtoul(buf, NULL, 10);
+ unsigned long val = simple_strtoul(buf, NULL, 10);
mutex_lock(&data->update_lock);
+ data->temp_beep = f71882fg_read8(data, F71882FG_REG_TEMP_BEEP);
if (val)
data->temp_beep |= 1 << nr;
else
@@ -1160,16 +1167,16 @@
{
struct f71882fg_data *data = f71882fg_update_device(dev);
int val, nr = to_sensor_dev_attr_2(devattr)->index;
+ mutex_lock(&data->update_lock);
if (data->pwm_enable & (1 << (2 * nr)))
/* PWM mode */
val = data->pwm[nr];
else {
/* RPM mode */
- mutex_lock(&data->update_lock);
val = 255 * fan_from_reg(data->fan_target[nr])
/ fan_from_reg(data->fan_full_speed[nr]);
- mutex_unlock(&data->update_lock);
}
+ mutex_unlock(&data->update_lock);
return sprintf(buf, "%d\n", val);
}
@@ -1177,23 +1184,26 @@
struct device_attribute *devattr, const char *buf,
size_t count)
{
- /* struct f71882fg_data *data = dev_get_drvdata(dev); */
- struct f71882fg_data *data = f71882fg_update_device(dev);
+ struct f71882fg_data *data = dev_get_drvdata(dev);
int nr = to_sensor_dev_attr_2(devattr)->index;
long val = simple_strtol(buf, NULL, 10);
val = SENSORS_LIMIT(val, 0, 255);
mutex_lock(&data->update_lock);
+ data->pwm_enable = f71882fg_read8(data, F71882FG_REG_PWM_ENABLE);
if (data->pwm_enable & (1 << (2 * nr))) {
/* PWM mode */
f71882fg_write8(data, F71882FG_REG_PWM(nr), val);
data->pwm[nr] = val;
} else {
/* RPM mode */
- int target = val * fan_from_reg(data->fan_full_speed[nr]) / 255;
- f71882fg_write16(data, F71882FG_REG_FAN_TARGET(nr),
- fan_to_reg(target));
- data->fan_target[nr] = fan_to_reg(target);
+ int target, full_speed;
+ full_speed = f71882fg_read16(data,
+ F71882FG_REG_FAN_FULL_SPEED(nr));
+ target = fan_to_reg(val * fan_from_reg(full_speed) / 255);
+ f71882fg_write16(data, F71882FG_REG_FAN_TARGET(nr), target);
+ data->fan_target[nr] = target;
+ data->fan_full_speed[nr] = full_speed;
}
mutex_unlock(&data->update_lock);
@@ -1225,6 +1235,7 @@
return -EINVAL;
mutex_lock(&data->update_lock);
+ data->pwm_enable = f71882fg_read8(data, F71882FG_REG_PWM_ENABLE);
switch (val) {
case 1:
data->pwm_enable |= 2 << (2 * nr);
@@ -1258,6 +1269,7 @@
int pwm = to_sensor_dev_attr_2(devattr)->index;
int point = to_sensor_dev_attr_2(devattr)->nr;
+ mutex_lock(&data->update_lock);
if (data->pwm_enable & (1 << (2 * pwm))) {
/* PWM mode */
result = data->pwm_auto_point_pwm[pwm][point];
@@ -1265,6 +1277,7 @@
/* RPM mode */
result = 32 * 255 / (32 + data->pwm_auto_point_pwm[pwm][point]);
}
+ mutex_unlock(&data->update_lock);
return sprintf(buf, "%d\n", result);
}
@@ -1273,14 +1286,14 @@
struct device_attribute *devattr,
const char *buf, size_t count)
{
- /* struct f71882fg_data *data = dev_get_drvdata(dev); */
- struct f71882fg_data *data = f71882fg_update_device(dev);
+ struct f71882fg_data *data = dev_get_drvdata(dev);
int pwm = to_sensor_dev_attr_2(devattr)->index;
int point = to_sensor_dev_attr_2(devattr)->nr;
- int val = simple_strtoul(buf, NULL, 10);
+ long val = simple_strtol(buf, NULL, 10);
val = SENSORS_LIMIT(val, 0, 255);
mutex_lock(&data->update_lock);
+ data->pwm_enable = f71882fg_read8(data, F71882FG_REG_PWM_ENABLE);
if (data->pwm_enable & (1 << (2 * pwm))) {
/* PWM mode */
} else {
@@ -1331,16 +1344,25 @@
struct device_attribute *devattr,
const char *buf, size_t count)
{
- struct f71882fg_data *data = f71882fg_update_device(dev);
+ struct f71882fg_data *data = dev_get_drvdata(dev);
int nr = to_sensor_dev_attr_2(devattr)->index;
int point = to_sensor_dev_attr_2(devattr)->nr;
long val = simple_strtol(buf, NULL, 10) / 1000;
mutex_lock(&data->update_lock);
+ data->pwm_auto_point_temp[nr][point] =
+ f71882fg_read8(data, F71882FG_REG_POINT_TEMP(nr, point));
val = SENSORS_LIMIT(val, data->pwm_auto_point_temp[nr][point] - 15,
data->pwm_auto_point_temp[nr][point]);
val = data->pwm_auto_point_temp[nr][point] - val;
+ if (nr == 0 || nr == 1) {
+ data->pwm_auto_point_hyst[0] =
+ f71882fg_read8(data, F71882FG_REG_FAN_HYST0, val);
+ } else {
+ data->pwm_auto_point_hyst[1] =
+ f71882fg_read8(data, F71882FG_REG_FAN_HYST1, val);
+ }
switch (nr) {
case 0:
val = (data->pwm_auto_point_hyst[0] & 0xf0) | val;
@@ -1383,11 +1405,13 @@
struct device_attribute *devattr,
const char *buf, size_t count)
{
- /* struct f71882fg_data *data = dev_get_drvdata(dev); */
- struct f71882fg_data *data = f71882fg_update_device(dev);
+ struct f71882fg_data *data = dev_get_drvdata(dev);
int nr = to_sensor_dev_attr_2(devattr)->index;
- int val = simple_strtoul(buf, NULL, 10);
+ unsigned long val = simple_strtoul(buf, NULL, 10);
+
mutex_lock(&data->update_lock);
+ data->pwm_auto_point_mapping[nr] =
+ f71882fg_read8(data, F71882FG_REG_POINT_MAPPING(nr));
if (val)
val = data->pwm_auto_point_mapping[nr] | (1 << 4);
else
@@ -1416,8 +1440,7 @@
struct device_attribute *devattr,
const char *buf, size_t count)
{
- /* struct f71882fg_data *data = dev_get_drvdata(dev); */
- struct f71882fg_data *data = f71882fg_update_device(dev);
+ struct f71882fg_data *data = dev_get_drvdata(dev);
int nr = to_sensor_dev_attr_2(devattr)->index;
long val = simple_strtol(buf, NULL, 10);
switch (val) {
@@ -1434,6 +1457,8 @@
return -EINVAL;
}
mutex_lock(&data->update_lock);
+ data->pwm_auto_point_mapping[nr] =
+ f71882fg_read8(data, F71882FG_REG_POINT_MAPPING(nr));
val = (data->pwm_auto_point_mapping[nr] & 0xfc) | val;
f71882fg_write8(data, F71882FG_REG_POINT_MAPPING(nr), val);
data->pwm_auto_point_mapping[nr] = val;
@@ -1459,8 +1484,7 @@
struct device_attribute *devattr,
const char *buf, size_t count)
{
- /* struct f71882fg_data *data = dev_get_drvdata(dev); */
- struct f71882fg_data *data = f71882fg_update_device(dev);
+ struct f71882fg_data *data = dev_get_drvdata(dev);
int pwm = to_sensor_dev_attr_2(devattr)->index;
int point = to_sensor_dev_attr_2(devattr)->nr;
long val = simple_strtol(buf, NULL, 10) / 1000;
[-- Attachment #3: Type: text/plain, Size: 153 bytes --]
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [lm-sensors] PATCH: fix various sysfs callback function issues
2008-12-11 22:21 [lm-sensors] PATCH: fix various sysfs callback function issues in Hans de Goede
2008-12-13 22:18 ` [lm-sensors] PATCH: fix various sysfs callback function issues Jean Delvare
2008-12-14 14:32 ` Hans de Goede
@ 2008-12-14 18:04 ` Jean Delvare
2008-12-14 18:40 ` Hans de Goede
2008-12-14 21:30 ` Jean Delvare
4 siblings, 0 replies; 6+ messages in thread
From: Jean Delvare @ 2008-12-14 18:04 UTC (permalink / raw)
To: lm-sensors
Hi Hans,
On Sun, 14 Dec 2008 15:32:04 +0100, Hans de Goede wrote:
> Attached is a new version with all mentioned issues fixed.
Doesn't build:
drivers/hwmon/f71882fg.c: In function ‘store_pwm_auto_point_temp_hyst’:
drivers/hwmon/f71882fg.c:1361: error: too many arguments to function ‘f71882fg_read8’
drivers/hwmon/f71882fg.c:1364: error: too many arguments to function ‘f71882fg_read8’
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [lm-sensors] PATCH: fix various sysfs callback function issues
2008-12-11 22:21 [lm-sensors] PATCH: fix various sysfs callback function issues in Hans de Goede
` (2 preceding siblings ...)
2008-12-14 18:04 ` Jean Delvare
@ 2008-12-14 18:40 ` Hans de Goede
2008-12-14 21:30 ` Jean Delvare
4 siblings, 0 replies; 6+ messages in thread
From: Hans de Goede @ 2008-12-14 18:40 UTC (permalink / raw)
To: lm-sensors
[-- Attachment #1: Type: text/plain, Size: 811 bytes --]
Jean Delvare wrote:
> Hi Hans,
>
> On Sun, 14 Dec 2008 15:32:04 +0100, Hans de Goede wrote:
>> Attached is a new version with all mentioned issues fixed.
>
> Doesn't build:
>
> drivers/hwmon/f71882fg.c: In function ‘store_pwm_auto_point_temp_hyst’:
> drivers/hwmon/f71882fg.c:1361: error: too many arguments to function ‘f71882fg_read8’
> drivers/hwmon/f71882fg.c:1364: error: too many arguments to function ‘f71882fg_read8’
>
oops. I guess I forgot to test compile that one particular in between patches
state, the next patch in the series fixes this compile error (by completely
nuking the affected lines.
Anyways attached is a fixed version, as well as an updated version of the 09
patch, so that it applies on top of the fixed version.
Regards,
Hans
[-- Attachment #2: hwmon-f71882fg-08-fix-store-functions-v3.patch --]
[-- Type: text/plain, Size: 11698 bytes --]
While working on adding f8000 support I noticed that various of the
store sysfs functions (and a few of the show also) had issues.
This patch fixes the following issues in these functions:
* store: storing the result of strto[u]l in an int, resulting in a possible
overflow before boundary checking
* store: use of f71882fg_update_device(), we don't want to read the whole
device in store functions, just the registers we need
* store: use of cached register values instead of reading the needed regs
in the store function, including cases where f71882fg_update_device() was
not used, this could cause real isues
* show: shown value is a calculation of 2 or more cached register reads,
without locking the data struct.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
--- linux/drivers/hwmon/f71882fg.c.07-applied 2008-12-14 14:06:02.000000000 +0100
+++ linux/drivers/hwmon/f71882fg.c 2008-12-14 14:19:47.000000000 +0100
@@ -835,6 +835,7 @@
val = fan_to_reg(val);
mutex_lock(&data->update_lock);
+ data->pwm_enable = f71882fg_read8(data, F71882FG_REG_PWM_ENABLE);
if (data->pwm_enable & (1 << (2 * nr)))
/* PWM mode */
count = -EINVAL;
@@ -865,9 +866,10 @@
{
struct f71882fg_data *data = dev_get_drvdata(dev);
int nr = to_sensor_dev_attr_2(devattr)->index;
- int val = simple_strtoul(buf, NULL, 10);
+ unsigned long val = simple_strtoul(buf, NULL, 10);
mutex_lock(&data->update_lock);
+ data->fan_beep = f71882fg_read8(data, F71882FG_REG_FAN_BEEP);
if (val)
data->fan_beep |= 1 << nr;
else
@@ -912,10 +914,8 @@
*devattr, const char *buf, size_t count)
{
struct f71882fg_data *data = dev_get_drvdata(dev);
- int val = simple_strtoul(buf, NULL, 10) / 8;
-
- if (val > 255)
- val = 255;
+ long val = simple_strtol(buf, NULL, 10) / 8;
+ val = SENSORS_LIMIT(val, 0, 255);
mutex_lock(&data->update_lock);
f71882fg_write8(data, F71882FG_REG_IN1_HIGH, val);
@@ -942,9 +942,10 @@
{
struct f71882fg_data *data = dev_get_drvdata(dev);
int nr = to_sensor_dev_attr_2(devattr)->index;
- int val = simple_strtoul(buf, NULL, 10);
+ unsigned long val = simple_strtoul(buf, NULL, 10);
mutex_lock(&data->update_lock);
+ data->in_beep = f71882fg_read8(data, F71882FG_REG_IN_BEEP);
if (val)
data->in_beep |= 1 << nr;
else
@@ -991,10 +992,8 @@
{
struct f71882fg_data *data = dev_get_drvdata(dev);
int nr = to_sensor_dev_attr_2(devattr)->index;
- int val = simple_strtoul(buf, NULL, 10) / 1000;
-
- if (val > 255)
- val = 255;
+ long val = simple_strtol(buf, NULL, 10) / 1000;
+ val = SENSORS_LIMIT(val, 0, 255);
mutex_lock(&data->update_lock);
f71882fg_write8(data, F71882FG_REG_TEMP_HIGH(nr), val);
@@ -1009,9 +1008,13 @@
{
struct f71882fg_data *data = f71882fg_update_device(dev);
int nr = to_sensor_dev_attr_2(devattr)->index;
+ int temp_max_hyst;
- return sprintf(buf, "%d\n",
- (data->temp_high[nr] - data->temp_hyst[nr]) * 1000);
+ mutex_lock(&data->update_lock);
+ temp_max_hyst = (data->temp_high[nr] - data->temp_hyst[nr]) * 1000;
+ mutex_unlock(&data->update_lock);
+
+ return sprintf(buf, "%d\n", temp_max_hyst);
}
static ssize_t store_temp_max_hyst(struct device *dev, struct device_attribute
@@ -1019,37 +1022,38 @@
{
struct f71882fg_data *data = dev_get_drvdata(dev);
int nr = to_sensor_dev_attr_2(devattr)->index;
- int val = simple_strtoul(buf, NULL, 10) / 1000;
+ long val = simple_strtol(buf, NULL, 10) / 1000;
ssize_t ret = count;
+ u8 reg;
mutex_lock(&data->update_lock);
/* convert abs to relative and check */
+ data->temp_high[nr] = f71882fg_read8(data, F71882FG_REG_TEMP_HIGH(nr));
+ val = SENSORS_LIMIT(val, data->temp_high[nr] - 15,
+ data->temp_high[nr]);
val = data->temp_high[nr] - val;
- if (val < 0 || val > 15) {
- ret = -EINVAL;
- goto store_temp_max_hyst_exit;
- }
-
data->temp_hyst[nr] = val;
/* convert value to register contents */
switch (nr) {
case 1:
- val = val << 4;
+ reg = f71882fg_read8(data, F71882FG_REG_TEMP_HYST1);
+ reg = (reg & 0x0f) | (val << 4);
break;
case 2:
- val = val | (data->temp_hyst[3] << 4);
+ reg = f71882fg_read8(data, F71882FG_REG_TEMP_HYST23);
+ reg = (reg & 0xf0) | val;
break;
case 3:
- val = data->temp_hyst[2] | (val << 4);
+ reg = f71882fg_read8(data, F71882FG_REG_TEMP_HYST23);
+ reg = (reg & 0x0f) | (val << 4);
break;
}
f71882fg_write8(data, (nr <= 1) ? F71882FG_REG_TEMP_HYST1 :
- F71882FG_REG_TEMP_HYST23, val);
+ F71882FG_REG_TEMP_HYST23, reg);
-store_temp_max_hyst_exit:
mutex_unlock(&data->update_lock);
return ret;
}
@@ -1068,10 +1072,8 @@
{
struct f71882fg_data *data = dev_get_drvdata(dev);
int nr = to_sensor_dev_attr_2(devattr)->index;
- int val = simple_strtoul(buf, NULL, 10) / 1000;
-
- if (val > 255)
- val = 255;
+ long val = simple_strtol(buf, NULL, 10) / 1000;
+ val = SENSORS_LIMIT(val, 0, 255);
mutex_lock(&data->update_lock);
f71882fg_write8(data, F71882FG_REG_TEMP_OVT(nr), val);
@@ -1086,9 +1088,13 @@
{
struct f71882fg_data *data = f71882fg_update_device(dev);
int nr = to_sensor_dev_attr_2(devattr)->index;
+ int temp_crit_hyst;
- return sprintf(buf, "%d\n",
- (data->temp_ovt[nr] - data->temp_hyst[nr]) * 1000);
+ mutex_lock(&data->update_lock);
+ temp_crit_hyst = (data->temp_ovt[nr] - data->temp_hyst[nr]) * 1000;
+ mutex_unlock(&data->update_lock);
+
+ return sprintf(buf, "%d\n", temp_crit_hyst);
}
static ssize_t show_temp_type(struct device *dev, struct device_attribute
@@ -1117,9 +1123,10 @@
{
struct f71882fg_data *data = dev_get_drvdata(dev);
int nr = to_sensor_dev_attr_2(devattr)->index;
- int val = simple_strtoul(buf, NULL, 10);
+ unsigned long val = simple_strtoul(buf, NULL, 10);
mutex_lock(&data->update_lock);
+ data->temp_beep = f71882fg_read8(data, F71882FG_REG_TEMP_BEEP);
if (val)
data->temp_beep |= 1 << nr;
else
@@ -1160,16 +1167,16 @@
{
struct f71882fg_data *data = f71882fg_update_device(dev);
int val, nr = to_sensor_dev_attr_2(devattr)->index;
+ mutex_lock(&data->update_lock);
if (data->pwm_enable & (1 << (2 * nr)))
/* PWM mode */
val = data->pwm[nr];
else {
/* RPM mode */
- mutex_lock(&data->update_lock);
val = 255 * fan_from_reg(data->fan_target[nr])
/ fan_from_reg(data->fan_full_speed[nr]);
- mutex_unlock(&data->update_lock);
}
+ mutex_unlock(&data->update_lock);
return sprintf(buf, "%d\n", val);
}
@@ -1177,23 +1184,26 @@
struct device_attribute *devattr, const char *buf,
size_t count)
{
- /* struct f71882fg_data *data = dev_get_drvdata(dev); */
- struct f71882fg_data *data = f71882fg_update_device(dev);
+ struct f71882fg_data *data = dev_get_drvdata(dev);
int nr = to_sensor_dev_attr_2(devattr)->index;
long val = simple_strtol(buf, NULL, 10);
val = SENSORS_LIMIT(val, 0, 255);
mutex_lock(&data->update_lock);
+ data->pwm_enable = f71882fg_read8(data, F71882FG_REG_PWM_ENABLE);
if (data->pwm_enable & (1 << (2 * nr))) {
/* PWM mode */
f71882fg_write8(data, F71882FG_REG_PWM(nr), val);
data->pwm[nr] = val;
} else {
/* RPM mode */
- int target = val * fan_from_reg(data->fan_full_speed[nr]) / 255;
- f71882fg_write16(data, F71882FG_REG_FAN_TARGET(nr),
- fan_to_reg(target));
- data->fan_target[nr] = fan_to_reg(target);
+ int target, full_speed;
+ full_speed = f71882fg_read16(data,
+ F71882FG_REG_FAN_FULL_SPEED(nr));
+ target = fan_to_reg(val * fan_from_reg(full_speed) / 255);
+ f71882fg_write16(data, F71882FG_REG_FAN_TARGET(nr), target);
+ data->fan_target[nr] = target;
+ data->fan_full_speed[nr] = full_speed;
}
mutex_unlock(&data->update_lock);
@@ -1225,6 +1235,7 @@
return -EINVAL;
mutex_lock(&data->update_lock);
+ data->pwm_enable = f71882fg_read8(data, F71882FG_REG_PWM_ENABLE);
switch (val) {
case 1:
data->pwm_enable |= 2 << (2 * nr);
@@ -1258,6 +1269,7 @@
int pwm = to_sensor_dev_attr_2(devattr)->index;
int point = to_sensor_dev_attr_2(devattr)->nr;
+ mutex_lock(&data->update_lock);
if (data->pwm_enable & (1 << (2 * pwm))) {
/* PWM mode */
result = data->pwm_auto_point_pwm[pwm][point];
@@ -1265,6 +1277,7 @@
/* RPM mode */
result = 32 * 255 / (32 + data->pwm_auto_point_pwm[pwm][point]);
}
+ mutex_unlock(&data->update_lock);
return sprintf(buf, "%d\n", result);
}
@@ -1273,14 +1286,14 @@
struct device_attribute *devattr,
const char *buf, size_t count)
{
- /* struct f71882fg_data *data = dev_get_drvdata(dev); */
- struct f71882fg_data *data = f71882fg_update_device(dev);
+ struct f71882fg_data *data = dev_get_drvdata(dev);
int pwm = to_sensor_dev_attr_2(devattr)->index;
int point = to_sensor_dev_attr_2(devattr)->nr;
- int val = simple_strtoul(buf, NULL, 10);
+ long val = simple_strtol(buf, NULL, 10);
val = SENSORS_LIMIT(val, 0, 255);
mutex_lock(&data->update_lock);
+ data->pwm_enable = f71882fg_read8(data, F71882FG_REG_PWM_ENABLE);
if (data->pwm_enable & (1 << (2 * pwm))) {
/* PWM mode */
} else {
@@ -1331,16 +1344,25 @@
struct device_attribute *devattr,
const char *buf, size_t count)
{
- struct f71882fg_data *data = f71882fg_update_device(dev);
+ struct f71882fg_data *data = dev_get_drvdata(dev);
int nr = to_sensor_dev_attr_2(devattr)->index;
int point = to_sensor_dev_attr_2(devattr)->nr;
long val = simple_strtol(buf, NULL, 10) / 1000;
mutex_lock(&data->update_lock);
+ data->pwm_auto_point_temp[nr][point] =
+ f71882fg_read8(data, F71882FG_REG_POINT_TEMP(nr, point));
val = SENSORS_LIMIT(val, data->pwm_auto_point_temp[nr][point] - 15,
data->pwm_auto_point_temp[nr][point]);
val = data->pwm_auto_point_temp[nr][point] - val;
+ if (nr == 0 || nr == 1) {
+ data->pwm_auto_point_hyst[0] =
+ f71882fg_read8(data, F71882FG_REG_FAN_HYST0);
+ } else {
+ data->pwm_auto_point_hyst[1] =
+ f71882fg_read8(data, F71882FG_REG_FAN_HYST1);
+ }
switch (nr) {
case 0:
val = (data->pwm_auto_point_hyst[0] & 0xf0) | val;
@@ -1383,11 +1405,13 @@
struct device_attribute *devattr,
const char *buf, size_t count)
{
- /* struct f71882fg_data *data = dev_get_drvdata(dev); */
- struct f71882fg_data *data = f71882fg_update_device(dev);
+ struct f71882fg_data *data = dev_get_drvdata(dev);
int nr = to_sensor_dev_attr_2(devattr)->index;
- int val = simple_strtoul(buf, NULL, 10);
+ unsigned long val = simple_strtoul(buf, NULL, 10);
+
mutex_lock(&data->update_lock);
+ data->pwm_auto_point_mapping[nr] =
+ f71882fg_read8(data, F71882FG_REG_POINT_MAPPING(nr));
if (val)
val = data->pwm_auto_point_mapping[nr] | (1 << 4);
else
@@ -1416,8 +1440,7 @@
struct device_attribute *devattr,
const char *buf, size_t count)
{
- /* struct f71882fg_data *data = dev_get_drvdata(dev); */
- struct f71882fg_data *data = f71882fg_update_device(dev);
+ struct f71882fg_data *data = dev_get_drvdata(dev);
int nr = to_sensor_dev_attr_2(devattr)->index;
long val = simple_strtol(buf, NULL, 10);
switch (val) {
@@ -1434,6 +1457,8 @@
return -EINVAL;
}
mutex_lock(&data->update_lock);
+ data->pwm_auto_point_mapping[nr] =
+ f71882fg_read8(data, F71882FG_REG_POINT_MAPPING(nr));
val = (data->pwm_auto_point_mapping[nr] & 0xfc) | val;
f71882fg_write8(data, F71882FG_REG_POINT_MAPPING(nr), val);
data->pwm_auto_point_mapping[nr] = val;
@@ -1459,8 +1484,7 @@
struct device_attribute *devattr,
const char *buf, size_t count)
{
- /* struct f71882fg_data *data = dev_get_drvdata(dev); */
- struct f71882fg_data *data = f71882fg_update_device(dev);
+ struct f71882fg_data *data = dev_get_drvdata(dev);
int pwm = to_sensor_dev_attr_2(devattr)->index;
int point = to_sensor_dev_attr_2(devattr)->nr;
long val = simple_strtol(buf, NULL, 10) / 1000;
[-- Attachment #3: hwmon-f71882fg-09-simplify-hyst-functions-v2.patch --]
[-- Type: text/plain, Size: 6549 bytes --]
Simplify fan and temp hyst. handling by treating the registers as an array of
nibbles instead of using switch cases. Also unify the way hysts are handled
between temp and fans, the temp code was storing the actual per temp hyst
values in 4 u8's, where as the fan code was stroing actual register values.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
--- linux/drivers/hwmon/f71882fg.c.08-applied 2008-12-14 14:22:40.000000000 +0100
+++ linux/drivers/hwmon/f71882fg.c 2008-12-14 15:16:30.000000000 +0100
@@ -68,8 +68,7 @@
#define F71882FG_REG_TEMP_HIGH(nr) (0x81 + 2 * (nr))
#define F71882FG_REG_TEMP_STATUS 0x62
#define F71882FG_REG_TEMP_BEEP 0x63
-#define F71882FG_REG_TEMP_HYST1 0x6C
-#define F71882FG_REG_TEMP_HYST23 0x6D
+#define F71882FG_REG_TEMP_HYST(nr) (0x6C + (nr))
#define F71882FG_REG_TEMP_TYPE 0x6B
#define F71882FG_REG_TEMP_DIODE_OPEN 0x6F
@@ -77,8 +76,7 @@
#define F71882FG_REG_PWM_TYPE 0x94
#define F71882FG_REG_PWM_ENABLE 0x96
-#define F71882FG_REG_FAN_HYST0 0x98
-#define F71882FG_REG_FAN_HYST1 0x99
+#define F71882FG_REG_FAN_HYST(nr) (0x98 + (nr))
#define F71882FG_REG_POINT_PWM(pwm, point) (0xAA + (point) + (16 * (pwm)))
#define F71882FG_REG_POINT_TEMP(pwm, point) (0xA6 + (point) + (16 * (pwm)))
@@ -144,7 +142,7 @@
u8 temp[4];
u8 temp_ovt[4];
u8 temp_high[4];
- u8 temp_hyst[4];
+ u8 temp_hyst[2]; /* 2 hysts stored per reg */
u8 temp_type[4];
u8 temp_status;
u8 temp_beep;
@@ -688,13 +686,11 @@
F71882FG_REG_TEMP_HIGH(nr));
}
- /* Have to hardcode hyst*/
- data->temp_hyst[1] = f71882fg_read8(data,
- F71882FG_REG_TEMP_HYST1) >> 4;
- /* Hyst temps 2 & 3 stored in same register */
- reg = f71882fg_read8(data, F71882FG_REG_TEMP_HYST23);
- data->temp_hyst[2] = reg & 0x0F;
- data->temp_hyst[3] = reg >> 4;
+ /* hyst */
+ data->temp_hyst[0] =
+ f71882fg_read8(data, F71882FG_REG_TEMP_HYST(0));
+ data->temp_hyst[1] =
+ f71882fg_read8(data, F71882FG_REG_TEMP_HYST(1));
/* Have to hardcode type, because temp1 is special */
reg = f71882fg_read8(data, F71882FG_REG_TEMP_TYPE);
@@ -715,10 +711,11 @@
data->pwm_enable = f71882fg_read8(data,
F71882FG_REG_PWM_ENABLE);
- data->pwm_auto_point_hyst[0] = f71882fg_read8(data,
- F71882FG_REG_FAN_HYST0);
- data->pwm_auto_point_hyst[1] = f71882fg_read8(data,
- F71882FG_REG_FAN_HYST1);
+ data->pwm_auto_point_hyst[0] =
+ f71882fg_read8(data, F71882FG_REG_FAN_HYST(0));
+ data->pwm_auto_point_hyst[1] =
+ f71882fg_read8(data, F71882FG_REG_FAN_HYST(1));
+
for (nr = 0; nr < nr_fans; nr++) {
data->pwm_auto_point_mapping[nr] =
f71882fg_read8(data,
@@ -1011,7 +1008,11 @@
int temp_max_hyst;
mutex_lock(&data->update_lock);
- temp_max_hyst = (data->temp_high[nr] - data->temp_hyst[nr]) * 1000;
+ if (nr & 1)
+ temp_max_hyst = data->temp_hyst[nr / 2] >> 4;
+ else
+ temp_max_hyst = data->temp_hyst[nr / 2] & 0x0f;
+ temp_max_hyst = (data->temp_high[nr] - temp_max_hyst) * 1000;
mutex_unlock(&data->update_lock);
return sprintf(buf, "%d\n", temp_max_hyst);
@@ -1033,26 +1034,15 @@
val = SENSORS_LIMIT(val, data->temp_high[nr] - 15,
data->temp_high[nr]);
val = data->temp_high[nr] - val;
- data->temp_hyst[nr] = val;
/* convert value to register contents */
- switch (nr) {
- case 1:
- reg = f71882fg_read8(data, F71882FG_REG_TEMP_HYST1);
- reg = (reg & 0x0f) | (val << 4);
- break;
- case 2:
- reg = f71882fg_read8(data, F71882FG_REG_TEMP_HYST23);
- reg = (reg & 0xf0) | val;
- break;
- case 3:
- reg = f71882fg_read8(data, F71882FG_REG_TEMP_HYST23);
- reg = (reg & 0x0f) | (val << 4);
- break;
- }
-
- f71882fg_write8(data, (nr <= 1) ? F71882FG_REG_TEMP_HYST1 :
- F71882FG_REG_TEMP_HYST23, reg);
+ reg = f71882fg_read8(data, F71882FG_REG_TEMP_HYST(nr / 2));
+ if (nr & 1)
+ reg = (reg & 0x0f) | (val << 4);
+ else
+ reg = (reg & 0xf0) | val;
+ f71882fg_write8(data, F71882FG_REG_TEMP_HYST(nr / 2), reg);
+ data->temp_hyst[nr / 2] = reg;
mutex_unlock(&data->update_lock);
return ret;
@@ -1091,7 +1081,11 @@
int temp_crit_hyst;
mutex_lock(&data->update_lock);
- temp_crit_hyst = (data->temp_ovt[nr] - data->temp_hyst[nr]) * 1000;
+ if (nr & 1)
+ temp_crit_hyst = data->temp_hyst[nr / 2] >> 4;
+ else
+ temp_crit_hyst = data->temp_hyst[nr / 2] & 0x0f;
+ temp_crit_hyst = (data->temp_ovt[nr] - temp_crit_hyst) * 1000;
mutex_unlock(&data->update_lock);
return sprintf(buf, "%d\n", temp_crit_hyst);
@@ -1320,20 +1314,10 @@
int point = to_sensor_dev_attr_2(devattr)->nr;
mutex_lock(&data->update_lock);
- switch (nr) {
- case 0:
- result = data->pwm_auto_point_hyst[0] & 0x0f;
- break;
- case 1:
- result = data->pwm_auto_point_hyst[0] >> 4;
- break;
- case 2:
- result = data->pwm_auto_point_hyst[1] & 0x0f;
- break;
- case 3:
- result = data->pwm_auto_point_hyst[1] >> 4;
- break;
- }
+ if (nr & 1)
+ result = data->pwm_auto_point_hyst[nr / 2] >> 4;
+ else
+ result = data->pwm_auto_point_hyst[nr / 2] & 0x0f;
result = 1000 * (data->pwm_auto_point_temp[nr][point] - result);
mutex_unlock(&data->update_lock);
@@ -1348,6 +1332,7 @@
int nr = to_sensor_dev_attr_2(devattr)->index;
int point = to_sensor_dev_attr_2(devattr)->nr;
long val = simple_strtol(buf, NULL, 10) / 1000;
+ u8 reg;
mutex_lock(&data->update_lock);
data->pwm_auto_point_temp[nr][point] =
@@ -1356,34 +1341,14 @@
data->pwm_auto_point_temp[nr][point]);
val = data->pwm_auto_point_temp[nr][point] - val;
- if (nr == 0 || nr == 1) {
- data->pwm_auto_point_hyst[0] =
- f71882fg_read8(data, F71882FG_REG_FAN_HYST0);
- } else {
- data->pwm_auto_point_hyst[1] =
- f71882fg_read8(data, F71882FG_REG_FAN_HYST1);
- }
- switch (nr) {
- case 0:
- val = (data->pwm_auto_point_hyst[0] & 0xf0) | val;
- break;
- case 1:
- val = (data->pwm_auto_point_hyst[0] & 0x0f) | (val << 4);
- break;
- case 2:
- val = (data->pwm_auto_point_hyst[1] & 0xf0) | val;
- break;
- case 3:
- val = (data->pwm_auto_point_hyst[1] & 0x0f) | (val << 4);
- break;
- }
- if (nr == 0 || nr == 1) {
- f71882fg_write8(data, F71882FG_REG_FAN_HYST0, val);
- data->pwm_auto_point_hyst[0] = val;
- } else {
- f71882fg_write8(data, F71882FG_REG_FAN_HYST1, val);
- data->pwm_auto_point_hyst[1] = val;
- }
+ reg = f71882fg_read8(data, F71882FG_REG_FAN_HYST(nr / 2));
+ if (nr & 1)
+ reg = (reg & 0x0f) | (val << 4);
+ else
+ reg = (reg & 0xf0) | val;
+
+ f71882fg_write8(data, F71882FG_REG_FAN_HYST(nr / 2), reg);
+ data->pwm_auto_point_hyst[nr / 2] = reg;
mutex_unlock(&data->update_lock);
return count;
[-- Attachment #4: Type: text/plain, Size: 153 bytes --]
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [lm-sensors] PATCH: fix various sysfs callback function issues
2008-12-11 22:21 [lm-sensors] PATCH: fix various sysfs callback function issues in Hans de Goede
` (3 preceding siblings ...)
2008-12-14 18:40 ` Hans de Goede
@ 2008-12-14 21:30 ` Jean Delvare
4 siblings, 0 replies; 6+ messages in thread
From: Jean Delvare @ 2008-12-14 21:30 UTC (permalink / raw)
To: lm-sensors
On Sun, 14 Dec 2008 19:40:37 +0100, Hans de Goede wrote:
>
>
> Jean Delvare wrote:
> > Hi Hans,
> >
> > On Sun, 14 Dec 2008 15:32:04 +0100, Hans de Goede wrote:
> >> Attached is a new version with all mentioned issues fixed.
> >
> > Doesn't build:
> >
> > drivers/hwmon/f71882fg.c: In function ‘store_pwm_auto_point_temp_hyst’:
> > drivers/hwmon/f71882fg.c:1361: error: too many arguments to function ‘f71882fg_read8’
> > drivers/hwmon/f71882fg.c:1364: error: too many arguments to function ‘f71882fg_read8’
>
> oops. I guess I forgot to test compile that one particular in between patches
> state, the next patch in the series fixes this compile error (by completely
> nuking the affected lines.
>
> Anyways attached is a fixed version, as well as an updated version of the 09
> patch, so that it applies on top of the fixed version.
Both applied, thanks.
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread