i2c driver changes for 2.5.72

#define IT87_INIT_TEMP_HIGH_1 600
#define IT87_INIT_TEMP_LOW_1 200

ChangeSet 1.1318.3.2, 2003/06/16 11:31:43-07:00, [email protected]

[PATCH] I2C: lm85 fixups

OK Here's the patch which :
1) Fixes the race conditions

(quoted diff) +0 -0

@@ -437,10 +434,13 @@
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct lm85_data *data = i2c_get_clientdata(client);

Quoted 6 lines · Greg KH

ChangeSet 1.1318.3.2, 2003/06/16 11:31:43-07:00, [email protected]

[PATCH] I2C: lm85 fixups

OK Here's the patch which :
1) Fixes the race conditions

<cut>

Quoted 6 lines

(quoted diff) +1 -0

@@ -437,10 +434,13 @@
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct lm85_data *data = i2c_get_clientdata(client);
+	int	val;
 

Review line 445

Quoted 8 lines

(quoted diff) +3 -1

-	int val = simple_strtol(buf, NULL, 10);
+	down(&data->update_lock);
+	val = simple_strtol(buf, NULL, 10);
 	data->fan_min[nr] = FAN_TO_REG(val);
 	lm85_write_value(client, LM85_REG_FAN_MIN(nr), data->fan_min[nr]);
+	up(&data->update_lock);
 	return count;
 }

Ugh. Looks like this sort of fix is needed in every single chip driver in 2.5. And the CVS chip drivers have the same problem.

The data->foo and device register need to be in sync. So I suppose both...

At first I thought that if I always updated the chip first and then the data->foo cached value, I'd be OK, but it seemed like there were too many exceptions where values were broken apart, or recombined...

The primary thing I was trying to avoid was one CPU writing a value while another CPU was reading a value (and therefore calling update_client) and the result being that the data->foo cached value was different from actual device register.

:v)

Quoted 35 lines · Greg KH

Quoted 17 lines · Mark M. Hoffman

Quoted 6 lines · Greg KH

ChangeSet 1.1318.3.2, 2003/06/16 11:31:43-07:00, [email protected]

[PATCH] I2C: lm85 fixups

OK Here's the patch which :
1) Fixes the race conditions

<cut>

Quoted 6 lines

(quoted diff) +1 -0

@@ -437,10 +434,13 @@
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct lm85_data *data = i2c_get_clientdata(client);
+	int	val;
 

Quoted 11 lines

Review line 445

Quoted 8 lines

(quoted diff) +3 -1

-	int val = simple_strtol(buf, NULL, 10);
+	down(&data->update_lock);
+	val = simple_strtol(buf, NULL, 10);
 	data->fan_min[nr] = FAN_TO_REG(val);
 	lm85_write_value(client, LM85_REG_FAN_MIN(nr), data->fan_min[nr]);
+	up(&data->update_lock);
 	return count;
 }

Ugh. Looks like this sort of fix is needed in every single chip driver in 2.5. And the CVS chip drivers have the same problem.

Hm, wait, what are we trying to protect with this lock? The data-> values? Or access to the hardware? Or something else?

Quoted 17 lines · Mark M. Hoffman

Quoted 6 lines · Greg KH

ChangeSet 1.1318.3.2, 2003/06/16 11:31:43-07:00, [email protected]

[PATCH] I2C: lm85 fixups

OK Here's the patch which :
1) Fixes the race conditions

<cut>

Quoted 6 lines

(quoted diff) +1 -0

@@ -437,10 +434,13 @@
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct lm85_data *data = i2c_get_clientdata(client);
+	int	val;
 

Quoted 11 lines

Review line 445

Quoted 8 lines

(quoted diff) +3 -1

-	int val = simple_strtol(buf, NULL, 10);
+	down(&data->update_lock);
+	val = simple_strtol(buf, NULL, 10);
 	data->fan_min[nr] = FAN_TO_REG(val);
 	lm85_write_value(client, LM85_REG_FAN_MIN(nr), data->fan_min[nr]);
+	up(&data->update_lock);
 	return count;
 }

Ugh. Looks like this sort of fix is needed in every single chip driver in 2.5. And the CVS chip drivers have the same problem.

Hm, wait, what are we trying to protect with this lock? The data-> values? Or access to the hardware? Or something else?

