Re: Ticket #1851 - PATCH for adm1026.c, kernel 2.6.10-bk6

Re: Ticket #1851 - PATCH for adm1026.c, kernel 2.6.10-bk6

[Jean Delvare]

Hi Justin,

> Sorry for the slow response.  Real World vacation time intervened.

Don't be sorry :)

> Yes, I confirmed the reported problem.  The patch below should fix
> it... It should also fix any other values-not-initialized-
> to-hardware-defaults  issues.
> (...)
> --- linux-2.6.10/drivers/i2c/chips/adm1026.c.orig	2005-01-02 15:21:58.000000000 -0800
> +++ linux-2.6.10/drivers/i2c/chips/adm1026.c	2005-01-02 16:09:52.000000000 -0800
> @@ -1752,6 +1752,10 @@ int adm1026_detect(struct i2c_adapter *a
>  	device_create_file(&new_client->dev,
>  	&dev_attr_temp2_auto_point2_pwm);
>  	device_create_file(&new_client->dev,
>  	&dev_attr_temp3_auto_point2_pwm);
>  	device_create_file(&new_client->dev, &dev_attr_analog_out);
> +
> +	/* Make sure hardware defaults are read into data structure */
> +	adm1026_update_device(&new_client->dev);
> +
>  	return 0;

Mmm, I don't like this fix.

For one thing, it still leaves some room for someone to call a sysfs
callback function before the values are all intialized (because you
create the sysfs files interface first, then intialize all the values).

For another, this change will significantly increase the driver loading
time. Just check it! SMBus is slow and the ADM1026 has a lot of
registers.

The issue was already discussed on the sensors mailing-list (because the
adm1026 is not the first affected driver, although others were not
subject to division by zero). I think I remember Mark Hoffman was
actually in favor of what you suggest [1], but I would like to see this
avoided if possible.

[1] http://archives.andrew.net.au/lm-sensors/msg26017.html

Alternatives I can think of are:

1* Only intialize the few values that actually can be needed before
the update function was ever called.

2* Call the update function from the write sysfs callback functions
where needed.

All drivers implement 1* (except those which are truly broken maybe) so
far IIRC. I guess that the best choice probably depends on how much
registers have to be read, and how hard it is to read them (because this
is code duplication). That said, the relevant code could be moved to a
separate function, called by both the detect/init and update functions,
so that no slowdown occurs (except for the extra function call, but
that's nothing compared to the SMBus slowness) and the code is still not
duplicated.

-- 
Jean Delvare
http://khali.linux-fr.org/

Re: Ticket #1851 - PATCH for adm1026.c, kernel 2.6.10-bk6

[Justin Thiessen]

On Sat, Jan 01, 2005 at 12:12:05AM +0100, Jean Delvare wrote:
> > anybody see how to get a divide by zero in 2.6 adm1026 set_fan_1_min()
> > ??? It looks fine to me...
> > 
> > <http://secure.netroedge.com/~lm78/readticket.cgi?ticket=1851>
> 
> Easy. Try setting fan1_min or fan1_div before ever *reading* from the
> sysfs files. The update fonction not having been called, fan_div[0] is
> equal to 0.
> 
> Justin, can you confirm and provide a patch to fix the issue?

Sorry for the slow response.  Real World vacation time intervened.

Yes, I confirmed the reported problem.  The patch below should fix it...
It should also fix any other values-not-initialized- to-hardware-defaults 
issues.

Signed-off-by: Justin Thiessen <jthiessen@penguincomputing.com>

----------------

--- linux-2.6.10/drivers/i2c/chips/adm1026.c.orig	2005-01-02 15:21:58.000000000 -0800
+++ linux-2.6.10/drivers/i2c/chips/adm1026.c	2005-01-02 16:09:52.000000000 -0800
@@ -1752,6 +1752,10 @@ int adm1026_detect(struct i2c_adapter *a
 	device_create_file(&new_client->dev, &dev_attr_temp2_auto_point2_pwm);
 	device_create_file(&new_client->dev, &dev_attr_temp3_auto_point2_pwm);
 	device_create_file(&new_client->dev, &dev_attr_analog_out);
+
+	/* Make sure hardware defaults are read into data structure */
+	adm1026_update_device(&new_client->dev);
+
 	return 0;
 
 	/* Error out and cleanup code */

Re: Ticket #1851 - PATCH (take 2) for adm1026.c, kernel 2.6.10-bk6

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 1694 bytes --]

On Jan 03, 2005  13:37 -0800, Justin Thiessen wrote:
> The amount of duplicated code is only a few lines, and I think the result is
> clearer if it is not extracted into a separate function.  See the following
> patch.

> +	value = adm1026_read_value(client, ADM1026_REG_FAN_DIV_0_3)
> +		| (adm1026_read_value(client, ADM1026_REG_FAN_DIV_4_7)
> +		<< 8);

The formatting of this makes it hard to follow the logic.  The "<< 8"
operation isn't aligned with the nesting parenthesis and at first I
thought there was an ambiguous order of operation "|" vs "<<".

How about the following:

--- linux-2.6.10/drivers/i2c/chips/adm1026.c.orig	2005-01-02 15:21:58.000000000 -0800
+++ linux-2.6.10/drivers/i2c/chips/adm1026.c	2005-01-02 18:27:40.695689832 -0800
@@ -452,6 +452,14 @@ void adm1026_init_client(struct i2c_clie
 		client->id, value);
 	data->config1 = value;
 	adm1026_write_value(client, ADM1026_REG_CONFIG1, value);
+
+	/* initialize fan_div[] to hardware defaults */
+	value = adm1026_read_value(client, ADM1026_REG_FAN_DIV_0_3) |
+		(adm1026_read_value(client, ADM1026_REG_FAN_DIV_4_7) << 8);
+	for (i = 0;i <= 7;++i) {
+		data->fan_div[i] = DIV_FROM_REG(value & 0x03);
+		value >>= 2;
+	}
 }
 
 void adm1026_print_gpio(struct i2c_client *client)



Also, on a completely "I don't know what the hell I'm talking about" point,
it seems odd that for values named "0_3" and "4_7" you would upshift the
"4_7" value 8 bits instead of 4, but it could be just a bad choice of
variable names.

Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://members.shaw.ca/adilger/             http://members.shaw.ca/golinux/


[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: Ticket #1851 - PATCH (take 2) for adm1026.c, kernel 2.6.10-bk6

[Jean Delvare]

> Also, on a completely "I don't know what the hell I'm talking about"
> point, it seems odd that for values named "0_3" and "4_7" you would
> upshift the "4_7" value 8 bits instead of 4, but it could be just a
> bad choice of variable names.

Actually each divider is stored on 2 bits, so both the names and the
shift look OK to me.

-- 
Jean Delvare
http://khali.linux-fr.org/

Re: Ticket #1851 - PATCH (take 2) for adm1026.c, kernel 2.6.10-bk6

[Justin Thiessen]

On Mon, Jan 03, 2005 at 08:10:56PM +0100, Jean Delvare wrote:
> Hi Justin,
> 

<patch snipped>

> Mmm, I don't like this fix.
> 
> For one thing, it still leaves some room for someone to call a sysfs
> callback function before the values are all intialized (because you
> create the sysfs files interface first, then intialize all the values).
>
> For another, this change will significantly increase the driver loading
> time. Just check it! SMBus is slow and the ADM1026 has a lot of
> registers.

Good points.  

<snilp>

> Alternatives I can think of are:
> 
> 1* Only intialize the few values that actually can be needed before
> the update function was ever called.
> 
> 2* Call the update function from the write sysfs callback functions
> where needed.

Ick.  Let's go with (1).  A quick review of the adm1026_set_* functions 
doesn't seem to turn up any other situations where unextracted hardware 
defaults should cause any problems.  In all other cases where a set
function depends on data->* values that may not have been set, the
data->* value default of 0 results in the desired behavior.

So let's just have the adm1026_init_client() function read the hardware
fan divisor values and store them in fan*_div.

<snip>

> ...That said, the relevant code could be moved to a
> separate function, called by both the detect/init and update functions,
> so that no slowdown occurs (except for the extra function call, but
> that's nothing compared to the SMBus slowness) and the code is still not
> duplicated.

The amount of duplicated code is only a few lines, and I think the result is
clearer if it is not extracted into a separate function.  See the following
patch.

Signed-off-by: Justin Thiessen <jthiessen@penguincomputing.com>

-------------------

--- linux-2.6.10/drivers/i2c/chips/adm1026.c.orig	2005-01-02 15:21:58.000000000 -0800
+++ linux-2.6.10/drivers/i2c/chips/adm1026.c	2005-01-02 18:27:40.695689832 -0800
@@ -452,6 +452,15 @@ void adm1026_init_client(struct i2c_clie
 		client->id, value);
 	data->config1 = value;
 	adm1026_write_value(client, ADM1026_REG_CONFIG1, value);
+
+	/* initialize fan_div[] to hardware defaults */
+	value = adm1026_read_value(client, ADM1026_REG_FAN_DIV_0_3)
+		| (adm1026_read_value(client, ADM1026_REG_FAN_DIV_4_7)
+		<< 8);
+	for (i = 0;i <= 7;++i) {
+		data->fan_div[i] = DIV_FROM_REG(value & 0x03);
+		value >>= 2;
+	}
 }
 
 void adm1026_print_gpio(struct i2c_client *client)

Re: Ticket #1851 - PATCH (take 2) for adm1026.c, kernel 2.6.10-bk6

[Justin Thiessen]

On Mon, Jan 03, 2005 at 01:52:31PM -0700, Andreas Dilger wrote:
> On Jan 03, 2005  13:37 -0800, Justin Thiessen wrote:
> > The amount of duplicated code is only a few lines, and I think the result is
> > clearer if it is not extracted into a separate function.  See the following
> > patch.
> 
> > +	value = adm1026_read_value(client, ADM1026_REG_FAN_DIV_0_3)
> > +		| (adm1026_read_value(client, ADM1026_REG_FAN_DIV_4_7)
> > +		<< 8);
> 
> The formatting of this makes it hard to follow the logic.  The "<< 8"
> operation isn't aligned with the nesting parenthesis and at first I
> thought there was an ambiguous order of operation "|" vs "<<".
> 
> How about the following:
> 
> --- linux-2.6.10/drivers/i2c/chips/adm1026.c.orig	2005-01-02 15:21:58.000000000 -0800
> +++ linux-2.6.10/drivers/i2c/chips/adm1026.c	2005-01-02 18:27:40.695689832 -0800
> @@ -452,6 +452,14 @@ void adm1026_init_client(struct i2c_clie
>  		client->id, value);
>  	data->config1 = value;
>  	adm1026_write_value(client, ADM1026_REG_CONFIG1, value);
> +
> +	/* initialize fan_div[] to hardware defaults */
> +	value = adm1026_read_value(client, ADM1026_REG_FAN_DIV_0_3) |
> +		(adm1026_read_value(client, ADM1026_REG_FAN_DIV_4_7) << 8);
> +	for (i = 0;i <= 7;++i) {
> +		data->fan_div[i] = DIV_FROM_REG(value & 0x03);
> +		value >>= 2;
> +	}
>  }
>  
>  void adm1026_print_gpio(struct i2c_client *client)


I'm fine with shifting the CRs around if it makes everyone happier.  I was
lazy when I cut and pasted the snippet of code and did nothing other than
change the number of tabs to match the surrounding code. This, of course, 
is what actually made the reformatting you did possible.  I've read these and 
similar lines so many times I'm not sure if I'd notice whether or not they
were hard or easy to parse.  Feedback is GOOD.

> Also, on a completely "I don't know what the hell I'm talking about" point,
> it seems odd that for values named "0_3" and "4_7" you would upshift the
> "4_7" value 8 bits instead of 4, but it could be just a bad choice of
> variable names.

It's a variable nomenclature that I inherited from the 2.4.X kernel series
driver.  If you look a bit closer the code will make sense.  Remember this:

(1) There are 8 fan divisors stored in 2 byte-size registers.
(2) Each fan divisor is represented by 2 bits.

And the result should be clear.

Justin Thiessen
---------------
jthiessen@penguincomputing.com

PATCH (take 3) for adm1026.c, kernel 2.6.10-bk14

[Justin Thiessen]

Ok, take 3 on the adm1026 patch.

In this patch:

(1) Code has been added which ensures that the fan divisor registers are 
    properly read into the data structure before fan minimum speeds are 
    determined.  This prevents a possible divide by zero error.  The line 
    which reads the hardware default fan divisor values has been reformatted 
    as suggested by Andreas Dilger to make the intent of the statement clearer.

(2) In a similar spirit, an unecessary carriage return from a "dev_dbg" 
    statement in the adm1026_print_gpio() function has been elminated,
    shortening the statement to a single line and making the code easier
    to read.

Signed-off-by: Justin Thiessen <jthiessen@penguincomputing.com

---------------------------------------

--- linux-2.6.10/drivers/i2c/chips/adm1026.c.orig	2005-01-12 10:28:15.000000000 -0800
+++ linux-2.6.10/drivers/i2c/chips/adm1026.c	2005-01-12 10:30:02.000000000 -0800
@@ -452,6 +452,14 @@ void adm1026_init_client(struct i2c_clie
 		client->id, value);
 	data->config1 = value;
 	adm1026_write_value(client, ADM1026_REG_CONFIG1, value);
+
+	/* initialize fan_div[] to hardware defaults */
+	value = adm1026_read_value(client, ADM1026_REG_FAN_DIV_0_3) |
+		(adm1026_read_value(client, ADM1026_REG_FAN_DIV_4_7) << 8);
+	for (i = 0;i <= 7;++i) {
+		data->fan_div[i] = DIV_FROM_REG(value & 0x03);
+		value >>= 2;
+	}
 }
 
 void adm1026_print_gpio(struct i2c_client *client)
