[lm-sensors] [PATCH] hwmon: (adt7473) Initialize

[lm-sensors] [PATCH] hwmon: (adt7473) Initialize

[Jean Delvare]

data->max_duty_at_overheat is not updated in adt7473_update_device,
so it might be used before it is initialized (if the user reads from
sysfs file max_duty_at_crit before writing to it.)

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Cc: Darrick J. Wong <djwong@us.ibm.com>
---
 drivers/hwmon/adt7473.c |    3 +++
 1 file changed, 3 insertions(+)

--- linux-2.6.26-rc0.orig/drivers/hwmon/adt7473.c	2008-04-26 15:36:41.000000000 +0200
+++ linux-2.6.26-rc0/drivers/hwmon/adt7473.c	2008-04-26 16:15:05.000000000 +0200
@@ -298,6 +298,9 @@ no_sensor_update:
 						ADT7473_REG_PWM_BHVR(i));
 	}
 
+	i = i2c_smbus_read_byte_data(client, ADT7473_REG_CFG4);
+	data->max_duty_at_overheat = !!(i & ADT7473_CFG4_MAX_DUTY_AT_OVT);
+
 	data->limits_last_updated = local_jiffies;
 	data->limits_valid = 1;
 


-- 
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: (adt7473) Initialize

[Paulius Zaleckas]

Jean Delvare wrote:
> --- linux-2.6.26-rc0.orig/drivers/hwmon/adt7473.c	2008-04-26 15:36:41.000000000 +0200
> +++ linux-2.6.26-rc0/drivers/hwmon/adt7473.c	2008-04-26 16:15:05.000000000 +0200
> @@ -298,6 +298,9 @@ no_sensor_update:
>  						ADT7473_REG_PWM_BHVR(i));
>  	}
>  
> +	i = i2c_smbus_read_byte_data(client, ADT7473_REG_CFG4);
> +	data->max_duty_at_overheat = !!(i & ADT7473_CFG4_MAX_DUTY_AT_OVT);

Maybe it would be more understandable to use:
data->max_duty_at_overheat = (i & ADT7473_CFG4_MAX_DUTY_AT_OVT) ? 1 : 0;

Although I don't know if gcc optimizes both to the same instructions.

> +
>  	data->limits_last_updated = local_jiffies;
>  	data->limits_valid = 1;


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

Re: [lm-sensors] [PATCH] hwmon: (adt7473) Initialize

[Jean Delvare]

Hi Paulius,

Thanks for the review.

On Mon, 28 Apr 2008 11:31:22 +0300, Paulius Zaleckas wrote:
> Jean Delvare wrote:
> > --- linux-2.6.26-rc0.orig/drivers/hwmon/adt7473.c	2008-04-26 15:36:41.000000000 +0200
> > +++ linux-2.6.26-rc0/drivers/hwmon/adt7473.c	2008-04-26 16:15:05.000000000 +0200
> > @@ -298,6 +298,9 @@ no_sensor_update:
> >  						ADT7473_REG_PWM_BHVR(i));
> >  	}
> >  
> > +	i = i2c_smbus_read_byte_data(client, ADT7473_REG_CFG4);
> > +	data->max_duty_at_overheat = !!(i & ADT7473_CFG4_MAX_DUTY_AT_OVT);
> 
> Maybe it would be more understandable to use:
> data->max_duty_at_overheat = (i & ADT7473_CFG4_MAX_DUTY_AT_OVT) ? 1 : 0;

The !! construct seems to be widely used in the kernel, I found 800
occurrences, so I guess it's OK to use it here. But if Darrick (as the
driver author) or Mark (as the subsystem maintainer) prefer your
variant, I can update the patch.

> Although I don't know if gcc optimizes both to the same instructions.

It seems so, yes (gcc 4.1.2).

> 
> > +
> >  	data->limits_last_updated = local_jiffies;
> >  	data->limits_valid = 1;

-- 
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: (adt7473) Initialize

[Darrick J. Wong]

On Sat, Apr 26, 2008 at 04:34:26PM +0200, Jean Delvare wrote:
> data->max_duty_at_overheat is not updated in adt7473_update_device,
> so it might be used before it is initialized (if the user reads from
> sysfs file max_duty_at_crit before writing to it.)
> 
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> Cc: Darrick J. Wong <djwong@us.ibm.com>

Thanks for catching this.  Looks/compiles/seems to run ok, so:
Acked-by: Darrick J. Wong <djwong@us.ibm.com>

As for !! vs (?:), I don't have any particular preference.

--D

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

Re: [lm-sensors] [PATCH] hwmon: (adt7473) Initialize

[Mark M. Hoffman]

Hi all:

* Darrick J. Wong <djwong@us.ibm.com> [2008-04-28 10:33:36 -0700]:
> On Sat, Apr 26, 2008 at 04:34:26PM +0200, Jean Delvare wrote:
> > data->max_duty_at_overheat is not updated in adt7473_update_device,
> > so it might be used before it is initialized (if the user reads from
> > sysfs file max_duty_at_crit before writing to it.)
> > 
> > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> > Cc: Darrick J. Wong <djwong@us.ibm.com>
> 
> Thanks for catching this.  Looks/compiles/seems to run ok, so:
> Acked-by: Darrick J. Wong <djwong@us.ibm.com>
> 
> As for !! vs (?:), I don't have any particular preference.

Applied to hwmon-2.6.git/testing, thanks.

-- 
Mark M. Hoffman
mhoffman@lightlink.com


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