drm/amd/powerplay: show gpu load when print gpu performance for Cz. (v2)

[Zhu, Rex]

Hi Dan,

I see, I will fix this warning.

Best Regards
Rex

-----Original Message-----
From: Dan Carpenter [mailto:dan.carpenter at 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