2012/1/8 Guenter Roeck <guenter.roeck@xxxxxxxxxxxx>
Hi Frans,
Usually that is reflected by the variable type and the conversion function.
On Sun, Jan 08, 2012 at 12:48:17PM -0500, Frans Meulenbroeks wrote:
>
>
> 2012/1/8 Guenter Roeck <guenter.roeck@xxxxxxxxxxxx>
>
> Hi Frans,
>
> On Sun, Jan 08, 2012 at 11:23:40AM -0500, Frans Meulenbroeks wrote:
> > Signed-off-by: Frans Meulenbroeks <fransmeulenbroeks@xxxxxxxxx>
> > ---
> > drivers/hwmon/i5k_amb.c | 15 ++++++++++++---
> > 1 files changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/hwmon/i5k_amb.c b/drivers/hwmon/i5k_amb.c
> > index d22f241..c516bdc 100644
> > --- a/drivers/hwmon/i5k_amb.c
> > +++ b/drivers/hwmon/i5k_amb.c
> > @@ -159,7 +159,10 @@ static ssize_t store_amb_min(struct device *dev,
> > {
> > struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> > struct i5k_amb_data *data = ""> > > - unsigned long temp = simple_strtoul(buf, NULL, 10) / 500;
> > + unsigned long temp;
> > + int ret = kstrtoul(buf, 10, &temp) / 500;
>
> This divides the error code by 500, not temp.
>
> > + if (ret < 0)
> > + return ret;
> >
> > if (temp > 255)
> > temp = 255;
> > @@ -175,7 +178,10 @@ static ssize_t store_amb_mid(struct device *dev,
> > {
> > struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> > struct i5k_amb_data *data = ""> > > - unsigned long temp = simple_strtoul(buf, NULL, 10) / 500;
> > + unsigned long temp;
> > + int ret = kstrtoul(buf, 10, &temp) / 500;
>
> Same here.
>
> > + if (ret < 0)
> > + return ret;
> >
> > if (temp > 255)
> > temp = 255;
> > @@ -191,7 +197,10 @@ static ssize_t store_amb_max(struct device *dev,
> > {
> > struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> > struct i5k_amb_data *data = ""> > > - unsigned long temp = simple_strtoul(buf, NULL, 10) / 500;
> > + unsigned long temp;
> > + int ret = kstrtoul(buf, 10, &temp) / 500;
>
> and here.
>
> Guenter
>
>
> Yes of course!
> That is one of the risks of these edits. At some point too much autopilot
> creeps in.
> I'll send an update. There are not otehr places that hae this issue.
>
> BTW, there is one other thing with this code that might be questionalble and
> that is the fact that temp is an unsigned long.
> Is it not possible with this sensor to have a temp < 0 (I'm assuming temp is
> degrees C or maybe F).
With unsigned long and kstrtoul(), negative values are not accepted,
so that is commonly used if the sensor only supports non-negative temperatures.
Which is exactly what happens here.
Am I missing something ?
Usually both should match. If you notice a mismatch, please let us know.
> This might also be an issue on other places. (actually I seem to recall having
> seen a place where the var was signed and the conversion function was unsigned
> (or the other way around))
>
One other related thing, not really a mismatch but could also be changed:
static ssize_t set_fan_min(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
int nr = to_sensor_dev_attr(attr)->index;
struct i2c_client *client = to_i2c_client(dev);
struct lm80_data *data = ""> long val = simple_strtoul(buf, NULL, 10);
val is a long, but fan_min is quite likely not going to be negative ;-)
This might happen in other places too, but don't really have a simple way to find these.
Best regards, Frans.
static ssize_t set_fan_min(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
int nr = to_sensor_dev_attr(attr)->index;
struct i2c_client *client = to_i2c_client(dev);
struct lm80_data *data = ""> long val = simple_strtoul(buf, NULL, 10);
val is a long, but fan_min is quite likely not going to be negative ;-)
This might happen in other places too, but don't really have a simple way to find these.
Best regards, Frans.
_______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
