Skip to contentSkip to conversation

hwmon: (atxp1) Fix device detection logic

PATCH In review Jean Delvare 2 participants 4 messages Jun 29, 2008 – Aug 19, 2008 Hardware monitoring

lore
Share & tools
lore

Jean Delvare

author
#0 lore raw

PATCH 1/1 hwmon: (atxp1) Fix device detection logic

The atxp1 device detection code has a major logic flaw, fix it. Not sure how we managed to miss this when the driver was merged...

Signed-off-by Jean Delvare <[email protected]>
Cc Sebastian Witt <[email protected]>

---
Sebastian, can you please review and test this patch? I've tested with i2c-stub but an additional test with a real device can't hurt.

 drivers/hwmon/atxp1.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/hwmon/atxp1.c +3 -2
@@ -31,7 +31,7 @@
 
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("System voltages control via Attansic ATXP1");
-MODULE_VERSION("0.6.2");
+MODULE_VERSION("0.6.3");
 MODULE_AUTHOR("Sebastian Witt <[email protected]>");
 
 #define ATXP1_VID	0x00

@@ -298,7 +298,8 @@ static int atxp1_detect(struct i2c_adapt
 	     (i2c_smbus_read_byte_data(new_client, 0x3f) = 0) &&
 	     (i2c_smbus_read_byte_data(new_client, 0xfe) = 0) &&
 	     (i2c_smbus_read_byte_data(new_client, 0xff) = 0) )) {
-
+		goto exit_free;
+	} else {
 		/* No vendor ID, now checking if registers 0x10,0x11 (non-existent)
 		 * showing the same as register 0x00 */
 		temp = i2c_smbus_read_byte_data(new_client, 0x00);
Signature · 6 lines
Jean Delvare

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
author
#1 lore raw

PATCH 1/1 hwmon: (atxp1) Fix device detection logic

Subject: hwmon: (atxp1) Fix device detection logic

The atxp1 device detection code has a major logic flaw, fix it. Not sure how we managed to miss this when the driver was merged...

Signed-off-by Jean Delvare <[email protected]>
Cc Sebastian Witt <[email protected]>

---
Patch refreshed to apply on top of 2.6.27-rc3.

I didn't hear of Sebastian in 6 weeks. Can anyone please review this?

 drivers/hwmon/atxp1.c |   20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)
drivers/hwmon/atxp1.c +10 -10
@@ -31,7 +31,7 @@
 
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("System voltages control via Attansic ATXP1");
-MODULE_VERSION("0.6.2");
+MODULE_VERSION("0.6.3");
 MODULE_AUTHOR("Sebastian Witt <[email protected]>");
 
 #define ATXP1_VID	0x00

@@ -289,16 +289,16 @@ static int atxp1_detect(struct i2c_clien
 	if (!((i2c_smbus_read_byte_data(new_client, 0x3e) = 0) &&
 	     (i2c_smbus_read_byte_data(new_client, 0x3f) = 0) &&
 	     (i2c_smbus_read_byte_data(new_client, 0xfe) = 0) &&
-	     (i2c_smbus_read_byte_data(new_client, 0xff) = 0) )) {
+	     (i2c_smbus_read_byte_data(new_client, 0xff) = 0)))
+		return -ENODEV;
 
-		/* No vendor ID, now checking if registers 0x10,0x11 (non-existent)
-		 * showing the same as register 0x00 */
-		temp = i2c_smbus_read_byte_data(new_client, 0x00);
-
-		if (!((i2c_smbus_read_byte_data(new_client, 0x10) = temp) &&
-			 (i2c_smbus_read_byte_data(new_client, 0x11) = temp) ))
-			return -ENODEV;
-	}
+	/* No vendor ID, now checking if registers 0x10,0x11 (non-existent)
+	 * showing the same as register 0x00 */
+	temp = i2c_smbus_read_byte_data(new_client, 0x00);
+
+	if (!((i2c_smbus_read_byte_data(new_client, 0x10) = temp) &&
+	      (i2c_smbus_read_byte_data(new_client, 0x11) = temp)))
+		return -ENODEV;
 
 	/* Get VRM */
 	temp = vid_which_vrm();
Signature · 6 lines
Jean Delvare

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
author
#2 lore raw
In reply to Jean Delvare

Hi Sebastian,

Please keep the lm-sensors list Cc'd.

Quoted 9 lines · Sebastian Witt

Hello Jean,

sorry, missed your mail. The test sequence was:

1. If vendor ID registers are all zero, assume ATXP1.
2. If not, do second test. If it succeeds it is a ATXP1.

I'm not sure anymore why it was this way, maybe I thought there would be a ATXP1 with vendor ID so it works for both...

That's indeed what the current code does, but that's not correct. We want to check that _both_ conditions are met. Even with that, the detection is rather weak... Without that it's very, very weak.

Quoted 2 lines

However aborting when the first test fails and doing always the second test should be fine, but I can't test it because I gave away the board some time ago.

If you look at the comments in the original code, that's what they say:
we have "No vendor ID" inside the block which is executed when there _are_ IDs. So it's clear to me that the code doesn't do what we intended to do in the first place, and was "working" only by accident.

Even if you can't test my patch, can you please review it and add your Acked-by, so that I can push it upstream? Thanks.

Signature · 6 lines
Jean Delvare

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Acked-by on v1 1/1
#3 lore raw
In reply to Jean Delvare
Quoted 6 lines · Jean Delvare

Subject: hwmon: (atxp1) Fix device detection logic

The atxp1 device detection code has a major logic flaw, fix it. Not sure how we managed to miss this when the driver was merged...

Signed-off-by Jean Delvare <[email protected]>
Acked-by Sebastian Witt <[email protected]>
Quoted 38 lines
Cc Sebastian Witt <[email protected]>

---
Patch refreshed to apply on top of 2.6.27-rc3.

I didn't hear of Sebastian in 6 weeks. Can anyone please review this?

 drivers/hwmon/atxp1.c |   20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)
drivers/hwmon/atxp1.c +3 -10
@@ -31,7 +31,7 @@
 
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("System voltages control via Attansic ATXP1");
-MODULE_VERSION("0.6.2");
+MODULE_VERSION("0.6.3");
 MODULE_AUTHOR("Sebastian Witt <[email protected]>");
 
 #define ATXP1_VID	0x00

@@ -289,16 +289,16 @@ static int atxp1_detect(struct i2c_clien
 	if (!((i2c_smbus_read_byte_data(new_client, 0x3e) = 0) &&
 	     (i2c_smbus_read_byte_data(new_client, 0x3f) = 0) &&
 	     (i2c_smbus_read_byte_data(new_client, 0xfe) = 0) &&
-	     (i2c_smbus_read_byte_data(new_client, 0xff) = 0) )) {
+	     (i2c_smbus_read_byte_data(new_client, 0xff) = 0)))
+		return -ENODEV;
 
-		/* No vendor ID, now checking if registers 0x10,0x11 (non-existent)
-		 * showing the same as register 0x00 */
-		temp = i2c_smbus_read_byte_data(new_client, 0x00);
-
-		if (!((i2c_smbus_read_byte_data(new_client, 0x10) = temp) &&
-			 (i2c_smbus_read_byte_data(new_client, 0x11) = temp) ))
-			return -ENODEV;
-	}

drivers/hwmon/atxp1.c static int atxp1_detect(struct i2c_clien · line 304

Quoted 10 lines
drivers/hwmon/atxp1.c +7 -0
+	/* No vendor ID, now checking if registers 0x10,0x11 (non-existent)
+	 * showing the same as register 0x00 */
+	temp = i2c_smbus_read_byte_data(new_client, 0x00);
+
+	if (!((i2c_smbus_read_byte_data(new_client, 0x10) = temp) &&
+	      (i2c_smbus_read_byte_data(new_client, 0x11) = temp)))
+		return -ENODEV;
 
 	/* Get VRM */
 	temp = vid_which_vrm();

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors