Hi Darrick,
On Mon, 06 Oct 2008 18:19:19 -0700, Darrick J. Wong wrote:
>
No description for this patch? IMHO it's large enough to deserve one.
> Signed-off-by: Darrick J. Wong <djwong at us.ibm.com>
> ---
>
> drivers/hwmon/adt7470.c | 77 +++++++++++++++++++++++++++++++++++++++--------
> 1 files changed, 64 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
> index ef26014..5acc800 100644
> --- a/drivers/hwmon/adt7470.c
> +++ b/drivers/hwmon/adt7470.c
> @@ -345,6 +345,11 @@ static ssize_t show_temp_min(struct device *dev,
> return sprintf(buf, "%d\n", 1000 * data->temp_min[attr->index]);
> }
>
> +static int range_check(const long val, const long min, const long max)
> +{
> + return val >= min && val <= max;
> +}
Should definitely be inlined.
> +
> static ssize_t set_temp_min(struct device *dev,
> struct device_attribute *devattr,
> const char *buf,
> @@ -353,7 +358,14 @@ static ssize_t set_temp_min(struct device *dev,
> struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> struct i2c_client *client = to_i2c_client(dev);
> struct adt7470_data *data = i2c_get_clientdata(client);
> - int temp = simple_strtol(buf, NULL, 10) / 1000;
> + long temp;
> +
> + if (strict_strtol(buf, 10, &temp))
> + return -EINVAL;
> +
> + temp /= 1000;
While you're here, proper rounding would be welcome.
> + if (!range_check(temp, 0, 255))
> + return -EINVAL;
For temperature, voltage and fan speed values, the standard behavior is
best effort: we pick the nearest available value instead of returning
an error. We have a function for that: SENSORS_LIMIT(). Please use it.
Rationale: the boundaries for these values are chip-dependent so it
isn't fair to cause user commands to fail. We do range checks for
values which are normalized, such as PWM values (range 0-255 for all
chips) or when discrete values have specific meanings so picking the
nearest possible value makes no sense (e.g. PWM modes.)
>
> mutex_lock(&data->lock);
> data->temp_min[attr->index] = temp;
> @@ -381,7 +393,14 @@ static ssize_t set_temp_max(struct device *dev,
> struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> struct i2c_client *client = to_i2c_client(dev);
> struct adt7470_data *data = i2c_get_clientdata(client);
> - int temp = simple_strtol(buf, NULL, 10) / 1000;
> + long temp;
> +
> + if (strict_strtol(buf, 10, &temp))
> + return -EINVAL;
> +
> + temp /= 1000;
> + if (!range_check(temp, 0, 255))
> + return -EINVAL;
>
> mutex_lock(&data->lock);
> data->temp_max[attr->index] = temp;
> @@ -430,11 +449,14 @@ static ssize_t set_fan_max(struct device *dev,
> struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> struct i2c_client *client = to_i2c_client(dev);
> struct adt7470_data *data = i2c_get_clientdata(client);
> - int temp = simple_strtol(buf, NULL, 10);
> + long temp;
>
> - if (!temp)
> + if (strict_strtol(buf, 10, &temp) || !temp)
> return -EINVAL;
> +
> temp = FAN_RPM_TO_PERIOD(temp);
> + if (!range_check(temp, 0, 65535))
Do you really want to accept 0 as a valid value? show_fan_max() will
consider it an invalid value.
> + return -EINVAL;
>
> mutex_lock(&data->lock);
> data->fan_max[attr->index] = temp;
> @@ -465,11 +487,14 @@ static ssize_t set_fan_min(struct device *dev,
> struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> struct i2c_client *client = to_i2c_client(dev);
> struct adt7470_data *data = i2c_get_clientdata(client);
> - int temp = simple_strtol(buf, NULL, 10);
> + long temp;
>
> - if (!temp)
> + if (strict_strtol(buf, 10, &temp) || !temp)
> return -EINVAL;
> +
> temp = FAN_RPM_TO_PERIOD(temp);
> + if (!range_check(temp, 0, 65535))
Ditto.
> + return -EINVAL;
>
> mutex_lock(&data->lock);
> data->fan_min[attr->index] = temp;
> @@ -507,9 +532,12 @@ static ssize_t set_force_pwm_max(struct device *dev,
> {
> struct i2c_client *client = to_i2c_client(dev);
> struct adt7470_data *data = i2c_get_clientdata(client);
> - int temp = simple_strtol(buf, NULL, 10);
> + long temp;
> u8 reg;
>
> + if (strict_strtol(buf, 10, &temp))
> + return -EINVAL;
> +
> mutex_lock(&data->lock);
> data->force_pwm_max = temp;
> reg = i2c_smbus_read_byte_data(client, ADT7470_REG_CFG);
> @@ -537,7 +565,10 @@ static ssize_t set_pwm(struct device *dev, struct device_attribute *devattr,
> struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> struct i2c_client *client = to_i2c_client(dev);
> struct adt7470_data *data = i2c_get_clientdata(client);
> - int temp = simple_strtol(buf, NULL, 10);
> + long temp;
> +
> + if (strict_strtol(buf, 10, &temp) || !range_check(temp, 0, 255))
> + return -EINVAL;
>
> mutex_lock(&data->lock);
> data->pwm[attr->index] = temp;
> @@ -564,7 +595,10 @@ static ssize_t set_pwm_max(struct device *dev,
> struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> struct i2c_client *client = to_i2c_client(dev);
> struct adt7470_data *data = i2c_get_clientdata(client);
> - int temp = simple_strtol(buf, NULL, 10);
> + long temp;
> +
> + if (strict_strtol(buf, 10, &temp) || !range_check(temp, 0, 255))
> + return -EINVAL;
>
> mutex_lock(&data->lock);
> data->pwm_max[attr->index] = temp;
> @@ -592,7 +626,10 @@ static ssize_t set_pwm_min(struct device *dev,
> struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> struct i2c_client *client = to_i2c_client(dev);
> struct adt7470_data *data = i2c_get_clientdata(client);
> - int temp = simple_strtol(buf, NULL, 10);
> + long temp;
> +
> + if (strict_strtol(buf, 10, &temp) || !range_check(temp, 0, 255))
> + return -EINVAL;
>
> mutex_lock(&data->lock);
> data->pwm_min[attr->index] = temp;
> @@ -630,7 +667,14 @@ static ssize_t set_pwm_tmin(struct device *dev,
> struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> struct i2c_client *client = to_i2c_client(dev);
> struct adt7470_data *data = i2c_get_clientdata(client);
> - int temp = simple_strtol(buf, NULL, 10) / 1000;
> + long temp;
> +
> + if (strict_strtol(buf, 10, &temp))
> + return -EINVAL;
> +
> + temp /= 1000;
> + if (!range_check(temp, 0, 255))
> + return -EINVAL;
>
> mutex_lock(&data->lock);
> data->pwm_tmin[attr->index] = temp;
> @@ -658,11 +702,14 @@ static ssize_t set_pwm_auto(struct device *dev,
> struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> struct i2c_client *client = to_i2c_client(dev);
> struct adt7470_data *data = i2c_get_clientdata(client);
> - int temp = simple_strtol(buf, NULL, 10);
> int pwm_auto_reg = ADT7470_REG_PWM_CFG(attr->index);
> int pwm_auto_reg_mask;
> + long temp;
> u8 reg;
>
> + if (strict_strtol(buf, 10, &temp))
> + return -EINVAL;
> +
> if (attr->index % 2)
> pwm_auto_reg_mask = ADT7470_PWM2_AUTO_MASK;
> else
> @@ -716,10 +763,14 @@ static ssize_t set_pwm_auto_temp(struct device *dev,
> struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> struct i2c_client *client = to_i2c_client(dev);
> struct adt7470_data *data = i2c_get_clientdata(client);
> - int temp = cvt_auto_temp(simple_strtol(buf, NULL, 10));
> int pwm_auto_reg = ADT7470_REG_PWM_AUTO_TEMP(attr->index);
> + long temp;
> u8 reg;
>
> + if (strict_strtol(buf, 10, &temp))
> + return -EINVAL;
> +
> + temp = cvt_auto_temp(temp);
> if (temp < 0)
> return temp;
>
Please send an updated patch.
Same comments apply to the adt7473 patch as well.
--
Jean Delvare
