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@gmail.com --- 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;
Thanks Nathan. The patch is reviewed-by: Evan Quan evan.quan@amd.com
-----Original Message----- From: Nathan Chancellor natechancellor@gmail.com Sent: Monday, August 05, 2019 4:37 AM To: Quan, Evan Evan.Quan@amd.com; Deucher, Alexander Alexander.Deucher@amd.com; Koenig, Christian Christian.Koenig@amd.com; Zhou, David(ChunMing) David1.Zhou@amd.com Cc: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux- kernel@vger.kernel.org; clang-built-linux@googlegroups.com; Nathan Chancellor natechancellor@gmail.com 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:
Assume the function pointer will never be NULL and eliminate the wrapper macros.
Have the wrapper macros initialize arg when the function pointer is NULL.
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.
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@gmail.com
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
Applied. Thanks!
Alex
On Sun, Aug 4, 2019 at 9:21 PM Quan, Evan Evan.Quan@amd.com wrote:
Thanks Nathan. The patch is reviewed-by: Evan Quan evan.quan@amd.com
-----Original Message----- From: Nathan Chancellor natechancellor@gmail.com Sent: Monday, August 05, 2019 4:37 AM To: Quan, Evan Evan.Quan@amd.com; Deucher, Alexander Alexander.Deucher@amd.com; Koenig, Christian Christian.Koenig@amd.com; Zhou, David(ChunMing) David1.Zhou@amd.com Cc: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux- kernel@vger.kernel.org; clang-built-linux@googlegroups.com; Nathan Chancellor natechancellor@gmail.com 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:
Assume the function pointer will never be NULL and eliminate the wrapper macros.
Have the wrapper macros initialize arg when the function pointer is NULL.
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.
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@gmail.com
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
amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Nathan Chancellor -
Quan, Evan