Cc'ing Dan Carpenter
On Fri, Oct 01, 2021 at 12:57:13PM +0300, Jani Nikula wrote:
On Fri, 01 Oct 2021, Chris Wilson chris@chris-wilson.co.uk wrote:
Quoting Lucas De Marchi (2021-10-01 08:40:41)
When trying to bring IS_ACTIVE to linux/kconfig.h I thought it wouldn't provide much value just encapsulating it in a boolean context. So I also added the support for handling undefined macros as the IS_ENABLED() counterpart. However the feedback received from Masahiro Yamada was that it is too ugly, not providing much value. And just wrapping in a boolean context is too dumb - we could simply open code it.
As detailed in commit babaab2f4738 ("drm/i915: Encapsulate kconfig constant values inside boolean predicates"), the IS_ACTIVE macro was added to workaround a compilation warning. However after checking again our current uses of IS_ACTIVE it turned out there is only 1 case in which it would potentially trigger a warning. All the others can simply use the shorter version, without wrapping it in any macro. And even that single one didn't trigger any warning in gcc 10.3.
So here I'm dialing all the way back to simply removing the macro. If it triggers warnings in future we may change the few cases to check for > 0 or != 0. Another possibility would be to use the great "not not operator" for all positive checks, which would allow us to maintain consistency. However let's try first the simplest form though, hopefully we don't hit broken compilers spitting a warning:
You didn't prevent the compilation warning this re-introduces.
drivers/gpu/drm/i915/i915_config.c:11 i915_fence_context_timeout() warn: should this be a bitwise op? drivers/gpu/drm/i915/i915_request.c:1679 i915_request_wait() warn: should this be a bitwise op?
Looks like that's a Smatch warning. The immediate fix would be to just add the != 0 in the relevant places. But this is stuff that's just going to get broken again unless we add Smatch to CI. Most people aren't running it on a regular basis.
clang gives a warning only in drivers/gpu/drm/i915/i915_config.c and the warning is gone if the condition swapped:
- if (context && CONFIG_DRM_I915_FENCE_TIMEOUT) + if (CONFIG_DRM_I915_FENCE_TIMEOUT && context)
which would make sense if we think about shortcutting the if condition. However smatch still reports the warning and an additional one in drivers/gpu/drm/i915/i915_request.c. The ways I found to stop the false positives with smatch are:
if (context && CONFIG_DRM_I915_FENCE_TIMEOUT != 0) or if (context && !!CONFIG_DRM_I915_FENCE_TIMEOUT) or if (context && CONFIG_DRM_I915_FENCE_TIMEOUT > 0)
Dan, anything else that we could do here? This is about this kind of code:
f (context && CONFIG_DRM_I915_FENCE_TIMEOUT)
in which context is a u64 variable, that gives this warning:
drivers/gpu/drm/i915/i915_config.c:11 i915_fence_context_timeout() warn: should this be a bitwise op?
thanks Lucas De Marchi
BR, Jani.
-- Jani Nikula, Intel Open Source Graphics Center