thanks,

greg k-h

Quoted 2 lines

Now put the data->fan_min[nr] = value; part into xxxx_write_value() (along with the proper locking) and be done with it; less code this

Are you saying that the assignment 'data->fan_min[nr] = value' would happen inside the xxxx_write_value function?

If so, how does xxxx_write_value know which data member to store into? Are you going to pass a pointer to data->fan_min[nr] as an argument to write_value? What about type and size issues. Some values are u8, others are s8, and u16.

The other issue I see is that of partially split values. Perhaps I'm the only one that's done this, but some registers contain bits for several different sensors. (fan divisors for example...) I have been breaking those appart and putting them back together as necessary for the job at hand. In fact fan_divisors are a excellent example since they *have* to be split apart to make the fan RPM calculations easy.

What if we hoist the down/up out of update_client instead? Is there someplace higher up that we could serialize access to all the show/set functions? Is that a worse idea?

:v)

Quoted 51 lines · Mark M. Hoffman

* Philip Pokorny <[email protected]> [2003-06-19 18:45:36 -0700]:

Quoted 2 lines

The data->foo and device register need to be in sync. So I suppose both...

The device registers are protected by the I2C bus locking or ISA bus locking in xxxx_write_value. The race (in the original) that I see looked like this:

process a in show_bar() process b in set_foo()

				data->foo = baz;

/* changes data->foo */
xxxx_update_client();

				/* wrong, doesn't = baz anymore */
				xxxx_write_value(..., data->foo);

But now that I look again, why not this?

{
	struct i2c_client *client = to_i2c_client(dev);
	u8 reg ;
	int val = simple_strtol(buf, NULL, 10);

	reg = FAN_TO_REG(val);
	lm85_write_value(client, LM85_REG_FAN_MIN(nr), reg);
	return count;
}

Now put the data->fan_min[nr] = value; part into xxxx_write_value() (along with the proper locking) and be done with it; less code this way and harder to accidentally break. Actually, the more I look at it, the more I like this idea (he writes after deleting 50 lines of his first attempt.) Comments?

Quoted 11 lines

At first I thought that if I always updated the chip first and then the data->foo cached value, I'd be OK, but it seemed like there were too many exceptions where values were broken apart, or recombined...

The primary thing I was trying to avoid was one CPU writing a value while another CPU was reading a value (and therefore calling update_client) and the result being that the data->foo cached value was different from actual device register.

:v)

Quoted 10 lines · Philip Pokorny

Quoted 2 lines

Now put the data->fan_min[nr] = value; part into xxxx_write_value() (along with the proper locking) and be done with it; less code this

Are you saying that the assignment 'data->fan_min[nr] = value' would happen inside the xxxx_write_value function?

If so, how does xxxx_write_value know which data member to store into? Are you going to pass a pointer to data->fan_min[nr] as an argument to write_value? What about type and size issues. Some values are u8, others are s8, and u16.

Ack. I was just about to modify lm78.c as an example when I realized the same thing just as your message came in.

Quoted 6 lines

The other issue I see is that of partially split values. Perhaps I'm the only one that's done this, but some registers contain bits for several different sensors. (fan divisors for example...) I have been breaking those appart and putting them back together as necessary for the job at hand. In fact fan_divisors are a excellent example since they *have* to be split apart to make the fan RPM calculations easy.

Yeah, this is common to many chip drivers.

Quoted 3 lines

What if we hoist the down/up out of update_client instead? Is there someplace higher up that we could serialize access to all the show/set functions? Is that a worse idea?

I don't know (to all three questions, more or less). I'm going to step back a bit and look at what we're trying to accomplish with this structure cache of the register values in the first place.

Right, the way these chips typically work... they stop sampling while they service read or write requests. We want to allow updates no more often than a defined frequency. So is there a better way to accomplish that?

Remove this line from all set_foo():

	data->foo = FOO_TO_REG(...);

And add this to xxxx_write_value():

	data->valid = 0;

Hmmm, probably still need data->update_lock in xxxx_write_value(), but that should take care of it no?

The data->foo and device register need to be in sync. So I suppose both...

At first I thought that if I always updated the chip first and then the data->foo cached value, I'd be OK, but it seemed like there were too many exceptions where values were broken apart, or recombined...

The primary thing I was trying to avoid was one CPU writing a value while another CPU was reading a value (and therefore calling update_client) and the result being that the data->foo cached value was different from actual device register.

Quoted 6 lines · Philip Pokorny

The data->foo and device register need to be in sync. So I suppose both...

At first I thought that if I always updated the chip first and then the data->foo cached value, I'd be OK, but it seemed like there were too many exceptions where values were broken apart, or recombined...

Quoted 4 lines

The primary thing I was trying to avoid was one CPU writing a value while another CPU was reading a value (and therefore calling update_client) and the result being that the data->foo cached value was different from actual device register.

But if that happens, then the worse thing that could happen is we would show "stale" data in the resulting value.

Hm, so I can understand if we need to protect access to the hardware when we read two values in a row, like this:

                res = i2c_smbus_read_byte_data(client, reg) & 0xff ;
                res |= i2c_smbus_read_byte_data(client, reg+1) << 8 ;

or the same thing when writing.

But what about this simpler patch, that pushes the lock down to the hardware reads and writes for multiple registers. And it doesn't try to mirror the write in the "data" structure, as it's always read with the latest values for every read.

Does it look ok? It has the added benefit of making the .c and .o files smaller :)

int lm85_write_value(struct i2c_client *client, u8 reg, int value) {

(quoted diff) +4 -0

+	struct lm85_data *data = i2c_get_clientdata(client);
 	int res ;
 
+	/* serialize access to the hardware */
+	down(&data->update_lock);
+
 	switch( reg ) {
 	case LM85_REG_FAN(0) :  /* Write WORD data */
 	case LM85_REG_FAN(1) :

(quoted diff) +0 -2

@@ -1070,8 +1062,6 @@
 	struct lm85_data *data = i2c_get_clientdata(client);
 	int i;
 
-	down(&data->update_lock);
-
 	if ( !data->valid ||
 	     (jiffies - data->last_reading > LM85_DATA_INTERVAL ) ) {
 		/* Things that change quickly */

Quoted 13 lines

(quoted diff) +0 -2

@@ -1070,8 +1062,6 @@
 	struct lm85_data *data = i2c_get_clientdata(client);
 	int i;
 
-	down(&data->update_lock);
-
 	if ( !data->valid ||
 	     (jiffies - data->last_reading > LM85_DATA_INTERVAL ) ) {
 		/* Things that change quickly */

@@ -1209,8 +1199,6 @@
 	};  /* last_config */
 

Review line 1202

Quoted 4 lines

(quoted diff) +0 -2

 	data->valid = 1;
-
-	up(&data->update_lock);
 }

We only want to service one update_client() call per some interval. I think this lock needs to stay.

Quoted 7 lines · Mark M. Hoffman

Quoted 5 lines · Greg KH

Hm, so I can understand if we need to protect access to the hardware when we read two values in a row, like this:

              res = i2c_smbus_read_byte_data(client, reg) & 0xff ;
              res |= i2c_smbus_read_byte_data(client, reg+1) << 8 ;

Quoted 7 lines

Quoted 1 line

or the same thing when writing.

Good point. The I2C core provides I2C/SMBus locking to prevent trashing the bus itself, but that doesn't help the above. Which brings about another question... why wasn't i2c_smbus_read_word_data() used in that case? Philip?

I actually tried that, but it didn't work. My original code had a #define that would let you try and re-enable it, but I ditched it when I didn't see any other drivers using read_word variants.

I should have spoken up that read_word seemed to be broken at the time.

But as I write this, I think I read in the chip data sheet that it doesn't support SMBus word reads... (Go figure...) That was probably the real reason I pulled the read_word code...

Quoted 1 line

And one last off-topic nit: data->fan_ppr is set but not used?

The fan_ppr features is only on the ADM1027 variant. Perhaps Margit didn't migrate that code? Otherwise, there are calls to PPR_FROM_REG() in the cvs code...

Quoted 10 lines · Philip Pokorny

The data->foo and device register need to be in sync. So I suppose both...

At first I thought that if I always updated the chip first and then the data->foo cached value, I'd be OK, but it seemed like there were too many exceptions where values were broken apart, or recombined...

The primary thing I was trying to avoid was one CPU writing a value while another CPU was reading a value (and therefore calling update_client) and the result being that the data->foo cached value was different from actual device register.

But if that happens, then the worse thing that could happen is we would show "stale" data in the resulting value.

Hm, so I can understand if we need to protect access to the hardware when we read two values in a row, like this:

                res = i2c_smbus_read_byte_data(client, reg) & 0xff ;
                res |= i2c_smbus_read_byte_data(client, reg+1) << 8 ;

or the same thing when writing.

But what about this simpler patch, that pushes the lock down to the hardware reads and writes for multiple registers. And it doesn't try to mirror the write in the "data" structure, as it's always read with the latest values for every read.

Does it look ok? It has the added benefit of making the .c and .o files smaller :)

thanks,

greg k-h

# I2C: clean up lm85's locking code.

diff -Nru a/drivers/i2c/chips/lm85.c b/drivers/i2c/chips/lm85.c

drivers/i2c/chips/lm85.c +14 -14

@@ -433,14 +433,11 @@
 		size_t count, int nr)
 {
 	struct i2c_client *client = to_i2c_client(dev);
-	struct lm85_data *data = i2c_get_clientdata(client);
 	int	val;
 
-	down(&data->update_lock);
 	val = simple_strtol(buf, NULL, 10);
-	data->fan_min[nr] = FAN_TO_REG(val);
-	lm85_write_value(client, LM85_REG_FAN_MIN(nr), data->fan_min[nr]);
-	up(&data->update_lock);
+	val = FAN_TO_REG(val);
+	lm85_write_value(client, LM85_REG_FAN_MIN(nr), val);
 	return count;
 }
 

@@ -527,14 +524,11 @@
 		size_t count, int nr)
 {
 	struct i2c_client *client = to_i2c_client(dev);
-	struct lm85_data *data = i2c_get_clientdata(client);
 	int	val;
 
-	down(&data->update_lock);
 	val = simple_strtol(buf, NULL, 10);
-	data->pwm[nr] = PWM_TO_REG(val);
-	lm85_write_value(client, LM85_REG_PWM(nr), data->pwm[nr]);
-	up(&data->update_lock);
+	val = PWM_TO_REG(val);
+	lm85_write_value(client, LM85_REG_PWM(nr), val);
 	return count;
 }
 [ snip  ]					\

@@ -955,8 +937,12 @@
 
 int lm85_read_value(struct i2c_client *client, u8 reg)
 {
+	struct lm85_data *data = i2c_get_clientdata(client);
 	int res;
 
+	/* serialize access to the hardware */
+	down(&data->update_lock);
+
 	/* What size location is it? */
 	switch( reg ) {
 	case LM85_REG_FAN(0) :  /* Read WORD data */

@@ -980,14 +966,19 @@
 		res = i2c_smbus_read_byte_data(client, reg);
 		break ;
 	}
+	up(&data->update_lock);
 
 	return res ;
 }
 
 int lm85_write_value(struct i2c_client *client, u8 reg, int value)
 {
+	struct lm85_data *data = i2c_get_clientdata(client);
 	int res ;
 
+	/* serialize access to the hardware */
+	down(&data->update_lock);
+
 	switch( reg ) {
 	case LM85_REG_FAN(0) :  /* Write WORD data */
 	case LM85_REG_FAN(1) :

@@ -1009,6 +1000,7 @@
 		res = i2c_smbus_write_byte_data(client, reg, value);
 		break ;
 	}
+	up(&data->update_lock);
 
 	return res ;
 }

@@ -1070,8 +1062,6 @@
 	struct lm85_data *data = i2c_get_clientdata(client);
 	int i;
 
-	down(&data->update_lock);
-
 	if ( !data->valid ||
 	     (jiffies - data->last_reading > LM85_DATA_INTERVAL ) ) {
 		/* Things that change quickly */

@@ -1209,8 +1199,6 @@
 	};  /* last_config */
 
 	data->valid = 1;
-
-	up(&data->update_lock);
 }
 
 

Quoted 8 lines · Greg KH

Quoted 6 lines · Philip Pokorny

The data->foo and device register need to be in sync. So I suppose both...

At first I thought that if I always updated the chip first and then the data->foo cached value, I'd be OK, but it seemed like there were too many exceptions where values were broken apart, or recombined...

Quoted 7 lines

Quoted 4 lines

The primary thing I was trying to avoid was one CPU writing a value while another CPU was reading a value (and therefore calling update_client) and the result being that the data->foo cached value was different from actual device register.

But if that happens, then the worse thing that could happen is we would show "stale" data in the resulting value.

Nope - as it was originally written (before Philip's most recent patch), we could lose the value we meant to write.

Quoted 5 lines

Hm, so I can understand if we need to protect access to the hardware when we read two values in a row, like this:

               res = i2c_smbus_read_byte_data(client, reg) & 0xff ;
               res |= i2c_smbus_read_byte_data(client, reg+1) << 8 ;

Quoted 1 line

or the same thing when writing.

Good point. The I2C core provides I2C/SMBus locking to prevent trashing the bus itself, but that doesn't help the above. Which brings about another question... why wasn't i2c_smbus_read_word_data() used in that case? Philip?

Quoted 4 lines

But what about this simpler patch, that pushes the lock down to the hardware reads and writes for multiple registers. And it doesn't try to mirror the write in the "data" structure, as it's always read with the latest values for every read.

It looks like this scheme would actually work for lm85.c But, for most chip drivers with fan divisors there would still be a race. E.g. look at set_fan_min() and set_fan_div() in lm78.c; the meaning of data->fan_min depends on data->fan_div so those two must be coherent. That locking can't be pushed further down.

That lm85 doesn't use fan divisors is the exception rather than the rule. I mean, I don't hate this patch... but it doesn't tell us anything about how to fix the rest of them. I still think Philip's original brute force locking ;) is necessary there.

Quoted 16 lines

Does it look ok? It has the added benefit of making the .c and .o files smaller :)

int lm85_write_value(struct i2c_client *client, u8 reg, int value) {

(quoted diff) +1 -0

+	struct lm85_data *data = i2c_get_clientdata(client);

	int res ;

drivers/i2c/chips/lm78.c new file +3 -0

+	/* serialize access to the hardware */
+	down(&data->update_lock);
+

	switch( reg ) {
	case LM85_REG_FAN(0) :  /* Write WORD data */
	case LM85_REG_FAN(1) :

Review line 1006

Quoted 7 lines

(quoted diff) +1 -0

@@ -1009,6 +1000,7 @@
 		res = i2c_smbus_write_byte_data(client, reg, value);
 		break ;
 	}
+	up(&data->update_lock);
 
 	return res ;
 }

I would add "data->valid = 0;" somewhere before the up(). Then you're guaranteed never to read stale data after a write.

Quoted 10 lines

(quoted diff) +0 -2

@@ -1070,8 +1062,6 @@
 	struct lm85_data *data = i2c_get_clientdata(client);
 	int i;
 
-	down(&data->update_lock);
-
 	if ( !data->valid ||
 	     (jiffies - data->last_reading > LM85_DATA_INTERVAL ) ) {
 		/* Things that change quickly */

Review line 1202

Quoted 6 lines

(quoted diff) +0 -2

@@ -1209,8 +1199,6 @@
 	};  /* last_config */
 
 	data->valid = 1;
-
-	up(&data->update_lock);
 }

We only want to service one update_client() call per some interval. I think this lock needs to stay.

And one last off-topic nit: data->fan_ppr is set but not used?

Quoted 22 lines · Philip Pokorny

Quoted 8 lines · Mark M. Hoffman

Quoted 5 lines · Greg KH

Hm, so I can understand if we need to protect access to the hardware when we read two values in a row, like this:

             res = i2c_smbus_read_byte_data(client, reg) & 0xff ;
             res |= i2c_smbus_read_byte_data(client, reg+1) << 8 ;

Quoted 7 lines

Quoted 1 line

or the same thing when writing.

Good point. The I2C core provides I2C/SMBus locking to prevent trashing the bus itself, but that doesn't help the above. Which brings about another question... why wasn't i2c_smbus_read_word_data() used in that case? Philip?

I actually tried that, but it didn't work. My original code had a #define that would let you try and re-enable it, but I ditched it when I didn't see any other drivers using read_word variants.

Quoted 5 lines

I should have spoken up that read_word seemed to be broken at the time.

But as I write this, I think I read in the chip data sheet that it doesn't support SMBus word reads... (Go figure...) That was probably the real reason I pulled the read_word code...

It's just as well... maybe some bus drivers don't support it either. It doesn't hurt to use lowest common denominator SMBus routines if possible.