w83781d fan_div code refactoring

PATCH New Jean Delvare 3 participants 6 messages May 19, 2005

lore

Share & tools

lore

Jean Delvare

author

2005-05-19 06:24 UTC #0 lore raw

PATCH 1/1 w83781d fan_div code refactoring

Quoting myself:

Quoted 3 lines

This tends to increase the size of the three set_store_regs_fan_div functions, and I am considering refactoring them at some point. Later though.

Here is the promised refactoring. Tested on my AS99127F rev.1, seems to work. As for the previous patch, there is a part that I cannot test with the AS99127F, so additional testing is welcome.

I agree this makes the code slightly less readable, but this saves 60 lines of code (1754 bytes, around 3% of the driver total), and is actually far less complex that I first feared.

While testing, I found a corner case that isn't handled properly. It doesn't seem to be handled by the lm78 and the asb100 either. Setting fanN_div before ever reading from the chip or setting fanN_min will make use of fanN_min while it was never initialized.

Mark, can you confirm this behavior with your asb100 driver? Maybe I missed something...

While a corner case, we better fix it quickly since it is very likely to happen. We used to set fan divisors before fan mins in sensors.conf because setting fan divisors was breaking fan mins, and this will trigger the bug, since we invite users to run "sensors -s" prior to running "sensors".

This patch doesn't address the issue. I plan to submit a cross-driver patch later for this instead.

I can think of three ways to solve the issue and would like to hear opinions about which we should use:

1* In the driver's init function, read fan_mins from the chip so they are initialized.

2* In the store_fan_div function, call update_device.

3* In the store_fan_div function, if data->valid = 0, read the fan_min from the chip.

Each method introduces a slowdown at some point. #1 slows the module loading, #2 slows the first "sensors -s", #3 slows setting fan divisors (each time). Additionally, #1 and #3 can be said to break the caching mechanism we implement in each chip driver.

I think I would go for #1, but have no strong preference actually.

Anyway, here's the refactoring patch. Greg, is this acceptable?

Thanks.

drivers/i2c/chips/w83781d.c renamed from drivers/i2c/chips/w83781d.c.orig +17 ~~-77~~

@@ -620,7 +620,7 @@
    least suprise; the user doesn't expect the fan minimum to change just
    because the divisor changed. */
 static ssize_t
-store_regs_fan_div_1(struct device *dev, const char *buf, size_t count)
+store_fan_div_reg(struct device *dev, const char *buf, size_t count, int nr)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct w83781d_data *data = i2c_get_clientdata(client);

@@ -628,92 +628,28 @@
 	u8 reg;
 
 	/* Save fan_min */
-	min = FAN_FROM_REG(data->fan_min[0],
-			   DIV_FROM_REG(data->fan_div[0]));
+	min = FAN_FROM_REG(data->fan_min[nr],
+			   DIV_FROM_REG(data->fan_div[nr]));
 
-	data->fan_div[0] = DIV_TO_REG(simple_strtoul(buf, NULL, 10),
+	data->fan_div[nr] = DIV_TO_REG(simple_strtoul(buf, NULL, 10),
 				      data->type);
 
-	reg = w83781d_read_value(client, W83781D_REG_VID_FANDIV) & 0xcf;
-	reg |= (data->fan_div[0] & 0x03) << 4;
-	w83781d_write_value(client, W83781D_REG_VID_FANDIV, reg);
+	reg = (w83781d_read_value(client, nr=2 ? W83781D_REG_PIN : W83781D_REG_VID_FANDIV)
+	       & (nr=0 ? 0xcf : 0x3f))
+	    | ((data->fan_div[nr] & 0x03) << (nr=0 ? 4 : 6));
+	w83781d_write_value(client, nr=2 ? W83781D_REG_PIN : W83781D_REG_VID_FANDIV, reg);
 
 	/* w83781d and as99127f don't have extended divisor bits */
 	if (data->type != w83781d && data->type != as99127f) {
-		reg = w83781d_read_value(client, W83781D_REG_VBAT) & 0xdf;
-		reg |= (data->fan_div[0] & 0x04) << 3;
+		reg = (w83781d_read_value(client, W83781D_REG_VBAT)
+		       & ~(1 << (5 + nr)))
+		    | ((data->fan_div[nr] & 0x04) << (3 + nr));
 		w83781d_write_value(client, W83781D_REG_VBAT, reg);
 	}
 
 	/* Restore fan_min */
-	data->fan_min[0] = FAN_TO_REG(min, DIV_FROM_REG(data->fan_div[0]));
-	w83781d_write_value(client, W83781D_REG_FAN_MIN(1), data->fan_min[0]);
-
-	return count;
-}
-
-static ssize_t
-store_regs_fan_div_2(struct device *dev, const char *buf, size_t count)
-{
-	struct i2c_client *client = to_i2c_client(dev);
-	struct w83781d_data *data = i2c_get_clientdata(client);
-	unsigned long min;
-	u8 reg;
-
-	/* Save fan_min */
-	min = FAN_FROM_REG(data->fan_min[1],
-			   DIV_FROM_REG(data->fan_div[1]));
-
-	data->fan_div[1] = DIV_TO_REG(simple_strtoul(buf, NULL, 10),
-				      data->type);
-
-	reg = w83781d_read_value(client, W83781D_REG_VID_FANDIV) & 0x3f;
-	reg |= (data->fan_div[1] & 0x03) << 6;
-	w83781d_write_value(client, W83781D_REG_VID_FANDIV, reg);
-
-	/* w83781d and as99127f don't have extended divisor bits */
-	if (data->type != w83781d && data->type != as99127f) {
-		reg = w83781d_read_value(client, W83781D_REG_VBAT) & 0xbf;
-		reg |= (data->fan_div[1] & 0x04) << 4;
-		w83781d_write_value(client, W83781D_REG_VBAT, reg);
-	}
-
-	/* Restore fan_min */
-	data->fan_min[1] = FAN_TO_REG(min, DIV_FROM_REG(data->fan_div[1]));
-	w83781d_write_value(client, W83781D_REG_FAN_MIN(2), data->fan_min[1]);
-
-	return count;
-}
-
-static ssize_t
-store_regs_fan_div_3(struct device *dev, const char *buf, size_t count)
-{
-	struct i2c_client *client = to_i2c_client(dev);
-	struct w83781d_data *data = i2c_get_clientdata(client);
-	unsigned long min;
-	u8 reg;
-
-	/* Save fan_min */
-	min = FAN_FROM_REG(data->fan_min[2],
-			   DIV_FROM_REG(data->fan_div[2]));
-
-	data->fan_div[2] = DIV_TO_REG(simple_strtoul(buf, NULL, 10),
-				      data->type);
-
-	reg = w83781d_read_value(client, W83781D_REG_PIN) & 0x3f;
-	reg |= (data->fan_div[2] & 0x03) << 6;
-	w83781d_write_value(client, W83781D_REG_PIN, reg);
-
-	/* w83781d and as99127f don't have extended divisor bits */
-	if (data->type != w83781d && data->type != as99127f) {
-		reg = w83781d_read_value(client, W83781D_REG_VBAT) & 0x7f;
-		reg |= (data->fan_div[2] & 0x04) << 5;
-		w83781d_write_value(client, W83781D_REG_VBAT, reg);
-	}
-
-	/* Restore fan_min */
-	data->fan_min[2] = FAN_TO_REG(min, DIV_FROM_REG(data->fan_div[2]));
-	w83781d_write_value(client, W83781D_REG_FAN_MIN(3), data->fan_min[2]);
+	data->fan_min[nr] = FAN_TO_REG(min, DIV_FROM_REG(data->fan_div[nr]));
+	w83781d_write_value(client, W83781D_REG_FAN_MIN(nr+1), data->fan_min[nr]);
 
 	return count;
 }

