* re: max17042_battery: Support regmap to access device's registers
@ 2015-08-14 9:55 Dan Carpenter
2015-08-17 5:26 ` Krzysztof Kozlowski
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2015-08-14 9:55 UTC (permalink / raw)
To: jonghwa3.lee; +Cc: linux-pm
Hello Jonghwa Lee,
The patch 39e7213edc4f: "max17042_battery: Support regmap to access
device's registers" from Oct 25, 2013, leads to the following static
checker warning:
drivers/power/max17042_battery.c:501 max17042_init_model()
warn: passing casted pointer 'temp_data' to 'max17042_model_data_compare()' 32 vs 16.
drivers/power/max17042_battery.c
469 static inline int max17042_model_data_compare(struct max17042_chip *chip,
470 u16 *data1, u16 *data2, int size)
471 {
472 int i;
473
474 if (memcmp(data1, data2, size)) {
^^^^
data2 is size * sizeof(u32) bytes long so we are only checking the first
quarter of the buffer. data1 is in size of u16s so I wonder how this
memcmp() works at all?
475 dev_err(&chip->client->dev, "%s compare failed\n", __func__);
476 for (i = 0; i < size; i++)
^^^^
Here size is in terms of u16 elements. The original code mixed u16
units with byte units so it was buggy, but now we are using u32 units so
it's even worse.
477 dev_info(&chip->client->dev, "0x%x, 0x%x",
478 data1[i], data2[i]);
479 dev_info(&chip->client->dev, "\n");
480 return -EINVAL;
481 }
482 return 0;
483 }
484
485 static int max17042_init_model(struct max17042_chip *chip)
486 {
487 int ret;
488 int table_size = ARRAY_SIZE(chip->pdata->config_data->cell_char_tbl);
data1 is any array of u16s.
489 u32 *temp_data;
490
491 temp_data = kcalloc(table_size, sizeof(*temp_data), GFP_KERNEL);
Here "size" (table_size) is in units of u32 elements.
492 if (!temp_data)
493 return -ENOMEM;
494
495 max10742_unlock_model(chip);
496 max17042_write_model_data(chip, MAX17042_MODELChrTbl,
497 table_size);
498 max17042_read_model_data(chip, MAX17042_MODELChrTbl, temp_data,
499 table_size);
^^^^^^^^^^
Here "size" (table_size) is in units of u32 elements.
500
501 ret = max17042_model_data_compare(
502 chip,
503 chip->pdata->config_data->cell_char_tbl,
504 (u16 *)temp_data,
^^^^^^^^^^^^^^^^
This cast is ugly and causes a static checker warning.
505 table_size);
506
507 max10742_lock_model(chip);
508 kfree(temp_data);
509
510 return ret;
511 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: max17042_battery: Support regmap to access device's registers
2015-08-14 9:55 max17042_battery: Support regmap to access device's registers Dan Carpenter
@ 2015-08-17 5:26 ` Krzysztof Kozlowski
2015-08-17 14:08 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Krzysztof Kozlowski @ 2015-08-17 5:26 UTC (permalink / raw)
To: Dan Carpenter; +Cc: jonghwa3.lee, linux-pm
2015-08-14 18:55 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
> Hello Jonghwa Lee,
>
> The patch 39e7213edc4f: "max17042_battery: Support regmap to access
> device's registers" from Oct 25, 2013, leads to the following static
> checker warning:
>
> drivers/power/max17042_battery.c:501 max17042_init_model()
> warn: passing casted pointer 'temp_data' to 'max17042_model_data_compare()' 32 vs 16.
>
> drivers/power/max17042_battery.c
> 469 static inline int max17042_model_data_compare(struct max17042_chip *chip,
> 470 u16 *data1, u16 *data2, int size)
> 471 {
> 472 int i;
> 473
> 474 if (memcmp(data1, data2, size)) {
> ^^^^
> data2 is size * sizeof(u32) bytes long so we are only checking the first
> quarter of the buffer. data1 is in size of u16s so I wonder how this
> memcmp() works at all?
>
> 475 dev_err(&chip->client->dev, "%s compare failed\n", __func__);
> 476 for (i = 0; i < size; i++)
> ^^^^
> Here size is in terms of u16 elements. The original code mixed u16
> units with byte units so it was buggy, but now we are using u32 units so
> it's even worse.
>
> 477 dev_info(&chip->client->dev, "0x%x, 0x%x",
> 478 data1[i], data2[i]);
> 479 dev_info(&chip->client->dev, "\n");
> 480 return -EINVAL;
> 481 }
> 482 return 0;
> 483 }
> 484
> 485 static int max17042_init_model(struct max17042_chip *chip)
> 486 {
> 487 int ret;
> 488 int table_size = ARRAY_SIZE(chip->pdata->config_data->cell_char_tbl);
>
> data1 is any array of u16s.
>
> 489 u32 *temp_data;
> 490
> 491 temp_data = kcalloc(table_size, sizeof(*temp_data), GFP_KERNEL);
>
> Here "size" (table_size) is in units of u32 elements.
>
> 492 if (!temp_data)
> 493 return -ENOMEM;
> 494
> 495 max10742_unlock_model(chip);
> 496 max17042_write_model_data(chip, MAX17042_MODELChrTbl,
> 497 table_size);
> 498 max17042_read_model_data(chip, MAX17042_MODELChrTbl, temp_data,
> 499 table_size);
> ^^^^^^^^^^
>
> Here "size" (table_size) is in units of u32 elements.
>
> 500
> 501 ret = max17042_model_data_compare(
> 502 chip,
> 503 chip->pdata->config_data->cell_char_tbl,
> 504 (u16 *)temp_data,
> ^^^^^^^^^^^^^^^^
> This cast is ugly and causes a static checker warning.
>
> 505 table_size);
> 506
> 507 max10742_lock_model(chip);
> 508 kfree(temp_data);
> 509
> 510 return ret;
> 511 }
The cast is indeed ugly but I think that in each loop the u16 pointers
are used so this should work. regmap also uses 16-bit configuration.
memcpy operates on bytes, not on words. The 'temp_data' (which is size
of u32*table_size) is just unnecessarily big, actually two times
bigger than needed.
Where exactly do you see an error (except wasted space)?
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: max17042_battery: Support regmap to access device's registers
2015-08-17 5:26 ` Krzysztof Kozlowski
@ 2015-08-17 14:08 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2015-08-17 14:08 UTC (permalink / raw)
To: Krzysztof Kozlowski; +Cc: jonghwa3.lee, linux-pm
The static checker is just complaining about the cast... I have looked
at it again and this code is definitely buggy.
max17042_read_model_data() reads u32s into tmp_data.
->cell_char_tbl is an array with 48 elements of u16.
We later assume that cell_char_tbl and temp_data have 48 bytes each when
we use memcmp() to compare them. This only compares the first half of
->cell_char_tbl with the first quarter of tmp_data[].
Then we assume that they hold u16s when we print out the warning
message.
regards,
dan carpetner
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-08-17 14:08 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-08-14 9:55 max17042_battery: Support regmap to access device's registers Dan Carpenter
2015-08-17 5:26 ` Krzysztof Kozlowski
2015-08-17 14:08 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).