| Message ID | 20210929183357.1490204-4-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Rename IS_ACTIVE() and move to kconfig.h | expand |
On Thu, Sep 30, 2021 at 3:34 AM Lucas De Marchi <lucas.demarchi@intel.com> wrote: > > The check for config value doesn't really belong to i915_utils.h - we > are trying to eliminate that utils helper and share them when possible > with other drivers and subsystems. > > Rationale for having such macro is in commit > babaab2f4738 ("drm/i915: Encapsulate kconfig constant values inside boolean predicates") > whereas later it is improved to not break the build if used with > undefined configs. The caveat is detailed in the documentation: unlike > IS_ENABLED(): it's not preprocessor-only logic so can't be used for > things like `#if IS_CONFIG_NONZERO(...)` > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> Hypothetical "it would be nice to have ..." is really unneeded. if (context && CONFIG_DRM_I915_FENCE_TIMEOUT > 0) return msecs_to_jiffies_timeout(CONFIG_DRM_I915_FENCE_TIMEOUT); is enough, and much cleaner. This warning is shown only when a constant is used together with '&&'. Most of IS_ACTIVE can go away. Given that, there are not many places where the IS_ACTIVE macro is useful, even in the i915 driver. For a few sources of the warnings, replacing it with != 0 or > 0 is just fine. Of course, such an ugly macro is not worth being moved to <linux/kconfig.h>
On Thu, Sep 30, 2021 at 11:01:36PM +0900, Masahiro Yamada wrote: >On Thu, Sep 30, 2021 at 3:34 AM Lucas De Marchi ><lucas.demarchi@intel.com> wrote: >> >> The check for config value doesn't really belong to i915_utils.h - we >> are trying to eliminate that utils helper and share them when possible >> with other drivers and subsystems. >> >> Rationale for having such macro is in commit >> babaab2f4738 ("drm/i915: Encapsulate kconfig constant values inside boolean predicates") >> whereas later it is improved to not break the build if used with >> undefined configs. The caveat is detailed in the documentation: unlike >> IS_ENABLED(): it's not preprocessor-only logic so can't be used for >> things like `#if IS_CONFIG_NONZERO(...)` >> >> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > > >Hypothetical "it would be nice to have ..." is really unneeded. > > if (context && CONFIG_DRM_I915_FENCE_TIMEOUT > 0) > return >msecs_to_jiffies_timeout(CONFIG_DRM_I915_FENCE_TIMEOUT); > > >is enough, and much cleaner. > > > >This warning is shown only when a constant is used >together with '&&'. > >Most of IS_ACTIVE can go away. > >Given that, there are not many places where the IS_ACTIVE macro >is useful, even in the i915 driver. > >For a few sources of the warnings, >replacing it with != 0 or > 0 is just fine. humn... maybe. Let me do a conversion in that direction and see what is the outcome. My original intention was to make IS_ENABLED() even uglier to cover the int case, but after some tries it seems impossible to do on preprocessor context, so I thought maybe it would be ok as a separate one. > >Of course, such an ugly macro is not worth being moved to <linux/kconfig.h> if we don't handle the undefined case and only worry about encapsulating it inside a boolean predicate, the macro would be very simple. Would that be worth having in kconfig.h maybe? thanks Lucas De Marchi
On Fri, Oct 1, 2021 at 12:55 AM Lucas De Marchi <lucas.demarchi@intel.com> wrote: > > On Thu, Sep 30, 2021 at 11:01:36PM +0900, Masahiro Yamada wrote: > >On Thu, Sep 30, 2021 at 3:34 AM Lucas De Marchi > ><lucas.demarchi@intel.com> wrote: > >> > >> The check for config value doesn't really belong to i915_utils.h - we > >> are trying to eliminate that utils helper and share them when possible > >> with other drivers and subsystems. > >> > >> Rationale for having such macro is in commit > >> babaab2f4738 ("drm/i915: Encapsulate kconfig constant values inside boolean predicates") > >> whereas later it is improved to not break the build if used with > >> undefined configs. The caveat is detailed in the documentation: unlike > >> IS_ENABLED(): it's not preprocessor-only logic so can't be used for > >> things like `#if IS_CONFIG_NONZERO(...)` > >> > >> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > > > > > >Hypothetical "it would be nice to have ..." is really unneeded. > > > > if (context && CONFIG_DRM_I915_FENCE_TIMEOUT > 0) > > return > >msecs_to_jiffies_timeout(CONFIG_DRM_I915_FENCE_TIMEOUT); > > > > > >is enough, and much cleaner. > > > > > > > >This warning is shown only when a constant is used > >together with '&&'. > > > >Most of IS_ACTIVE can go away. > > > >Given that, there are not many places where the IS_ACTIVE macro > >is useful, even in the i915 driver. > > > >For a few sources of the warnings, > >replacing it with != 0 or > 0 is just fine. > > humn... maybe. Let me do a conversion in that direction and see what is > the outcome. > > My original intention was to make IS_ENABLED() even uglier to cover the > int case, but after some tries it seems impossible to do on preprocessor > context, so I thought maybe it would be ok as a separate one. > > > > >Of course, such an ugly macro is not worth being moved to <linux/kconfig.h> > > if we don't handle the undefined case and only worry about encapsulating > it inside a boolean predicate, the macro would be very simple. Would > that be worth having in kconfig.h maybe? I do not think so. #define IS_CONFIG_NONZERO(config) ((config) != 0) seems like a stupid macro. What is bad about writing the direct code? if (x && CONFIG_FOO > 0) .... > > > thanks > Lucas De Marchi
diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h index 436ce612c46a..62f189e064a9 100644 --- a/drivers/gpu/drm/i915/i915_utils.h +++ b/drivers/gpu/drm/i915/i915_utils.h @@ -28,7 +28,6 @@ #include <linux/list.h> #include <linux/overflow.h> #include <linux/sched.h> -#include <linux/stringify.h> #include <linux/types.h> #include <linux/workqueue.h> @@ -459,20 +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_CONFIG_NONZERO(config) ( \ - (__stringify_1(config)[0] > '0' && __stringify_1(config)[0] < '9') || \ - __stringify_1(config)[0] == '-' \ -) - #endif /* !__I915_UTILS_H */ diff --git a/include/linux/kconfig.h b/include/linux/kconfig.h index 20d1079e92b4..e84f7c1c8e26 100644 --- a/include/linux/kconfig.h +++ b/include/linux/kconfig.h @@ -2,6 +2,7 @@ #ifndef __LINUX_KCONFIG_H #define __LINUX_KCONFIG_H +#include <linux/stringify.h> #include <generated/autoconf.h> #ifdef CONFIG_CPU_BIG_ENDIAN @@ -26,8 +27,8 @@ #define ____or(arg1_or_junk, y) __take_second_arg(arg1_or_junk 1, y) /* - * Helper macros to use CONFIG_ options in C/CPP expressions. Note that - * these only work with boolean and tristate options. + * Helper macros to use CONFIG_ options in C/CPP expressions. Note that except + * for IS_CONFIG_NONZERO, these only work with boolean and tristate options. */ /* @@ -72,4 +73,15 @@ */ #define IS_ENABLED(option) __or(IS_BUILTIN(option), IS_MODULE(option)) +/* + * This is a lookalike for IS_ENABLED(), but works with int kconfig options + * with the caveat that it can't be used on preprocessor checks. + * + * Returns 0 if @config is 0 or undefined, 1 if set to any value. + */ +#define IS_CONFIG_NONZERO(config) ( \ + (__stringify_1(config)[0] > '0' && __stringify_1(config)[0] < '9') || \ + __stringify_1(config)[0] == '-' \ +) + #endif /* __LINUX_KCONFIG_H */
The check for config value doesn't really belong to i915_utils.h - we are trying to eliminate that utils helper and share them when possible with other drivers and subsystems. Rationale for having such macro is in commit babaab2f4738 ("drm/i915: Encapsulate kconfig constant values inside boolean predicates") whereas later it is improved to not break the build if used with undefined configs. The caveat is detailed in the documentation: unlike IS_ENABLED(): it's not preprocessor-only logic so can't be used for things like `#if IS_CONFIG_NONZERO(...)` Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> --- drivers/gpu/drm/i915/i915_utils.h | 17 ----------------- include/linux/kconfig.h | 16 ++++++++++++++-- 2 files changed, 14 insertions(+), 19 deletions(-)