Re: [RFC][PATCHv2 4/7] printk: make alt_printk available when config printk set

[Petr Mladek <pmladek@suse.com>]

On Sat 2016-10-01 00:17:55, Sergey Senozhatsky wrote:
> alt_printk must be enabled on systems that have CONFIG_PRINTK set,
> while NMI related functions must depend on CONFIG_PRINTK_NMI.
>
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
>  include/linux/printk.h     | 21 +++++++++++++++------
>  kernel/printk/Makefile     |  2 +-
>  kernel/printk/alt_printk.c | 30 +++++++++++++++++++++++++-----
>  3 files changed, 41 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index 7510613..068a124 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -131,17 +131,11 @@ void early_printk(const char *s, ...) { }
>  #endif
>  
>  #ifdef CONFIG_PRINTK_NMI
> -extern void alt_printk_init(void);
>  extern void printk_nmi_enter(void);
>  extern void printk_nmi_exit(void);
> -extern void alt_printk_flush(void);
> -extern void alt_printk_flush_on_panic(void);
>  #else
> -static inline void alt_printk_init(void) { }
>  static inline void printk_nmi_enter(void) { }
>  static inline void printk_nmi_exit(void) { }
> -static inline void alt_printk_flush(void) { }
> -static inline void alt_printk_flush_on_panic(void) { }
>  #endif /* PRINTK_NMI */
>  
>  #ifdef CONFIG_PRINTK
> @@ -193,6 +187,9 @@ void __init setup_log_buf(int early);
>  __printf(1, 2) void dump_stack_set_arch_desc(const char *fmt, ...);
>  void dump_stack_print_info(const char *log_lvl);
>  void show_regs_print_info(const char *log_lvl);
> +extern void alt_printk_init(void);
> +extern void alt_printk_flush(void);
> +extern void alt_printk_flush_on_panic(void);
>  #else
>  static inline __printf(1, 0)
>  int vprintk(const char *s, va_list args)
> @@ -252,6 +249,18 @@ static inline void dump_stack_print_info(const char *log_lvl)
>  static inline void show_regs_print_info(const char *log_lvl)
>  {
>  }
> +
> +static inline void alt_printk_init(void)
> +{
> +}
> +
> +static inline void alt_printk_flush(void)
> +{
> +}
> +
> +static inline void alt_printk_flush_on_panic(void)
> +{
> +}
>  #endif
>  
>  extern asmlinkage void dump_stack(void) __cold;
> diff --git a/kernel/printk/Makefile b/kernel/printk/Makefile
> index 3fc5e4e..d11838c 100644
> --- a/kernel/printk/Makefile
> +++ b/kernel/printk/Makefile
> @@ -1,3 +1,3 @@
>  obj-y	= printk.o
> -obj-$(CONFIG_PRINTK_NMI)		+= alt_printk.o
> +obj-$(CONFIG_PRINTK)		+= alt_printk.o
>  obj-$(CONFIG_A11Y_BRAILLE_CONSOLE)	+= braille.o
> diff --git a/kernel/printk/alt_printk.c b/kernel/printk/alt_printk.c
> index 4bc1e7d..db0bfc8 100644
> --- a/kernel/printk/alt_printk.c
> +++ b/kernel/printk/alt_printk.c
> @@ -40,7 +40,6 @@
>   * were handled or when IRQs are blocked.
>   */
>  static int alt_printk_irq_ready;
> -atomic_t nmi_message_lost;
>  
>  #define ALT_LOG_BUF_LEN ((1 << CONFIG_ALT_PRINTK_LOG_BUF_SHIFT) -	\
>  			 sizeof(atomic_t) - sizeof(struct irq_work))
> @@ -50,12 +49,16 @@ struct alt_printk_seq_buf {
>  	struct irq_work		work;	/* IRQ work that flushes the buffer */
>  	unsigned char		buffer[ALT_LOG_BUF_LEN];
>  };
> -static DEFINE_PER_CPU(struct alt_printk_seq_buf, nmi_print_seq);
>  
>  static DEFINE_PER_CPU(struct alt_printk_seq_buf, alt_print_seq);
>  static DEFINE_PER_CPU(int, alt_printk_ctx);
>  static DEFINE_PER_CPU(unsigned long, alt_printk_irq_flags);
>  
> +#ifdef CONFIG_PRINTK_NMI
> +static DEFINE_PER_CPU(struct alt_printk_seq_buf, nmi_print_seq);
> +atomic_t nmi_message_lost;
> +#endif
> +
>  static int alt_printk_log_store(struct alt_printk_seq_buf *s,
>  		const char *fmt, va_list args)
>  {
> @@ -204,8 +207,12 @@ void alt_printk_flush(void)
>  {
>  	int cpu;
>  
> -	for_each_possible_cpu(cpu)
> +	for_each_possible_cpu(cpu) {
> +#ifdef CONFIG_PRINTK_NMI
>  		__alt_printk_flush(&per_cpu(nmi_print_seq, cpu).work);
> +#endif
> +		__alt_printk_flush(&per_cpu(alt_print_seq, cpu).work);

This line should be added already in the 3rd patch.

> +	}
>  }
>  
>  /**
> @@ -235,6 +242,8 @@ void alt_printk_flush_on_panic(void)
>  	alt_printk_flush();
>  }
>  
> +#ifdef CONFIG_PRINTK_NMI
> +
>  /*
>   * Safe printk() for NMI context. It uses a per-CPU buffer to
>   * store the message. NMIs are not nested, so there is always only
> @@ -263,6 +272,15 @@ void printk_nmi_exit(void)
>  	this_cpu_and(alt_printk_ctx, ~ALT_PRINTK_NMI_CONTEXT_MASK);
>  }
>  
> +#else
> +
> +static int vprintk_nmi(const char *fmt, va_list args)
> +{
> +	return 0;
> +}
> +
> +#endif /* CONFIG_PRINTK_NMI */
> +
>  /*
>   * Lockless printk(), to avoid deadlocks should the printk() recurse
>   * into itself. It uses a per-CPU buffer to store the message, just like
> @@ -317,12 +335,14 @@ void __init alt_printk_init(void)
>  	int cpu;
>  
>  	for_each_possible_cpu(cpu) {
> -		struct alt_printk_seq_buf *s = &per_cpu(nmi_print_seq, cpu);
> +		struct alt_printk_seq_buf *s = &per_cpu(alt_print_seq, cpu);
>  
>  		init_irq_work(&s->work, __alt_printk_flush);
>  
> -		s = &per_cpu(alt_print_seq, cpu);
> +#ifdef CONFIG_PRINTK_NMI
> +		s = &per_cpu(nmi_print_seq, cpu);
>  		init_irq_work(&s->work, __alt_printk_flush);
> +#endif

Also the initialization of both irq works should be done already
in the 3rd patch.


BTW: I have tried to simulate the compilation on an architecture
without NMI (commented out HAVE_NMI in arch/x86/Kconfig
and I have got the following build error with the first three
patches only:

  LD      init/built-in.o
kernel/built-in.o: In function `vprintk':
/prace/kernel/linux/kernel/printk/printk.c:1914: undefined reference to `vprintk_func'
kernel/built-in.o: In function `printk':
/prace/kernel/linux/kernel/printk/printk.c:1976: undefined reference to `vprintk_func'
Makefile:949: recipe for target 'vmlinux' failed


Where the two lines are in:

asmlinkage int vprintk(const char *fmt, va_list args)
{
	return vprintk_func(fmt, args);
}

and

asmlinkage __visible int printk(const char *fmt, ...)
{
	va_list args;
	int r;

	va_start(args, fmt);
	r = vprintk_func(fmt, args);
	va_end(args);

	return r;
}


I know that it is a pain to make it all correctly. I suffered from headaches
when preparing the WARN_*DEFERRED() patchset :-)


Best Regards,
Petr