* [bug report] soc: mediatek: SVS: add mt8192 SVS GPU driver
@ 2022-06-22 6:47 Dan Carpenter
2022-06-22 9:18 ` Roger Lu (陸瑞傑)
0 siblings, 1 reply; 3+ 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] 3+ messages in thread
* RE: [bug report] soc: mediatek: SVS: add mt8192 SVS GPU driver
2022-06-22 6:47 [bug report] soc: mediatek: SVS: add mt8192 SVS GPU driver Dan Carpenter
@ 2022-06-22 9:18 ` Roger Lu (陸瑞傑)
2022-06-22 9:34 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Roger Lu (陸瑞傑) @ 2022-06-22 9:18 UTC (permalink / raw)
To: Dan Carpenter
Cc: linux-mediatek, kernel-janitors, Fan Chen (陳凡),
Allen-yy Lin (林奕穎),
Jia-wei Chang (張佳偉)
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.
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] 3+ messages in thread
* Re: [bug report] soc: mediatek: SVS: add mt8192 SVS GPU driver
2022-06-22 9:18 ` Roger Lu (陸瑞傑)
@ 2022-06-22 9:34 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2022-06-22 9:34 UTC (permalink / raw)
To: Roger Lu (陸瑞傑)
Cc: linux-mediatek, kernel-janitors, 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] 3+ messages in thread
end of thread, other threads:[~2022-06-22 9:35 UTC | newest]
Thread overview: 3+ 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
2022-06-22 9:18 ` Roger Lu (陸瑞傑)
2022-06-22 9:34 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.