Hi Dan, I see, I will fix this warning. Best Regards Rex -----Original Message----- From: Dan Carpenter [mailto:dan.carpenter@xxxxxxxxxx] Sent: Friday, January 08, 2016 4:39 AM To: Zhu, Rex Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx 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 mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
