* [bug report] soc: mediatek: SVS: add mt8192 SVS GPU driver
@ 2022-06-22 6:47 Dan Carpenter
[not found] ` <HK0PR03MB34269237A97091637A9A9A13F1B29@HK0PR03MB3426.apcprd03.prod.outlook.com>
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2022-06-22 6:47 UTC (permalink / raw)
To: roger.lu; +Cc: linux-mediatek, kernel-janitors
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?
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;
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
^ permalink raw reply [flat|nested] 2+ messages in thread[parent not found: <HK0PR03MB34269237A97091637A9A9A13F1B29@HK0PR03MB3426.apcprd03.prod.outlook.com>]
* Re: [bug report] soc: mediatek: SVS: add mt8192 SVS GPU driver [not found] ` <HK0PR03MB34269237A97091637A9A9A13F1B29@HK0PR03MB3426.apcprd03.prod.outlook.com> @ 2022-06-22 9:34 ` Dan Carpenter 0 siblings, 0 replies; 2+ messages in thread From: Dan Carpenter @ 2022-06-22 9:34 UTC (permalink / raw) To: Roger Lu (陸瑞傑) Cc: linux-mediatek@lists.infradead.org, kernel-janitors@vger.kernel.org, Fan Chen (陳凡), Allen-yy Lin (林奕穎), Jia-wei Chang (張佳偉) 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 ^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-06-22 9:35 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-06-22 6:47 [bug report] soc: mediatek: SVS: add mt8192 SVS GPU driver Dan Carpenter
[not found] ` <HK0PR03MB34269237A97091637A9A9A13F1B29@HK0PR03MB3426.apcprd03.prod.outlook.com>
2022-06-22 9:34 ` Dan Carpenter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox