* [PATCH 1/2] x86: dumpstack, use pr_cont @ 2015-10-06 12:29 Jiri Slaby 2015-10-06 12:29 ` [PATCH 2/2] x86: text_poke, check if text_mutex is held Jiri Slaby 2015-10-06 16:00 ` [PATCH 1/2] x86: dumpstack, use pr_cont Rasmus Villemoes 0 siblings, 2 replies; 18+ messages in thread From: Jiri Slaby @ 2015-10-06 12:29 UTC (permalink / raw) To: Ingo Molnar Cc: linux-kernel, Jiri Slaby, Thomas Gleixner, H. Peter Anvin, x86 When dumping flags with which the kernel was built, we print them one by one in separate printks. Let's use pr_cont as they are continuation prints. Signed-off-by: Jiri Slaby <jslaby@suse.cz> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@redhat.com> Cc: "H. Peter Anvin" <hpa@zytor.com> Cc: x86@kernel.org --- arch/x86/kernel/dumpstack.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c index 9c30acfadae2..3850c992f767 100644 --- a/arch/x86/kernel/dumpstack.c +++ b/arch/x86/kernel/dumpstack.c @@ -260,18 +260,18 @@ int __die(const char *str, struct pt_regs *regs, long err) printk(KERN_DEFAULT "%s: %04lx [#%d] ", str, err & 0xffff, ++die_counter); #ifdef CONFIG_PREEMPT - printk("PREEMPT "); + pr_cont("PREEMPT "); #endif #ifdef CONFIG_SMP - printk("SMP "); + pr_cont("SMP "); #endif #ifdef CONFIG_DEBUG_PAGEALLOC - printk("DEBUG_PAGEALLOC "); + pr_cont("DEBUG_PAGEALLOC"); #endif #ifdef CONFIG_KASAN - printk("KASAN"); + pr_cont("KASAN"); #endif - printk("\n"); + pr_cont("\n"); if (notify_die(DIE_OOPS, str, regs, err, current->thread.trap_nr, SIGSEGV) == NOTIFY_STOP) return 1; -- 2.6.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/2] x86: text_poke, check if text_mutex is held 2015-10-06 12:29 [PATCH 1/2] x86: dumpstack, use pr_cont Jiri Slaby @ 2015-10-06 12:29 ` Jiri Slaby 2015-10-06 16:00 ` [PATCH 1/2] x86: dumpstack, use pr_cont Rasmus Villemoes 1 sibling, 0 replies; 18+ messages in thread From: Jiri Slaby @ 2015-10-06 12:29 UTC (permalink / raw) To: Ingo Molnar Cc: linux-kernel, Jiri Slaby, Thomas Gleixner, H. Peter Anvin, x86 This is required by the function, su use lockdep (if available) to check the dependency. Signed-off-by: Jiri Slaby <jslaby@suse.cz> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@redhat.com> Cc: "H. Peter Anvin" <hpa@zytor.com> Cc: x86@kernel.org --- arch/x86/kernel/alternative.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c index 25f909362b7a..3985c2517a93 100644 --- a/arch/x86/kernel/alternative.c +++ b/arch/x86/kernel/alternative.c @@ -693,6 +693,8 @@ void *text_poke(void *addr, const void *opcode, size_t len) struct page *pages[2]; int i; + lockdep_assert_held(&text_mutex); + if (!core_kernel_text((unsigned long)addr)) { pages[0] = vmalloc_to_page(addr); pages[1] = vmalloc_to_page(addr + PAGE_SIZE); -- 2.6.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] x86: dumpstack, use pr_cont 2015-10-06 12:29 [PATCH 1/2] x86: dumpstack, use pr_cont Jiri Slaby 2015-10-06 12:29 ` [PATCH 2/2] x86: text_poke, check if text_mutex is held Jiri Slaby @ 2015-10-06 16:00 ` Rasmus Villemoes 2015-10-06 16:16 ` Jiri Slaby 1 sibling, 1 reply; 18+ messages in thread From: Rasmus Villemoes @ 2015-10-06 16:00 UTC (permalink / raw) To: Jiri Slaby Cc: Ingo Molnar, linux-kernel, Thomas Gleixner, H. Peter Anvin, x86 On Tue, Oct 06 2015, Jiri Slaby <jslaby@suse.cz> wrote: > When dumping flags with which the kernel was built, we print them one > by one in separate printks. Let's use pr_cont as they are > continuation prints. > > Signed-off-by: Jiri Slaby <jslaby@suse.cz> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: "H. Peter Anvin" <hpa@zytor.com> > Cc: x86@kernel.org > --- > arch/x86/kernel/dumpstack.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c > index 9c30acfadae2..3850c992f767 100644 > --- a/arch/x86/kernel/dumpstack.c > +++ b/arch/x86/kernel/dumpstack.c > @@ -260,18 +260,18 @@ int __die(const char *str, struct pt_regs *regs, long err) > printk(KERN_DEFAULT > "%s: %04lx [#%d] ", str, err & 0xffff, ++die_counter); > #ifdef CONFIG_PREEMPT > - printk("PREEMPT "); > + pr_cont("PREEMPT "); > #endif > #ifdef CONFIG_SMP > - printk("SMP "); > + pr_cont("SMP "); > #endif > #ifdef CONFIG_DEBUG_PAGEALLOC > - printk("DEBUG_PAGEALLOC "); > + pr_cont("DEBUG_PAGEALLOC"); cosmetic: this lost a space. May I suggest diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c index 9c30acfadae2..b473a47d1851 100644 --- a/arch/x86/kernel/dumpstack.c +++ b/arch/x86/kernel/dumpstack.c @@ -257,21 +257,23 @@ int __die(const char *str, struct pt_regs *regs, long err) unsigned short ss; unsigned long sp; #endif - printk(KERN_DEFAULT - "%s: %04lx [#%d] ", str, err & 0xffff, ++die_counter); + static const char build_flags[] = "" #ifdef CONFIG_PREEMPT - printk("PREEMPT "); + " PREEMPT" #endif #ifdef CONFIG_SMP - printk("SMP "); + " SMP" #endif #ifdef CONFIG_DEBUG_PAGEALLOC - printk("DEBUG_PAGEALLOC "); + " DEBUG_PAGEALLOC" #endif #ifdef CONFIG_KASAN - printk("KASAN"); + " KASAN" #endif - printk("\n"); + ; + printk(KERN_DEFAULT + "%s: %04lx [#%d]%s\n", str, err & 0xffff, ++die_counter, + build_flags); if (notify_die(DIE_OOPS, str, regs, err, current->thread.trap_nr, SIGSEGV) == NOTIFY_STOP) return 1; instead, so that there's only one printk call and the pr_cont issue goes away? Rasmus ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] x86: dumpstack, use pr_cont 2015-10-06 16:00 ` [PATCH 1/2] x86: dumpstack, use pr_cont Rasmus Villemoes @ 2015-10-06 16:16 ` Jiri Slaby 2015-10-06 21:05 ` [PATCH 1/2] linux/kconfig.h: generalize IS_ENABLED logic Rasmus Villemoes 0 siblings, 1 reply; 18+ messages in thread From: Jiri Slaby @ 2015-10-06 16:16 UTC (permalink / raw) To: Rasmus Villemoes Cc: Ingo Molnar, linux-kernel, Thomas Gleixner, H. Peter Anvin, x86 On 10/06/2015, 06:00 PM, Rasmus Villemoes wrote: >> --- a/arch/x86/kernel/dumpstack.c >> +++ b/arch/x86/kernel/dumpstack.c >> @@ -260,18 +260,18 @@ int __die(const char *str, struct pt_regs *regs, long err) >> printk(KERN_DEFAULT >> "%s: %04lx [#%d] ", str, err & 0xffff, ++die_counter); >> #ifdef CONFIG_PREEMPT >> - printk("PREEMPT "); >> + pr_cont("PREEMPT "); >> #endif >> #ifdef CONFIG_SMP >> - printk("SMP "); >> + pr_cont("SMP "); >> #endif >> #ifdef CONFIG_DEBUG_PAGEALLOC >> - printk("DEBUG_PAGEALLOC "); >> + pr_cont("DEBUG_PAGEALLOC"); > > cosmetic: this lost a space. Oh, good catch. This was left from the times when DEBUG_PAGEALLOC was last. Now KASAN is, but I added a space to both of them locally for the time being. Anyway: > May I suggest > > diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c > index 9c30acfadae2..b473a47d1851 100644 > --- a/arch/x86/kernel/dumpstack.c > +++ b/arch/x86/kernel/dumpstack.c > @@ -257,21 +257,23 @@ int __die(const char *str, struct pt_regs *regs, long err) > unsigned short ss; > unsigned long sp; > #endif > - printk(KERN_DEFAULT > - "%s: %04lx [#%d] ", str, err & 0xffff, ++die_counter); > + static const char build_flags[] = "" > #ifdef CONFIG_PREEMPT > - printk("PREEMPT "); > + " PREEMPT" > #endif > #ifdef CONFIG_SMP > - printk("SMP "); > + " SMP" > #endif > #ifdef CONFIG_DEBUG_PAGEALLOC > - printk("DEBUG_PAGEALLOC "); > + " DEBUG_PAGEALLOC" > #endif > #ifdef CONFIG_KASAN > - printk("KASAN"); > + " KASAN" > #endif > - printk("\n"); > + ; > + printk(KERN_DEFAULT > + "%s: %04lx [#%d]%s\n", str, err & 0xffff, ++die_counter, > + build_flags); > if (notify_die(DIE_OOPS, str, regs, err, > current->thread.trap_nr, SIGSEGV) == NOTIFY_STOP) > return 1; > > instead, so that there's only one printk call and the pr_cont issue goes > away? I like this. So if you resend as a proper patch, I will definitely ack it. But it's up to the maintainers which one they prefer. thanks, -- js suse labs ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/2] linux/kconfig.h: generalize IS_ENABLED logic 2015-10-06 16:16 ` Jiri Slaby @ 2015-10-06 21:05 ` Rasmus Villemoes 2015-10-06 21:05 ` [PATCH 2/2] x86: dumpstack: eliminate some #ifdefs Rasmus Villemoes ` (3 more replies) 0 siblings, 4 replies; 18+ messages in thread From: Rasmus Villemoes @ 2015-10-06 21:05 UTC (permalink / raw) To: Paul Gortmaker, Jiri Slaby, Ingo Molnar, Andrew Morton, Thomas Gleixner, H. Peter Anvin, Michal Marek Cc: Rasmus Villemoes, linux-kernel It's not hard to generalize the macro magic used to build the IS_ENABLED macro and friends to produce a few other potentially useful macros: CHOOSE_EXPR(CONFIG_FOO, expr): if CONFIG_FOO is set expands to expr, otherwise expands to nothing. CHOOSE_EXPR(CONFIG_FOO, expr1, expr2): if CONFIG_FOO is set, expands to expr1, otherwise expands to expr2. While the latter is roughly the same as __builtin_choose_expr(IS_ENABLED(CONFIG_FOO), expr1, expr2), the macro version has the advantage that expr1 and expr2 may be string literals, and they would preserve their ability to be concatenated with other string literals. For example, this little snippet #ifdef CONFIG_X86_64 " x86-tsc: TSC cycle counter\n" #endif from kernel/trace/trace.c (which is surrounded by other string literals) could be written as CHOOSE_EXPR(CONFIG_X86_64, " x86-tsc: TSC cycle counter\n") We're also not really restricted to expressions in the C sense; the only limitation I can see is that they cannot contain unparenthesized commas. (Obviously, if one starts getting too creative, readability will suffer rather than increase.) Similarly, we can define helpers for conditional struct members and their associated initializers. It would probably take some time to get used to reading, to pick another random example, struct task_struct { ... COND_DECLARATION(CONFIG_KASAN, unsigned int kasan_depth) ... } #define INIT_KASAN(tsk) COND_INITIALIZER(CONFIG_KASAN, .kasan_depth = 1) [and I'm certainly not proposing any mass conversion], but I think it might be nice to avoid lots of short #ifdef/#else/#endif sections. The above would replace 3 and 5 lines, respectively. Also, git grep'ing for CONFIG_KASAN currently just reveals that _something_ in sched.h and init_task.h depends on it; with the above, one could at least deduce that it's guarding a certain member of some struct. Namewise, I think CHOOSE_EXPR is appropriate because of its similarity to __builtin_choose_expr, but I'm not sure about the COND_* ones. Feel free to suggest better names, and/or to flame this idea to death. Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> --- include/linux/kconfig.h | 75 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/include/linux/kconfig.h b/include/linux/kconfig.h index b33c7797eb57..ac209814b111 100644 --- a/include/linux/kconfig.h +++ b/include/linux/kconfig.h @@ -51,4 +51,79 @@ #define IS_ENABLED(option) \ (IS_BUILTIN(option) || IS_MODULE(option)) +/* + * CHOOSE_EXPR, COND_DECLARATION and COND_INITIALIZER only work for + * boolean config options. + * + * CHOOSE_EXPR(CONFIG_FOO, expr): if CONFIG_FOO is set expands to + * expr, otherwise expands to nothing. + * + * CHOOSE_EXPR(CONFIG_FOO, expr1, expr2): if CONFIG_FOO is set, + * expands to expr1, otherwise expands to expr2. + * + * COND_DECLARATION(CONFIG_FOO, decl): if CONFIG_FOO is set, expands to + * + * decl; + * + * (a semicolon should not be part of decl), otherwise expands to + * nothing. + * + * COND_INITIALIZER(CONFIG_FOO, init): if CONFIG_FOO is set, expands to + * + * init, + * + * otherwise expands to nothing. + * + * CHOOSE_EXPR(CONFIG_FOO, expr1, expr2) is roughly equivalent to + * __builtin_choose_expr(IS_ENABLED(CONFIG_FOO), expr1, + * expr2). However, since the expansion is done by the preprocessor, + * expr1 and expr2 can be string literals which can then participate + * in string concatenation. Also, we're not really limited to + * expressions, and can choose to expand to nothing (this is also used + * internally by the COND_* macros). The only limitation is that expr1 + * and expr2 cannot contain unparenthesized commas. + * + * COND_DECLARATION can, for example, be used inside a struct + * declaration to eliminate a #ifdef/#endif pair. This would look + * something like + * + * struct foo { + * int a; + * COND_DECLARATION(CONFIG_FOO_DEBUG, int b) + * int c; + * }; + * + * COND_INITIALIZER is the companion for initializing such + * conditionally defined members, again for eliminating the bracketing + * #ifdef/#endif pair. + * + * struct foo f = { + * .a = 1, + * COND_INITIALIZER(CONFIG_FOO_DEBUG, .b = 2) + * .c = 3 + * }; + * + * This is mostly useful when only a single or a few members would be + * protected by the #ifdef/#endif. One advantage of the COND_* macros + * is that git grep'ing for CONFIG_FOO_DEBUG reveals more information + * (above, we would see that it protects the "b" member of some + * struct). + */ + +#define _COMMA , +#define _COND_PUNCTUATION_0(p) +#define _COND_PUNCTUATION_1(p) p + +#define CHOOSE_EXPR(cfg, expr, ...) _CHOOSE_EXPR(cfg, expr, ##__VA_ARGS__, /* empty defalt arg */) +#define _CHOOSE_EXPR(cfg, expr, def, ...) __CHOOSE_EXPR(__ARG_PLACEHOLDER_##cfg, expr, def) +#define __CHOOSE_EXPR(arg1_or_junk, expr, def) ___CHOOSE_EXPR(arg1_or_junk expr, def) +#define ___CHOOSE_EXPR(__ignored, expr, ...) expr + +#define COND_DECLARATION(cfg, decl) _COND_DECLARATION(cfg, decl, CHOOSE_EXPR(cfg, 1, 0)) +#define _COND_DECLARATION(cfg, decl, sfx) __COND_DECLARATION(cfg, decl, sfx) +#define __COND_DECLARATION(cfg, decl, sfx) CHOOSE_EXPR(cfg, decl) _COND_PUNCTUATION_##sfx(;) +#define COND_INITIALIZER(cfg, init) _COND_INITIALIZER(cfg, init, CHOOSE_EXPR(cfg, 1, 0)) +#define _COND_INITIALIZER(cfg, init, sfx) __COND_INITIALIZER(cfg, init, sfx) +#define __COND_INITIALIZER(cfg, init, sfx) CHOOSE_EXPR(cfg, init) _COND_PUNCTUATION_##sfx(_COMMA) + #endif /* __LINUX_KCONFIG_H */ -- 2.1.3 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/2] x86: dumpstack: eliminate some #ifdefs 2015-10-06 21:05 ` [PATCH 1/2] linux/kconfig.h: generalize IS_ENABLED logic Rasmus Villemoes @ 2015-10-06 21:05 ` Rasmus Villemoes 2015-10-07 7:03 ` Ingo Molnar 2016-03-26 20:40 ` [PATCH] x86: dumpstack: combine some printks Rasmus Villemoes 2015-10-07 6:57 ` [PATCH 1/2] linux/kconfig.h: generalize IS_ENABLED logic Ingo Molnar ` (2 subsequent siblings) 3 siblings, 2 replies; 18+ messages in thread From: Rasmus Villemoes @ 2015-10-06 21:05 UTC (permalink / raw) To: Paul Gortmaker, Jiri Slaby, Ingo Molnar, Andrew Morton, Thomas Gleixner, H. Peter Anvin, Michal Marek Cc: Rasmus Villemoes, linux-kernel Jiri Slaby noted that these #ifdef protected printks should really be pr_cont. However, we might as well get completely rid of both the multiple printk calls and the tiny #ifdef sections by just building an appropriate string and passing that to the first printk call. Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> --- Mostly an example of how 1/2 can be used. arch/x86/kernel/dumpstack.c | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c index 9c30acfadae2..6ae7e65e734e 100644 --- a/arch/x86/kernel/dumpstack.c +++ b/arch/x86/kernel/dumpstack.c @@ -257,21 +257,16 @@ int __die(const char *str, struct pt_regs *regs, long err) unsigned short ss; unsigned long sp; #endif + static const char build_flags[] = "" + CHOOSE_EXPR(CONFIG_PREEMPT, " PREEMPT") + CHOOSE_EXPR(CONFIG_SMP, " SMP") + CHOOSE_EXPR(CONFIG_DEBUG_PAGEALLOC, " DEBUG_PAGEALLOC") + CHOOSE_EXPR(CONFIG_KASAN, " KASAN"); + printk(KERN_DEFAULT - "%s: %04lx [#%d] ", str, err & 0xffff, ++die_counter); -#ifdef CONFIG_PREEMPT - printk("PREEMPT "); -#endif -#ifdef CONFIG_SMP - printk("SMP "); -#endif -#ifdef CONFIG_DEBUG_PAGEALLOC - printk("DEBUG_PAGEALLOC "); -#endif -#ifdef CONFIG_KASAN - printk("KASAN"); -#endif - printk("\n"); + "%s: %04lx [#%d]%s\n", str, err & 0xffff, ++die_counter, + build_flags); + if (notify_die(DIE_OOPS, str, regs, err, current->thread.trap_nr, SIGSEGV) == NOTIFY_STOP) return 1; -- 2.1.3 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] x86: dumpstack: eliminate some #ifdefs 2015-10-06 21:05 ` [PATCH 2/2] x86: dumpstack: eliminate some #ifdefs Rasmus Villemoes @ 2015-10-07 7:03 ` Ingo Molnar 2016-03-26 20:40 ` [PATCH] x86: dumpstack: combine some printks Rasmus Villemoes 1 sibling, 0 replies; 18+ messages in thread From: Ingo Molnar @ 2015-10-07 7:03 UTC (permalink / raw) To: Rasmus Villemoes Cc: Paul Gortmaker, Jiri Slaby, Ingo Molnar, Andrew Morton, Thomas Gleixner, H. Peter Anvin, Michal Marek, linux-kernel * Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > + static const char build_flags[] = "" > + CHOOSE_EXPR(CONFIG_PREEMPT, " PREEMPT") > + CHOOSE_EXPR(CONFIG_SMP, " SMP") > + CHOOSE_EXPR(CONFIG_DEBUG_PAGEALLOC, " DEBUG_PAGEALLOC") > + CHOOSE_EXPR(CONFIG_KASAN, " KASAN"); > + > printk(KERN_DEFAULT > - "%s: %04lx [#%d] ", str, err & 0xffff, ++die_counter); > -#ifdef CONFIG_PREEMPT > - printk("PREEMPT "); > -#endif > -#ifdef CONFIG_SMP > - printk("SMP "); > -#endif > -#ifdef CONFIG_DEBUG_PAGEALLOC > - printk("DEBUG_PAGEALLOC "); > -#endif > -#ifdef CONFIG_KASAN > - printk("KASAN"); > -#endif > - printk("\n"); > + "%s: %04lx [#%d]%s\n", str, err & 0xffff, ++die_counter, > + build_flags); > + > if (notify_die(DIE_OOPS, str, regs, err, > current->thread.trap_nr, SIGSEGV) == NOTIFY_STOP) > return 1; Looks cleaner than what we had before, but I have a naming nit: CHOOSE_EXPR() is not something I'd be able to remember, I'd have to look it up again all the time, because it does not have 'config' in its name, and because 'choose' is usually associated with different constructs. So how about something more intuitive, like: COND_CONFIG(CONFIG_PREEMPT, " PREEMPT") COND_CONFIG(CONFIG_SMP, " SMP") COND_CONFIG(CONFIG_DEBUG_PAGEALLOC, " DEBUG_PAGEALLOC") COND_CONFIG(CONFIG_KASAN, " KASAN"); or: IF_CONFIG(CONFIG_PREEMPT, " PREEMPT") IF_CONFIG(CONFIG_SMP, " SMP") IF_CONFIG(CONFIG_DEBUG_PAGEALLOC, " DEBUG_PAGEALLOC") IF_CONFIG(CONFIG_KASAN, " KASAN"); ? Both names are still unused in the kernel repo. Thanks, Ingo ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] x86: dumpstack: combine some printks 2015-10-06 21:05 ` [PATCH 2/2] x86: dumpstack: eliminate some #ifdefs Rasmus Villemoes 2015-10-07 7:03 ` Ingo Molnar @ 2016-03-26 20:40 ` Rasmus Villemoes 2016-04-01 6:37 ` [tip:x86/debug] x86/dumpstack: Combine some printk()s tip-bot for Rasmus Villemoes 1 sibling, 1 reply; 18+ messages in thread From: Rasmus Villemoes @ 2016-03-26 20:40 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86 Cc: Jiri Slaby, Rasmus Villemoes, linux-kernel Long ago, Jiri Slaby noted that the subsequent printks should be pr_cont. Let's instead get rid of the multiple printk calls. Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> --- I originally suggested getting rid of the extra printks by building a suitable string at compile time, but then ended up going out on a tangent. Since I've basically shelved the COND_CONFIG idea and 288cf3c64e anyway makes it impossible to build the entire string at compile time, I now suggest this. arch/x86/kernel/dumpstack.c | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c index 8efa57a5f29e..2bb25c3fe2e8 100644 --- a/arch/x86/kernel/dumpstack.c +++ b/arch/x86/kernel/dumpstack.c @@ -260,19 +260,12 @@ int __die(const char *str, struct pt_regs *regs, long err) unsigned long sp; #endif printk(KERN_DEFAULT - "%s: %04lx [#%d] ", str, err & 0xffff, ++die_counter); -#ifdef CONFIG_PREEMPT - printk("PREEMPT "); -#endif -#ifdef CONFIG_SMP - printk("SMP "); -#endif - if (debug_pagealloc_enabled()) - printk("DEBUG_PAGEALLOC "); -#ifdef CONFIG_KASAN - printk("KASAN"); -#endif - printk("\n"); + "%s: %04lx [#%d]%s%s%s%s\n", str, err & 0xffff, ++die_counter, + IS_ENABLED(CONFIG_PREEMPT) ? " PREEMPT" : "", + IS_ENABLED(CONFIG_SMP) ? " SMP" : "", + debug_pagealloc_enabled() ? " DEBUG_PAGEALLOC" : "", + IS_ENABLED(CONFIG_KASAN) ? " KASAN" : ""); + if (notify_die(DIE_OOPS, str, regs, err, current->thread.trap_nr, SIGSEGV) == NOTIFY_STOP) return 1; -- 2.1.4 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [tip:x86/debug] x86/dumpstack: Combine some printk()s 2016-03-26 20:40 ` [PATCH] x86: dumpstack: combine some printks Rasmus Villemoes @ 2016-04-01 6:37 ` tip-bot for Rasmus Villemoes 0 siblings, 0 replies; 18+ messages in thread From: tip-bot for Rasmus Villemoes @ 2016-04-01 6:37 UTC (permalink / raw) To: linux-tip-commits Cc: tglx, jslaby, linux-kernel, hpa, linux, torvalds, mingo, peterz Commit-ID: 8fad7ec51e1b9e262e0bdd34e800ac1ea5e84dec Gitweb: http://git.kernel.org/tip/8fad7ec51e1b9e262e0bdd34e800ac1ea5e84dec Author: Rasmus Villemoes <linux@rasmusvillemoes.dk> AuthorDate: Sat, 26 Mar 2016 21:40:16 +0100 Committer: Ingo Molnar <mingo@kernel.org> CommitDate: Thu, 31 Mar 2016 15:33:03 +0200 x86/dumpstack: Combine some printk()s Long ago, Jiri Slaby noted that the subsequent printk()s should be pr_cont(). Let's instead get rid of the multiple printk calls. Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> Cc: Jiri Slaby <jslaby@suse.cz> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Thomas Gleixner <tglx@linutronix.de> Link: http://lkml.kernel.org/r/1459024817-27122-1-git-send-email-linux@rasmusvillemoes.dk Signed-off-by: Ingo Molnar <mingo@kernel.org> --- arch/x86/kernel/dumpstack.c | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c index 8efa57a..2bb25c3 100644 --- a/arch/x86/kernel/dumpstack.c +++ b/arch/x86/kernel/dumpstack.c @@ -260,19 +260,12 @@ int __die(const char *str, struct pt_regs *regs, long err) unsigned long sp; #endif printk(KERN_DEFAULT - "%s: %04lx [#%d] ", str, err & 0xffff, ++die_counter); -#ifdef CONFIG_PREEMPT - printk("PREEMPT "); -#endif -#ifdef CONFIG_SMP - printk("SMP "); -#endif - if (debug_pagealloc_enabled()) - printk("DEBUG_PAGEALLOC "); -#ifdef CONFIG_KASAN - printk("KASAN"); -#endif - printk("\n"); + "%s: %04lx [#%d]%s%s%s%s\n", str, err & 0xffff, ++die_counter, + IS_ENABLED(CONFIG_PREEMPT) ? " PREEMPT" : "", + IS_ENABLED(CONFIG_SMP) ? " SMP" : "", + debug_pagealloc_enabled() ? " DEBUG_PAGEALLOC" : "", + IS_ENABLED(CONFIG_KASAN) ? " KASAN" : ""); + if (notify_die(DIE_OOPS, str, regs, err, current->thread.trap_nr, SIGSEGV) == NOTIFY_STOP) return 1; ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] linux/kconfig.h: generalize IS_ENABLED logic 2015-10-06 21:05 ` [PATCH 1/2] linux/kconfig.h: generalize IS_ENABLED logic Rasmus Villemoes 2015-10-06 21:05 ` [PATCH 2/2] x86: dumpstack: eliminate some #ifdefs Rasmus Villemoes @ 2015-10-07 6:57 ` Ingo Molnar 2015-10-07 8:15 ` Michal Marek 2015-11-16 8:00 ` using IS_ENABLED(CONFIG_xyz) effectively Vineet Gupta 3 siblings, 0 replies; 18+ messages in thread From: Ingo Molnar @ 2015-10-07 6:57 UTC (permalink / raw) To: Rasmus Villemoes Cc: Paul Gortmaker, Jiri Slaby, Ingo Molnar, Andrew Morton, Thomas Gleixner, H. Peter Anvin, Michal Marek, linux-kernel * Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > +#define _COMMA , > +#define _COND_PUNCTUATION_0(p) > +#define _COND_PUNCTUATION_1(p) p > + > +#define CHOOSE_EXPR(cfg, expr, ...) _CHOOSE_EXPR(cfg, expr, ##__VA_ARGS__, /* empty defalt arg */) > +#define _CHOOSE_EXPR(cfg, expr, def, ...) __CHOOSE_EXPR(__ARG_PLACEHOLDER_##cfg, expr, def) > +#define __CHOOSE_EXPR(arg1_or_junk, expr, def) ___CHOOSE_EXPR(arg1_or_junk expr, def) > +#define ___CHOOSE_EXPR(__ignored, expr, ...) expr > + > +#define COND_DECLARATION(cfg, decl) _COND_DECLARATION(cfg, decl, CHOOSE_EXPR(cfg, 1, 0)) > +#define _COND_DECLARATION(cfg, decl, sfx) __COND_DECLARATION(cfg, decl, sfx) > +#define __COND_DECLARATION(cfg, decl, sfx) CHOOSE_EXPR(cfg, decl) _COND_PUNCTUATION_##sfx(;) > +#define COND_INITIALIZER(cfg, init) _COND_INITIALIZER(cfg, init, CHOOSE_EXPR(cfg, 1, 0)) > +#define _COND_INITIALIZER(cfg, init, sfx) __COND_INITIALIZER(cfg, init, sfx) > +#define __COND_INITIALIZER(cfg, init, sfx) CHOOSE_EXPR(cfg, init) _COND_PUNCTUATION_##sfx(_COMMA) Pet peeve, mind structuring this in a typographically more readable fashion, by adding some common-sense vertical structure to the definitions: #define CHOOSE_EXPR(cfg, expr, ...) _CHOOSE_EXPR(cfg, expr, ##__VA_ARGS__, /* empty defalt arg */) #define _CHOOSE_EXPR(cfg, expr, def, ...) __CHOOSE_EXPR(__ARG_PLACEHOLDER_##cfg, expr, def) #define __CHOOSE_EXPR(arg1_or_junk, expr, def) ___CHOOSE_EXPR(arg1_or_junk expr, def) #define ___CHOOSE_EXPR(__ignored, expr, ...) expr #define COND_DECLARATION(cfg, decl) _COND_DECLARATION(cfg, decl, CHOOSE_EXPR(cfg, 1, 0)) #define _COND_DECLARATION(cfg, decl, sfx) __COND_DECLARATION(cfg, decl, sfx) #define __COND_DECLARATION(cfg, decl, sfx) CHOOSE_EXPR(cfg, decl) _COND_PUNCTUATION_##sfx(;) #define COND_INITIALIZER(cfg, init) _COND_INITIALIZER(cfg, init, CHOOSE_EXPR(cfg, 1, 0)) #define _COND_INITIALIZER(cfg, init, sfx) __COND_INITIALIZER(cfg, init, sfx) #define __COND_INITIALIZER(cfg, init, sfx) CHOOSE_EXPR(cfg, init) _COND_PUNCTUATION_##sfx(_COMMA) ? It's still a mouthful, but at least readable at a glance. Thanks, Ingo ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] linux/kconfig.h: generalize IS_ENABLED logic 2015-10-06 21:05 ` [PATCH 1/2] linux/kconfig.h: generalize IS_ENABLED logic Rasmus Villemoes 2015-10-06 21:05 ` [PATCH 2/2] x86: dumpstack: eliminate some #ifdefs Rasmus Villemoes 2015-10-07 6:57 ` [PATCH 1/2] linux/kconfig.h: generalize IS_ENABLED logic Ingo Molnar @ 2015-10-07 8:15 ` Michal Marek 2015-10-07 21:33 ` Rasmus Villemoes 2015-11-16 8:00 ` using IS_ENABLED(CONFIG_xyz) effectively Vineet Gupta 3 siblings, 1 reply; 18+ messages in thread From: Michal Marek @ 2015-10-07 8:15 UTC (permalink / raw) To: Rasmus Villemoes Cc: Paul Gortmaker, Jiri Slaby, Ingo Molnar, Andrew Morton, Thomas Gleixner, H. Peter Anvin, linux-kernel On 2015-10-06 23:05, Rasmus Villemoes wrote: > It's not hard to generalize the macro magic used to build the > IS_ENABLED macro and friends to produce a few other potentially useful > macros: > > CHOOSE_EXPR(CONFIG_FOO, expr): if CONFIG_FOO is set expands to > expr, otherwise expands to nothing. > > CHOOSE_EXPR(CONFIG_FOO, expr1, expr2): if CONFIG_FOO is set, > expands to expr1, otherwise expands to expr2. FWIW, I agree with Ingo that the CHOOSE_EXPR name is not really obvious. IF_CONFIG is a better alternative IMO, since the average programmer probably does not know __builtin_choose_expr() to see the analogy. > Similarly, we can define helpers for conditional struct members and > their associated initializers. It would probably take some time to get > used to reading, to pick another random example, > > struct task_struct { > ... > COND_DECLARATION(CONFIG_KASAN, unsigned int kasan_depth) > ... > } While the C standard syntax requires struct-declaration to actually declare a member, the compiler will happily ignore the extra semicolon if you write truct task_struct { ... CHOOSE_EXPR(CONFIG_KASAN, unsigned int kasan_depth); ... } So I think that the COND_DECLARATION macro is not necessary. > #define INIT_KASAN(tsk) COND_INITIALIZER(CONFIG_KASAN, .kasan_depth = 1) COND_INITIALIZER on the other hand is useful (CHOOSE_EXPR(CONFIG_KASAN, .kasan_depth = 1 _COMMA) does does not work, unfortunately). > [and I'm certainly not proposing any mass conversion], but I think it > might be nice to avoid lots of short #ifdef/#else/#endif sections. It should be accompanied by a patch to scripts/tags.sh teaching ctags/etags about the new macros. Michal ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] linux/kconfig.h: generalize IS_ENABLED logic 2015-10-07 8:15 ` Michal Marek @ 2015-10-07 21:33 ` Rasmus Villemoes 2015-10-08 11:40 ` Michal Marek 0 siblings, 1 reply; 18+ messages in thread From: Rasmus Villemoes @ 2015-10-07 21:33 UTC (permalink / raw) To: Michal Marek Cc: Paul Gortmaker, Jiri Slaby, Ingo Molnar, Andrew Morton, Thomas Gleixner, H. Peter Anvin, linux-kernel On Wed, Oct 07 2015, Michal Marek <mmarek@suse.cz> wrote: > On 2015-10-06 23:05, Rasmus Villemoes wrote: >> It's not hard to generalize the macro magic used to build the >> IS_ENABLED macro and friends to produce a few other potentially useful >> macros: >> >> CHOOSE_EXPR(CONFIG_FOO, expr): if CONFIG_FOO is set expands to >> expr, otherwise expands to nothing. >> >> CHOOSE_EXPR(CONFIG_FOO, expr1, expr2): if CONFIG_FOO is set, >> expands to expr1, otherwise expands to expr2. > > FWIW, I agree with Ingo that the CHOOSE_EXPR name is not really obvious. > IF_CONFIG is a better alternative IMO, since the average programmer > probably does not know __builtin_choose_expr() to see the analogy. OK, CHOOSE_EXPR is out. But I think IF_CONFIG/COND_CONFIG might be a little annoying or redundant, since "CONFIG" would also always be part of the first argument. Come to think of it, since this would be a primitive for conditional compilation whose primary purpose is to eliminate the verbosity of #ifdef/#endif, I'd prefer plain and simple COND, with COND_INITIALIZER as a sidekick for that special purpose. Unfortunately, COND is already used in a few places :( So I'll go with COND_CONFIG for now, but wait a few days before sending v2, to see if anyone else has comments or naming suggestions. > While the C standard syntax requires struct-declaration to actually > declare a member, the compiler will happily ignore the extra semicolon > if you write > > truct task_struct { > ... > CHOOSE_EXPR(CONFIG_KASAN, unsigned int kasan_depth); > ... > } > > So I think that the COND_DECLARATION macro is not necessary. Thanks, I didn't know that. I see that both gcc and clang accept it whether the extra semicolon is at the beginning or end of the struct, and whether there's even a single actual member. OK, then COND_DECLARATION is redundant (though it might still be useful as a natural buddy to COND_INITIALIZER). >> #define INIT_KASAN(tsk) COND_INITIALIZER(CONFIG_KASAN, .kasan_depth = 1) > > COND_INITIALIZER on the other hand is useful (CHOOSE_EXPR(CONFIG_KASAN, > .kasan_depth = 1 _COMMA) does does not work, unfortunately). Yeah, since we need to do the multiple expansion thing there's no way of preventing _COMMA from expanding too early, so I'm pretty sure one would need some specialized version of CHOOSE_EXPR (or whatever the name ends up being). Also, I wouldn't really want users to have to supply the _COMMA. One could also consider making COND_ARGUMENT an alias for it - that could be useful for some seq_printf calls generating /proc files (where the format string would be built with COND_CONFIG). >> [and I'm certainly not proposing any mass conversion], but I think it >> might be nice to avoid lots of short #ifdef/#else/#endif sections. > > It should be accompanied by a patch to scripts/tags.sh teaching > ctags/etags about the new macros. Do you mean that something like --regex-c='/COND_CONFIG\([^,]*,([^,]*)\)/\1/' should be added so ctags would pick up the text in the true branch? I'm not very familiar with ctags. Thanks for the feedback, Michal and Ingo. Rasmus ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] linux/kconfig.h: generalize IS_ENABLED logic 2015-10-07 21:33 ` Rasmus Villemoes @ 2015-10-08 11:40 ` Michal Marek 0 siblings, 0 replies; 18+ messages in thread From: Michal Marek @ 2015-10-08 11:40 UTC (permalink / raw) To: Rasmus Villemoes Cc: Paul Gortmaker, Jiri Slaby, Ingo Molnar, Andrew Morton, Thomas Gleixner, H. Peter Anvin, linux-kernel On 2015-10-07 23:33, Rasmus Villemoes wrote: > On Wed, Oct 07 2015, Michal Marek <mmarek@suse.cz> wrote: >> It should be accompanied by a patch to scripts/tags.sh teaching >> ctags/etags about the new macros. > > Do you mean that something like > > --regex-c='/COND_CONFIG\([^,]*,([^,]*)\)/\1/' > > should be added so ctags would pick up the text in the true branch? I'm > not very familiar with ctags. Something like this, yes. This particular rule does not work for me, though and I don't see an obvious reason why. Michal ^ permalink raw reply [flat|nested] 18+ messages in thread
* using IS_ENABLED(CONFIG_xyz) effectively 2015-10-06 21:05 ` [PATCH 1/2] linux/kconfig.h: generalize IS_ENABLED logic Rasmus Villemoes ` (2 preceding siblings ...) 2015-10-07 8:15 ` Michal Marek @ 2015-11-16 8:00 ` Vineet Gupta 2015-11-16 8:28 ` Geert Uytterhoeven 3 siblings, 1 reply; 18+ messages in thread From: Vineet Gupta @ 2015-11-16 8:00 UTC (permalink / raw) To: Rasmus Villemoes, Paul Gortmaker, Ingo Molnar, Andrew Morton, Thomas Gleixner, H. Peter Anvin, Michal Marek Cc: linux-arch, linux-kernel, arcml Hi, I've been using IS_ENABLED for some time and once in a while run into an issue which prevents seamless use. Hence posing this question to experts in the area. C macro processor evaluates the ensuing control block even if IS_ENABLED evaluates to false. This requires dummy #defines or worse still removing usage of IS_ENABLED altogether. e.g. In example below even for ARCOMPACT builds, we need the ARCV2 specific define ARCV2_IRQ_DEF_PRIO. void arch_cpu_idle(void) { if (is_isa_arcompact()) { <---- IS_ENABLED(CONFIG_ISA_ARCOMPACT) __asm__("sleep 0x3"); } else { const int arg = 0x10 | ARCV2_IRQ_DEF_PRIO; __asm__("sleep 0x10"); } } One could argue that the interface needs to be cleanly defined to not have such specific #defines in common code in first place. However sometime that becomes just too tedious. Is there a way to get around by this ? Thx, -Vineet ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: using IS_ENABLED(CONFIG_xyz) effectively 2015-11-16 8:00 ` using IS_ENABLED(CONFIG_xyz) effectively Vineet Gupta @ 2015-11-16 8:28 ` Geert Uytterhoeven 2015-11-16 8:35 ` Vineet Gupta 0 siblings, 1 reply; 18+ messages in thread From: Geert Uytterhoeven @ 2015-11-16 8:28 UTC (permalink / raw) To: Vineet Gupta Cc: Rasmus Villemoes, Paul Gortmaker, Ingo Molnar, Andrew Morton, Thomas Gleixner, H. Peter Anvin, Michal Marek, linux-arch, linux-kernel, arcml Hi Vineet, On Mon, Nov 16, 2015 at 9:00 AM, Vineet Gupta <Vineet.Gupta1@synopsys.com> wrote: > I've been using IS_ENABLED for some time and once in a while run into an issue > which prevents seamless use. Hence posing this question to experts in the area. > > C macro processor evaluates the ensuing control block even if IS_ENABLED evaluates > to false. This requires dummy #defines or worse still removing usage of IS_ENABLED > altogether. > > e.g. In example below even for ARCOMPACT builds, we need the ARCV2 specific define > ARCV2_IRQ_DEF_PRIO. > > void arch_cpu_idle(void) > { > if (is_isa_arcompact()) { <---- IS_ENABLED(CONFIG_ISA_ARCOMPACT) > __asm__("sleep 0x3"); > } else { > const int arg = 0x10 | ARCV2_IRQ_DEF_PRIO; > __asm__("sleep 0x10"); > } > } > > One could argue that the interface needs to be cleanly defined to not have such > specific #defines in common code in first place. However sometime that becomes > just too tedious. > > Is there a way to get around by this ? Use #ifdef CONFIG_...? The advantage of IS_ENABLED() over #ifdef is that it allows compile-testing of the disabled code path. Of course it should only be compiled if it makes sense. And that's exactly what you're running into. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: using IS_ENABLED(CONFIG_xyz) effectively 2015-11-16 8:28 ` Geert Uytterhoeven @ 2015-11-16 8:35 ` Vineet Gupta 2015-11-16 8:52 ` Arnd Bergmann 0 siblings, 1 reply; 18+ messages in thread From: Vineet Gupta @ 2015-11-16 8:35 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Rasmus Villemoes, Paul Gortmaker, Ingo Molnar, Andrew Morton, Thomas Gleixner, H. Peter Anvin, Michal Marek, linux-arch, linux-kernel, arcml Hi Geert, On Monday 16 November 2015 01:58 PM, Geert Uytterhoeven wrote: > Hi Vineet, > > On Mon, Nov 16, 2015 at 9:00 AM, Vineet Gupta > <Vineet.Gupta1@synopsys.com> wrote: >> I've been using IS_ENABLED for some time and once in a while run into an issue >> which prevents seamless use. Hence posing this question to experts in the area. >> >> C macro processor evaluates the ensuing control block even if IS_ENABLED evaluates >> to false. This requires dummy #defines or worse still removing usage of IS_ENABLED >> altogether. >> >> e.g. In example below even for ARCOMPACT builds, we need the ARCV2 specific define >> ARCV2_IRQ_DEF_PRIO. >> >> void arch_cpu_idle(void) >> { >> if (is_isa_arcompact()) { <---- IS_ENABLED(CONFIG_ISA_ARCOMPACT) >> __asm__("sleep 0x3"); >> } else { >> const int arg = 0x10 | ARCV2_IRQ_DEF_PRIO; >> __asm__("sleep 0x10"); >> } >> } >> >> One could argue that the interface needs to be cleanly defined to not have such >> specific #defines in common code in first place. However sometime that becomes >> just too tedious. >> >> Is there a way to get around by this ? > Use #ifdef CONFIG_...? > > The advantage of IS_ENABLED() over #ifdef is that it allows compile-testing of > the disabled code path. Of course it should only be compiled if it makes > sense. And that's exactly what you're running into. And I thought it was to de-uglify the code with same semantics - which doesn't seem to be the case ! Oh well ! Thx, -Vineet ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: using IS_ENABLED(CONFIG_xyz) effectively 2015-11-16 8:35 ` Vineet Gupta @ 2015-11-16 8:52 ` Arnd Bergmann 2015-11-16 10:03 ` Vineet Gupta 0 siblings, 1 reply; 18+ messages in thread From: Arnd Bergmann @ 2015-11-16 8:52 UTC (permalink / raw) To: Vineet Gupta Cc: Geert Uytterhoeven, Rasmus Villemoes, Paul Gortmaker, Ingo Molnar, Andrew Morton, Thomas Gleixner, H. Peter Anvin, Michal Marek, linux-arch, linux-kernel, arcml On Monday 16 November 2015 08:35:05 Vineet Gupta wrote: > Hi Geert, > > On Monday 16 November 2015 01:58 PM, Geert Uytterhoeven wrote: > > Hi Vineet, > > > > On Mon, Nov 16, 2015 at 9:00 AM, Vineet Gupta > > <Vineet.Gupta1@synopsys.com> wrote: > >> I've been using IS_ENABLED for some time and once in a while run into an issue > >> which prevents seamless use. Hence posing this question to experts in the area. > >> > >> C macro processor evaluates the ensuing control block even if IS_ENABLED evaluates > >> to false. This requires dummy #defines or worse still removing usage of IS_ENABLED > >> altogether. > >> > >> e.g. In example below even for ARCOMPACT builds, we need the ARCV2 specific define > >> ARCV2_IRQ_DEF_PRIO. > >> > >> void arch_cpu_idle(void) > >> { > >> if (is_isa_arcompact()) { <---- IS_ENABLED(CONFIG_ISA_ARCOMPACT) > >> __asm__("sleep 0x3"); > >> } else { > >> const int arg = 0x10 | ARCV2_IRQ_DEF_PRIO; > >> __asm__("sleep 0x10"); > >> } > >> } > >> > >> One could argue that the interface needs to be cleanly defined to not have such > >> specific #defines in common code in first place. However sometime that becomes > >> just too tedious. > >> > >> Is there a way to get around by this ? > > Use #ifdef CONFIG_...? > > > > The advantage of IS_ENABLED() over #ifdef is that it allows compile-testing of > > the disabled code path. Of course it should only be compiled if it makes > > sense. And that's exactly what you're running into. > > And I thought it was to de-uglify the code with same semantics - which doesn't > seem to be the case ! I would still try to do this with if(IS_ENABLED()) or another macro like you have above. The problem you ran into with the macro definitions is that you have conflicting header files: #ifdef CONFIG_ISA_ARCOMPACT #include <asm/irqflags-compact.h> #else #include <asm/irqflags-arcv2.h> #endif This has other (small) problems too, because you get possibly conflicting symbols (e.g. arch_local_irq_enable but also the register names) that make it harder to follow what's going on when reading the code. If you prefix all the defines in the two headers with the respective name, you can just include both headers and avoid this: static inline void arch_local_irq_enable(void) { if (IS_ENABLED(CONFIG_ISA_ARCCOMPACT)) return arccompact_local_irq_enable(); else return arcv2_local_irq_enable(); } Arnd ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: using IS_ENABLED(CONFIG_xyz) effectively 2015-11-16 8:52 ` Arnd Bergmann @ 2015-11-16 10:03 ` Vineet Gupta 0 siblings, 0 replies; 18+ messages in thread From: Vineet Gupta @ 2015-11-16 10:03 UTC (permalink / raw) To: Arnd Bergmann Cc: Geert Uytterhoeven, Rasmus Villemoes, Paul Gortmaker, Ingo Molnar, Andrew Morton, Thomas Gleixner, H. Peter Anvin, Michal Marek, linux-arch, linux-kernel, arcml On Monday 16 November 2015 02:22 PM, Arnd Bergmann wrote: > On Monday 16 November 2015 08:35:05 Vineet Gupta wrote: >> Hi Geert, >> >> On Monday 16 November 2015 01:58 PM, Geert Uytterhoeven wrote: >>> Hi Vineet, >>> >>> On Mon, Nov 16, 2015 at 9:00 AM, Vineet Gupta >>> <Vineet.Gupta1@synopsys.com> wrote: >>>> I've been using IS_ENABLED for some time and once in a while run into an issue >>>> which prevents seamless use. Hence posing this question to experts in the area. >>>> >>>> C macro processor evaluates the ensuing control block even if IS_ENABLED evaluates >>>> to false. This requires dummy #defines or worse still removing usage of IS_ENABLED >>>> altogether. >>>> >>>> e.g. In example below even for ARCOMPACT builds, we need the ARCV2 specific define >>>> ARCV2_IRQ_DEF_PRIO. >>>> >>>> void arch_cpu_idle(void) >>>> { >>>> if (is_isa_arcompact()) { <---- IS_ENABLED(CONFIG_ISA_ARCOMPACT) >>>> __asm__("sleep 0x3"); >>>> } else { >>>> const int arg = 0x10 | ARCV2_IRQ_DEF_PRIO; >>>> __asm__("sleep 0x10"); >>>> } >>>> } >>>> >>>> One could argue that the interface needs to be cleanly defined to not have such >>>> specific #defines in common code in first place. However sometime that becomes >>>> just too tedious. >>>> >>>> Is there a way to get around by this ? >>> Use #ifdef CONFIG_...? >>> >>> The advantage of IS_ENABLED() over #ifdef is that it allows compile-testing of >>> the disabled code path. Of course it should only be compiled if it makes >>> sense. And that's exactly what you're running into. >> And I thought it was to de-uglify the code with same semantics - which doesn't >> seem to be the case ! > I would still try to do this with if(IS_ENABLED()) or another macro > like you have above. The problem you ran into with the macro definitions > is that you have conflicting header files: > > #ifdef CONFIG_ISA_ARCOMPACT > #include <asm/irqflags-compact.h> > #else > #include <asm/irqflags-arcv2.h> > #endif > > This has other (small) problems too, because you get possibly conflicting > symbols (e.g. arch_local_irq_enable but also the register names) that > make it harder to follow what's going on when reading the code. > If you prefix all the defines in the two headers with the respective name, > you can just include both headers and avoid this: > > static inline void arch_local_irq_enable(void) > { > if (IS_ENABLED(CONFIG_ISA_ARCCOMPACT)) > return arccompact_local_irq_enable(); > else > return arcv2_local_irq_enable(); > } > > Arnd So my approach was different - I felt it was cleaner to export same "interface" from ISA specific code to ISA common code. It seemed more readable vs. the explicit if check above in each function which was dependent on ISA. Plus given that the ISA are not compatible, I wanted to ensure we were not mixing (even by accident) any code for one into a build for other. This ofcourse means that my original question / request is wrong in first place. With current structuring of code, I need to make sure that such ISA specific #defines don't end in ISA common code anyways. So something like below -------------> diff --git a/arch/arc/include/asm/irqflags-arcv2.h b/arch/arc/include/asm/irqflags-arcv2.h index ad481c24070d..f7c8d3cbeaa1 100644 --- a/arch/arc/include/asm/irqflags-arcv2.h +++ b/arch/arc/include/asm/irqflags-arcv2.h @@ -37,6 +37,8 @@ #define ISA_INIT_STATUS_BITS (STATUS_IE_MASK | STATUS_AD_MASK | \ (ARCV2_IRQ_DEF_PRIO << 1)) +#define ISA_SLEEP_ARG 0x10 + #ifndef __ASSEMBLY__ /* diff --git a/arch/arc/include/asm/irqflags-compact.h b/arch/arc/include/asm/irqflags-compact.h index d8c608174617..c1d36458bfb7 100644 --- a/arch/arc/include/asm/irqflags-compact.h +++ b/arch/arc/include/asm/irqflags-compact.h @@ -43,6 +43,8 @@ #define ISA_INIT_STATUS_BITS STATUS_IE_MASK +#define ISA_SLEEP_ARG 0x3 + #ifndef __ASSEMBLY__ /****************************************************************** diff --git a/arch/arc/kernel/process.c b/arch/arc/kernel/process.c index 91d5a0f1f3f7..a3f750e76b68 100644 --- a/arch/arc/kernel/process.c +++ b/arch/arc/kernel/process.c @@ -44,11 +44,10 @@ SYSCALL_DEFINE0(arc_gettls) void arch_cpu_idle(void) { /* sleep, but enable all interrupts before committing */ - if (is_isa_arcompact()) { - __asm__("sleep 0x3"); - } else { - __asm__("sleep 0x10"); - } + __asm__ __volatile__( + "sleep %0 \n" + : + :"I"(ISA_SLEEP_ARG)); /* can't be "r" has to be embedded const */ } asmlinkage void ret_from_fork(void); -- -Vineet ^ permalink raw reply related [flat|nested] 18+ messages in thread
end of thread, other threads:[~2016-04-01 6:37 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-10-06 12:29 [PATCH 1/2] x86: dumpstack, use pr_cont Jiri Slaby 2015-10-06 12:29 ` [PATCH 2/2] x86: text_poke, check if text_mutex is held Jiri Slaby 2015-10-06 16:00 ` [PATCH 1/2] x86: dumpstack, use pr_cont Rasmus Villemoes 2015-10-06 16:16 ` Jiri Slaby 2015-10-06 21:05 ` [PATCH 1/2] linux/kconfig.h: generalize IS_ENABLED logic Rasmus Villemoes 2015-10-06 21:05 ` [PATCH 2/2] x86: dumpstack: eliminate some #ifdefs Rasmus Villemoes 2015-10-07 7:03 ` Ingo Molnar 2016-03-26 20:40 ` [PATCH] x86: dumpstack: combine some printks Rasmus Villemoes 2016-04-01 6:37 ` [tip:x86/debug] x86/dumpstack: Combine some printk()s tip-bot for Rasmus Villemoes 2015-10-07 6:57 ` [PATCH 1/2] linux/kconfig.h: generalize IS_ENABLED logic Ingo Molnar 2015-10-07 8:15 ` Michal Marek 2015-10-07 21:33 ` Rasmus Villemoes 2015-10-08 11:40 ` Michal Marek 2015-11-16 8:00 ` using IS_ENABLED(CONFIG_xyz) effectively Vineet Gupta 2015-11-16 8:28 ` Geert Uytterhoeven 2015-11-16 8:35 ` Vineet Gupta 2015-11-16 8:52 ` Arnd Bergmann 2015-11-16 10:03 ` Vineet Gupta
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).