Re: [EXTERNAL] [bug report] power: supply: bq256xx: Introduce the BQ256XX charger driver

[Dan Carpenter <dan.carpenter@oracle.com>]

On Tue, Jan 12, 2021 at 05:44:52PM -0600, Ricardo Rivera-Matos wrote:
> Dan,
> 
> On 1/12/21 2:54 AM, Dan Carpenter wrote:
> > 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.
> ACK, BQ256XX_NUM_WD_VAL should actually be 4 instead of 8
> > 
> > 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.
> 

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