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.
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
