| Message ID | 20211001074041.2076538-1-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: remove IS_ACTIVE | expand |
On Fri, 01 Oct 2021, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > 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: > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> Acked-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_context.c | 2 +- > drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +- > drivers/gpu/drm/i915/gt/intel_engine.h | 4 ++-- > drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c | 2 +- > drivers/gpu/drm/i915/gt/intel_engine_types.h | 2 +- > .../gpu/drm/i915/gt/intel_execlists_submission.c | 2 +- > .../gpu/drm/i915/gt/selftest_engine_heartbeat.c | 4 ++-- > drivers/gpu/drm/i915/gt/selftest_execlists.c | 14 +++++++------- > drivers/gpu/drm/i915/i915_config.c | 2 +- > drivers/gpu/drm/i915/i915_request.c | 2 +- > drivers/gpu/drm/i915/i915_utils.h | 13 ------------- > 11 files changed, 18 insertions(+), 31 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c > index 8208fd5b72c3..be60bcf8069c 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c > @@ -761,7 +761,7 @@ static int intel_context_set_gem(struct intel_context *ce, > intel_engine_has_semaphores(ce->engine)) > __set_bit(CONTEXT_USE_SEMAPHORES, &ce->flags); > > - if (IS_ACTIVE(CONFIG_DRM_I915_REQUEST_TIMEOUT) && > + if (CONFIG_DRM_I915_REQUEST_TIMEOUT && > ctx->i915->params.request_timeout_ms) { > unsigned int timeout_ms = ctx->i915->params.request_timeout_ms; > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c > index 5130e8ed9564..65fc6ff5f59d 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c > @@ -395,7 +395,7 @@ static vm_fault_t vm_fault_gtt(struct vm_fault *vmf) > /* Track the mmo associated with the fenced vma */ > vma->mmo = mmo; > > - if (IS_ACTIVE(CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND)) > + if (CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND) > intel_wakeref_auto(&i915->ggtt.userfault_wakeref, > msecs_to_jiffies_timeout(CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND)); > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h > index 87579affb952..6aba239a10e8 100644 > --- a/drivers/gpu/drm/i915/gt/intel_engine.h > +++ b/drivers/gpu/drm/i915/gt/intel_engine.h > @@ -273,7 +273,7 @@ static inline bool intel_engine_uses_guc(const struct intel_engine_cs *engine) > static inline bool > intel_engine_has_preempt_reset(const struct intel_engine_cs *engine) > { > - if (!IS_ACTIVE(CONFIG_DRM_I915_PREEMPT_TIMEOUT)) > + if (!CONFIG_DRM_I915_PREEMPT_TIMEOUT) > return false; > > return intel_engine_has_preemption(engine); > @@ -300,7 +300,7 @@ intel_virtual_engine_has_heartbeat(const struct intel_engine_cs *engine) > static inline bool > intel_engine_has_heartbeat(const struct intel_engine_cs *engine) > { > - if (!IS_ACTIVE(CONFIG_DRM_I915_HEARTBEAT_INTERVAL)) > + if (!CONFIG_DRM_I915_HEARTBEAT_INTERVAL) > return false; > > if (intel_engine_is_virtual(engine)) > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c > index 74775ae961b2..a3698f611f45 100644 > --- a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c > +++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c > @@ -207,7 +207,7 @@ static void heartbeat(struct work_struct *wrk) > > void intel_engine_unpark_heartbeat(struct intel_engine_cs *engine) > { > - if (!IS_ACTIVE(CONFIG_DRM_I915_HEARTBEAT_INTERVAL)) > + if (!CONFIG_DRM_I915_HEARTBEAT_INTERVAL) > return; > > next_heartbeat(engine); > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h > index 5ae1207c363b..9167ce52487c 100644 > --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h > +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h > @@ -556,7 +556,7 @@ intel_engine_has_semaphores(const struct intel_engine_cs *engine) > static inline bool > intel_engine_has_timeslices(const struct intel_engine_cs *engine) > { > - if (!IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION)) > + if (!CONFIG_DRM_I915_TIMESLICE_DURATION) > return false; > > return engine->flags & I915_ENGINE_HAS_TIMESLICES; > diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c > index 7147fe80919e..73a79c2acd3a 100644 > --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c > +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c > @@ -3339,7 +3339,7 @@ logical_ring_default_vfuncs(struct intel_engine_cs *engine) > engine->flags |= I915_ENGINE_HAS_SEMAPHORES; > if (can_preempt(engine)) { > engine->flags |= I915_ENGINE_HAS_PREEMPTION; > - if (IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION)) > + if (CONFIG_DRM_I915_TIMESLICE_DURATION) > engine->flags |= I915_ENGINE_HAS_TIMESLICES; > } > } > diff --git a/drivers/gpu/drm/i915/gt/selftest_engine_heartbeat.c b/drivers/gpu/drm/i915/gt/selftest_engine_heartbeat.c > index 317eebf086c3..6e6e4d747cca 100644 > --- a/drivers/gpu/drm/i915/gt/selftest_engine_heartbeat.c > +++ b/drivers/gpu/drm/i915/gt/selftest_engine_heartbeat.c > @@ -290,7 +290,7 @@ static int live_heartbeat_fast(void *arg) > int err = 0; > > /* Check that the heartbeat ticks at the desired rate. */ > - if (!IS_ACTIVE(CONFIG_DRM_I915_HEARTBEAT_INTERVAL)) > + if (!CONFIG_DRM_I915_HEARTBEAT_INTERVAL) > return 0; > > for_each_engine(engine, gt, id) { > @@ -352,7 +352,7 @@ static int live_heartbeat_off(void *arg) > int err = 0; > > /* Check that we can turn off heartbeat and not interrupt VIP */ > - if (!IS_ACTIVE(CONFIG_DRM_I915_HEARTBEAT_INTERVAL)) > + if (!CONFIG_DRM_I915_HEARTBEAT_INTERVAL) > return 0; > > for_each_engine(engine, gt, id) { > diff --git a/drivers/gpu/drm/i915/gt/selftest_execlists.c b/drivers/gpu/drm/i915/gt/selftest_execlists.c > index b3863abc51f5..25a8c4f62b0d 100644 > --- a/drivers/gpu/drm/i915/gt/selftest_execlists.c > +++ b/drivers/gpu/drm/i915/gt/selftest_execlists.c > @@ -992,7 +992,7 @@ static int live_timeslice_preempt(void *arg) > * need to preempt the current task and replace it with another > * ready task. > */ > - if (!IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION)) > + if (!CONFIG_DRM_I915_TIMESLICE_DURATION) > return 0; > > obj = i915_gem_object_create_internal(gt->i915, PAGE_SIZE); > @@ -1122,7 +1122,7 @@ static int live_timeslice_rewind(void *arg) > * but only a few of those requests, forcing us to rewind the > * RING_TAIL of the original request. > */ > - if (!IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION)) > + if (!CONFIG_DRM_I915_TIMESLICE_DURATION) > return 0; > > for_each_engine(engine, gt, id) { > @@ -1299,7 +1299,7 @@ static int live_timeslice_queue(void *arg) > * ELSP[1] is already occupied, so must rely on timeslicing to > * eject ELSP[0] in favour of the queue.) > */ > - if (!IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION)) > + if (!CONFIG_DRM_I915_TIMESLICE_DURATION) > return 0; > > obj = i915_gem_object_create_internal(gt->i915, PAGE_SIZE); > @@ -1420,7 +1420,7 @@ static int live_timeslice_nopreempt(void *arg) > * We should not timeslice into a request that is marked with > * I915_REQUEST_NOPREEMPT. > */ > - if (!IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION)) > + if (!CONFIG_DRM_I915_TIMESLICE_DURATION) > return 0; > > if (igt_spinner_init(&spin, gt)) > @@ -2260,7 +2260,7 @@ static int __cancel_hostile(struct live_preempt_cancel *arg) > int err; > > /* Preempt cancel non-preemptible spinner in ELSP0 */ > - if (!IS_ACTIVE(CONFIG_DRM_I915_PREEMPT_TIMEOUT)) > + if (!CONFIG_DRM_I915_PREEMPT_TIMEOUT) > return 0; > > if (!intel_has_reset_engine(arg->engine->gt)) > @@ -2316,7 +2316,7 @@ static int __cancel_fail(struct live_preempt_cancel *arg) > struct i915_request *rq; > int err; > > - if (!IS_ACTIVE(CONFIG_DRM_I915_PREEMPT_TIMEOUT)) > + if (!CONFIG_DRM_I915_PREEMPT_TIMEOUT) > return 0; > > if (!intel_has_reset_engine(engine->gt)) > @@ -3375,7 +3375,7 @@ static int live_preempt_timeout(void *arg) > * Check that we force preemption to occur by cancelling the previous > * context if it refuses to yield the GPU. > */ > - if (!IS_ACTIVE(CONFIG_DRM_I915_PREEMPT_TIMEOUT)) > + if (!CONFIG_DRM_I915_PREEMPT_TIMEOUT) > return 0; > > if (!intel_has_reset_engine(gt)) > diff --git a/drivers/gpu/drm/i915/i915_config.c b/drivers/gpu/drm/i915/i915_config.c > index b79b5f6d2cfa..ad228dd96a0b 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 && IS_ACTIVE(CONFIG_DRM_I915_FENCE_TIMEOUT)) > + if (context && CONFIG_DRM_I915_FENCE_TIMEOUT) > return msecs_to_jiffies_timeout(CONFIG_DRM_I915_FENCE_TIMEOUT); > > return 0; > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c > index 79da5eca60af..91bd6f4e9909 100644 > --- a/drivers/gpu/drm/i915/i915_request.c > +++ b/drivers/gpu/drm/i915/i915_request.c > @@ -1852,7 +1852,7 @@ long i915_request_wait(struct i915_request *rq, > * completion. That requires having a good predictor for the request > * duration, which we currently lack. > */ > - if (IS_ACTIVE(CONFIG_DRM_I915_MAX_REQUEST_BUSYWAIT) && > + if (CONFIG_DRM_I915_MAX_REQUEST_BUSYWAIT && > __i915_spin_request(rq, state)) > goto out; > > diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h > index 5259edacde38..62f189e064a9 100644 > --- a/drivers/gpu/drm/i915/i915_utils.h > +++ b/drivers/gpu/drm/i915/i915_utils.h > @@ -458,17 +458,4 @@ static inline bool timer_expired(const struct timer_list *t) > return timer_active(t) && !timer_pending(t); > } > > -/* > - * This is a lookalike for IS_ENABLED() that takes a kconfig value, > - * e.g. CONFIG_DRM_I915_SPIN_REQUEST, and evaluates whether it is non-zero > - * i.e. whether the configuration is active. Wrapping up the config inside > - * a boolean context prevents clang and smatch from complaining about potential > - * issues in confusing logical-&& with bitwise-& for constants. > - * > - * Sadly IS_ENABLED() itself does not work with kconfig values. > - * > - * Returns 0 if @config is 0, 1 if set to any value. > - */ > -#define IS_ACTIVE(config) ((config) != 0) > - > #endif /* !__I915_UTILS_H */
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
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. BR, Jani.
Hi Lucas,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on drm-tip/drm-tip drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next v5.15-rc3 next-20210922]
[cannot apply to airlied/drm-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Lucas-De-Marchi/drm-i915-remove-IS_ACTIVE/20211001-154226
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-a015-20211001 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 962e503cc8bc411f7523cc393acae8aae425b1c4)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/50006042f1d264599bd1be1942f9958112e15c01
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Lucas-De-Marchi/drm-i915-remove-IS_ACTIVE/20211001-154226
git checkout 50006042f1d264599bd1be1942f9958112e15c01
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/i915/i915_config.c:11:14: warning: use of logical '&&' with constant operand [-Wconstant-logical-operand]
if (context && CONFIG_DRM_I915_FENCE_TIMEOUT)
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/i915/i915_config.c:11:14: note: use '&' for a bitwise operation
if (context && CONFIG_DRM_I915_FENCE_TIMEOUT)
^~
&
drivers/gpu/drm/i915/i915_config.c:11:14: note: remove constant to silence this warning
if (context && CONFIG_DRM_I915_FENCE_TIMEOUT)
~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
vim +11 drivers/gpu/drm/i915/i915_config.c
7
8 unsigned long
9 i915_fence_context_timeout(const struct drm_i915_private *i915, u64 context)
10 {
> 11 if (context && CONFIG_DRM_I915_FENCE_TIMEOUT)
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
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. 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
Hi Lucas,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on drm-tip/drm-tip drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next v5.15-rc3 next-20210823]
[cannot apply to airlied/drm-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Lucas-De-Marchi/drm-i915-remove-IS_ACTIVE/20211001-154226
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-r024-20211001 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 962e503cc8bc411f7523cc393acae8aae425b1c4)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/50006042f1d264599bd1be1942f9958112e15c01
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Lucas-De-Marchi/drm-i915-remove-IS_ACTIVE/20211001-154226
git checkout 50006042f1d264599bd1be1942f9958112e15c01
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> drivers/gpu/drm/i915/i915_config.c:11:14: error: use of logical '&&' with constant operand [-Werror,-Wconstant-logical-operand]
if (context && CONFIG_DRM_I915_FENCE_TIMEOUT)
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/i915/i915_config.c:11:14: note: use '&' for a bitwise operation
if (context && CONFIG_DRM_I915_FENCE_TIMEOUT)
^~
&
drivers/gpu/drm/i915/i915_config.c:11:14: note: remove constant to silence this warning
if (context && CONFIG_DRM_I915_FENCE_TIMEOUT)
~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
vim +11 drivers/gpu/drm/i915/i915_config.c
7
8 unsigned long
9 i915_fence_context_timeout(const struct drm_i915_private *i915, u64 context)
10 {
> 11 if (context && CONFIG_DRM_I915_FENCE_TIMEOUT)
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
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
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 <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. 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
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 <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. > >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 >
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index 8208fd5b72c3..be60bcf8069c 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -761,7 +761,7 @@ static int intel_context_set_gem(struct intel_context *ce, intel_engine_has_semaphores(ce->engine)) __set_bit(CONTEXT_USE_SEMAPHORES, &ce->flags); - if (IS_ACTIVE(CONFIG_DRM_I915_REQUEST_TIMEOUT) && + if (CONFIG_DRM_I915_REQUEST_TIMEOUT && ctx->i915->params.request_timeout_ms) { unsigned int timeout_ms = ctx->i915->params.request_timeout_ms; diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c index 5130e8ed9564..65fc6ff5f59d 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c @@ -395,7 +395,7 @@ static vm_fault_t vm_fault_gtt(struct vm_fault *vmf) /* Track the mmo associated with the fenced vma */ vma->mmo = mmo; - if (IS_ACTIVE(CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND)) + if (CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND) intel_wakeref_auto(&i915->ggtt.userfault_wakeref, msecs_to_jiffies_timeout(CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND)); diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h index 87579affb952..6aba239a10e8 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine.h +++ b/drivers/gpu/drm/i915/gt/intel_engine.h @@ -273,7 +273,7 @@ static inline bool intel_engine_uses_guc(const struct intel_engine_cs *engine) static inline bool intel_engine_has_preempt_reset(const struct intel_engine_cs *engine) { - if (!IS_ACTIVE(CONFIG_DRM_I915_PREEMPT_TIMEOUT)) + if (!CONFIG_DRM_I915_PREEMPT_TIMEOUT) return false; return intel_engine_has_preemption(engine); @@ -300,7 +300,7 @@ intel_virtual_engine_has_heartbeat(const struct intel_engine_cs *engine) static inline bool intel_engine_has_heartbeat(const struct intel_engine_cs *engine) { - if (!IS_ACTIVE(CONFIG_DRM_I915_HEARTBEAT_INTERVAL)) + if (!CONFIG_DRM_I915_HEARTBEAT_INTERVAL) return false; if (intel_engine_is_virtual(engine)) diff --git a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c index 74775ae961b2..a3698f611f45 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c @@ -207,7 +207,7 @@ static void heartbeat(struct work_struct *wrk) void intel_engine_unpark_heartbeat(struct intel_engine_cs *engine) { - if (!IS_ACTIVE(CONFIG_DRM_I915_HEARTBEAT_INTERVAL)) + if (!CONFIG_DRM_I915_HEARTBEAT_INTERVAL) return; next_heartbeat(engine); diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h index 5ae1207c363b..9167ce52487c 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h @@ -556,7 +556,7 @@ intel_engine_has_semaphores(const struct intel_engine_cs *engine) static inline bool intel_engine_has_timeslices(const struct intel_engine_cs *engine) { - if (!IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION)) + if (!CONFIG_DRM_I915_TIMESLICE_DURATION) return false; return engine->flags & I915_ENGINE_HAS_TIMESLICES; diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c index 7147fe80919e..73a79c2acd3a 100644 --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c @@ -3339,7 +3339,7 @@ logical_ring_default_vfuncs(struct intel_engine_cs *engine) engine->flags |= I915_ENGINE_HAS_SEMAPHORES; if (can_preempt(engine)) { engine->flags |= I915_ENGINE_HAS_PREEMPTION; - if (IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION)) + if (CONFIG_DRM_I915_TIMESLICE_DURATION) engine->flags |= I915_ENGINE_HAS_TIMESLICES; } } diff --git a/drivers/gpu/drm/i915/gt/selftest_engine_heartbeat.c b/drivers/gpu/drm/i915/gt/selftest_engine_heartbeat.c index 317eebf086c3..6e6e4d747cca 100644 --- a/drivers/gpu/drm/i915/gt/selftest_engine_heartbeat.c +++ b/drivers/gpu/drm/i915/gt/selftest_engine_heartbeat.c @@ -290,7 +290,7 @@ static int live_heartbeat_fast(void *arg) int err = 0; /* Check that the heartbeat ticks at the desired rate. */ - if (!IS_ACTIVE(CONFIG_DRM_I915_HEARTBEAT_INTERVAL)) + if (!CONFIG_DRM_I915_HEARTBEAT_INTERVAL) return 0; for_each_engine(engine, gt, id) { @@ -352,7 +352,7 @@ static int live_heartbeat_off(void *arg) int err = 0; /* Check that we can turn off heartbeat and not interrupt VIP */ - if (!IS_ACTIVE(CONFIG_DRM_I915_HEARTBEAT_INTERVAL)) + if (!CONFIG_DRM_I915_HEARTBEAT_INTERVAL) return 0; for_each_engine(engine, gt, id) { diff --git a/drivers/gpu/drm/i915/gt/selftest_execlists.c b/drivers/gpu/drm/i915/gt/selftest_execlists.c index b3863abc51f5..25a8c4f62b0d 100644 --- a/drivers/gpu/drm/i915/gt/selftest_execlists.c +++ b/drivers/gpu/drm/i915/gt/selftest_execlists.c @@ -992,7 +992,7 @@ static int live_timeslice_preempt(void *arg) * need to preempt the current task and replace it with another * ready task. */ - if (!IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION)) + if (!CONFIG_DRM_I915_TIMESLICE_DURATION) return 0; obj = i915_gem_object_create_internal(gt->i915, PAGE_SIZE); @@ -1122,7 +1122,7 @@ static int live_timeslice_rewind(void *arg) * but only a few of those requests, forcing us to rewind the * RING_TAIL of the original request. */ - if (!IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION)) + if (!CONFIG_DRM_I915_TIMESLICE_DURATION) return 0; for_each_engine(engine, gt, id) { @@ -1299,7 +1299,7 @@ static int live_timeslice_queue(void *arg) * ELSP[1] is already occupied, so must rely on timeslicing to * eject ELSP[0] in favour of the queue.) */ - if (!IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION)) + if (!CONFIG_DRM_I915_TIMESLICE_DURATION) return 0; obj = i915_gem_object_create_internal(gt->i915, PAGE_SIZE); @@ -1420,7 +1420,7 @@ static int live_timeslice_nopreempt(void *arg) * We should not timeslice into a request that is marked with * I915_REQUEST_NOPREEMPT. */ - if (!IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION)) + if (!CONFIG_DRM_I915_TIMESLICE_DURATION) return 0; if (igt_spinner_init(&spin, gt)) @@ -2260,7 +2260,7 @@ static int __cancel_hostile(struct live_preempt_cancel *arg) int err; /* Preempt cancel non-preemptible spinner in ELSP0 */ - if (!IS_ACTIVE(CONFIG_DRM_I915_PREEMPT_TIMEOUT)) + if (!CONFIG_DRM_I915_PREEMPT_TIMEOUT) return 0; if (!intel_has_reset_engine(arg->engine->gt)) @@ -2316,7 +2316,7 @@ static int __cancel_fail(struct live_preempt_cancel *arg) struct i915_request *rq; int err; - if (!IS_ACTIVE(CONFIG_DRM_I915_PREEMPT_TIMEOUT)) + if (!CONFIG_DRM_I915_PREEMPT_TIMEOUT) return 0; if (!intel_has_reset_engine(engine->gt)) @@ -3375,7 +3375,7 @@ static int live_preempt_timeout(void *arg) * Check that we force preemption to occur by cancelling the previous * context if it refuses to yield the GPU. */ - if (!IS_ACTIVE(CONFIG_DRM_I915_PREEMPT_TIMEOUT)) + if (!CONFIG_DRM_I915_PREEMPT_TIMEOUT) return 0; if (!intel_has_reset_engine(gt)) diff --git a/drivers/gpu/drm/i915/i915_config.c b/drivers/gpu/drm/i915/i915_config.c index b79b5f6d2cfa..ad228dd96a0b 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 && IS_ACTIVE(CONFIG_DRM_I915_FENCE_TIMEOUT)) + if (context && CONFIG_DRM_I915_FENCE_TIMEOUT) return msecs_to_jiffies_timeout(CONFIG_DRM_I915_FENCE_TIMEOUT); return 0; diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c index 79da5eca60af..91bd6f4e9909 100644 --- a/drivers/gpu/drm/i915/i915_request.c +++ b/drivers/gpu/drm/i915/i915_request.c @@ -1852,7 +1852,7 @@ long i915_request_wait(struct i915_request *rq, * completion. That requires having a good predictor for the request * duration, which we currently lack. */ - if (IS_ACTIVE(CONFIG_DRM_I915_MAX_REQUEST_BUSYWAIT) && + if (CONFIG_DRM_I915_MAX_REQUEST_BUSYWAIT && __i915_spin_request(rq, state)) goto out; diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h index 5259edacde38..62f189e064a9 100644 --- a/drivers/gpu/drm/i915/i915_utils.h +++ b/drivers/gpu/drm/i915/i915_utils.h @@ -458,17 +458,4 @@ static inline bool timer_expired(const struct timer_list *t) return timer_active(t) && !timer_pending(t); } -/* - * This is a lookalike for IS_ENABLED() that takes a kconfig value, - * e.g. CONFIG_DRM_I915_SPIN_REQUEST, and evaluates whether it is non-zero - * i.e. whether the configuration is active. Wrapping up the config inside - * a boolean context prevents clang and smatch from complaining about potential - * issues in confusing logical-&& with bitwise-& for constants. - * - * Sadly IS_ENABLED() itself does not work with kconfig values. - * - * Returns 0 if @config is 0, 1 if set to any value. - */ -#define IS_ACTIVE(config) ((config) != 0) - #endif /* !__I915_UTILS_H */
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: Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> --- drivers/gpu/drm/i915/gem/i915_gem_context.c | 2 +- drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +- drivers/gpu/drm/i915/gt/intel_engine.h | 4 ++-- drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c | 2 +- drivers/gpu/drm/i915/gt/intel_engine_types.h | 2 +- .../gpu/drm/i915/gt/intel_execlists_submission.c | 2 +- .../gpu/drm/i915/gt/selftest_engine_heartbeat.c | 4 ++-- drivers/gpu/drm/i915/gt/selftest_execlists.c | 14 +++++++------- drivers/gpu/drm/i915/i915_config.c | 2 +- drivers/gpu/drm/i915/i915_request.c | 2 +- drivers/gpu/drm/i915/i915_utils.h | 13 ------------- 11 files changed, 18 insertions(+), 31 deletions(-)