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

[Axel Lin]

2011/8/31 Jean Delvare <khali@xxxxxxxxxxxx>:
> 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@xxxxxxxxx>
>> ---
>> 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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/