Re: [Intel-gfx] [PATCH] drm/i915: remove IS_ACTIVE
On Tue, Oct 05, 2021 at 09:19:39AM +0300, Dan Carpenter wrote:
On Mon, Oct 04, 2021 at 01:52:27PM -0700, Lucas De Marchi wrote:
Cc'ing Dan Carpenter
On Fri, Oct 01, 2021 at 12:57:13PM +0300, Jani Nikula wrote:
> On Fri, 01 Oct 2021, Chris Wilson 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.
I would really prefer that instead of ensuring that code doesn't
generate Smatch warnings, people just look over the warnings and then
mass mark them all as false positives and never look at them again.
It let's us warn about more complicated things without worrying so much
about being perfect. When code is fresh in your head then warnings are
not a big deal to review and you want to warn about every possible issue
After a year then they take forever and so you really want them to be
correct or it's a huge waste of time. I'd prefer Smatch live in the
space where people run it when the code is fresh.
You would have received some automated emails about this Smatch warning
but I look over the zero day output and filter the results.
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)
I like this rule that when the constant is on the left it's not a mask.
That makes sense. I will add that.
thanks, that would be great, so we can really get rid of the macro by
sticking this rule since it works for smatch and clang (and gcc doesn't
give this warning).
thanks
Lucas De Marchi
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)
I guess I prefer the first and third but I'll add the rule that Clang
uses to silence the warning.
regards,
dan carpenter
Re: [Intel-gfx] [PATCH] drm/i915: remove IS_ACTIVE
On Mon, Oct 04, 2021 at 01:52:27PM -0700, Lucas De Marchi wrote:
> Cc'ing Dan Carpenter
>
> On Fri, Oct 01, 2021 at 12:57:13PM +0300, Jani Nikula wrote:
> > On Fri, 01 Oct 2021, Chris Wilson 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.
I would really prefer that instead of ensuring that code doesn't
generate Smatch warnings, people just look over the warnings and then
mass mark them all as false positives and never look at them again.
It let's us warn about more complicated things without worrying so much
about being perfect. When code is fresh in your head then warnings are
not a big deal to review and you want to warn about every possible issue
After a year then they take forever and so you really want them to be
correct or it's a huge waste of time. I'd prefer Smatch live in the
space where people run it when the code is fresh.
You would have received some automated emails about this Smatch warning
but I look over the zero day output and filter the results.
>
> 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)
I like this rule that when the constant is on the left it's not a mask.
That makes sense. I will add that.
>
> 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)
>
I guess I prefer the first and third but I'll add the rule that Clang
uses to silence the warning.
regards,
dan carpenter
Re: [Intel-gfx] [PATCH] drm/i915: remove IS_ACTIVE
Cc'ing Dan Carpenter
On Fri, Oct 01, 2021 at 12:57:13PM +0300, Jani Nikula wrote:
On Fri, 01 Oct 2021, Chris Wilson 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
Re: [Intel-gfx] [PATCH] drm/i915: remove IS_ACTIVE
On Fri, Oct 01, 2021 at 12:57:13PM +0300, Jani Nikula wrote:
On Fri, 01 Oct 2021, Chris Wilson 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.
it looks like the warning is also triggered with clang an as per
0day test. What if we change all the positive caes to use !!? That
would make it consistent and IMO not as ugly as the != 0.
what about this?
-8<
diff --git a/drivers/gpu/drm/i915/i915_config.c
b/drivers/gpu/drm/i915/i915_config.c
index ad228dd96a0b..0df3efa2e72d 100644
--- a/drivers/gpu/drm/i915/i915_config.c
+++ b/drivers/gpu/drm/i915/i915_config.c
@@ -8,7 +8,7 @@
unsigned long
i915_fence_context_timeout(const struct drm_i915_private *i915, u64 context)
{
- if (context && CONFIG_DRM_I915_FENCE_TIMEOUT)
+ if (context && !!CONFIG_DRM_I915_FENCE_TIMEOUT)
return msecs_to_jiffies_timeout(CONFIG_DRM_I915_FENCE_TIMEOUT);
return 0;
-8<
Lucas De Marchi
Re: [Intel-gfx] [PATCH] drm/i915: remove IS_ACTIVE
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?
-Chris
Re: [Intel-gfx] [PATCH] drm/i915: remove IS_ACTIVE
On Fri, 01 Oct 2021, Chris Wilson 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.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
