hwmon: (pmbus) Fix the logic of checking if no id is matched
PATCH v2 1/1 hwmon: (pmbus) Fix the logic of checking if no id is matched
If no id is matched, the mid pointer is not NULL in current implementation.
---
v2:
It seems we don't need to check strlen(mid->name) here.
If there is a match, strlen(mid->name) is always not 0.
If ther is no match, comparing variable i with ARRAY_SIZE(ucd9000_id) or ARRAY_SIZE(ucd9200_id) is enough.
drivers/hwmon/pmbus/ucd9000.c | 3 +-- drivers/hwmon/pmbus/ucd9200.c | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-)
drivers/hwmon/pmbus/ucd9000.c +1 -2
@@ -141,13 +141,12 @@ static int ucd9000_probe(struct i2c_client *client, block_buffer[ret] ='\0' ;dev_info (&client->dev,"Device ID %s\n" , block_buffer); - mid =NULL ;for (i =0 ; i <ARRAY_SIZE (ucd9000_id); i++) { mid = &ucd9000_id[i];if (!strncasecmp (mid->name, block_buffer,strlen (mid->name)))break ; } -if (!mid || !strlen (mid->name)) { +if (i =ARRAY_SIZE (ucd9000_id)) {dev_err (&client->dev,"Unsupported device\n" );return -ENODEV ; }
drivers/hwmon/pmbus/ucd9200.c +1 -2
@@ -68,13 +68,12 @@ static int ucd9200_probe(struct i2c_client *client, block_buffer[ret] ='\0' ;dev_info (&client->dev,"Device ID %s\n" , block_buffer); - mid =NULL ;for (i =0 ; i <ARRAY_SIZE (ucd9200_id); i++) { mid = &ucd9200_id[i];if (!strncasecmp (mid->name, block_buffer,strlen (mid->name)))break ; } -if (!mid || !strlen (mid->name)) { +if (i =ARRAY_SIZE (ucd9200_id)) {dev_err (&client->dev,"Unsupported device\n" );return -ENODEV ; }
Signature · 9 lines
1.7.4.1 _______________________________________________ lm-sensors mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Hi Alex,
Quoted 1 line · Axel Lin
If no id is matched, the mid pointer is not NULL in current implementation.
The NULL check is presumably there to catch the (impossible) case ARRAY_SIZE(ucd9000_id) = 0 (array of ids is empty), not the case "no id is matched". The initialization of mid to NULL is for the same reason. Both should be unnecessary but may have been motivated by a compiler warning (although I would think gcc is smart enough to not emit these when it can check that the array isn't empty.) Guenter should be able to tell more.
The check for "no id is matched" is !strlen(mid->name), which works as intended as far as I can see. Did you actually hit a bug with the current code? I bet not.
Now I would agree that the current code is somewhat misleading because mixing null-terminated arrays with ARRAY_SIZE() is unusual (and inefficient - the last iteration always fails.) Also, strlen() is relatively slow and would rather be avoided when only testing if a string is empty or not: it's faster to test for mid->name[0].
So if anything I would propose the following changes (for performance and readability, NOT bug fix), untested:
drivers/hwmon/pmbus/ucd9000.c +2 -4
@@ -141,13 +141,11 @@ static int ucd9000_probe(struct i2c_clie block_buffer[ret] ='\0' ;dev_info (&client->dev,"Device ID %s\n" , block_buffer); - mid =NULL ; -for (i =0 ; i <ARRAY_SIZE (ucd9000_id); i++) { - mid = &ucd9000_id[i]; +for (mid = ucd9000_id; mid->name[0 ]; mid++) {if (!strncasecmp (mid->name, block_buffer,strlen (mid->name)))break ; } -if (!mid || !strlen (mid->name)) { +if (!mid->name[0 ]) {dev_err (&client->dev,"Unsupported device\n" );return -ENODEV ; }
drivers/hwmon/pmbus/ucd9200.c +2 -4
@@ -68,13 +68,11 @@ static int ucd9200_probe(struct i2c_clie block_buffer[ret] ='\0' ;dev_info (&client->dev,"Device ID %s\n" , block_buffer); - mid =NULL ; -for (i =0 ; i <ARRAY_SIZE (ucd9200_id); i++) { - mid = &ucd9200_id[i]; +for (mid = ucd9200_id; mid->name[0 ]; mid++) {if (!strncasecmp (mid->name, block_buffer,strlen (mid->name)))break ; } -if (!mid || !strlen (mid->name)) { +if (!mid->name[0 ]) {dev_err (&client->dev,"Unsupported device\n" );return -ENODEV ; }
Quoted 51 lines
Signed-off-by Axel Lin <[email protected]>---
v2:
It seems we don't need to checkstrlen(mid->name)here.
If there is a match,strlen(mid->name)is always not 0.
If ther is no match, comparing variable i withARRAY_SIZE(ucd9000_id)orARRAY_SIZE(ucd9200_id)is enough.drivers/hwmon/pmbus/ucd9000.c | 3 +-- drivers/hwmon/pmbus/ucd9200.c | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-)drivers/hwmon/pmbus/ucd9000.c +1
-2@@ -141,13 +141,12 @@ static int ucd9000_probe(struct i2c_client *client, block_buffer[ret] ='\0' ;dev_info (&client->dev,"Device ID %s\n" , block_buffer); - mid =NULL ;for (i =0 ; i <ARRAY_SIZE (ucd9000_id); i++) { mid = &ucd9000_id[i];if (!strncasecmp (mid->name, block_buffer,strlen (mid->name)))break ; } -if (!mid || !strlen (mid->name)) { +if (i =ARRAY_SIZE (ucd9000_id)) {dev_err (&client->dev,"Unsupported device\n" );return -ENODEV ; }drivers/hwmon/pmbus/ucd9200.c +1
-2@@ -68,13 +68,12 @@ static int ucd9200_probe(struct i2c_client *client, block_buffer[ret] ='\0' ;dev_info (&client->dev,"Device ID %s\n" , block_buffer); - mid =NULL ;for (i =0 ; i <ARRAY_SIZE (ucd9200_id); i++) { mid = &ucd9200_id[i];if (!strncasecmp (mid->name, block_buffer,strlen (mid->name)))break ; } -if (!mid || !strlen (mid->name)) { +if (i =ARRAY_SIZE (ucd9200_id)) {dev_err (&client->dev,"Unsupported device\n" );return -ENODEV ; }
Signature · 6 lines
Jean Delvare _______________________________________________ lm-sensors mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
2011/8/31 Jean Delvare <[email protected]>:
Quoted 1 line
Hi Alex,
It's Axel.
Quoted 12 lines · Axel Lin
Quoted 1 line
If no id is matched, the mid pointer is not NULL in current implementation.
The NULL check is presumably there to catch the (impossible) case
ARRAY_SIZE(ucd9000_id)= 0 (array of ids is empty), not the case "no id is matched". The initialization of mid to NULL is for the same reason. Both should be unnecessary but may have been motivated by a compiler warning (although I would think gcc is smart enough to not emit these when it can check that the array isn't empty.) Guenter should be able to tell more.
Quoted 3 lines
The check for "no id is matched" is !strlen(mid->name), which works as intended as far as I can see. Did you actually hit a bug with the current code? I bet not.
No, I didn't hit the bug. Just reading the code.
Quoted 9 lines
Now I would agree that the current code is somewhat misleading because mixing null-terminated arrays with
ARRAY_SIZE()is unusual (and inefficient - the last iteration always fails.) Also,strlen()is relatively slow and would rather be avoided when only testing if a string is empty or not: it's faster to test for mid->name[0].So if anything I would propose the following changes (for performance and readability, NOT bug fix), untested:
Your fix looks good to me. ( Although I don't have the h/w for testing ).
Quoted 79 lines
drivers/hwmon/pmbus/ucd9000.c 2011-08-31 11:53:28.000000000 +0200 renamed from drivers/hwmon/pmbus/ucd9000.c 2011-08-30 13:41:32.000000000 +0200 +2
-4@@ -141,13 +141,11 @@ static int ucd9000_probe(struct i2c_clie block_buffer[ret] = '\0'; dev_info(&client->dev, "Device ID %s\n", block_buffer); - mid = NULL; - for (i = 0; i < ARRAY_SIZE(ucd9000_id); i++) { - mid = &ucd9000_id[i]; + for (mid = ucd9000_id; mid->name[0]; mid++) { if (!strncasecmp(mid->name, block_buffer, strlen(mid->name))) break; } - if (!mid || !strlen(mid->name)) { + if (!mid->name[0]) { dev_err(&client->dev, "Unsupported device\n"); return -ENODEV; }drivers/hwmon/pmbus/ucd9200.c 2011-08-31 11:53:20.000000000 +0200 renamed from drivers/hwmon/pmbus/ucd9200.c 2011-08-30 13:41:32.000000000 +0200 +2
-4@@ -68,13 +68,11 @@ static int ucd9200_probe(struct i2c_clie block_buffer[ret] = '\0'; dev_info(&client->dev, "Device ID %s\n", block_buffer); - mid = NULL; - for (i = 0; i < ARRAY_SIZE(ucd9200_id); i++) { - mid = &ucd9200_id[i]; + for (mid = ucd9200_id; mid->name[0]; mid++) { if (!strncasecmp(mid->name, block_buffer, strlen(mid->name))) break; } - if (!mid || !strlen(mid->name)) { + if (!mid->name[0]) { dev_err(&client->dev, "Unsupported device\n"); return -ENODEV; }Quoted 42 lines
Signed-off-by Axel Lin <[email protected]>---
v2:
It seems we don't need to checkstrlen(mid->name)here.
If there is a match,strlen(mid->name)is always not 0.
If ther is no match, comparing variable i withARRAY_SIZE(ucd9000_id)orARRAY_SIZE(ucd9200_id)is enough.drivers/hwmon/pmbus/ucd9000.c | 3 +--
drivers/hwmon/pmbus/ucd9200.c | 3 +--2 files changed, 2 insertions(+), 4 deletions(-)drivers/hwmon/pmbus/ucd9000.c +1
-2@@ -141,13 +141,12 @@ static int ucd9000_probe(struct i2c_client *client, block_buffer[ret] = '\0'; dev_info(&client->dev, "Device ID %s\n", block_buffer); - mid = NULL; for (i = 0; i < ARRAY_SIZE(ucd9000_id); i++) { mid = &ucd9000_id[i]; if (!strncasecmp(mid->name, block_buffer, strlen(mid->name))) break; } - if (!mid || !strlen(mid->name)) { + if (i = ARRAY_SIZE(ucd9000_id)) { dev_err(&client->dev, "Unsupported device\n"); return -ENODEV; }drivers/hwmon/pmbus/ucd9200.c +0
-0@@ -68,13 +68,12 @@ static int ucd9200_probe(struct i2c_client *client, block_buffer[ret] = '\0'; dev_info(&client->dev, "Device ID %s\n", block_buffer);
Quoted 16 lines
drivers/hwmon/pmbus/ucd9200.c static int ucd9200_probe(struct i2c_client *client, · line 79
Quoted 11 lines
drivers/hwmon/pmbus/ucd9200.c +1
-2- mid = NULL; for (i = 0; i < ARRAY_SIZE(ucd9200_id); i++) { mid = &ucd9200_id[i]; if (!strncasecmp(mid->name, block_buffer, strlen(mid->name))) break; } - if (!mid || !strlen(mid->name)) { + if (i = ARRAY_SIZE(ucd9200_id)) { dev_err(&client->dev, "Unsupported device\n"); return -ENODEV; }--
Jean Delvare
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 16 lines · Axel Lin
2011/8/31 Jean Delvare <[email protected]>:
Quoted 1 line
Hi Alex,
It's Axel.
Quoted 12 lines · Axel Lin
Quoted 1 line
If no id is matched, the mid pointer is not NULL in current implementation.
The NULL check is presumably there to catch the (impossible) case
ARRAY_SIZE(ucd9000_id)= 0 (array of ids is empty), not the case "no id is matched". The initialization of mid to NULL is for the same reason. Both should be unnecessary but may have been motivated by a compiler warning (although I would think gcc is smart enough to not emit these when it can check that the array isn't empty.) Guenter should be able to tell more.
Yes, that was the idea, and as far as I recall I did get a compiler warning at the time. The loop aborts at the last entry due to the strncasecmp() match on the zero length string. Axel's patch won't work, since i = ARRAY_SIZE(ucd9000_id) is never true for the same reason.
Quoted 5 lines
Quoted 3 lines
The check for "no id is matched" is !strlen(mid->name), which works as intended as far as I can see. Did you actually hit a bug with the current code? I bet not.
No, I didn't hit the bug. Just reading the code.
Finally someone else looking into that code ... thanks, I really appreciate that.
Quoted 11 lines
Quoted 9 lines
Now I would agree that the current code is somewhat misleading because mixing null-terminated arrays with
ARRAY_SIZE()is unusual (and inefficient - the last iteration always fails.) Also,strlen()is relatively slow and would rather be avoided when only testing if a string is empty or not: it's faster to test for mid->name[0].So if anything I would propose the following changes (for performance and readability, NOT bug fix), untested:
Your fix looks good to me. ( Although I don't have the h/w for testing ).
I like it too. I'll dig out my test boards and test it. Jean, care to submit complete patches ? Otherwise I'll create a set myself and send it out for review once I tested it.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors