[PATCH 1/2] drm/amd/powerplay: smu_v11_0: fix uninitialized variable use

[PATCH 1/2] drm/amd/powerplay: smu_v11_0: fix uninitialized variable use

[Arnd Bergmann]

A mistake in the error handling caused an uninitialized
variable to be used:

drivers/gpu/drm/amd/amdgpu/../powerplay/smu_v11_0.c:1102:10: error: variable 'freq' is used uninitialized whenever '?:' condition is false [-Werror,-Wsometimes-uninitialized]
                ret =  smu_get_current_clk_freq_by_table(smu, clk_id, &freq);
                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/amd/amdgpu/../powerplay/inc/amdgpu_smu.h:880:3: note: expanded from macro 'smu_get_current_clk_freq_by_table'
        ((smu)->ppt_funcs->get_current_clk_freq_by_table ? (smu)->ppt_funcs->get_current_clk_freq_by_table((smu), (clk_type), (value)) : 0)
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/amd/amdgpu/../powerplay/smu_v11_0.c:1114:2: note: uninitialized use occurs here
        freq *= 100;
        ^~~~
drivers/gpu/drm/amd/amdgpu/../powerplay/smu_v11_0.c:1102:10: note: remove the '?:' if its condition is always true
                ret =  smu_get_current_clk_freq_by_table(smu, clk_id, &freq);
                       ^
drivers/gpu/drm/amd/amdgpu/../powerplay/inc/amdgpu_smu.h:880:3: note: expanded from macro 'smu_get_current_clk_freq_by_table'
        ((smu)->ppt_funcs->get_current_clk_freq_by_table ? (smu)->ppt_funcs->get_current_clk_freq_by_table((smu), (clk_type), (value)) : 0)
         ^
drivers/gpu/drm/amd/amdgpu/../powerplay/smu_v11_0.c:1095:15: note: initialize the variable 'freq' to silence this warning
        uint32_t freq;
                     ^
                      = 0

Bail out of smu_v11_0_get_current_clk_freq() before we get there.