@@ -459,8 +467,7 @@ void adm1026_print_gpio(struct i2c_clien
 	struct adm1026_data *data = i2c_get_clientdata(client);
 	int  i;
 
-	dev_dbg(&client->dev, "(%d): GPIO config is:",
-			    client->id);
+	dev_dbg(&client->dev, "(%d): GPIO config is:", client->id);
 	for (i = 0;i <= 7;++i) {
 		if (data->config2 & (1 << i)) {
 			dev_dbg(&client->dev, "\t(%d): %sGP%s%d\n", client->id,

Re: PATCH (take 3) for adm1026.c, kernel 2.6.10-bk14

[Greg KH]

On Wed, Jan 12, 2005 at 10:50:55AM -0800, Justin Thiessen wrote:
> Ok, take 3 on the adm1026 patch.
> 
> In this patch:
> 
> (1) Code has been added which ensures that the fan divisor registers are 
>     properly read into the data structure before fan minimum speeds are 
>     determined.  This prevents a possible divide by zero error.  The line 
>     which reads the hardware default fan divisor values has been reformatted 
>     as suggested by Andreas Dilger to make the intent of the statement clearer.
> 
> (2) In a similar spirit, an unecessary carriage return from a "dev_dbg" 
>     statement in the adm1026_print_gpio() function has been elminated,
>     shortening the statement to a single line and making the code easier
>     to read.
> 
> Signed-off-by: Justin Thiessen <jthiessen@penguincomputing.com

Applied, thanks.

greg k-h