@@ -722,6 +658,10 @@
 static ssize_t show_regs_fan_div_##offset (struct device *dev, char *buf) \
 { \
 	return show_fan_div_reg(dev, buf, offset); \
+} \
+static ssize_t store_regs_fan_div_##offset (struct device *dev, const char *buf, size_t count) \
+{ \
+	return store_fan_div_reg(dev, buf, count, offset - 1); \
 } \
 static DEVICE_ATTR(fan##offset##_div, S_IRUGO | S_IWUSR, show_regs_fan_div_##offset, store_regs_fan_div_##offset)

Signature · 2 lines

Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/

Jean Delvare

author

2005-05-19 06:24 UTC #1 lore raw

PATCH 1/1 w83781d fan_div code refactoring

Quoting "Mark M. Hoffman" <[email protected]>:

Quoted 8 lines

Quoted 5 lines
While a corner case, we better fix it quickly since it is very likely to happen. We used to set fan divisors before fan mins in sensors.conf because setting fan divisors was breaking fan mins, and this will trigger the bug, since we invite users to run "sensors -s" prior to running "sensors".
But it's not real dangerous, since who inits fanX_div without also init'ing fanX_min immediately after?

You're correct, this is what will most likely happen in a majority of cases. But not necessarily all the cases. One can comment out the set fanXmin lines in sensors.conf. And anyway, it still temporarily writes a random value for fanN_min into the chip, which could trigger an alarm and possibly other nasty actions. Remember these reports of "as I load module X, my computer instantly goes off"...

Quoted 8 lines

Quoted 8 lines
I can think of three ways to solve the issue and would like to hear opinions about which we should use:
1* In the driver's init function, read fan_mins from the chip so they are initialized.
2* In the store_fan_div function, call update_device.

Quoted 8 lines

Quoted 3 lines
3* In the store_fan_div function, if data->valid = 0, read the fan_min from the chip.
(...)
I prefer #1: it fixes the whole *class* of bugs which can arise from interactions between set/show functions and uninit'ed <driver>_data structure elements. E.g. I can see this type of bug creeping in to smart PWM control where it could be much more dangerous.

I don't think that #1 as I described it fixes the whole class, since it only works for fan_mins. Instead, what you seem to suggest would be:

4* Call update_device upon driver init.

This would really slow the driver loading, for sure. But then we're safe, and we even could get rid of data->valid (providing we add a "force" parameter to update_device; not sure it's worth it).

I also can think of another possibility now:

5* Do not try to preserve fanN_min if data->valid = 0.

This one doesn't slow anything significantly. Ironically, this is the first thing I tried when finding the bug, and I forgot to mention it as a possible solution.

Of course, this one does only work for the store_fan_div case, other cases would have to handle the issue by themselves, in the same way or a different one.

Solution #4 can also be modified to something more subtle. We could divide internal register data in two sets. One set with registers we want to read at driver init, one set with the other registers. Each set would have its own "valid" and/or last_updated variables. This is somewhat similar to what Phil Pokorny does in the lm85 driver (one timestamp for limits, one for readings, each set has a different buffer validity limit) or me in the eeprom driver (each slice has its own validity bit and last_updated timestamp).

This requires more code, of course, but would slow down the init only as
much as stricly necessary. I'm not sure the benefit is worth the work however.

I think I still prefer solution #1. Looks like the best tradeoff. If other sysfs functions need other registers, we simply add initializing reads for them in the init function.

Signature · 2 lines

Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/

Mark M. Hoffman

2005-05-19 06:24 UTC #2 lore raw

PATCH 1/1 w83781d fan_div code refactoring

In reply to Jean Delvare

Quoted 7 lines · Jean Delvare

While testing, I found a corner case that isn't handled properly. It doesn't seem to be handled by the lm78 and the asb100 either. Setting fanN_div before ever reading from the chip or setting fanN_min will make use of fanN_min while it was never initialized.
Mark, can you confirm this behavior with your asb100 driver? Maybe I missed something...

You're right, it's a bug...

Quoted 5 lines

While a corner case, we better fix it quickly since it is very likely to happen. We used to set fan divisors before fan mins in sensors.conf because setting fan divisors was breaking fan mins, and this will trigger the bug, since we invite users to run "sensors -s" prior to running "sensors".

But it's not real dangerous, since who inits fanX_div without also init'ing fanX_min immediately after?

Quoted 14 lines

This patch doesn't address the issue. I plan to submit a cross-driver patch later for this instead.
I can think of three ways to solve the issue and would like to hear opinions about which we should use:
1* In the driver's init function, read fan_mins from the chip so they are initialized.
2* In the store_fan_div function, call update_device.
3* In the store_fan_div function, if data->valid = 0, read the fan_min from the chip.

Quoted 6 lines

Each method introduces a slowdown at some point. #1 slows the module loading, #2 slows the first "sensors -s", #3 slows setting fan divisors (each time). Additionally, #1 and #3 can be said to break the caching mechanism we implement in each chip driver.
I think I would go for #1, but have no strong preference actually.

I prefer #1: it fixes the whole *class* of bugs which can arise from interactions between set/show functions and uninit'ed <driver>_data structure elements. E.g. I can see this type of bug creeping in to smart PWM control where it could be much more dangerous.

Regards,

Signature · 2 lines

Mark M. Hoffman
[email protected]

Greg KH

2005-05-19 06:24 UTC #3 lore raw

PATCH 1/1 w83781d fan_div code refactoring

In reply to Jean Delvare

Quoted 6 lines · Jean Delvare

Quoting myself:
Quoted 3 lines
This tends to increase the size of the three set_store_regs_fan_div functions, and I am considering refactoring them at some point. Later though.

Quoted 7 lines

Here is the promised refactoring. Tested on my AS99127F rev.1, seems to work. As for the previous patch, there is a part that I cannot test with the AS99127F, so additional testing is welcome.
I agree this makes the code slightly less readable, but this saves 60 lines of code (1754 bytes, around 3% of the driver total), and is actually far less complex that I first feared.

The patch looks fine to me, so I've applied it, thanks.

greg k-h

Jean Delvare

author

2005-05-19 06:24 UTC #4 lore raw

PATCH 1/1 w83781d fan_div code refactoring

In reply to Jean Delvare

According to Mark M. Hoffman:

Quoted 2 lines

I disagree, but I see that I'm a little late on this one since patches have already been accepted.

You can still propose a different solution. If we tend to agree that it's better, there's no reason we wouldn't change from "my" method to yours. My main concern was to have a specific issue fixed. Now that this is done we have more time to discuss a more global solution if you want.

Quoted 3 lines

As I mention in another message, I just don't think that the update speed of these drivers is worth any extra code... and I really do mean *any at all*.

As mentioned in a previous post, it's not necessarily a matter of speed, it's also a matter of i2c bus load.

Quoted 2 lines

What is the value of returning a result 1/2 second faster using "sensors" at the command line?

Not much, agreed.

Quoted 3 lines

More likely people are using something like gkrellm, or capturing the data in a script and plotting it, so they never experience that delay anyway.

I don't quite agree there. The fact that they don't experience the delay doesn't mean that the load isn't there. And now that you point this out, such uses of the drivers are quite different from runnig sensors manually in that the values are repeatedly read. I think that for example gkrellm users can configure it so that it updates the values every 2 seconds or so. And most people will do, since they want to see their CPU temperature changes in near real-time. Now you may be refreshing all the registers of your chip (probably near 40 for some of them) every 2 seconds. This is likely to have the chip stop quite often for quite a long time. And in the case of a noisy-poker BIOS, the likeliness of collisions becomes quite high. Again, my work on the w83l785ts influences me much in my views.

That said:
1* All this probably only applies to I2C (as opposed to ISA) chip drivers.
2* I am not going to convert all drivers to Philip Pokorny's method. It is more like I was asking if I would start a thermonuclear war by implementing the same in the w83l785ts driver.

Thanks.

Signature · 2 lines

Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/

Mark M. Hoffman

2005-05-19 06:24 UTC #5 lore raw

PATCH 1/1 w83781d fan_div code refactoring

In reply to Jean Delvare

(purposefully not trimming context this message is about a week late)

Quoted 35 lines · Jean Delvare

Quoting "Mark M. Hoffman" <[email protected]>:
Quoted 8 lines
Quoted 5 lines
While a corner case, we better fix it quickly since it is very likely to happen. We used to set fan divisors before fan mins in sensors.conf because setting fan divisors was breaking fan mins, and this will trigger the bug, since we invite users to run "sensors -s" prior to running "sensors".
But it's not real dangerous, since who inits fanX_div without also init'ing fanX_min immediately after?
You're correct, this is what will most likely happen in a majority of cases. But not necessarily all the cases. One can comment out the set fanXmin lines in sensors.conf. And anyway, it still temporarily writes a random value for fanN_min into the chip, which could trigger an alarm and possibly other nasty actions. Remember these reports of "as I load module X, my computer instantly goes off"...
Quoted 8 lines
Quoted 8 lines
I can think of three ways to solve the issue and would like to hear opinions about which we should use:
1* In the driver's init function, read fan_mins from the chip so they are initialized.
2* In the store_fan_div function, call update_device.
Quoted 8 lines
Quoted 3 lines
3* In the store_fan_div function, if data->valid = 0, read the fan_min from the chip.
(...)
I prefer #1: it fixes the whole *class* of bugs which can arise from interactions between set/show functions and uninit'ed <driver>_data structure elements. E.g. I can see this type of bug creeping in to smart PWM control where it could be much more dangerous.

Quoted 4 lines

I don't think that #1 as I described it fixes the whole class, since it only works for fan_mins. Instead, what you seem to suggest would be:
4* Call update_device upon driver init.

You're right, this is what I meant.

Quoted 25 lines

This would really slow the driver loading, for sure. But then we're safe, and we even could get rid of data->valid (providing we add a "force" parameter to update_device; not sure it's worth it).
I also can think of another possibility now:
5* Do not try to preserve fanN_min if data->valid = 0.
This one doesn't slow anything significantly. Ironically, this is the first thing I tried when finding the bug, and I forgot to mention it as a possible solution.
Of course, this one does only work for the store_fan_div case, other cases would have to handle the issue by themselves, in the same way or a different one.
Solution #4 can also be modified to something more subtle. We could divide internal register data in two sets. One set with registers we want to read at driver init, one set with the other registers. Each set would have its own "valid" and/or last_updated variables. This is somewhat similar to what Phil Pokorny does in the lm85 driver (one timestamp for limits, one for readings, each set has a different buffer validity limit) or me in the eeprom driver (each slice has its own validity bit and last_updated timestamp).

Quoted 8 lines

This requires more code, of course, but would slow down the init only as
much as stricly necessary. I'm not sure the benefit is worth the work however.
I think I still prefer solution #1. Looks like the best tradeoff. If other sysfs functions need other registers, we simply add initializing reads for them in the init function.

I disagree, but I see that I'm a little late on this one since patches have already been accepted. As I mention in another message, I just don't think that the update speed of these drivers is worth any extra code... and I really do mean *any at all*. What is the value of returning a result 1/2 second faster using "sensors" at the command line? More likely people are using something like gkrellm, or capturing the data in a script and plotting it, so they never experience that delay anyway.

Regards,

Signature · 2 lines

Mark M. Hoffman
[email protected]