Fixes: e36182490dec ("drm/amd/powerplay: fix dpm freq unit error (10KHz -> Mhz)")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/amd/powerplay/smu_v11_0.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
index c3f9714e9047..bd89a13b6679 100644
--- a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
+++ b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
@@ -1099,9 +1099,11 @@ static int smu_v11_0_get_current_clk_freq(struct smu_context *smu,
 		return -EINVAL;
 
 	/* if don't has GetDpmClockFreq Message, try get current clock by SmuMetrics_t */
-	if (smu_msg_get_index(smu, SMU_MSG_GetDpmClockFreq) == 0)
+	if (smu_msg_get_index(smu, SMU_MSG_GetDpmClockFreq) == 0) {
 		ret =  smu_get_current_clk_freq_by_table(smu, clk_id, &freq);
-	else {
+		if (ret)
+			return ret;
+	} else {
 		ret = smu_send_smc_msg_with_param(smu, SMU_MSG_GetDpmClockFreq,
 						  (smu_clk_get_index(smu, clk_id) << 16));
 		if (ret)
-- 
2.20.0

[PATCH 2/2] drm/amd/powerplay: vega20: fix uninitialized variable use

[Arnd Bergmann]

If smu_get_current_rpm() fails, we can't use the output,
as that may be uninitialized:

drivers/gpu/drm/amd/amdgpu/../powerplay/vega20_ppt.c:3023:8: error: variable 'current_rpm' is used uninitialized whenever '?:' condition is false [-Werror,-Wsometimes-uninitialized]
        ret = smu_get_current_rpm(smu, &current_rpm);
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/amd/amdgpu/../powerplay/inc/amdgpu_smu.h:735:3: note: expanded from macro 'smu_get_current_rpm'
        ((smu)->funcs->get_current_rpm ? (smu)->funcs->get_current_rpm((smu), (speed)) : 0)
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/amd/amdgpu/../powerplay/vega20_ppt.c:3024:12: note: uninitialized use occurs here
        percent = current_rpm * 100 / pptable->FanMaximumRpm;
                  ^~~~~~~~~~~
drivers/gpu/drm/amd/amdgpu/../powerplay/vega20_ppt.c:3023:8: note: remove the '?:' if its condition is always true
        ret = smu_get_current_rpm(smu, &current_rpm);
              ^
drivers/gpu/drm/amd/amdgpu/../powerplay/inc/amdgpu_smu.h:735:3: note: expanded from macro 'smu_get_current_rpm'
        ((smu)->funcs->get_current_rpm ? (smu)->funcs->get_current_rpm((smu), (speed)) : 0)
         ^
drivers/gpu/drm/amd/amdgpu/../powerplay/vega20_ppt.c:3020:22: note: initialize the variable 'current_rpm' to silence this warning
        uint32_t current_rpm;

Propagate the error code in that case.

Fixes: ee0db82027ee ("drm/amd/powerplay: move PPTable_t uses into asic level")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/amd/powerplay/vega20_ppt.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c
index 9ce3f1c8ae0f..20d477f8dc84 100644
--- a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c
+++ b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c
@@ -3021,10 +3021,13 @@ static int vega20_get_fan_speed_percent(struct smu_context *smu,
 	PPTable_t *pptable = smu->smu_table.driver_pptable;
 
 	ret = smu_get_current_rpm(smu, &current_rpm);
+	if (ret)
+		return ret;
+
 	percent = current_rpm * 100 / pptable->FanMaximumRpm;
 	*speed = percent > 100 ? 100 : percent;
 
-	return ret;
+	return 0;
 }
 
 static int vega20_get_gpu_power(struct smu_context *smu, uint32_t *value)
-- 
2.20.0

Re: [1/2] drm/amd/powerplay: smu_v11_0: fix uninitialized variable use

[Nathan Chancellor]

On Mon, Jul 08, 2019 at 04:07:58PM +0200, Arnd Bergmann wrote:
> A mistake in the error handling caused an uninitialized
> variable to be used:
> 
> drivers/gpu/drm/amd/amdgpu/../powerplay/smu_v11_0.c:1102:10: error: variable 'freq' is used uninitialized whenever '?:' condition is false [-Werror,-Wsometimes-uninitialized]
>                 ret =  smu_get_current_clk_freq_by_table(smu, clk_id, &freq);
>                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/amd/amdgpu/../powerplay/inc/amdgpu_smu.h:880:3: note: expanded from macro 'smu_get_current_clk_freq_by_table'
>         ((smu)->ppt_funcs->get_current_clk_freq_by_table ? (smu)->ppt_funcs->get_current_clk_freq_by_table((smu), (clk_type), (value)) : 0)
>          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/amd/amdgpu/../powerplay/smu_v11_0.c:1114:2: note: uninitialized use occurs here
>         freq *= 100;
>         ^~~~
> drivers/gpu/drm/amd/amdgpu/../powerplay/smu_v11_0.c:1102:10: note: remove the '?:' if its condition is always true
>                 ret =  smu_get_current_clk_freq_by_table(smu, clk_id, &freq);
>                        ^
> drivers/gpu/drm/amd/amdgpu/../powerplay/inc/amdgpu_smu.h:880:3: note: expanded from macro 'smu_get_current_clk_freq_by_table'
>         ((smu)->ppt_funcs->get_current_clk_freq_by_table ? (smu)->ppt_funcs->get_current_clk_freq_by_table((smu), (clk_type), (value)) : 0)
>          ^
> drivers/gpu/drm/amd/amdgpu/../powerplay/smu_v11_0.c:1095:15: note: initialize the variable 'freq' to silence this warning
>         uint32_t freq;
>                      ^
>                       = 0
> 
> Bail out of smu_v11_0_get_current_clk_freq() before we get there.
> 
> Fixes: e36182490dec ("drm/amd/powerplay: fix dpm freq unit error (10KHz -> Mhz)")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/gpu/drm/amd/powerplay/smu_v11_0.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
> index c3f9714e9047..bd89a13b6679 100644
> --- a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
> +++ b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
> @@ -1099,9 +1099,11 @@ static int smu_v11_0_get_current_clk_freq(struct smu_context *smu,
>  		return -EINVAL;
>  
>  	/* if don't has GetDpmClockFreq Message, try get current clock by SmuMetrics_t */
> -	if (smu_msg_get_index(smu, SMU_MSG_GetDpmClockFreq) == 0)
> +	if (smu_msg_get_index(smu, SMU_MSG_GetDpmClockFreq) == 0) {
>  		ret =  smu_get_current_clk_freq_by_table(smu, clk_id, &freq);
> -	else {
> +		if (ret)
> +			return ret;

I am kind of surprised that this fixes the warning. If I am interpreting
the warning correctly, it is complaining that the member
get_current_clk_freq_by_table could be NULL and not be called to
initialize freq and that entire statement will become 0. If that is the
case, it seems like this added error handling won't fix that problem,
right?

Cheers,
Nathan

Re: [1/2] drm/amd/powerplay: smu_v11_0: fix uninitialized variable use

[Arnd Bergmann]

On Mon, Jul 8, 2019 at 5:02 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
> On Mon, Jul 08, 2019 at 04:07:58PM +0200, Arnd Bergmann wrote:
> >       /* if don't has GetDpmClockFreq Message, try get current clock by SmuMetrics_t */
> > -     if (smu_msg_get_index(smu, SMU_MSG_GetDpmClockFreq) == 0)
> > +     if (smu_msg_get_index(smu, SMU_MSG_GetDpmClockFreq) == 0) {
> >               ret =  smu_get_current_clk_freq_by_table(smu, clk_id, &freq);
> > -     else {
> > +             if (ret)
> > +                     return ret;
>
> I am kind of surprised that this fixes the warning. If I am interpreting
> the warning correctly, it is complaining that the member
> get_current_clk_freq_by_table could be NULL and not be called to
> initialize freq and that entire statement will become 0. If that is the
> case, it seems like this added error handling won't fix that problem,
> right?

No, I'm fairly sure this is the right fix. Looking at the whole function:

| static int smu_v11_0_get_current_clk_freq(struct smu_context *smu,
|                                          enum smu_clk_type clk_id,
|                                          uint32_t *value)
|{
|        int ret = 0;
|        uint32_t freq;
|
|        if (clk_id >= SMU_CLK_COUNT || !value)
|                return -EINVAL;
|
|        /* if don't has GetDpmClockFreq Message, try get current
clock by SmuMetrics_t */
|        if (smu_msg_get_index(smu, SMU_MSG_GetDpmClockFreq) == 0) {
|                ret =  smu_get_current_clk_freq_by_table(smu, clk_id, &freq);

Here &freq may or may not get initialized

|        } else {
|                ret = smu_send_smc_msg_with_param(smu, SMU_MSG_GetDpmClockFreq,
|
(smu_clk_get_index(smu, clk_id) << 16));
|                if (ret)
|                        return ret;
|
|               ret = smu_read_smc_arg(smu, &freq);
|                if (ret)
|                        return ret;

Same here, but if it's not initialized, we bail out

|        }
|
|       freq *= 100;
|        *value = freq;

And here it gets assigned to *value

|        return ret;
|}

    Arnd

Re: [PATCH 2/2] drm/amd/powerplay: vega20: fix uninitialized variable use

[Alex Deucher]

On Mon, Jul 8, 2019 at 10:08 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> If smu_get_current_rpm() fails, we can't use the output,
> as that may be uninitialized:
>
> drivers/gpu/drm/amd/amdgpu/../powerplay/vega20_ppt.c:3023:8: error: variable 'current_rpm' is used uninitialized whenever '?:' condition is false [-Werror,-Wsometimes-uninitialized]
>         ret = smu_get_current_rpm(smu, &current_rpm);
>               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/amd/amdgpu/../powerplay/inc/amdgpu_smu.h:735:3: note: expanded from macro 'smu_get_current_rpm'
>         ((smu)->funcs->get_current_rpm ? (smu)->funcs->get_current_rpm((smu), (speed)) : 0)
>          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/amd/amdgpu/../powerplay/vega20_ppt.c:3024:12: note: uninitialized use occurs here
>         percent = current_rpm * 100 / pptable->FanMaximumRpm;
>                   ^~~~~~~~~~~
> drivers/gpu/drm/amd/amdgpu/../powerplay/vega20_ppt.c:3023:8: note: remove the '?:' if its condition is always true
>         ret = smu_get_current_rpm(smu, &current_rpm);
>               ^
> drivers/gpu/drm/amd/amdgpu/../powerplay/inc/amdgpu_smu.h:735:3: note: expanded from macro 'smu_get_current_rpm'
>         ((smu)->funcs->get_current_rpm ? (smu)->funcs->get_current_rpm((smu), (speed)) : 0)
>          ^
> drivers/gpu/drm/amd/amdgpu/../powerplay/vega20_ppt.c:3020:22: note: initialize the variable 'current_rpm' to silence this warning
>         uint32_t current_rpm;
>
> Propagate the error code in that case.
>
> Fixes: ee0db82027ee ("drm/amd/powerplay: move PPTable_t uses into asic level")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Applied.  thanks!

Alex

> ---
>  drivers/gpu/drm/amd/powerplay/vega20_ppt.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c
> index 9ce3f1c8ae0f..20d477f8dc84 100644
> --- a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c
> +++ b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c
> @@ -3021,10 +3021,13 @@ static int vega20_get_fan_speed_percent(struct smu_context *smu,
>         PPTable_t *pptable = smu->smu_table.driver_pptable;
>
>         ret = smu_get_current_rpm(smu, &current_rpm);
> +       if (ret)
> +               return ret;
> +
>         percent = current_rpm * 100 / pptable->FanMaximumRpm;
>         *speed = percent > 100 ? 100 : percent;
>
> -       return ret;
> +       return 0;
>  }
>
>  static int vega20_get_gpu_power(struct smu_context *smu, uint32_t *value)
> --
> 2.20.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel