On 8/12/20 2:43 PM, StDenis, Tom wrote:
[AMD Official Use Only - Internal Distribution Only] Possibly, but since the arcturus_get_smu_metrics_data() can error out we should check that return value no?
Yes, we need that return check.
(also setting *value to 0 avoids this bug in the future...).
I think caller should initialize the result value before passing it to arcturus_get_smu_metrics_data
as the warning is generated from the calling function. My comment is only concerning about setting "value"
to 0, which seems wrong. Rest of the patch is fine. Nirmoy
Tom ________________________________________ From: Das, Nirmoy <Nirmoy.Das@xxxxxxx> Sent: Wednesday, August 12, 2020 08:40 To: StDenis, Tom; amd-gfx@xxxxxxxxxxxxxxxxxxxxx Subject: Re: [PATCH] drm/amd/powerplay: Fix uninitialized warning in arcturus ppt driver On 8/12/20 2:20 PM, Tom St Denis wrote:Fixes: CC [M] drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm_mst_types.o drivers/gpu/drm/amd/amdgpu/../powerplay/arcturus_ppt.c: In function ‘arcturus_log_thermal_throttling_event’: drivers/gpu/drm/amd/amdgpu/../powerplay/arcturus_ppt.c:2223:24: warning: ‘throttler_status’ may be used uninitialized in this function [-Wmaybe-uninitialized] 2223 | if (throttler_status & logging_label[throttler_idx].feature_mask) { by making arcturus_get_smu_metrics_data() assign a default value (of zero) before any possible return point as well as simply error out of arcturus_log_thermal_throttling_event() if it fails. Signed-off-by: Tom St Denis <tom.stdenis@xxxxxxx> --- drivers/gpu/drm/amd/powerplay/arcturus_ppt.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c index 8b1025dc54fd..78f7ec95e4f5 100644 --- a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c +++ b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c @@ -551,6 +551,9 @@ static int arcturus_get_smu_metrics_data(struct smu_context *smu, mutex_lock(&smu->metrics_lock); + // assign default value + *value = 0; + ret = smu_cmn_get_metrics_table_locked(smu, NULL, false); @@ -2208,15 +2211,20 @@ static const struct throttling_logging_label { }; static void arcturus_log_thermal_throttling_event(struct smu_context *smu) { - int throttler_idx, throtting_events = 0, buf_idx = 0; + int throttler_idx, throtting_events = 0, buf_idx = 0, ret; struct amdgpu_device *adev = smu->adev; uint32_t throttler_status;I think initializing throttler_status here should resolve the warning.char log_buf[256]; - arcturus_get_smu_metrics_data(smu, + ret = arcturus_get_smu_metrics_data(smu, METRICS_THROTTLER_STATUS, &throttler_status); + if (ret) { + dev_err(adev->dev, "Could not read from arcturus_get_smu_metrics_data()\n"); + return; + } + memset(log_buf, 0, sizeof(log_buf)); for (throttler_idx = 0; throttler_idx < ARRAY_SIZE(logging_label); throttler_idx++) {
_______________________________________________ amd-gfx mailing list amd-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx
