soc: mediatek: SVS: add mt8192 SVS GPU driver

bug report Dan Carpenter 2 participants 3 messages Jun 22, 2022

Subsystems (2)

Kernel janitors Arm MediaTek

lore

Share & tools

lore

Roger Lu (陸瑞傑)

2022-06-22 09:18 UTC #1 lore raw

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 <[email protected]>
Sent: Wednesday, June 22, 2022 2:48 PM
To: Roger Lu (陸瑞傑) <[email protected]>
Cc: [email protected]; [email protected]
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

Dan Carpenter

author

2022-06-22 09:35 UTC #2 lore raw

Quoted 21 lines · Roger Lu (陸瑞傑)

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 <[email protected]>
Sent: Wednesday, June 22, 2022 2:48 PM
To: Roger Lu (陸瑞傑) <[email protected]>
Cc: [email protected]; [email protected]
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'

Quoted 8 lines

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

Quoted 102 lines

    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;

    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