Hello Rex Zhu,
The patch 605ed21929fe: "drm/amd/powerplay: show gpu load when print gpu performance for Cz. (v2)" from Dec 17, 2015, leads to the following static checker warning:
drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/cz_hwmgr.c:1545 cz_print_current_perforce_level() warn: 'result' can be either negative or positive
drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/cz_hwmgr.c 1535 } 1536 1537 result = smum_send_msg_to_smc(hwmgr->smumgr, PPSMC_MSG_GetAverageGraphicsActivity); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 1538 if (0 == result) { ^^^^^^^^^^^^^^^^^^
smum_send_msg_to_smc() returns either a negative error code or zero or one. There is no documentation. What does it mean when it returns 1?
Btw, Yoda code is just makes it harder to understand for no reason. It triggers a checkpatch.pl warning these days.
I studied this back in 2002 and == vs = bugs were very rare even then. These days the tools are much stricter so normally we get about 2 bugs caused by == vs = typos per year. We might not have had any in 2015?
Let's say you write it as:
if (result = 0)
1) First we get a GCC warning:
CC [M] drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/cz_hwmgr.o drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/cz_hwmgr.c: In function ‘cz_print_current_perforce_level’: drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/cz_hwmgr.c:1538:2: warning: suggest parentheses around assignment used as truth value [-Wparentheses] if (result = 0) {
It's true that some people use extra parenthesis around every condition which disables the GCC warning. Don't do that.
2) Second that code will also trigger a checkpatch.pl warning.
ERROR: do not use assignment in if condition #1538: FILE: drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/cz_hwmgr.c:1538: + if (result = 0) {
It can be easy to miss that warning if your code has a ton of warnings like this driver does. If you care about bugs, you should probably take the energy from writing in Yoda code and use it fix checkpatch.pl warnings instead.
3) Third it triggers a Smatch warning: drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/cz_hwmgr.c:1538 cz_print_current_perforce_level() warn: was '== 0' instead of '='
We miss Smatch warnings for non-x86 arches. So yes it's still possible to have a == vs = bug in 2016 era but it's unlikely and it's sort of your fault if that happens because it means you write messy code to foil GCC, have checkpatch warnings and don't run Smatch. Also testing, I suppose.
1539 active_percent = cgs_read_register(hwmgr->device, mmSMU_MP1_SRBM2P_ARG_0); 1540 active_percent = active_percent > 100 ? 100 : active_percent; 1541 } else { 1542 active_percent = 50; 1543 } 1544 1545 seq_printf(m, "\n [GPU load]: %u %%\n\n", active_percent);
regards, dan carpenter
Hi Dan,
It is (result == 0).
From code, smum_send_msg_to_smc() will return negative a error code or zero.
0 mean success.
Best Regards Rex -----Original Message----- From: Dan Carpenter [mailto:dan.carpenter@oracle.com] Sent: Friday, January 08, 2016 4:39 AM To: Zhu, Rex Cc: dri-devel@lists.freedesktop.org Subject: re: drm/amd/powerplay: show gpu load when print gpu performance for Cz. (v2)
Hello Rex Zhu,
The patch 605ed21929fe: "drm/amd/powerplay: show gpu load when print gpu performance for Cz. (v2)" from Dec 17, 2015, leads to the following static checker warning:
drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/cz_hwmgr.c:1545 cz_print_current_perforce_level() warn: 'result' can be either negative or positive
drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/cz_hwmgr.c 1535 } 1536 1537 result = smum_send_msg_to_smc(hwmgr->smumgr, PPSMC_MSG_GetAverageGraphicsActivity); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 1538 if (0 == result) { ^^^^^^^^^^^^^^^^^^
smum_send_msg_to_smc() returns either a negative error code or zero or one. There is no documentation. What does it mean when it returns 1?
Btw, Yoda code is just makes it harder to understand for no reason. It triggers a checkpatch.pl warning these days.
I studied this back in 2002 and == vs = bugs were very rare even then. These days the tools are much stricter so normally we get about 2 bugs caused by == vs = typos per year. We might not have had any in 2015?
Let's say you write it as:
if (result = 0)
1) First we get a GCC warning:
CC [M] drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/cz_hwmgr.o drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/cz_hwmgr.c: In function ‘cz_print_current_perforce_level’: drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/cz_hwmgr.c:1538:2: warning: suggest parentheses around assignment used as truth value [-Wparentheses] if (result = 0) {
It's true that some people use extra parenthesis around every condition which disables the GCC warning. Don't do that.
2) Second that code will also trigger a checkpatch.pl warning.
ERROR: do not use assignment in if condition #1538: FILE: drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/cz_hwmgr.c:1538: + if (result = 0) {
It can be easy to miss that warning if your code has a ton of warnings like this driver does. If you care about bugs, you should probably take the energy from writing in Yoda code and use it fix checkpatch.pl warnings instead.
3) Third it triggers a Smatch warning: drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/cz_hwmgr.c:1538 cz_print_current_perforce_level() warn: was '== 0' instead of '='
We miss Smatch warnings for non-x86 arches. So yes it's still possible to have a == vs = bug in 2016 era but it's unlikely and it's sort of your fault if that happens because it means you write messy code to foil GCC, have checkpatch warnings and don't run Smatch. Also testing, I suppose.
1539 active_percent = cgs_read_register(hwmgr->device, mmSMU_MP1_SRBM2P_ARG_0); 1540 active_percent = active_percent > 100 ? 100 : active_percent; 1541 } else { 1542 active_percent = 50; 1543 } 1544 1545 seq_printf(m, "\n [GPU load]: %u %%\n\n", active_percent);
regards, dan carpenter
On Fri, Jan 08, 2016 at 03:44:51AM +0000, Zhu, Rex wrote:
Hi Dan,
It is (result == 0).
From code, smum_send_msg_to_smc() will return negative a error code or zero.
0 mean success.
Hm.. I have investigated more and the problem is typos in tonga_send_msg_to_smc().
drivers/gpu/drm/amd/powerplay/smumgr/tonga_smumgr.c 183 /** 184 * Send a message to the SMC, and wait for its response. 185 * 186 * @param smumgr the address of the powerplay hardware manager. 187 * @param msg the message to send. 188 * @return The response that came from the SMC. 189 */ 190 static int tonga_send_msg_to_smc(struct pp_smumgr *smumgr, uint16_t msg) 191 { 192 if (smumgr == NULL || smumgr->device == NULL) 193 return -EINVAL; 194 195 if (!tonga_is_smc_ram_running(smumgr)) 196 return -1; 197 198 SMUM_WAIT_FIELD_UNEQUAL(smumgr, SMC_RESP_0, SMC_RESP, 0); 199 PP_ASSERT_WITH_CODE( 200 1 == SMUM_READ_FIELD(smumgr->device, SMC_RESP_0, SMC_RESP), 201 "Failed to send Previous Message.", 202 return 1); ^^^^^^^^ We intended to return -EINVAL here. (returning -1 means -EPERM but it's sloppy to use -1 instead of -EPERM, -1 is never a valid error code).
203 204 cgs_write_register(smumgr->device, mmSMC_MESSAGE_0, msg); 205 206 SMUM_WAIT_FIELD_UNEQUAL(smumgr, SMC_RESP_0, SMC_RESP, 0); 207 PP_ASSERT_WITH_CODE( 208 1 == SMUM_READ_FIELD(smumgr->device, SMC_RESP_0, SMC_RESP), 209 "Failed to send Message.", 210 return 1); ^^^^^^^^
return -EINVAL;
211 212 return 0; 213 }
regards, dan carpenter
Hi Dan,
I see, I will fix this warning.
Best Regards Rex
-----Original Message----- From: Dan Carpenter [mailto:dan.carpenter@oracle.com] Sent: Friday, January 08, 2016 4:39 AM To: Zhu, Rex Cc: dri-devel@lists.freedesktop.org Subject: re: drm/amd/powerplay: show gpu load when print gpu performance for Cz. (v2)
Hello Rex Zhu,
The patch 605ed21929fe: "drm/amd/powerplay: show gpu load when print gpu performance for Cz. (v2)" from Dec 17, 2015, leads to the following static checker warning:
drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/cz_hwmgr.c:1545 cz_print_current_perforce_level() warn: 'result' can be either negative or positive
drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/cz_hwmgr.c 1535 } 1536 1537 result = smum_send_msg_to_smc(hwmgr->smumgr, PPSMC_MSG_GetAverageGraphicsActivity); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 1538 if (0 == result) { ^^^^^^^^^^^^^^^^^^
smum_send_msg_to_smc() returns either a negative error code or zero or one. There is no documentation. What does it mean when it returns 1?
Btw, Yoda code is just makes it harder to understand for no reason. It triggers a checkpatch.pl warning these days.
I studied this back in 2002 and == vs = bugs were very rare even then. These days the tools are much stricter so normally we get about 2 bugs caused by == vs = typos per year. We might not have had any in 2015?
Let's say you write it as:
if (result = 0)
1) First we get a GCC warning:
CC [M] drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/cz_hwmgr.o drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/cz_hwmgr.c: In function ‘cz_print_current_perforce_level’: drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/cz_hwmgr.c:1538:2: warning: suggest parentheses around assignment used as truth value [-Wparentheses] if (result = 0) {
It's true that some people use extra parenthesis around every condition which disables the GCC warning. Don't do that.
2) Second that code will also trigger a checkpatch.pl warning.
ERROR: do not use assignment in if condition #1538: FILE: drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/cz_hwmgr.c:1538: + if (result = 0) {
It can be easy to miss that warning if your code has a ton of warnings like this driver does. If you care about bugs, you should probably take the energy from writing in Yoda code and use it fix checkpatch.pl warnings instead.
3) Third it triggers a Smatch warning: drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/cz_hwmgr.c:1538 cz_print_current_perforce_level() warn: was '== 0' instead of '='
We miss Smatch warnings for non-x86 arches. So yes it's still possible to have a == vs = bug in 2016 era but it's unlikely and it's sort of your fault if that happens because it means you write messy code to foil GCC, have checkpatch warnings and don't run Smatch. Also testing, I suppose.
1539 active_percent = cgs_read_register(hwmgr->device, mmSMU_MP1_SRBM2P_ARG_0); 1540 active_percent = active_percent > 100 ? 100 : active_percent; 1541 } else { 1542 active_percent = 50; 1543 } 1544 1545 seq_printf(m, "\n [GPU load]: %u %%\n\n", active_percent);
regards, dan carpenter
dri-devel@lists.freedesktop.org
-
Dan Carpenter -
Zhu, Rex