[PATCH v2] hwmon: (pmbus) Fix the logic of checking if no id is matched

[PATCH v2] hwmon: (pmbus) Fix the logic of checking if no id is matched

[Axel Lin]

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

Re: [PATCH v2] hwmon: (pmbus) Fix the logic of checking if no id is matched

[Jean Delvare]

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

Re: [PATCH v2] hwmon: (pmbus) Fix the logic of checking if no id is matched

[Axel Lin]

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
>

Re: [PATCH v2] hwmon: (pmbus) Fix the logic of checking if no id is matched

[Guenter Roeck]

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