power: supply: bq256xx: Introduce the BQ256XX charger driver
Hello Ricardo Rivera-Matos,
The patch 32e4978bb920: "power: supply: bq256xx: Introduce the BQ256XX charger driver" from Jan 6, 2021, leads to the following static checker warning:
drivers/power/supply/bq256xx_charger.c:1512 bq256xx_hw_init() error: buffer overflow 'bq256xx_watchdog_time' 8 <= 8
drivers/power/supply/bq256xx_charger.c
1503 static int bq256xx_hw_init(struct bq256xx_device *bq) 1504 {
1505 struct power_supply_battery_info bat_info = { };
1506 int wd_reg_val = BQ256XX_WATCHDOG_DIS;
1507 int ret = 0;
1508 int i;
1509
1510 for (i = 0; i < BQ256XX_NUM_WD_VAL; i++) {
1511 if (bq->watchdog_timer > bq256xx_watchdog_time[i] &&
1512 bq->watchdog_timer < bq256xx_watchdog_time[i + 1])
^^^^^^^^^^^^^^^^^^^^^^^^^^^^The last four members of this array are all zero.
On the last iteration through the loop this will read beyond the end of the array possibly setting "wd_reg_val = 7" uninitentionally.
1513 wd_reg_val = i;
1514 }
1515 ret = regmap_update_bits(bq->regmap, BQ256XX_CHARGER_CONTROL_1,
1516 BQ256XX_WATCHDOG_MASK, wd_reg_val <<
1517 BQ256XX_WDT_BIT_SHIFT);
regards,
dan carpenter
Dan,
Quoted 21 lines · Dan Carpenter
Hello Ricardo Rivera-Matos,
The patch
32e4978bb920: "power: supply: bq256xx: Introduce the BQ256XX charger driver" from Jan 6, 2021, leads to the following static checker warning:drivers/power/supply/bq256xx_charger.c:1512 bq256xx_hw_init() error: buffer overflow 'bq256xx_watchdog_time' 8 <= 8
drivers/power/supply/bq256xx_charger.c
1503 static intbq256xx_hw_init(struct bq256xx_device *bq)1504 { 1505 struct power_supply_battery_info bat_info = { }; 1506 int wd_reg_val = BQ256XX_WATCHDOG_DIS; 1507 int ret = 0; 1508 int i; 1509 1510 for (i = 0; i < BQ256XX_NUM_WD_VAL; i++) { 1511 if (bq->watchdog_timer > bq256xx_watchdog_time[i] && 1512 bq->watchdog_timer < bq256xx_watchdog_time[i + 1]) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Quoted 1 line
The last four members of this array are all zero.
ACK, BQ256XX_NUM_WD_VAL should actually be 4 instead of 8
Quoted 3 lines
On the last iteration through the loop this will read beyond the end of the array possibly setting "
wd_reg_val= 7" uninitentionally.
ACK, bq256xx_parse_dt() will clamp bq->watchdog_timer so then in bq256xx_hw_init()
for (i = 0; i < BQ256XX_NUM_WD_VAL; i++) {
if (bq->watchdog_timer == bq256xx_watchdog_time[i]) { wd_reg_val = i;
break;
}
if (bq->watchdog_timer > bq256xx_watchdog_time[i] && bq->watchdog_timer < bq256xx_watchdog_time[i + 1]) wd_reg_val = i;
}
The first if will catch the exact matches and the second if will catch the "in-betweens" and round down. The final iteration will always fall into the first if statement and break.
Quoted 9 lines
1513 wd_reg_val = i; 1514 } 1515 ret = regmap_update_bits(bq->regmap, BQ256XX_CHARGER_CONTROL_1, 1516 BQ256XX_WATCHDOG_MASK, wd_reg_val << 1517 BQ256XX_WDT_BIT_SHIFT);regards,
dan carpenter
Ricardo
Quoted 42 lines · Ricardo Rivera-Matos
Dan,
Quoted 21 lines · Dan Carpenter
Hello Ricardo Rivera-Matos,
The patch
32e4978bb920: "power: supply: bq256xx: Introduce the BQ256XX charger driver" from Jan 6, 2021, leads to the following static checker warning:drivers/power/supply/bq256xx_charger.c:1512 bq256xx_hw_init() error: buffer overflow 'bq256xx_watchdog_time' 8 <= 8
drivers/power/supply/bq256xx_charger.c
1503 static intbq256xx_hw_init(struct bq256xx_device *bq)1504 { 1505 struct power_supply_battery_info bat_info = { }; 1506 int wd_reg_val = BQ256XX_WATCHDOG_DIS; 1507 int ret = 0; 1508 int i; 1509 1510 for (i = 0; i < BQ256XX_NUM_WD_VAL; i++) { 1511 if (bq->watchdog_timer > bq256xx_watchdog_time[i] && 1512 bq->watchdog_timer < bq256xx_watchdog_time[i + 1]) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^Quoted 1 line
The last four members of this array are all zero.
ACK,
BQ256XX_NUM_WD_VALshould actually be 4 instead of 8Quoted 3 lines
On the last iteration through the loop this will read beyond the end of the array possibly setting "
wd_reg_val= 7" uninitentionally.ACK,
bq256xx_parse_dt()will clampbq->watchdog_timerso then inbq256xx_hw_init()for (i = 0; i <
BQ256XX_NUM_WD_VAL; i++) {
if (bq->watchdog_timer== bq256xx_watchdog_time[i]) {wd_reg_val= i;
break;
}
if (bq->watchdog_timer> bq256xx_watchdog_time[i] &&bq->watchdog_timer< bq256xx_watchdog_time[i + 1])wd_reg_val= i;
}
Quoted 4 lines
The first if will catch the exact matches and the second if will catch the "in-betweens" and round down. The final iteration will always fall into the first if statement and break.
This looks good. This is a patch you are proposing or it's already merged in an upstream tree somewhere? Either way, that sounds fine. Thank!
regards,
dan carpenter
Dan,
Quoted 42 lines · Dan Carpenter
Quoted 41 lines · Ricardo Rivera-Matos
Dan,
Quoted 21 lines · Dan Carpenter
Hello Ricardo Rivera-Matos,
The patch
32e4978bb920: "power: supply: bq256xx: Introduce the BQ256XX charger driver" from Jan 6, 2021, leads to the following static checker warning:drivers/power/supply/bq256xx_charger.c:1512 bq256xx_hw_init() error: buffer overflow 'bq256xx_watchdog_time' 8 <= 8
drivers/power/supply/bq256xx_charger.c1503 static int bq256xx_hw_init(struct bq256xx_device *bq) 1504 { 1505 struct power_supply_battery_info bat_info = { }; 1506 int wd_reg_val = BQ256XX_WATCHDOG_DIS; 1507 int ret = 0; 1508 int i; 1509 1510 for (i = 0; i < BQ256XX_NUM_WD_VAL; i++) { 1511 if (bq->watchdog_timer > bq256xx_watchdog_time[i] && 1512 bq->watchdog_timer < bq256xx_watchdog_time[i + 1]) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^Quoted 1 line
The last four members of this array are all zero.
ACK,
BQ256XX_NUM_WD_VALshould actually be 4 instead of 8Quoted 2 lines
On the last iteration through the loop this will read beyond the end of the array possibly setting "
wd_reg_val= 7" uninitentionally.ACK,
bq256xx_parse_dt()will clampbq->watchdog_timerso then inbq256xx_hw_init()for (i = 0; i <
BQ256XX_NUM_WD_VAL; i++) {
if (bq->watchdog_timer== bq256xx_watchdog_time[i]) {wd_reg_val= i;
break;
}
if (bq->watchdog_timer> bq256xx_watchdog_time[i] &&bq->watchdog_timer< bq256xx_watchdog_time[i + 1])wd_reg_val= i;
}
Quoted 7 lines
Quoted 4 lines
The first if will catch the exact matches and the second if will catch the "in-betweens" and round down. The final iteration will always fall into the first if statement and break.
This looks good. This is a patch you are proposing or it's already merged in an upstream tree somewhere? Either way, that sounds fine. Thank!
I am proposing it. I will CC you when I send off the patchset.
Quoted 4 lines
regards,
dan carpenter
Best Regards,
Ricardo