RE: [PATCH] drm/amd/powerplay: Zero initialize some variables

[Quan, Evan]

Thanks Nathan. The patch is reviewed-by: Evan Quan <evan.quan@xxxxxxx>

> -----Original Message-----
> From: Nathan Chancellor <natechancellor@xxxxxxxxx>
> Sent: Monday, August 05, 2019 4:37 AM
> To: Quan, Evan <Evan.Quan@xxxxxxx>; Deucher, Alexander
> <Alexander.Deucher@xxxxxxx>; Koenig, Christian
> <Christian.Koenig@xxxxxxx>; Zhou, David(ChunMing)
> <David1.Zhou@xxxxxxx>
> Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; clang-built-linux@xxxxxxxxxxxxxxxx; Nathan
> Chancellor <natechancellor@xxxxxxxxx>
> Subject: [PATCH] drm/amd/powerplay: Zero initialize some variables
> 
> Clang warns (only Navi warning shown but Arcturus warns as well):
> 
> drivers/gpu/drm/amd/amdgpu/../powerplay/navi10_ppt.c:1534:4: warning:
> variable 'asic_default_power_limit' is used uninitialized whenever '?:'
> condition is false [-Wsometimes-uninitialized]
>                         smu_read_smc_arg(smu, &asic_default_power_limit);
>                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/amd/amdgpu/../powerplay/inc/amdgpu_smu.h:588:3:
> note:
> expanded from macro 'smu_read_smc_arg'
>         ((smu)->funcs->read_smc_arg? (smu)->funcs->read_smc_arg((smu),
> (arg)) : 0)
>          ^~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/amd/amdgpu/../powerplay/navi10_ppt.c:1550:30: note:
> uninitialized use occurs here
>                 smu->default_power_limit = asic_default_power_limit;
>                                            ^~~~~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/amd/amdgpu/../powerplay/navi10_ppt.c:1534:4: note:
> remove the '?:' if its condition is always true
>                         smu_read_smc_arg(smu, &asic_default_power_limit);
>                         ^
> drivers/gpu/drm/amd/amdgpu/../powerplay/inc/amdgpu_smu.h:588:3:
> note:
> expanded from macro 'smu_read_smc_arg'
>         ((smu)->funcs->read_smc_arg? (smu)->funcs->read_smc_arg((smu),
> (arg)) : 0)
>          ^
> drivers/gpu/drm/amd/amdgpu/../powerplay/navi10_ppt.c:1517:35: note:
> initialize the variable 'asic_default_power_limit' to silence this warning
>         uint32_t asic_default_power_limit;
>                                          ^
>                                           = 0
> 1 warning generated.
> 
> As the code is currently written, if read_smc_arg were ever NULL, arg would
> fail to be initialized but the code would continue executing as normal
> because the return value would just be zero.
> 
> There are a few different possible solutions to resolve this class of warnings
> which have appeared in these drivers before:
> 
> 1. Assume the function pointer will never be NULL and eliminate the
>    wrapper macros.
> 
> 2. Have the wrapper macros initialize arg when the function pointer is
>    NULL.
> 
> 3. Have the wrapper macros return an error code instead of 0 when the
>    function pointer is NULL so that the callsites can properly bail out
>    before arg can be used.
> 
> 4. Initialize arg at the top of its function.
> 
> Number four is the path of least resistance right now as every other change
> will be driver wide so do that here. I only make the comment now as food for
> thought.
> 
> Fixes: b4af964e75c4 ("drm/amd/powerplay: make power limit retrieval as
> asic specific")
> Link: https://github.com/ClangBuiltLinux/linux/issues/627
> Signed-off-by: Nathan Chancellor <natechancellor@xxxxxxxxx>
> ---
>  drivers/gpu/drm/amd/powerplay/arcturus_ppt.c | 2 +-
>  drivers/gpu/drm/amd/powerplay/navi10_ppt.c   | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> index 215f7173fca8..b92eded7374f 100644
> --- a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> +++ b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> @@ -1326,7 +1326,7 @@ static int arcturus_get_power_limit(struct
> smu_context *smu,
>  				     bool asic_default)
>  {
>  	PPTable_t *pptable = smu->smu_table.driver_pptable;
> -	uint32_t asic_default_power_limit;
> +	uint32_t asic_default_power_limit = 0;
>  	int ret = 0;
>  	int power_src;
> 
> diff --git a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
> b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
> index 106352a4fb82..d844bc8411aa 100644
> --- a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
> +++ b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
> @@ -1514,7 +1514,7 @@ static int navi10_get_power_limit(struct
> smu_context *smu,
>  				     bool asic_default)
>  {
>  	PPTable_t *pptable = smu->smu_table.driver_pptable;
> -	uint32_t asic_default_power_limit;
> +	uint32_t asic_default_power_limit = 0;
>  	int ret = 0;
>  	int power_src;
> 
> --
> 2.23.0.rc1