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 at 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 at 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
