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

* re: drm/amd/powerplay: show gpu load when print gpu performance for Cz. (v2)
@ 2016-01-07 20:39 Dan Carpenter
  2016-01-08  3:44 ` Zhu, Rex
  2016-01-11  3:51 ` Zhu, Rex
  0 siblings, 2 replies; 4+ messages in thread
From: Dan Carpenter @ 2016-01-07 20:39 UTC (permalink / raw)
  To: Rex.Zhu; +Cc: dri-devel

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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 4+ messages in thread