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
>
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
