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@xxxxxxxxxx>
> Sent: Wednesday, June 22, 2022 2:48 PM
> To: Roger Lu (陸瑞傑) <Roger.Lu@xxxxxxxxxxxx>
> Cc: linux-mediatek@xxxxxxxxxxxxxxxxxxx; kernel-janitors@xxxxxxxxxxxxxxx
> 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
