Handle read errors in w83l785ts

Hi Greg,

Here is a patch that adds proper read error handling in the w83l785ts driver. This is needed for this driver because on many Asus boards the BIOS or something accesses the chip in our back and causes collisions or something similar that causes many read errors. For now, these errors make the driver return temperature values of -1 from times to times.

I have been working with James Bolt on this. See the thread on the lm_sensors mailing list for details. The patch is fairly well tested. It is against 2.6.2-rc3 + your current stack of patches as I know it.

The "retry until it works" method is the best I could think of, since we have no information from Asus and I don't expect to get any. If it still doesn't work after an arbitrary number (5) of tries it returns the previously known value and generates an error in the logs. James' tests showed that it will probably never happen though (highest retry count was 3 and happened once out of 3000 reads) unless we lower the arbitrary number (but we don't want to, do we?)

I inserted incremental delays as reads fail, I felt like it should help avoid collisions with whatever-is-bugging-us. Seems to work OK, but it's not perfect (since we sometimes need 3 retries) and I didn't test with a different policy (no delay, constant delay or different increment).

I plan to backport the changes to our CVS (2.4) driver someday as time permits.

Please comment and apply if/when you feel like it, Thanks.

@@ -38,6 +38,9 @@
 #include <linux/i2c.h>
 #include <linux/i2c-sensor.h>
 
+/* How many retries on register read error */
+#define MAX_RETRIES	5
+
 /*
  * Address to scan
  * Address is fully defined internally and cannot be changed.

@@ -82,6 +85,7 @@
 static int w83l785ts_detect(struct i2c_adapter *adapter, int address,
 	int kind);
 static int w83l785ts_detach_client(struct i2c_client *client);
+static u8 w83l785ts_read_value(struct i2c_client *client, u8 reg, u8 defval);
 static void w83l785ts_update_client(struct i2c_client *client);
 
 /*

@@ -196,10 +200,10 @@
 	 * are skipped.
 	 */
 	if (kind < 0) { /* detection */
-		if (((i2c_smbus_read_byte_data(new_client,
-		      W83L785TS_REG_CONFIG) & 0x80) != 0x00)
-		 || ((i2c_smbus_read_byte_data(new_client,
-		      W83L785TS_REG_TYPE) & 0xFC) != 0x00)) {
+		if (((w83l785ts_read_value(new_client,
+		      W83L785TS_REG_CONFIG, 0) & 0x80) != 0x00)
+		 || ((w83l785ts_read_value(new_client,
+		      W83L785TS_REG_TYPE, 0) & 0xFC) != 0x00)) {
 			dev_dbg(&adapter->dev,
 				"W83L785TS-S detection failed at 0x%02x.\n",
 				address);

@@ -211,12 +215,12 @@
 		u16 man_id;
 		u8 chip_id;
 
-		man_id = (i2c_smbus_read_byte_data(new_client,
-			 W83L785TS_REG_MAN_ID1) << 8) +
-			 i2c_smbus_read_byte_data(new_client,
-			 W83L785TS_REG_MAN_ID2);
-		chip_id = i2c_smbus_read_byte_data(new_client,
-			  W83L785TS_REG_CHIP_ID);
+		man_id = (w83l785ts_read_value(new_client,
+			 W83L785TS_REG_MAN_ID1, 0) << 8) +
+			 w83l785ts_read_value(new_client,
+			 W83L785TS_REG_MAN_ID2, 0);
+		chip_id = w83l785ts_read_value(new_client,
+			  W83L785TS_REG_CHIP_ID, 0);
 
 		if (man_id = 0x5CA3) { /* Winbond */
 			if (chip_id = 0x70) { /* W83L785TS-S */

@@ -239,6 +243,9 @@
 	data->valid = 0;
 	init_MUTEX(&data->update_lock);
 
+	/* Default values in case the first read fails (unlikely). */
+	data->temp_over = data->temp = 0;
+
 	/* Tell the I2C layer a new client has arrived. */
 	if ((err = i2c_attach_client(new_client))) 
 		goto exit_free;

@@ -274,6 +281,27 @@
 	return 0;
 }
 
+static u8 w83l785ts_read_value(struct i2c_client *client, u8 reg, u8 defval)
+{
+	int value, i;
+
+	/* Frequent read errors have been reported on Asus boards, so we
+	 * retry on read errors. If it still fails (unlikely), return the
+	 * default value requested by the caller. */
+	for (i = 1; i <= MAX_RETRIES; i++)
+	{
+		value = i2c_smbus_read_byte_data(client, reg);
+		if (value >= 0)
+			return value;
+		dev_dbg(&client->dev, "Read failed, will retry in %d.\n", i);
+		i2c_delay(i);
+	}
+
+	dev_err(&client->dev, "Couldn't read value from register. "
+		"Please report.\n");
+	return defval;
+}
+
 static void w83l785ts_update_client(struct i2c_client *client)
 {
 	struct w83l785ts_data *data = i2c_get_clientdata(client);

@@ -284,10 +312,10 @@
 	 || (jiffies - data->last_updated > HZ * 2)
 	 || (jiffies < data->last_updated)) {
 		dev_dbg(&client->dev, "Updating w83l785ts data.\n");
-		data->temp = i2c_smbus_read_byte_data(client,
-			     W83L785TS_REG_TEMP);
-		data->temp_over = i2c_smbus_read_byte_data(client,
-				  W83L785TS_REG_TEMP_OVER);
+		data->temp = w83l785ts_read_value(client,
+			     W83L785TS_REG_TEMP, data->temp);
+		data->temp_over = w83l785ts_read_value(client,
+				  W83L785TS_REG_TEMP_OVER, data->temp_over);
 
 		data->last_updated = jiffies;
 		data->valid = 1;

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

Correct, sorry for that. I work with two or three different coding styles every day, so it is no surprise that I sometimes miss a thing. Will try to be more careful next time.

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

Hi James,

Your great perl script gave me an idea. Maybe it would be interesting to alter the delays between reads when errors occur. For now I used a short incremental delay (i, where i is the retry count). Other choices are possible though:
* no delay at all (remove i2c_delay())
* incremental, bigger delay (say i2c_delay(4*i))
* constant delay (say i2c_delay(5))

If you have some spare time and can do some additional experiments with some of these settings, it would be great. My goals are to lower the probablity of 2 or more retries needed, and, if possible, to also lower the probability of a first retry being needed.

My only fear is that the randomness could be too high for us to conclude.

Anyway, my request is mostly the expression of the scientific mind sleeping in me. The driver seems to work pretty well the way it is now, so you are invited to decline unless you are looking for new ways to waste your time or are yourself interested by the experiment.

Thanks.

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

Looks good, I've applied this (with one fix for your misplacement of the '{' character in the if() statement you added...)

thanks,

greg k-h

Hi,

I've uploaded some execution logs to:

http://james.evilpenguin.com/sensors/i2cdelay_test/

I've started testing using sleep values of 3 & 5 seconds

There seems to be a noticable decrease in the number of errors by eliminating the read delay (45 compared to 191 with i2c_delay(3*i)) using a 3 second sleep value. However, as you said, this could be completely random. It might be advisable to run some more tests with the same parameters to confirm a decrease in the number of retry errors. Note that those figures also don't include multiple errors that occur in succession.

Filenames and directory structure of i2cdelay_test should again be pretty self explanatory.

It's not really much of burden, it doesn't require many system resources and most of it can be automated and run in the background.

Well it doesn't really waste much of my time, so why not? :P No harm in seeing if it can't be a little more efficient.

Will upload the rest of the logs as it churns them out.

JB

-----BEGIN PGP PUBLIC KEY BLOCK-----
Version: GnuPG v1.2.4 (GNU/Linux)

mQGiBD7s3PIRBAC2KWjpIG0YfRLgIHUxBrFQmTiIq0yQlaBa7vgjSkKroEtvSjBS
sk5N9NE0y92rGa7C2pfG0w1/rmEghyTLRRfUKyXAnLr/jE6QEnT4yIN+4mRodwZg
bDlSML0AQ09JUv+QBidGtd1kH01lvRdnbCvsMyMNmsp6ryS6pneRKJQ7AwCgyR5I
0rkR4s+sGE3OIfgkw2z5PIUEAKST5r8Lg07T4FSWdOE/EtwX3Xe4vWGBayZKHh0t
A2U8IbaMMLhVHk9g7c3jZ6ptIJqY3yWpp26A+kZNTk53wohnmawtLFEs1HLgFeRc
ppJMNB7TgpyVWUTTYLtZ5GCeA4A67Iq8+CaqX4Tf7Dq1X9dgyUR+b64ajJFd/igt
F/TQA/9zemvSCw1iGgjK+X3WCiAmeYpI9CT6r+XcNkjNI07EpcJyb6upsClLPgqr
Gpc5O6fv+1zmD409xwTi/1QykogkyeYoNbMVnG35ZQ2K5702HA9ZF6lWXLNU3Rho
/q8nRnqgpj8re6koAiQZfNatEAD5x0zktEfWC4N6DQDUCM9RdrQiSmFtZXMgQm9s
dCA8amFtZXNAZXZpbHBlbmd1aW4uY29tPohXBBMRAgAXBQI+7NzyBQsHCgMEAxUD
AgMWAgECF4AACgkQzRkMO5+4Zu3ljQCfVCXWrTxkP1UdtYe+Kfa/4w4h0SAAoI0o
QYXKsXvXHDDw1GgxAD4DvH+1uQENBD7s3PIQBADQkIB2Ml9tAPNOk1BhbdpSF1oM
8XYkHpzA8IviczaDqr/ArSPfbkoxyDr165T+hhqGx+zhQSpJ2re7L4tsLpZvojUW
LEH65rUZ5OJNKUt4ayAM3MFMZFLfZoUSQoqjkZOvJSZQJGCA6lKTtYnLglJdS7vv
DrqZbHjZIqDlCuHkFwADBQQAiuP4Wfmg950F/UJ4zbup5jn/ABnMasUbd3dmnK5F
nNNiWhE7l4/oEh2zgF6g+3qT9s/7m77nn/ccheuA7TTeO82uLiqWdmde8+l+RMpp
rCa4Gr4bAN5OLzr85dcd31mepYw7CmQG3iiKUCHgSxuBvlR8MXWaWnei3VQPCHeo
veqIRgQYEQIABgUCPuzc8gAKCRDNGQw7n7hm7ZQjAJ0faoGh6Eq4WSxYk1xMf4rC
6/IehwCdH/Tq0Yjis8PDCKZiIKJlqHkJfiE=r11q
-----END PGP PUBLIC KEY BLOCK-----

I downloaded them and tried to make some stats. I could hardly come to a conclusion so far. See below.

I think you mean i2c_delay(i). No test was run with 3*i AFAIK.

The no_delay version may have less errors, but the retries breakdown doesn't please me much:
1 retry: 46

2 retries:  8
3 retries:  5
4 retries:  3
5 retries:  2

No other test needed more than 3 retries. If you count the total additional delay, the no_delay version has slightly more (128 for 1000 runs, 124 for i2c_delay(i)). Not significant.

I saw you did it. Looks like the results differ much between two series with the same settings. This means that our tests aren't realy reliable.

Correct. Not only that, but I have noticed another strange thing. Maybe you'll be able to explain it to me. When no message is logged, you seem to assume that it means that syslogd is not repeating the previous message. I would then expect a "Repeated N times" message with N being equal to the number of messageless runs + 1. This isn't always the case. Also, I'd expect at least "Updating w83l785ts data." each time. Why is it that when errors occur, this message doesn't show?

Isn't it possible to temporary configure syslogd so that it logs all messages without trying to limit the amount of messages written?

BTW, why do you check the log after each command, instead of parsing it after all the runs?

One strange thing is that the number of times where one retry is needed varies so much. I'd understand that our delay policy changes how many retries are needed. I can't get how it would change the number of "first" errors.

To me, this means that these stats don't help much. There are too many things we don't control or don't understand, generating too much noise, so we can't conclude :( For one thing, we don't know how frequently the BIOS (if that is it) accesses the chip, nor if this frequency depends on something (the temperature, for example).

So I think I'll leave things the way they are now, since my original policy doesn't seem to be any worse than the others.

If you want to play more with it, do as you wish, but frankly I don't think it's worth it.

Thanks a lot for the tests anyway. I realized that I forgot to mention your name in the driver. Sorry for that, I'll send a patch to Greg KH in a while to fix that.

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

Hi,

Sorry for the delay - have been preparing for holiday and sorting out university places.

Anyway...

Indeed I did, probably got confused w/ the sleep time.

It is, yes. I really should have thought about reconfiguring it. At the time I guess I thought it wasn't necessary because I was just looking for errors (restricting its output shouldn't make an impact on errors recorded, should it?) and not for success.

That might have been more sensible, especially if I'd filtered the log to eliminate all messages bar those related to sensors. Or perhaps I could have just generated a large array of execution timestamps to retrieve afterwards... Regardless, it was considerably easier to automate it this way at the time, and I couldn't see any major disadvantages provided I accomodated for read/write delays properly - even if it was a bit of a dirty hack.

Well I think I'll have to agree with you, although to be honest I'd have a hard job disagreeing really. I think the benefits of trying to tweak the driver are nullified by Asus' unwillingness to tell us anything about it.

That said, maybe I will do some extra tweaking in a few weeks just to get some more refined data.

Although, I'd be lying if I said it wasn't a little monotonous...

No problem, I see the patch has made it into 2.6.3-rc2 :)

It was nice to have made some kind of contribution,

JB

-----BEGIN PGP PUBLIC KEY BLOCK-----
Version: GnuPG v1.2.4 (GNU/Linux)

mQGiBD7s3PIRBAC2KWjpIG0YfRLgIHUxBrFQmTiIq0yQlaBa7vgjSkKroEtvSjBS
sk5N9NE0y92rGa7C2pfG0w1/rmEghyTLRRfUKyXAnLr/jE6QEnT4yIN+4mRodwZg
bDlSML0AQ09JUv+QBidGtd1kH01lvRdnbCvsMyMNmsp6ryS6pneRKJQ7AwCgyR5I
0rkR4s+sGE3OIfgkw2z5PIUEAKST5r8Lg07T4FSWdOE/EtwX3Xe4vWGBayZKHh0t
A2U8IbaMMLhVHk9g7c3jZ6ptIJqY3yWpp26A+kZNTk53wohnmawtLFEs1HLgFeRc
ppJMNB7TgpyVWUTTYLtZ5GCeA4A67Iq8+CaqX4Tf7Dq1X9dgyUR+b64ajJFd/igt
F/TQA/9zemvSCw1iGgjK+X3WCiAmeYpI9CT6r+XcNkjNI07EpcJyb6upsClLPgqr
Gpc5O6fv+1zmD409xwTi/1QykogkyeYoNbMVnG35ZQ2K5702HA9ZF6lWXLNU3Rho
/q8nRnqgpj8re6koAiQZfNatEAD5x0zktEfWC4N6DQDUCM9RdrQiSmFtZXMgQm9s
dCA8amFtZXNAZXZpbHBlbmd1aW4uY29tPohXBBMRAgAXBQI+7NzyBQsHCgMEAxUD
AgMWAgECF4AACgkQzRkMO5+4Zu3ljQCfVCXWrTxkP1UdtYe+Kfa/4w4h0SAAoI0o
QYXKsXvXHDDw1GgxAD4DvH+1uQENBD7s3PIQBADQkIB2Ml9tAPNOk1BhbdpSF1oM
8XYkHpzA8IviczaDqr/ArSPfbkoxyDr165T+hhqGx+zhQSpJ2re7L4tsLpZvojUW
LEH65rUZ5OJNKUt4ayAM3MFMZFLfZoUSQoqjkZOvJSZQJGCA6lKTtYnLglJdS7vv
DrqZbHjZIqDlCuHkFwADBQQAiuP4Wfmg950F/UJ4zbup5jn/ABnMasUbd3dmnK5F
nNNiWhE7l4/oEh2zgF6g+3qT9s/7m77nn/ccheuA7TTeO82uLiqWdmde8+l+RMpp
rCa4Gr4bAN5OLzr85dcd31mepYw7CmQG3iiKUCHgSxuBvlR8MXWaWnei3VQPCHeo
veqIRgQYEQIABgUCPuzc8gAKCRDNGQw7n7hm7ZQjAJ0faoGh6Eq4WSxYk1xMf4rC
6/IehwCdH/Tq0Yjis8PDCKZiIKJlqHkJfiE=r11q
-----END PGP PUBLIC KEY BLOCK-----