Re: [Intel-gfx] [PATCH] drm/i915: remove IS_ACTIVE

[Lucas De Marchi <lucas.demarchi@intel.com>]

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