* [lm-sensors] [PATCH v2] hwmon: (pmbus) Fix the logic of checking if @ 2011-08-31 4:58 ` Axel Lin 0 siblings, 0 replies; 8+ messages in thread From: Axel Lin @ 2011-08-31 4:58 UTC (permalink / raw) To: linux-kernel; +Cc: Guenter Roeck, Jean Delvare, lm-sensors If no id is matched, the mid pointer is not NULL in current implementation. Signed-off-by: Axel Lin <axel.lin@gmail.com> --- 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(-) diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c index 285bb15..a2d4a72 100644 --- a/drivers/hwmon/pmbus/ucd9000.c +++ b/drivers/hwmon/pmbus/ucd9000.c @@ -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; } diff --git a/drivers/hwmon/pmbus/ucd9200.c b/drivers/hwmon/pmbus/ucd9200.c index 786f6cd..a72e55e 100644 --- a/drivers/hwmon/pmbus/ucd9200.c +++ b/drivers/hwmon/pmbus/ucd9200.c @@ -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; } -- 1.7.4.1 _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2] hwmon: (pmbus) Fix the logic of checking if no id is matched @ 2011-08-31 4:58 ` Axel Lin 0 siblings, 0 replies; 8+ messages in thread From: Axel Lin @ 2011-08-31 4:58 UTC (permalink / raw) To: linux-kernel; +Cc: Guenter Roeck, Jean Delvare, lm-sensors If no id is matched, the mid pointer is not NULL in current implementation. Signed-off-by: Axel Lin <axel.lin@gmail.com> --- 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(-) diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c index 285bb15..a2d4a72 100644 --- a/drivers/hwmon/pmbus/ucd9000.c +++ b/drivers/hwmon/pmbus/ucd9000.c @@ -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; } diff --git a/drivers/hwmon/pmbus/ucd9200.c b/drivers/hwmon/pmbus/ucd9200.c index 786f6cd..a72e55e 100644 --- a/drivers/hwmon/pmbus/ucd9200.c +++ b/drivers/hwmon/pmbus/ucd9200.c @@ -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; } -- 1.7.4.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [lm-sensors] [PATCH v2] hwmon: (pmbus) Fix the logic of 2011-08-31 4:58 ` [PATCH v2] hwmon: (pmbus) Fix the logic of checking if no id is matched Axel Lin @ 2011-08-31 10:05 ` Jean Delvare -1 siblings, 0 replies; 8+ messages in thread From: Jean Delvare @ 2011-08-31 10:05 UTC (permalink / raw) To: Axel Lin; +Cc: linux-kernel, Guenter Roeck, lm-sensors Hi Alex, On Wed, 31 Aug 2011 12:58:19 +0800, Axel Lin wrote: > 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: --- linux-3.1-rc4.orig/drivers/hwmon/pmbus/ucd9000.c 2011-08-30 13:41:32.000000000 +0200 +++ linux-3.1-rc4/drivers/hwmon/pmbus/ucd9000.c 2011-08-31 11:53:28.000000000 +0200 @@ -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; } --- linux-3.1-rc4.orig/drivers/hwmon/pmbus/ucd9200.c 2011-08-30 13:41:32.000000000 +0200 +++ linux-3.1-rc4/drivers/hwmon/pmbus/ucd9200.c 2011-08-31 11:53:20.000000000 +0200 @@ -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; } > > Signed-off-by: Axel Lin <axel.lin@gmail.com> > --- > 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(-) > > diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c > index 285bb15..a2d4a72 100644 > --- a/drivers/hwmon/pmbus/ucd9000.c > +++ b/drivers/hwmon/pmbus/ucd9000.c > @@ -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; > } > diff --git a/drivers/hwmon/pmbus/ucd9200.c b/drivers/hwmon/pmbus/ucd9200.c > index 786f6cd..a72e55e 100644 > --- a/drivers/hwmon/pmbus/ucd9200.c > +++ b/drivers/hwmon/pmbus/ucd9200.c > @@ -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; > } -- Jean Delvare _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] hwmon: (pmbus) Fix the logic of checking if no id is matched @ 2011-08-31 10:05 ` Jean Delvare 0 siblings, 0 replies; 8+ messages in thread From: Jean Delvare @ 2011-08-31 10:05 UTC (permalink / raw) To: Axel Lin; +Cc: linux-kernel, Guenter Roeck, lm-sensors Hi Alex, On Wed, 31 Aug 2011 12:58:19 +0800, Axel Lin wrote: > 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: --- linux-3.1-rc4.orig/drivers/hwmon/pmbus/ucd9000.c 2011-08-30 13:41:32.000000000 +0200 +++ linux-3.1-rc4/drivers/hwmon/pmbus/ucd9000.c 2011-08-31 11:53:28.000000000 +0200 @@ -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; } --- linux-3.1-rc4.orig/drivers/hwmon/pmbus/ucd9200.c 2011-08-30 13:41:32.000000000 +0200 +++ linux-3.1-rc4/drivers/hwmon/pmbus/ucd9200.c 2011-08-31 11:53:20.000000000 +0200 @@ -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; } > > Signed-off-by: Axel Lin <axel.lin@gmail.com> > --- > 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(-) > > diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c > index 285bb15..a2d4a72 100644 > --- a/drivers/hwmon/pmbus/ucd9000.c > +++ b/drivers/hwmon/pmbus/ucd9000.c > @@ -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; > } > diff --git a/drivers/hwmon/pmbus/ucd9200.c b/drivers/hwmon/pmbus/ucd9200.c > index 786f6cd..a72e55e 100644 > --- a/drivers/hwmon/pmbus/ucd9200.c > +++ b/drivers/hwmon/pmbus/ucd9200.c > @@ -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; > } -- Jean Delvare ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [lm-sensors] [PATCH v2] hwmon: (pmbus) Fix the logic of 2011-08-31 10:05 ` [PATCH v2] hwmon: (pmbus) Fix the logic of checking if no id is matched Jean Delvare @ 2011-08-31 14:30 ` Axel Lin -1 siblings, 0 replies; 8+ messages in thread From: Axel Lin @ 2011-08-31 14:30 UTC (permalink / raw) To: Jean Delvare; +Cc: linux-kernel, Guenter Roeck, lm-sensors 2011/8/31 Jean Delvare <khali@linux-fr.org>: > Hi Alex, It's Axel. > > On Wed, 31 Aug 2011 12:58:19 +0800, Axel Lin wrote: >> 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. No, I didn't hit the bug. Just reading the code. > > 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 ). > > --- linux-3.1-rc4.orig/drivers/hwmon/pmbus/ucd9000.c 2011-08-30 13:41:32.000000000 +0200 > +++ linux-3.1-rc4/drivers/hwmon/pmbus/ucd9000.c 2011-08-31 11:53:28.000000000 +0200 > @@ -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; > } > --- linux-3.1-rc4.orig/drivers/hwmon/pmbus/ucd9200.c 2011-08-30 13:41:32.000000000 +0200 > +++ linux-3.1-rc4/drivers/hwmon/pmbus/ucd9200.c 2011-08-31 11:53:20.000000000 +0200 > @@ -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; > } > > >> >> Signed-off-by: Axel Lin <axel.lin@gmail.com> >> --- >> 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(-) >> >> diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c >> index 285bb15..a2d4a72 100644 >> --- a/drivers/hwmon/pmbus/ucd9000.c >> +++ b/drivers/hwmon/pmbus/ucd9000.c >> @@ -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; >> } >> diff --git a/drivers/hwmon/pmbus/ucd9200.c b/drivers/hwmon/pmbus/ucd9200.c >> index 786f6cd..a72e55e 100644 >> --- a/drivers/hwmon/pmbus/ucd9200.c >> +++ b/drivers/hwmon/pmbus/ucd9200.c >> @@ -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; >> } > > > -- > Jean Delvare > _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] hwmon: (pmbus) Fix the logic of checking if no id is matched @ 2011-08-31 14:30 ` Axel Lin 0 siblings, 0 replies; 8+ messages in thread From: Axel Lin @ 2011-08-31 14:30 UTC (permalink / raw) To: Jean Delvare; +Cc: linux-kernel, Guenter Roeck, lm-sensors 2011/8/31 Jean Delvare <khali@linux-fr.org>: > Hi Alex, It's Axel. > > On Wed, 31 Aug 2011 12:58:19 +0800, Axel Lin wrote: >> 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. No, I didn't hit the bug. Just reading the code. > > 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 ). > > --- linux-3.1-rc4.orig/drivers/hwmon/pmbus/ucd9000.c 2011-08-30 13:41:32.000000000 +0200 > +++ linux-3.1-rc4/drivers/hwmon/pmbus/ucd9000.c 2011-08-31 11:53:28.000000000 +0200 > @@ -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; > } > --- linux-3.1-rc4.orig/drivers/hwmon/pmbus/ucd9200.c 2011-08-30 13:41:32.000000000 +0200 > +++ linux-3.1-rc4/drivers/hwmon/pmbus/ucd9200.c 2011-08-31 11:53:20.000000000 +0200 > @@ -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; > } > > >> >> Signed-off-by: Axel Lin <axel.lin@gmail.com> >> --- >> 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(-) >> >> diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c >> index 285bb15..a2d4a72 100644 >> --- a/drivers/hwmon/pmbus/ucd9000.c >> +++ b/drivers/hwmon/pmbus/ucd9000.c >> @@ -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; >> } >> diff --git a/drivers/hwmon/pmbus/ucd9200.c b/drivers/hwmon/pmbus/ucd9200.c >> index 786f6cd..a72e55e 100644 >> --- a/drivers/hwmon/pmbus/ucd9200.c >> +++ b/drivers/hwmon/pmbus/ucd9200.c >> @@ -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; >> } > > > -- > Jean Delvare > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [lm-sensors] [PATCH v2] hwmon: (pmbus) Fix the logic of 2011-08-31 14:30 ` [PATCH v2] hwmon: (pmbus) Fix the logic of checking if no id is matched Axel Lin @ 2011-08-31 15:27 ` Guenter Roeck -1 siblings, 0 replies; 8+ messages in thread From: Guenter Roeck @ 2011-08-31 15:27 UTC (permalink / raw) To: axel.lin@gmail.com Cc: Jean Delvare, linux-kernel@vger.kernel.org, lm-sensors@lm-sensors.org On Wed, 2011-08-31 at 10:30 -0400, Axel Lin wrote: > 2011/8/31 Jean Delvare <khali@linux-fr.org>: > > Hi Alex, > It's Axel. > > > > > On Wed, 31 Aug 2011 12:58:19 +0800, Axel Lin wrote: > >> 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. > > 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. > > > > 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 lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] hwmon: (pmbus) Fix the logic of checking if no id is matched @ 2011-08-31 15:27 ` Guenter Roeck 0 siblings, 0 replies; 8+ messages in thread From: Guenter Roeck @ 2011-08-31 15:27 UTC (permalink / raw) To: axel.lin@gmail.com Cc: Jean Delvare, linux-kernel@vger.kernel.org, lm-sensors@lm-sensors.org On Wed, 2011-08-31 at 10:30 -0400, Axel Lin wrote: > 2011/8/31 Jean Delvare <khali@linux-fr.org>: > > Hi Alex, > It's Axel. > > > > > On Wed, 31 Aug 2011 12:58:19 +0800, Axel Lin wrote: > >> 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. > > 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. > > > > 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2011-08-31 15:28 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-08-31 4:58 [lm-sensors] [PATCH v2] hwmon: (pmbus) Fix the logic of checking if Axel Lin 2011-08-31 4:58 ` [PATCH v2] hwmon: (pmbus) Fix the logic of checking if no id is matched Axel Lin 2011-08-31 10:05 ` [lm-sensors] [PATCH v2] hwmon: (pmbus) Fix the logic of Jean Delvare 2011-08-31 10:05 ` [PATCH v2] hwmon: (pmbus) Fix the logic of checking if no id is matched Jean Delvare 2011-08-31 14:30 ` [lm-sensors] [PATCH v2] hwmon: (pmbus) Fix the logic of Axel Lin 2011-08-31 14:30 ` [PATCH v2] hwmon: (pmbus) Fix the logic of checking if no id is matched Axel Lin 2011-08-31 15:27 ` [lm-sensors] [PATCH v2] hwmon: (pmbus) Fix the logic of Guenter Roeck 2011-08-31 15:27 ` [PATCH v2] hwmon: (pmbus) Fix the logic of checking if no id is matched Guenter Roeck
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.