[lm-sensors] [PATCH] hwmon: fix common race conditions, batch 2

[lm-sensors] [PATCH] hwmon: fix common race conditions, batch 2

[Mark M. Hoffman]

Hi:

 atxp1.c    |    2 ++
 coretemp.c |    2 ++
 dme1737.c  |    6 ++++++
 f71882fg.c |   14 ++++++++++----
 gl518sm.c  |   18 ++++++++++++++----
 gl520sm.c  |   16 ++++++++++++----
 it87.c     |   26 ++++++++++++++++----------
 7 files changed, 62 insertions(+), 22 deletions(-)

NB: reviewers are also encouraged to have a look at these files, in which I
found no race condtions of this class:

ds1621.c
f71805f.c
f75375s.c
fscher.c
fschmd.c
fscpos.c
i5k_amb.c
ibmpex.c

commit e62da7be499f5553ce682aa6830a4e51ab293619
Author: Mark M. Hoffman <mhoffman@lightlink.com>
Date:   Sun Apr 6 13:45:00 2008 -0400

    hwmon: fix common race conditions, batch 2
    
    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@lightlink.com>

diff --git a/drivers/hwmon/atxp1.c b/drivers/hwmon/atxp1.c
index 01c17e3..aca58c9 100644
--- a/drivers/hwmon/atxp1.c
+++ b/drivers/hwmon/atxp1.c
@@ -136,10 +136,12 @@ static ssize_t atxp1_storevcore(struct device *dev, struct device_attribute *att
 	}
 
 	/* If output enabled, use control register value. Otherwise original CPU VID */
+	mutex_lock(&data->update_lock);
 	if (data->reg.vid & ATXP1_VIDENA)
 		cvid = data->reg.vid & ATXP1_VIDMASK;
 	else
 		cvid = data->reg.cpu_vid;
+	mutex_unlock(&data->update_lock);
 
 	/* Nothing changed, aborting */
 	if (vid = cvid)
diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index 70239ac..1fbaeff 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -93,12 +93,14 @@ static ssize_t show_temp(struct device *dev,
 	struct coretemp_data *data = coretemp_update_device(dev);
 	int err;
 
+	mutex_lock(&data->update_lock);
 	if (attr->index = SHOW_TEMP)
 		err = data->valid ? sprintf(buf, "%d\n", data->temp) : -EAGAIN;
 	else if (attr->index = SHOW_TJMAX)
 		err = sprintf(buf, "%d\n", data->tjmax);
 	else
 		err = sprintf(buf, "%d\n", data->ttarget);
+	mutex_unlock(&data->update_lock);
 	return err;
 }
 
diff --git a/drivers/hwmon/dme1737.c b/drivers/hwmon/dme1737.c
index 7673f65..28c9d25 100644
--- a/drivers/hwmon/dme1737.c
+++ b/drivers/hwmon/dme1737.c
@@ -878,6 +878,7 @@ static ssize_t show_zone(struct device *dev, struct device_attribute *attr,
 	int fn = sensor_attr_2->nr;
 	int res;
 
+	mutex_lock(&data->update_lock);
 	switch (fn) {
 	case SYS_ZONE_AUTO_CHANNELS_TEMP:
 		/* check config2 for non-standard temp-to-zone mapping */
@@ -906,6 +907,7 @@ static ssize_t show_zone(struct device *dev, struct device_attribute *attr,
 		res = 0;
 		dev_dbg(dev, "Unknown function %d.\n", fn);
 	}
+	mutex_unlock(&data->update_lock);
 
 	return sprintf(buf, "%d\n", res);
 }
@@ -987,6 +989,7 @@ static ssize_t show_fan(struct device *dev, struct device_attribute *attr,
 	int fn = sensor_attr_2->nr;
 	int res;
 
+	mutex_lock(&data->update_lock);
 	switch (fn) {
 	case SYS_FAN_INPUT:
 		res = FAN_FROM_REG(data->fan[ix],
@@ -1013,6 +1016,7 @@ static ssize_t show_fan(struct device *dev, struct device_attribute *attr,
 		res = 0;
 		dev_dbg(dev, "Unknown function %d.\n", fn);
 	}
+	mutex_unlock(&data->update_lock);
 
 	return sprintf(buf, "%d\n", res);
 }
@@ -1099,6 +1103,7 @@ static ssize_t show_pwm(struct device *dev, struct device_attribute *attr,
 	int fn = sensor_attr_2->nr;
 	int res;
 
+	mutex_lock(&data->update_lock);
 	switch (fn) {
 	case SYS_PWM:
 		if (PWM_EN_FROM_REG(data->pwm_config[ix]) = 0) {
@@ -1149,6 +1154,7 @@ static ssize_t show_pwm(struct device *dev, struct device_attribute *attr,
 		res = 0;
 		dev_dbg(dev, "Unknown function %d.\n", fn);
 	}
+	mutex_unlock(&data->update_lock);
 
 	return sprintf(buf, "%d\n", res);
 }
diff --git a/drivers/hwmon/f71882fg.c b/drivers/hwmon/f71882fg.c
index cbeb498..a97ebde 100644
--- a/drivers/hwmon/f71882fg.c
+++ b/drivers/hwmon/f71882fg.c
@@ -593,9 +593,12 @@ static ssize_t show_temp_max_hyst(struct device *dev, struct device_attribute
 {
 	struct f71882fg_data *data = f71882fg_update_device(dev);
 	int nr = to_sensor_dev_attr(devattr)->index;
+	int res;
 
-	return sprintf(buf, "%d\n",
-		(data->temp_high[nr] - data->temp_hyst[nr]) * 1000);
+	mutex_lock(&data->update_lock);
+	res = (data->temp_high[nr] - data->temp_hyst[nr]) * 1000;
+	mutex_unlock(&data->update_lock);
+	return sprintf(buf, "%d\n", res);
 }
 
 static ssize_t store_temp_max_hyst(struct device *dev, struct device_attribute
@@ -670,9 +673,12 @@ static ssize_t show_temp_crit_hyst(struct device *dev, struct device_attribute
 {
 	struct f71882fg_data *data = f71882fg_update_device(dev);
 	int nr = to_sensor_dev_attr(devattr)->index;
+	int res;
 
-	return sprintf(buf, "%d\n",
-		(data->temp_ovt[nr] - data->temp_hyst[nr]) * 1000);
+	mutex_lock(&data->update_lock);
+	res = (data->temp_ovt[nr] - data->temp_hyst[nr]) * 1000;
+	mutex_unlock(&data->update_lock);
+	return sprintf(buf, "%d\n", res);
 }
 
 static ssize_t show_temp_type(struct device *dev, struct device_attribute
diff --git a/drivers/hwmon/gl518sm.c b/drivers/hwmon/gl518sm.c
index 33e9e8a..8b83690 100644
--- a/drivers/hwmon/gl518sm.c
+++ b/drivers/hwmon/gl518sm.c
@@ -191,8 +191,13 @@ static ssize_t show_fan_input(struct device *dev,
 {
 	int nr = to_sensor_dev_attr(attr)->index;
 	struct gl518_data *data = gl518_update_device(dev);
-	return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan_in[nr],
-					DIV_FROM_REG(data->fan_div[nr])));
+	int result;
+
+	mutex_lock(&data->update_lock);
+	result = FAN_FROM_REG(data->fan_in[nr],
+				DIV_FROM_REG(data->fan_div[nr]));
+	mutex_unlock(&data->update_lock);
+	return sprintf(buf, "%d\n", result);
 }
 
 static ssize_t show_fan_min(struct device *dev,
@@ -200,8 +205,13 @@ static ssize_t show_fan_min(struct device *dev,
 {
 	int nr = to_sensor_dev_attr(attr)->index;
 	struct gl518_data *data = gl518_update_device(dev);
-	return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan_min[nr],
-					DIV_FROM_REG(data->fan_div[nr])));
+	int result;
+
+	mutex_lock(&data->update_lock);
+	result = FAN_FROM_REG(data->fan_min[nr],
+				DIV_FROM_REG(data->fan_div[nr]));
+	mutex_unlock(&data->update_lock);
+	return sprintf(buf, "%d\n", result);
 }
 
 static ssize_t show_fan_div(struct device *dev,
diff --git a/drivers/hwmon/gl520sm.c b/drivers/hwmon/gl520sm.c
index 8984ef1..804018d 100644
--- a/drivers/hwmon/gl520sm.c
+++ b/drivers/hwmon/gl520sm.c
@@ -273,9 +273,13 @@ static ssize_t get_fan_input(struct device *dev, struct device_attribute *attr,
 {
 	int n = to_sensor_dev_attr(attr)->index;
 	struct gl520_data *data = gl520_update_device(dev);
+	int result;
 
-	return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan_input[n],
-						 data->fan_div[n]));
+	mutex_lock(&data->update_lock);
+	result = FAN_FROM_REG(data->fan_input[n],
+				data->fan_div[n]);
+	mutex_unlock(&data->update_lock);
+	return sprintf(buf, "%d\n", result);
 }
 
 static ssize_t get_fan_min(struct device *dev, struct device_attribute *attr,
@@ -283,9 +287,13 @@ static ssize_t get_fan_min(struct device *dev, struct device_attribute *attr,
 {
 	int n = to_sensor_dev_attr(attr)->index;
 	struct gl520_data *data = gl520_update_device(dev);
+	int result;
 
-	return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan_min[n],
-						 data->fan_div[n]));
+	mutex_lock(&data->update_lock);
+	result = FAN_FROM_REG(data->fan_min[n],
+				 data->fan_div[n]);
+	mutex_unlock(&data->update_lock);
+	return sprintf(buf, "%d\n", result);
 }
 
 static ssize_t get_fan_div(struct device *dev, struct device_attribute *attr,
diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
index e12c132..45a216f 100644
--- a/drivers/hwmon/it87.c
+++ b/drivers/hwmon/it87.c
@@ -502,22 +502,28 @@ show_sensor_offset(3);
 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 it87_data *data = it87_update_device(dev);
-	return sprintf(buf,"%d\n", FAN_FROM_REG(data->fan[nr], 
-				DIV_FROM_REG(data->fan_div[nr])));
+	int result;
+
+	mutex_lock(&data->update_lock);
+	result = FAN_FROM_REG(data->fan[nr],
+			DIV_FROM_REG(data->fan_div[nr]));
+	mutex_unlock(&data->update_lock);
+	return sprintf(buf, "%d\n", result);
 }
 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 it87_data *data = it87_update_device(dev);
-	return sprintf(buf,"%d\n",
-		FAN_FROM_REG(data->fan_min[nr], DIV_FROM_REG(data->fan_div[nr])));
+	int result;
+
+	mutex_lock(&data->update_lock);
+	result = FAN_FROM_REG(data->fan_min[nr],
+			DIV_FROM_REG(data->fan_div[nr]));
+	mutex_unlock(&data->update_lock);
+	return sprintf(buf, "%d\n", result);
 }
 static ssize_t show_fan_div(struct device *dev, struct device_attribute *attr,
 		char *buf)

Regards,

-- 
Mark M. Hoffman
mhoffman@lightlink.com


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: fix common race conditions, batch 2

[Hans de Goede]

Mark M. Hoffman wrote:
> Hi:
> 
>  atxp1.c    |    2 ++
>  coretemp.c |    2 ++
>  dme1737.c  |    6 ++++++
>  f71882fg.c |   14 ++++++++++----
>  gl518sm.c  |   18 ++++++++++++++----
>  gl520sm.c  |   16 ++++++++++++----
>  it87.c     |   26 ++++++++++++++++----------
>  7 files changed, 62 insertions(+), 22 deletions(-)
> 

<snip>

> diff --git a/drivers/hwmon/f71882fg.c b/drivers/hwmon/f71882fg.c
> index cbeb498..a97ebde 100644
> --- a/drivers/hwmon/f71882fg.c
> +++ b/drivers/hwmon/f71882fg.c
> @@ -593,9 +593,12 @@ static ssize_t show_temp_max_hyst(struct device *dev, struct device_attribute
>  {
>  	struct f71882fg_data *data = f71882fg_update_device(dev);
>  	int nr = to_sensor_dev_attr(devattr)->index;
> +	int res;
>  
> -	return sprintf(buf, "%d\n",
> -		(data->temp_high[nr] - data->temp_hyst[nr]) * 1000);
> +	mutex_lock(&data->update_lock);
> +	res = (data->temp_high[nr] - data->temp_hyst[nr]) * 1000;
> +	mutex_unlock(&data->update_lock);
> +	return sprintf(buf, "%d\n", res);
>  }
>  
>  static ssize_t store_temp_max_hyst(struct device *dev, struct device_attribute
> @@ -670,9 +673,12 @@ static ssize_t show_temp_crit_hyst(struct device *dev, struct device_attribute
>  {
>  	struct f71882fg_data *data = f71882fg_update_device(dev);
>  	int nr = to_sensor_dev_attr(devattr)->index;
> +	int res;
>  
> -	return sprintf(buf, "%d\n",
> -		(data->temp_ovt[nr] - data->temp_hyst[nr]) * 1000);
> +	mutex_lock(&data->update_lock);
> +	res = (data->temp_ovt[nr] - data->temp_hyst[nr]) * 1000;
> +	mutex_unlock(&data->update_lock);
> +	return sprintf(buf, "%d\n", res);
>  }
>  
>  static ssize_t show_temp_type(struct device *dev, struct device_attribute

I don't believe this is necessary, as data->temp_ovt / data->temp_high can be 
changed independend of data->temp_hyst. SO if a reader and a writer are racing 
with the locks the reader will either get the value from before or after the 
read, and without the lock, the same.

There are no parts of the code which change both data->temp_ovt/data->temp_high 
and data->temp_hyst at _the same time_. With the one exception being the code 
reading the values back from the IC in update_device(), so unless something is 
mucking with the hardware underneath us, in which case we have much bigger 
problems! , there us no race here.

The non locking in these functions was done by design.

With that said, I have no objections against this patch if it gives people a 
nice safe warm fuzzy feeling to have this patch present.

---

Talking about something else touching the hardware, maybe we should change the 
code which reads non changing registers periodically to actually check for 
unexpected changes, and yell if these are found, because AFAIK thats the only 
reason why we read these registers periodically even though they should 
theoretically never change.

Regards,

Hans

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: fix common race conditions, batch 2

[Jean Delvare]

Hi Mark,

On Sun, 6 Apr 2008 14:07:21 -0400, Mark M. Hoffman wrote:
> Hi:
> 
>  atxp1.c    |    2 ++
>  coretemp.c |    2 ++
>  dme1737.c  |    6 ++++++
>  f71882fg.c |   14 ++++++++++----
>  gl518sm.c  |   18 ++++++++++++++----
>  gl520sm.c  |   16 ++++++++++++----
>  it87.c     |   26 ++++++++++++++++----------
>  7 files changed, 62 insertions(+), 22 deletions(-)
> 
> NB: reviewers are also encouraged to have a look at these files, in which I
> found no race condtions of this class:
> 
> ds1621.c
> f71805f.c
> f75375s.c
> fscher.c
> fschmd.c
> fscpos.c
> i5k_amb.c
> ibmpex.c

I checked ds1621.c and f71805f.c and I didn't find any race condition
either.

> 
> commit e62da7be499f5553ce682aa6830a4e51ab293619
> Author: Mark M. Hoffman <mhoffman@lightlink.com>
> Date:   Sun Apr 6 13:45:00 2008 -0400
> 
>     hwmon: fix common race conditions, batch 2
>     
>     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@lightlink.com>
> 

Looks good, suggestions for improvement inline below. I also tested the
it87 part, no problem.

> diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
> index 70239ac..1fbaeff 100644
> --- a/drivers/hwmon/coretemp.c
> +++ b/drivers/hwmon/coretemp.c
> @@ -93,12 +93,14 @@ static ssize_t show_temp(struct device *dev,
>  	struct coretemp_data *data = coretemp_update_device(dev);
>  	int err;
>  
> +	mutex_lock(&data->update_lock);
>  	if (attr->index = SHOW_TEMP)
>  		err = data->valid ? sprintf(buf, "%d\n", data->temp) : -EAGAIN;
>  	else if (attr->index = SHOW_TJMAX)
>  		err = sprintf(buf, "%d\n", data->tjmax);
>  	else
>  		err = sprintf(buf, "%d\n", data->ttarget);
> +	mutex_unlock(&data->update_lock);

You really only need to grab the mutex for the first case (attr->index
= SHOW_TEMP).

>  	return err;
>  }
>  

<snip>

> diff --git a/drivers/hwmon/gl520sm.c b/drivers/hwmon/gl520sm.c
> index 8984ef1..804018d 100644
> --- a/drivers/hwmon/gl520sm.c
> +++ b/drivers/hwmon/gl520sm.c
> @@ -273,9 +273,13 @@ static ssize_t get_fan_input(struct device *dev, struct device_attribute *attr,
>  {
>  	int n = to_sensor_dev_attr(attr)->index;
>  	struct gl520_data *data = gl520_update_device(dev);
> +	int result;
>  
> -	return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan_input[n],
> -						 data->fan_div[n]));
> +	mutex_lock(&data->update_lock);
> +	result = FAN_FROM_REG(data->fan_input[n],
> +				data->fan_div[n]);

Would fit on a single line.

> +	mutex_unlock(&data->update_lock);
> +	return sprintf(buf, "%d\n", result);
>  }
>  
>  static ssize_t get_fan_min(struct device *dev, struct device_attribute *attr,
> @@ -283,9 +287,13 @@ static ssize_t get_fan_min(struct device *dev, struct device_attribute *attr,
>  {
>  	int n = to_sensor_dev_attr(attr)->index;
>  	struct gl520_data *data = gl520_update_device(dev);
> +	int result;
>  
> -	return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan_min[n],
> -						 data->fan_div[n]));
> +	mutex_lock(&data->update_lock);
> +	result = FAN_FROM_REG(data->fan_min[n],
> +				 data->fan_div[n]);

Here too.

> +	mutex_unlock(&data->update_lock);
> +	return sprintf(buf, "%d\n", result);
>  }
>  
>  static ssize_t get_fan_div(struct device *dev, struct device_attribute *attr,


-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: fix common race conditions, batch 2

[Jean Delvare]

Hi Hans,

On Mon, 07 Apr 2008 07:49:06 +0200, Hans de Goede wrote:
> Mark M. Hoffman wrote:
> > Hi:
> > 
> >  atxp1.c    |    2 ++
> >  coretemp.c |    2 ++
> >  dme1737.c  |    6 ++++++
> >  f71882fg.c |   14 ++++++++++----
> >  gl518sm.c  |   18 ++++++++++++++----
> >  gl520sm.c  |   16 ++++++++++++----
> >  it87.c     |   26 ++++++++++++++++----------
> >  7 files changed, 62 insertions(+), 22 deletions(-)
> > 
> 
> <snip>
> 
> > diff --git a/drivers/hwmon/f71882fg.c b/drivers/hwmon/f71882fg.c
> > index cbeb498..a97ebde 100644
> > --- a/drivers/hwmon/f71882fg.c
> > +++ b/drivers/hwmon/f71882fg.c
> > @@ -593,9 +593,12 @@ static ssize_t show_temp_max_hyst(struct device *dev, struct device_attribute
> >  {
> >  	struct f71882fg_data *data = f71882fg_update_device(dev);
> >  	int nr = to_sensor_dev_attr(devattr)->index;
> > +	int res;
> >  
> > -	return sprintf(buf, "%d\n",
> > -		(data->temp_high[nr] - data->temp_hyst[nr]) * 1000);
> > +	mutex_lock(&data->update_lock);
> > +	res = (data->temp_high[nr] - data->temp_hyst[nr]) * 1000;
> > +	mutex_unlock(&data->update_lock);
> > +	return sprintf(buf, "%d\n", res);
> >  }
> >  
> >  static ssize_t store_temp_max_hyst(struct device *dev, struct device_attribute
> > @@ -670,9 +673,12 @@ static ssize_t show_temp_crit_hyst(struct device *dev, struct device_attribute
> >  {
> >  	struct f71882fg_data *data = f71882fg_update_device(dev);
> >  	int nr = to_sensor_dev_attr(devattr)->index;
> > +	int res;
> >  
> > -	return sprintf(buf, "%d\n",
> > -		(data->temp_ovt[nr] - data->temp_hyst[nr]) * 1000);
> > +	mutex_lock(&data->update_lock);
> > +	res = (data->temp_ovt[nr] - data->temp_hyst[nr]) * 1000;
> > +	mutex_unlock(&data->update_lock);
> > +	return sprintf(buf, "%d\n", res);
> >  }
> >  
> >  static ssize_t show_temp_type(struct device *dev, struct device_attribute
> 
> I don't believe this is necessary, as data->temp_ovt / data->temp_high can be 
> changed independend of data->temp_hyst. SO if a reader and a writer are racing 
> with the locks the reader will either get the value from before or after the 
> read, and without the lock, the same.
> 
> There are no parts of the code which change both data->temp_ovt/data->temp_high 
> and data->temp_hyst at _the same time_. With the one exception being the code 
> reading the values back from the IC in update_device(), so unless something is 
> mucking with the hardware underneath us, in which case we have much bigger 
> problems! , there us no race here.

update_device() is exactly what Mark's patch attempts to protect the
sysfs callbacks from.

> 
> The non locking in these functions was done by design.
> 
> With that said, I have no objections against this patch if it gives people a 
> nice safe warm fuzzy feeling to have this patch present.
> 
> ---
> 
> Talking about something else touching the hardware, maybe we should change the 
> code which reads non changing registers periodically to actually check for 
> unexpected changes, and yell if these are found, because AFAIK thats the only 
> reason why we read these registers periodically even though they should 
> theoretically never change.

You can't read on a regular basis from registers which you don't expect
to change, and at the same time not handle the case properly if it were
to happen. We have to be consistent in our choices. The general
decision for Linux hardware monitoring drivers was to read the
registers that aren't supposed to change. While this decision could be
discussed (as you seem to be willing to do), as long as we don't change
it, I agree with Mark that we should have a locking model consistent
with this choice.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: fix common race conditions, batch 2

[Hans de Goede]

Jean Delvare wrote:
> Hi Hans,
> 
> On Mon, 07 Apr 2008 07:49:06 +0200, Hans de Goede wrote:
>> I don't believe this is necessary, as data->temp_ovt / data->temp_high can be 
>> changed independend of data->temp_hyst. SO if a reader and a writer are racing 
>> with the locks the reader will either get the value from before or after the 
>> read, and without the lock, the same.
>>
>> There are no parts of the code which change both data->temp_ovt/data->temp_high 
>> and data->temp_hyst at _the same time_. With the one exception being the code 
>> reading the values back from the IC in update_device(), so unless something is 
>> mucking with the hardware underneath us, in which case we have much bigger 
>> problems! , there us no race here.
> 
> update_device() is exactly what Mark's patch attempts to protect the
> sysfs callbacks from.
> 

As already said I have no objections to adding the locking, I just thought it 
was worthwhile to notice that it isn't entirely needed.

>> Talking about something else touching the hardware, maybe we should change the 
>> code which reads non changing registers periodically to actually check for 
>> unexpected changes, and yell if these are found, because AFAIK thats the only 
>> reason why we read these registers periodically even though they should 
>> theoretically never change.
> 
> You can't read on a regular basis from registers which you don't expect
> to change, and at the same time not handle the case properly if it were
> to happen.

I agree.

> We have to be consistent in our choices. The general
> decision for Linux hardware monitoring drivers was to read the
> registers that aren't supposed to change. While this decision could be
> discussed (as you seem to be willing to do),

Yes, I wonder why we read registers which are not supposed to change?
The only reason I can come up with is that we suspect something maybe meddling 
with the hardware underneath us (most likely ACPI). If this is the reason, then 
we should check if that has _actually happened_ and if it does report it 
(through printk/dmesg). Thinking about this I wonder of the use of reading 
these registers regulary at all, even if ACPI is communicating with the hwmon 
device, I think its highly unlikely it will be changing any settings (like for 
example limits) after boot.

So my vote goes out to rather then adding this locking, remove the periodically 
reading of registers which never change. With that said, I still have no 
objections to this patch going in in the mean time, until we decide upon how to 
handle this in the future and then start preparing the necessary patches.

> as long as we don't change
> it, I agree with Mark that we should have a locking model consistent
> with this choice.

Ok.

Regards,

Hans

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: fix common race conditions, batch 2

[Jean Delvare]

Hi Hans,

On Mon, 07 Apr 2008 22:04:24 +0200, Hans de Goede wrote:
> Jean Delvare wrote:
> > We have to be consistent in our choices. The general
> > decision for Linux hardware monitoring drivers was to read the
> > registers that aren't supposed to change. While this decision could be
> > discussed (as you seem to be willing to do),
> 
> Yes, I wonder why we read registers which are not supposed to change?
> The only reason I can come up with is that we suspect something maybe meddling 
> with the hardware underneath us (most likely ACPI). If this is the reason, then 

ACPI or SMM, yes. Could also be user-space but then it's really the
user's responsibility.

> we should check if that has _actually happened_ and if it does report it 
> (through printk/dmesg). Thinking about this I wonder of the use of reading 
> these registers regulary at all, even if ACPI is communicating with the hwmon 
> device, I think its highly unlikely it will be changing any settings (like for 
> example limits) after boot.

I've seen it at least once. The chip had an output pin going low when
either the low or the high temperature limit was crossed. The BIOS was
changing the limits when the pin went low, so it was sort of emulating
multiple levels of thermal alert while the chip only supported one in
hardware.

> So my vote goes out to rather then adding this locking, remove the periodically 
> reading of registers which never change. With that said, I still have no 
> objections to this patch going in in the mean time, until we decide upon how to 
> handle this in the future and then start preparing the necessary patches.

I don't like the current approach either. In fact it seems that every
new developer finds it odd at first, and in the end we only keep doing
it because it has been so from the beginning of the lm-sensors project.
The fact is that our drivers would perform much better if we did not
repeatedly read from registers which aren't supposed to change. Some
drivers mitigate the performance penalty by reading from these
registers less frequently, but then it somewhat misses the original
point of always reflecting the current register state.

But on the other hand, anything that helps spot conflicts with ACPI or
SMM is welcome, as these issues can be very hard to investigate - in
particular for SMBus-based chips or chips with index/data pair I/O port
access, where simultaneous access can result in data corruption. Thomas
Renninger and myself proposed something that should help for ACPI in
some cases but Linus rejected our approach. Removing the extra reads
from our drivers would be one less way to spot that kind of problem. Of
course we can look at the registers directly using i2cdump or isadump
in most cases, but users are less likely to do that, while they usually
do notice when limits change mysteriously.

So all in all I have to admit that I don't really know what to do.
Maybe the extra reads should be made a compilation-time or (probably
better) a run-time option, so we leave the decision to the user?

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: fix common race conditions, batch 2

[Hans de Goede]

Hi Jean,

Jean Delvare wrote:
> Hi Hans,
> 
> On Mon, 07 Apr 2008 22:04:24 +0200, Hans de Goede wrote:
>> So my vote goes out to rather then adding this locking, remove the periodically 
>> reading of registers which never change. With that said, I still have no 
>> objections to this patch going in in the mean time, until we decide upon how to 
>> handle this in the future and then start preparing the necessary patches.
> 
> I don't like the current approach either. In fact it seems that every
> new developer finds it odd at first, and in the end we only keep doing
> it because it has been so from the beginning of the lm-sensors project.
> The fact is that our drivers would perform much better if we did not
> repeatedly read from registers which aren't supposed to change. Some
> drivers mitigate the performance penalty by reading from these
> registers less frequently, but then it somewhat misses the original
> point of always reflecting the current register state.
> 
> But on the other hand, anything that helps spot conflicts with ACPI or
> SMM is welcome, as these issues can be very hard to investigate - in
> particular for SMBus-based chips or chips with index/data pair I/O port
> access, where simultaneous access can result in data corruption. Thomas
> Renninger and myself proposed something that should help for ACPI in
> some cases but Linus rejected our approach. Removing the extra reads
> from our drivers would be one less way to spot that kind of problem. Of
> course we can look at the registers directly using i2cdump or isadump
> in most cases, but users are less likely to do that, while they usually
> do notice when limits change mysteriously.
> 
> So all in all I have to admit that I don't really know what to do.
> Maybe the extra reads should be made a compilation-time or (probably
> better) a run-time option, so we leave the decision to the user?
> 

I think given the slowness of reading these registers (esp in the i2c / smbus 
case) making it an option is a good idea, I think we should combine this with 
when this option is activated (it should be default off) actually checking if 
things were changed and if so complain.

Then when we suspect faulplay by acpi/smm we can ask the user to turn on the 
option.

Does that sound like a plan?

Regards,

Hans


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: fix common race conditions,

[Jean Delvare]

On Fri, 11 Apr 2008 09:18:24 +0200, Hans de Goede wrote:
> Jean Delvare wrote:
> > I don't like the current approach either. In fact it seems that every
> > new developer finds it odd at first, and in the end we only keep doing
> > it because it has been so from the beginning of the lm-sensors project.
> > The fact is that our drivers would perform much better if we did not
> > repeatedly read from registers which aren't supposed to change. Some
> > drivers mitigate the performance penalty by reading from these
> > registers less frequently, but then it somewhat misses the original
> > point of always reflecting the current register state.
> > 
> > But on the other hand, anything that helps spot conflicts with ACPI or
> > SMM is welcome, as these issues can be very hard to investigate - in
> > particular for SMBus-based chips or chips with index/data pair I/O port
> > access, where simultaneous access can result in data corruption. Thomas
> > Renninger and myself proposed something that should help for ACPI in
> > some cases but Linus rejected our approach. Removing the extra reads
> > from our drivers would be one less way to spot that kind of problem. Of
> > course we can look at the registers directly using i2cdump or isadump
> > in most cases, but users are less likely to do that, while they usually
> > do notice when limits change mysteriously.
> > 
> > So all in all I have to admit that I don't really know what to do.
> > Maybe the extra reads should be made a compilation-time or (probably
> > better) a run-time option, so we leave the decision to the user?
> 
> I think given the slowness of reading these registers (esp in the i2c / smbus 
> case) making it an option is a good idea, I think we should combine this with 
> when this option is activated (it should be default off) actually checking if 
> things were changed and if so complain.
> 
> Then when we suspect faulplay by acpi/smm we can ask the user to turn on the 
> option.
> 
> Does that sound like a plan?

Fine with me, as long as this is a run-time option and not a build-time
option. Rebuilding a kernel module is not an option for many users.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: fix common race conditions,

[Hans de Goede]

Jean Delvare wrote:
> On Fri, 11 Apr 2008 09:18:24 +0200, Hans de Goede wrote:
>> Jean Delvare wrote:
>>> I don't like the current approach either. In fact it seems that every
>>> new developer finds it odd at first, and in the end we only keep doing
>>> it because it has been so from the beginning of the lm-sensors project.
>>> The fact is that our drivers would perform much better if we did not
>>> repeatedly read from registers which aren't supposed to change. Some
>>> drivers mitigate the performance penalty by reading from these
>>> registers less frequently, but then it somewhat misses the original
>>> point of always reflecting the current register state.
>>>
>>> But on the other hand, anything that helps spot conflicts with ACPI or
>>> SMM is welcome, as these issues can be very hard to investigate - in
>>> particular for SMBus-based chips or chips with index/data pair I/O port
>>> access, where simultaneous access can result in data corruption. Thomas
>>> Renninger and myself proposed something that should help for ACPI in
>>> some cases but Linus rejected our approach. Removing the extra reads
>>> from our drivers would be one less way to spot that kind of problem. Of
>>> course we can look at the registers directly using i2cdump or isadump
>>> in most cases, but users are less likely to do that, while they usually
>>> do notice when limits change mysteriously.
>>>
>>> So all in all I have to admit that I don't really know what to do.
>>> Maybe the extra reads should be made a compilation-time or (probably
>>> better) a run-time option, so we leave the decision to the user?
>> I think given the slowness of reading these registers (esp in the i2c / smbus 
>> case) making it an option is a good idea, I think we should combine this with 
>> when this option is activated (it should be default off) actually checking if 
>> things were changed and if so complain.
>>
>> Then when we suspect faulplay by acpi/smm we can ask the user to turn on the 
>> option.
>>
>> Does that sound like a plan?
> 
> Fine with me, as long as this is a run-time option and not a build-time
> option. Rebuilding a kernel module is not an option for many users.
> 

Yes it should definitely be a runtime option, even one which can changed 
through sysfs.

So now I guess its time for everyone to start writing patches for the drivers 
they maintain to implement this. I'll start working on patches for my hwmon 
drivers as time permits (iow it may take a while, as I'm awefully busy lately).

Regards,

Hans


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors