Re: [bug report] soc: mediatek: SVS: add mt8192 SVS GPU driver

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

On Wed, Jun 22, 2022 at 09:18:25AM +0000, Roger Lu (陸瑞傑) wrote:
> Hi Dan,
> 
> Excuse me, I didn't see the warning raised and explain in-line from your email. Please search "Roger (0622)" for the reply. Thanks a lot.
> 
> Sincerely,
> Roger Lu.
> 
> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@oracle.com> 
> Sent: Wednesday, June 22, 2022 2:48 PM
> To: Roger Lu (陸瑞傑) <Roger.Lu@mediatek.com>
> Cc: linux-mediatek@lists.infradead.org; kernel-janitors@vger.kernel.org
> Subject: [bug report] soc: mediatek: SVS: add mt8192 SVS GPU driver
> 
> Hello Roger Lu,
> 
> The patch 0bbb09b2af9d: "soc: mediatek: SVS: add mt8192 SVS GPU driver" from May 16, 2022, leads to the following (unpublished) Smatch static checker warning:
> 
> 	drivers/soc/mediatek/mtk-svs.c:532 svs_adjust_pm_opp_volts()
> 	warn: loop overwrites return value 'ret'
> 
> drivers/soc/mediatek/mtk-svs.c
>     487 static int svs_adjust_pm_opp_volts(struct svs_bank *svsb)
>     488 {
>     489         int ret = -EPERM, tzone_temp = 0;
>                     ^^^^^^^^^^^^^
> What is this -EPERM for?
> Roger (0622): This -EPERM initialization is required. The `ret` value assignment in this function is put into if-statement so it needs the initialization.
> 

There is no if statement which uses this assignment.  I have copy and
pasted the code from linux-next into this email so it should be easy to
check.

regards,
dan carpenter

>     490         u32 i, svsb_volt, opp_volt, temp_voffset = 0, opp_start, opp_stop;
>     491 
>     492         mutex_lock(&svsb->lock);
>     493 
>     494         /*
>     495          * 2-line bank updates its corresponding opp volts.
>     496          * 1-line bank updates all opp volts.
>     497          */
>     498         if (svsb->type == SVSB_HIGH) {
>     499                 opp_start = 0;
>     500                 opp_stop = svsb->turn_pt;
>     501         } else if (svsb->type == SVSB_LOW) {
>     502                 opp_start = svsb->turn_pt;
>     503                 opp_stop = svsb->opp_count;
>     504         } else {
>     505                 opp_start = 0;
>     506                 opp_stop = svsb->opp_count;
>     507         }
>     508 
>     509         /* Get thermal effect */
>     510         if (svsb->phase == SVSB_PHASE_MON) {
>     511                 ret = thermal_zone_get_temp(svsb->tzd, &tzone_temp);
>     512                 if (ret || (svsb->temp > SVSB_TEMP_UPPER_BOUND &&
>     513                             svsb->temp < SVSB_TEMP_LOWER_BOUND)) {
>     514                         dev_err(svsb->dev, "%s: %d (0x%x), run default volts\n",
>     515                                 svsb->tzone_name, ret, svsb->temp);
>     516                         svsb->phase = SVSB_PHASE_ERROR;
> 
> ret is set here but there is no goto unlock_mutex;
> Roger (0622): We don't need goto here. If we cannot get temperature here, SVS bank will apply default voltages to DVFS. So we change SVS bank's phase to `SVSB_PHASE_ERROR` only.
> 
>     517                 }
>     518 
>     519                 if (tzone_temp >= svsb->tzone_htemp)
>     520                         temp_voffset += svsb->tzone_htemp_voffset;
>     521                 else if (tzone_temp <= svsb->tzone_ltemp)
>     522                         temp_voffset += svsb->tzone_ltemp_voffset;
>     523 
>     524                 /* 2-line bank update all opp volts when running mon mode */
>     525                 if (svsb->type == SVSB_HIGH || svsb->type == SVSB_LOW) {
>     526                         opp_start = 0;
>     527                         opp_stop = svsb->opp_count;
>     528                 }
>     529         }
>     530 
>     531         /* vmin <= svsb_volt (opp_volt) <= default opp voltage */
> --> 532         for (i = opp_start; i < opp_stop; i++) {
> 
> I guess the bug is that there was supposed to be an explicit check?
> 
> 	if (opp_start == opp_stop) {
> 		ret = -EPERM;
> 		goto unlock_mutex;
> 	}
> 
> Otherwise, we are possibly returning the ret from the /* Get thermal effect */ block.
> 
>     533                 switch (svsb->phase) {
>     534                 case SVSB_PHASE_ERROR:
>     535                         opp_volt = svsb->opp_dvolt[i];
>     536                         break;
>     537                 case SVSB_PHASE_INIT01:
>     538                         /* do nothing */
>     539                         goto unlock_mutex;
>     540                 case SVSB_PHASE_INIT02:
>     541                         svsb_volt = max(svsb->volt[i], svsb->vmin);
>     542                         opp_volt = svs_bank_volt_to_opp_volt(svsb_volt,
>     543                                                              svsb->volt_step,
>     544                                                              svsb->volt_base);
>     545                         break;
>     546                 case SVSB_PHASE_MON:
>     547                         svsb_volt = max(svsb->volt[i] + temp_voffset, svsb->vmin);
>     548                         opp_volt = svs_bank_volt_to_opp_volt(svsb_volt,
>     549                                                              svsb->volt_step,
>     550                                                              svsb->volt_base);
>     551                         break;
>     552                 default:
>     553                         dev_err(svsb->dev, "unknown phase: %u\n", svsb->phase);
>     554                         ret = -EINVAL;
>     555                         goto unlock_mutex;
>     556                 }
>     557 
>     558                 opp_volt = min(opp_volt, svsb->opp_dvolt[i]);
>     559                 ret = dev_pm_opp_adjust_voltage(svsb->opp_dev,
>     560                                                 svsb->opp_dfreq[i],
>     561                                                 opp_volt, opp_volt,
>     562                                                 svsb->opp_dvolt[i]);
>     563                 if (ret) {
>     564                         dev_err(svsb->dev, "set %uuV fail: %d\n",
>     565                                 opp_volt, ret);
>     566                         goto unlock_mutex;
>     567                 }
>     568         }
>     569 
>     570 unlock_mutex:
>     571         mutex_unlock(&svsb->lock);
>     572 
>     573         return ret;
>     574 }
> 
> regards,
> dan carpenter