Hi Dan,

I see, I will fix this warning.

Best Regards
Rex

-----Original Message-----
From: Dan Carpenter [mailto:dan.carpen...@oracle.com] 
Sent: Friday, January 08, 2016 4:39 AM
To: Zhu, Rex
Cc: dri-devel at 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

Reply via email to