[lm-sensors] [RFC/PATCH] hwmon: fix common race conditions, batch 1
Mark M. Hoffman
mhoffman at lightlink.com
Tue Mar 4 15:54:40 CET 2008
Hi all:
Here is the first in a batch of patches which aim to fix a common class
of race conditions in most hwmon drivers. Credit goes to Herbert
Valerio Riedel for pointing this out, here:
http://lists.lm-sensors.org/pipermail/lm-sensors/2007-November/022014.html
To actually trip on any of these race conditions in practice would be
*exceedingly* rare. In fact, I don't think we've ever seen a report for
such a case. OTOH, one can see by looking at this patch that the fixes
are not always completely trivial.
I will concentrate on cranking out the remaining patches as I have time.
But of course I would like thorough reviews, especially from the driver
author/maintainer if possible. To all potential reviewers: look not
just at the patch itself; please also scan the entire driver to see if
there are any races that I missed. Testing is also much appreciated.
Thanks & regards,
commit 2475ee13652b392d9720b39f274205c815979557
Author: Mark M. Hoffman <mhoffman at lightlink.com>
Date: Mon Mar 3 23:35:52 2008 -0500
hwmon: fix common race conditions, batch 1
Most hwmon drivers have one or more race conditions that could cause the driver
to display incorrect data; in the absolute worst case, these races could allow
divide-by-zero/OOPS. The most common of these involves a race between setting
a fan divider and displaying the fan data (RPMs). This patch fixes these race
conditions by taking the update lock (or equivalent) as necessary.
Signed-off-by: Mark M. Hoffman <mhoffman at lightlink.com>
diff --git a/drivers/hwmon/adm1026.c b/drivers/hwmon/adm1026.c
index 904c6ce..92bed53 100644
--- a/drivers/hwmon/adm1026.c
+++ b/drivers/hwmon/adm1026.c
@@ -838,20 +838,24 @@ static SENSOR_DEVICE_ATTR(in16_max, S_IRUGO | S_IWUSR, show_in16_max, set_in16_m
static ssize_t show_fan(struct device *dev, struct device_attribute *attr,
char *buf)
{
- struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
- int nr = sensor_attr->index;
+ int nr = to_sensor_dev_attr(attr)->index;
struct adm1026_data *data = adm1026_update_device(dev);
- return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan[nr],
- data->fan_div[nr]));
+ int val;
+ mutex_lock(&data->update_lock);
+ val = FAN_FROM_REG(data->fan[nr], data->fan_div[nr]);
+ mutex_unlock(&data->update_lock);
+ return sprintf(buf, "%d\n", val);
}
static ssize_t show_fan_min(struct device *dev, struct device_attribute *attr,
char *buf)
{
- struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
- int nr = sensor_attr->index;
+ int nr = to_sensor_dev_attr(attr)->index;
struct adm1026_data *data = adm1026_update_device(dev);
- return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan_min[nr],
- data->fan_div[nr]));
+ int val;
+ mutex_lock(&data->update_lock);
+ val = FAN_FROM_REG(data->fan_min[nr], data->fan_div[nr]);
+ mutex_unlock(&data->update_lock);
+ return sprintf(buf, "%d\n", val);
}
static ssize_t set_fan_min(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
diff --git a/drivers/hwmon/adm1029.c b/drivers/hwmon/adm1029.c
index 2c6608d..9080372 100644
--- a/drivers/hwmon/adm1029.c
+++ b/drivers/hwmon/adm1029.c
@@ -165,27 +165,33 @@ show_temp(struct device *dev, struct device_attribute *devattr, char *buf)
static ssize_t
show_fan(struct device *dev, struct device_attribute *devattr, char *buf)
{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+ int index = to_sensor_dev_attr(devattr)->index;
struct adm1029_data *data = adm1029_update_device(dev);
u16 val;
- if (data->fan[attr->index] == 0 || data->fan_div[attr->index] == 0
- || data->fan[attr->index] == 255) {
- return sprintf(buf, "0\n");
- }
-
- val = 1880 * 120 / DIV_FROM_REG(data->fan_div[attr->index])
- / data->fan[attr->index];
+ mutex_lock(&data->update_lock);
+ if (data->fan[index] == 0 || data->fan_div[index] == 0
+ || data->fan[index] == 255)
+ val = 0;
+ else
+ val = 1880 * 120 / DIV_FROM_REG(data->fan_div[index])
+ / data->fan[index];
+ mutex_unlock(&data->update_lock);
return sprintf(buf, "%d\n", val);
}
static ssize_t
show_fan_div(struct device *dev, struct device_attribute *devattr, char *buf)
{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+ int index = to_sensor_dev_attr(devattr)->index;
struct adm1029_data *data = adm1029_update_device(dev);
- if (data->fan_div[attr->index] == 0)
- return sprintf(buf, "0\n");
- return sprintf(buf, "%d\n", DIV_FROM_REG(data->fan_div[attr->index]));
+ int val;
+ mutex_lock(&data->update_lock);
+ if (data->fan_div[index] == 0)
+ val = 0;
+ else
+ val = DIV_FROM_REG(data->fan_div[index]);
+ mutex_unlock(&data->update_lock);
+ return sprintf(buf, "%d\n", val);
}
static ssize_t set_fan_div(struct device *dev,
diff --git a/drivers/hwmon/adm1031.c b/drivers/hwmon/adm1031.c
index 2bffcab..c88187a 100644
--- a/drivers/hwmon/adm1031.c
+++ b/drivers/hwmon/adm1031.c
@@ -466,8 +466,10 @@ static ssize_t show_fan(struct device *dev,
struct adm1031_data *data = adm1031_update_device(dev);
int value;
+ mutex_lock(&data->update_lock);
value = trust_fan_readings(data, nr) ? FAN_FROM_REG(data->fan[nr],
FAN_DIV_FROM_REG(data->fan_div[nr])) : 0;
+ mutex_unlock(&data->update_lock);
return sprintf(buf, "%d\n", value);
}
@@ -483,9 +485,13 @@ static ssize_t show_fan_min(struct device *dev,
{
int nr = to_sensor_dev_attr(attr)->index;
struct adm1031_data *data = adm1031_update_device(dev);
- return sprintf(buf, "%d\n",
- FAN_FROM_REG(data->fan_min[nr],
- FAN_DIV_FROM_REG(data->fan_div[nr])));
+ int value;
+
+ mutex_lock(&data->update_lock);
+ value = FAN_FROM_REG(data->fan_min[nr],
+ FAN_DIV_FROM_REG(data->fan_div[nr]));
+ mutex_unlock(&data->update_lock);
+ return sprintf(buf, "%d\n", value);
}
static ssize_t set_fan_min(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
@@ -567,11 +573,15 @@ static ssize_t show_temp(struct device *dev,
{
int nr = to_sensor_dev_attr(attr)->index;
struct adm1031_data *data = adm1031_update_device(dev);
- int ext;
+ int ext, val;
+
+ mutex_lock(&data->update_lock);
ext = nr == 0 ?
((data->ext_temp[nr] >> 6) & 0x3) * 2 :
(((data->ext_temp[nr] >> ((nr - 1) * 3)) & 7));
- return sprintf(buf, "%d\n", TEMP_FROM_REG_EXT(data->temp[nr], ext));
+ val = TEMP_FROM_REG_EXT(data->temp[nr], ext);
+ mutex_unlock(&data->update_lock);
+ return sprintf(buf, "%d\n", val);
}
static ssize_t show_temp_min(struct device *dev,
struct device_attribute *attr, char *buf)
diff --git a/drivers/hwmon/adm9240.c b/drivers/hwmon/adm9240.c
index 149ef25..0023d44 100644
--- a/drivers/hwmon/adm9240.c
+++ b/drivers/hwmon/adm9240.c
@@ -290,19 +290,25 @@ vin(5);
static ssize_t show_fan(struct device *dev,
struct device_attribute *devattr, char *buf)
{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+ int index = to_sensor_dev_attr(devattr)->index;
struct adm9240_data *data = adm9240_update_device(dev);
- return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan[attr->index],
- 1 << data->fan_div[attr->index]));
+ int val;
+ mutex_lock(&data->update_lock);
+ val = FAN_FROM_REG(data->fan[index], 1 << data->fan_div[index]);
+ mutex_unlock(&data->update_lock);
+ return sprintf(buf, "%d\n", val);
}
static ssize_t show_fan_min(struct device *dev,
struct device_attribute *devattr, char *buf)
{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+ int index = to_sensor_dev_attr(devattr)->index;
struct adm9240_data *data = adm9240_update_device(dev);
- return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan_min[attr->index],
- 1 << data->fan_div[attr->index]));
+ int val;
+ mutex_lock(&data->update_lock);
+ val = FAN_FROM_REG(data->fan_min[index], 1 << data->fan_div[index]);
+ mutex_unlock(&data->update_lock);
+ return sprintf(buf, "%d\n", val);
}
static ssize_t show_fan_div(struct device *dev,
diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
index 6b5325f..2c398d5 100644
--- a/drivers/hwmon/adt7470.c
+++ b/drivers/hwmon/adt7470.c
@@ -402,14 +402,16 @@ static ssize_t show_fan_max(struct device *dev,
struct device_attribute *devattr,
char *buf)
{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+ int index = to_sensor_dev_attr(devattr)->index;
struct adt7470_data *data = adt7470_update_device(dev);
-
- if (FAN_DATA_VALID(data->fan_max[attr->index]))
- return sprintf(buf, "%d\n",
- FAN_PERIOD_TO_RPM(data->fan_max[attr->index]));
+ int val;
+ mutex_lock(&data->lock);
+ if (FAN_DATA_VALID(data->fan_max[index]))
+ val = FAN_PERIOD_TO_RPM(data->fan_max[index]);
else
- return sprintf(buf, "0\n");
+ val = 0;
+ mutex_unlock(&data->lock);
+ return sprintf(buf, "%d\n", val);
}
static ssize_t set_fan_max(struct device *dev,
@@ -437,14 +439,16 @@ static ssize_t show_fan_min(struct device *dev,
struct device_attribute *devattr,
char *buf)
{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+ int index = to_sensor_dev_attr(devattr)->index;
struct adt7470_data *data = adt7470_update_device(dev);
-
- if (FAN_DATA_VALID(data->fan_min[attr->index]))
- return sprintf(buf, "%d\n",
- FAN_PERIOD_TO_RPM(data->fan_min[attr->index]));
+ int val;
+ mutex_lock(&data->lock);
+ if (FAN_DATA_VALID(data->fan_min[index]))
+ val = FAN_PERIOD_TO_RPM(data->fan_min[index]);
else
- return sprintf(buf, "0\n");
+ val = 0;
+ mutex_unlock(&data->lock);
+ return sprintf(buf, "%d\n", val);
}
static ssize_t set_fan_min(struct device *dev,
@@ -471,14 +475,16 @@ static ssize_t set_fan_min(struct device *dev,
static ssize_t show_fan(struct device *dev, struct device_attribute *devattr,
char *buf)
{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+ int index = to_sensor_dev_attr(devattr)->index;
struct adt7470_data *data = adt7470_update_device(dev);
-
- if (FAN_DATA_VALID(data->fan[attr->index]))
- return sprintf(buf, "%d\n",
- FAN_PERIOD_TO_RPM(data->fan[attr->index]));
+ int val;
+ mutex_lock(&data->lock);
+ if (FAN_DATA_VALID(data->fan[index]))
+ val = FAN_PERIOD_TO_RPM(data->fan[index]);
else
- return sprintf(buf, "0\n");
+ val = 0;
+ mutex_unlock(&data->lock);
+ return sprintf(buf, "%d\n", val);
}
static ssize_t show_force_pwm_max(struct device *dev,
@@ -678,14 +684,15 @@ static ssize_t show_pwm_auto_temp(struct device *dev,
struct device_attribute *devattr,
char *buf)
{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+ int index = to_sensor_dev_attr(devattr)->index;
struct adt7470_data *data = adt7470_update_device(dev);
- u8 ctrl = data->pwm_auto_temp[attr->index];
-
- if (ctrl)
- return sprintf(buf, "%d\n", 1 << (ctrl - 1));
- else
- return sprintf(buf, "%d\n", ADT7470_PWM_ALL_TEMPS);
+ u8 ctrl;
+ int val;
+ mutex_lock(&data->lock);
+ ctrl = data->pwm_auto_temp[index];
+ val = ctrl ? (1 << (ctrl - 1)) : ADT7470_PWM_ALL_TEMPS;
+ mutex_unlock(&data->lock);
+ return sprintf(buf, "%d\n", val);
}
static int cvt_auto_temp(int input)
diff --git a/drivers/hwmon/adt7473.c b/drivers/hwmon/adt7473.c
index 9587869..3807062 100644
--- a/drivers/hwmon/adt7473.c
+++ b/drivers/hwmon/adt7473.c
@@ -422,11 +422,9 @@ static ssize_t show_volt(struct device *dev, struct device_attribute *devattr,
* number in the range -128 to 127, or as an unsigned number that must
* be offset by 64.
*/
-static int decode_temp(struct adt7473_data *data, u8 raw)
+static int decode_temp(u8 twos_complement, u8 raw)
{
- if (data->temp_twos_complement)
- return (s8)raw;
- return raw - 64;
+ return twos_complement ? (s8)raw : raw - 64;
}
static u8 encode_temp(struct adt7473_data *data, int cooked)
@@ -440,10 +438,14 @@ static ssize_t show_temp_min(struct device *dev,
struct device_attribute *devattr,
char *buf)
{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+ int index = to_sensor_dev_attr(devattr)->index;
struct adt7473_data *data = adt7473_update_device(dev);
- return sprintf(buf, "%d\n",
- 1000 * decode_temp(data, data->temp_min[attr->index]));
+ int val;
+ mutex_lock(&data->lock);
+ val = 1000 * decode_temp(data->temp_twos_complement,
+ data->temp_min[index]);
+ mutex_unlock(&data->lock);
+ return sprintf(buf, "%d\n", val);
}
static ssize_t set_temp_min(struct device *dev,
@@ -470,10 +472,14 @@ static ssize_t show_temp_max(struct device *dev,
struct device_attribute *devattr,
char *buf)
{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+ int index = to_sensor_dev_attr(devattr)->index;
struct adt7473_data *data = adt7473_update_device(dev);
- return sprintf(buf, "%d\n",
- 1000 * decode_temp(data, data->temp_max[attr->index]));
+ int val;
+ mutex_lock(&data->lock);
+ val = 1000 * decode_temp(data->temp_twos_complement,
+ data->temp_max[index]);
+ mutex_unlock(&data->lock);
+ return sprintf(buf, "%d\n", val);
}
static ssize_t set_temp_max(struct device *dev,
@@ -499,24 +505,30 @@ static ssize_t set_temp_max(struct device *dev,
static ssize_t show_temp(struct device *dev, struct device_attribute *devattr,
char *buf)
{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+ int index = to_sensor_dev_attr(devattr)->index;
struct adt7473_data *data = adt7473_update_device(dev);
- return sprintf(buf, "%d\n",
- 1000 * decode_temp(data, data->temp[attr->index]));
+ int val;
+ mutex_lock(&data->lock);
+ val = 1000 * decode_temp(data->temp_twos_complement,
+ data->temp[index]);
+ mutex_unlock(&data->lock);
+ return sprintf(buf, "%d\n", val);
}
static ssize_t show_fan_min(struct device *dev,
struct device_attribute *devattr,
char *buf)
{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+ int index = to_sensor_dev_attr(devattr)->index;
struct adt7473_data *data = adt7473_update_device(dev);
-
- if (FAN_DATA_VALID(data->fan_min[attr->index]))
- return sprintf(buf, "%d\n",
- FAN_PERIOD_TO_RPM(data->fan_min[attr->index]));
+ int val;
+ mutex_lock(&data->lock);
+ if (FAN_DATA_VALID(data->fan_min[index]))
+ val = FAN_PERIOD_TO_RPM(data->fan_min[index]);
else
- return sprintf(buf, "0\n");
+ val = 0;
+ mutex_unlock(&data->lock);
+ return sprintf(buf, "%d\n", val);
}
static ssize_t set_fan_min(struct device *dev,
@@ -543,14 +555,16 @@ static ssize_t set_fan_min(struct device *dev,
static ssize_t show_fan(struct device *dev, struct device_attribute *devattr,
char *buf)
{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+ int index = to_sensor_dev_attr(devattr)->index;
struct adt7473_data *data = adt7473_update_device(dev);
-
- if (FAN_DATA_VALID(data->fan[attr->index]))
- return sprintf(buf, "%d\n",
- FAN_PERIOD_TO_RPM(data->fan[attr->index]));
+ int val;
+ mutex_lock(&data->lock);
+ if (FAN_DATA_VALID(data->fan[index]))
+ val = FAN_PERIOD_TO_RPM(data->fan[index]);
else
- return sprintf(buf, "0\n");
+ val = 0;
+ mutex_unlock(&data->lock);
+ return sprintf(buf, "%d\n", val);
}
static ssize_t show_max_duty_at_crit(struct device *dev,
@@ -669,10 +683,14 @@ static ssize_t show_temp_tmax(struct device *dev,
struct device_attribute *devattr,
char *buf)
{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+ int index = to_sensor_dev_attr(devattr)->index;
struct adt7473_data *data = adt7473_update_device(dev);
- return sprintf(buf, "%d\n",
- 1000 * decode_temp(data, data->temp_tmax[attr->index]));
+ int val;
+ mutex_lock(&data->lock);
+ val = 1000 * decode_temp(data->temp_twos_complement,
+ data->temp_tmax[index]);
+ mutex_unlock(&data->lock);
+ return sprintf(buf, "%d\n", val);
}
static ssize_t set_temp_tmax(struct device *dev,
@@ -699,10 +717,14 @@ static ssize_t show_temp_tmin(struct device *dev,
struct device_attribute *devattr,
char *buf)
{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+ int index = to_sensor_dev_attr(devattr)->index;
struct adt7473_data *data = adt7473_update_device(dev);
- return sprintf(buf, "%d\n",
- 1000 * decode_temp(data, data->temp_tmin[attr->index]));
+ int val;
+ mutex_lock(&data->lock);
+ val = 1000 * decode_temp(data->temp_twos_complement,
+ data->temp_tmin[index]);
+ mutex_unlock(&data->lock);
+ return sprintf(buf, "%d\n", val);
}
static ssize_t set_temp_tmin(struct device *dev,
diff --git a/drivers/hwmon/asb100.c b/drivers/hwmon/asb100.c
index fe2eea4..2aeac1a 100644
--- a/drivers/hwmon/asb100.c
+++ b/drivers/hwmon/asb100.c
@@ -276,8 +276,12 @@ static ssize_t show_fan(struct device *dev, struct device_attribute *attr,
{
int nr = to_sensor_dev_attr(attr)->index;
struct asb100_data *data = asb100_update_device(dev);
- return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan[nr],
- DIV_FROM_REG(data->fan_div[nr])));
+ int val;
+
+ mutex_lock(&data->update_lock);
+ val = FAN_FROM_REG(data->fan[nr], DIV_FROM_REG(data->fan_div[nr]));
+ mutex_unlock(&data->update_lock);
+ return sprintf(buf, "%d\n", val);
}
static ssize_t show_fan_min(struct device *dev, struct device_attribute *attr,
@@ -285,8 +289,12 @@ static ssize_t show_fan_min(struct device *dev, struct device_attribute *attr,
{
int nr = to_sensor_dev_attr(attr)->index;
struct asb100_data *data = asb100_update_device(dev);
- return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan_min[nr],
- DIV_FROM_REG(data->fan_div[nr])));
+ int val;
+
+ mutex_lock(&data->update_lock);
+ val = FAN_FROM_REG(data->fan_min[nr], DIV_FROM_REG(data->fan_div[nr]));
+ mutex_unlock(&data->update_lock);
+ return sprintf(buf, "%d\n", val);
}
static ssize_t show_fan_div(struct device *dev, struct device_attribute *attr,
--
Mark M. Hoffman
mhoffman at lightlink.com
More information about the lm-sensors
mailing list