printk: ignore recursion_bug flag when MCE in progress

From: ShuoX Liu <[email protected]>

When MCE happens in printk, we ignore recursion_bug to make sure some MCE logs printed out. Re-use mce_entry variable.

Signed-off-by Yanmin Zhang <[email protected]>

Signed-off-by ShuoX Liu <[email protected]>

---
I found mce_entry was introduced by commit 553f265f, but it's not used now. Why not removed?

---
 arch/x86/include/asm/mce.h       |    2 --
 arch/x86/kernel/cpu/mcheck/mce.c |    2 --
 include/linux/kernel.h           |    1 +
 kernel/printk.c                  |    4 +++-
 4 files changed, 4 insertions(+), 5 deletions(-)

arch/x86/include/asm/mce.h +0 -2

index 441520e..aeda4cc 100644
@@ -187,8 +187,6 @@ int mce_available(struct cpuinfo_x86 *c);
 DECLARE_PER_CPU(unsigned, mce_exception_count);
 DECLARE_PER_CPU(unsigned, mce_poll_count);
 
-extern atomic_t mce_entry;
-
 typedef DECLARE_BITMAP(mce_banks_t, MAX_NR_BANKS);
 DECLARE_PER_CPU(mce_banks_t, mce_poll_banks);
 

arch/x86/kernel/cpu/mcheck/mce.c +0 -2

index 11c9166..6073354 100644
@@ -61,8 +61,6 @@ int mce_disabled __read_mostly;
 
 #define SPINUNIT 100	/* 100ns */
 
-atomic_t mce_entry;
-
 DEFINE_PER_CPU(unsigned, mce_exception_count);
 
 /*

include/linux/kernel.h +1 -0

index 645231c..24af685 100644
@@ -354,6 +354,7 @@ unsigned long int_sqrt(unsigned long);
 extern void bust_spinlocks(int yes);
 extern void wake_up_klogd(void);
 extern int oops_in_progress;		/* If set, an oops, panic(), BUG() or die() is in progress */
+extern atomic_t mce_entry;
 extern int panic_timeout;
 extern int panic_on_oops;
 extern int panic_on_unrecovered_nmi;

kernel/printk.c +1 -0

index 473afdb..2bae087 100644
@@ -79,6 +79,7 @@ int console_printk[4] = {
 int oops_in_progress;
 EXPORT_SYMBOL(oops_in_progress);
 
+atomic_t mce_entry;
 /*
  * console_sem protects the console_drivers list, and also
  * provides serialisation for access to the entire console

+ Tony

Quoted 63 lines · ShuoX Liu

From: ShuoX Liu <[email protected]>

When MCE happens in printk, we ignore recursion_bug to make sure some MCE logs printed out. Re-use mce_entry variable.

Signed-off-by Yanmin Zhang <[email protected]>

Signed-off-by ShuoX Liu <[email protected]>

---
I found mce_entry was introduced by commit 553f265f, but it's not used now. Why not removed?

---
 arch/x86/include/asm/mce.h       |    2 --
 arch/x86/kernel/cpu/mcheck/mce.c |    2 --
 include/linux/kernel.h           |    1 +
 kernel/printk.c                  |    4 +++-
 4 files changed, 4 insertions(+), 5 deletions(-)

arch/x86/include/asm/mce.h +0 -2

index 441520e..aeda4cc 100644
@@ -187,8 +187,6 @@ int mce_available(struct cpuinfo_x86 *c);
 DECLARE_PER_CPU(unsigned, mce_exception_count);
 DECLARE_PER_CPU(unsigned, mce_poll_count);
 
-extern atomic_t mce_entry;
-
 typedef DECLARE_BITMAP(mce_banks_t, MAX_NR_BANKS);
 DECLARE_PER_CPU(mce_banks_t, mce_poll_banks);
 

arch/x86/kernel/cpu/mcheck/mce.c +0 -2

index 11c9166..6073354 100644
@@ -61,8 +61,6 @@ int mce_disabled __read_mostly;
 
 #define SPINUNIT 100	/* 100ns */
 
-atomic_t mce_entry;
-
 DEFINE_PER_CPU(unsigned, mce_exception_count);
 
 /*

include/linux/kernel.h +1 -0

index 645231c..24af685 100644
@@ -354,6 +354,7 @@ unsigned long int_sqrt(unsigned long);
 extern void bust_spinlocks(int yes);
 extern void wake_up_klogd(void);
 extern int oops_in_progress;		/* If set, an oops, panic(), BUG() or die() is in progress */
+extern atomic_t mce_entry;
 extern int panic_timeout;
 extern int panic_on_oops;
 extern int panic_on_unrecovered_nmi;

kernel/printk.c +0 -0

index 473afdb..2bae087 100644
@@ -79,6 +79,7 @@ int console_printk[4] = {
 int oops_in_progress;
 EXPORT_SYMBOL(oops_in_progress);
 

kernel/printk.c asmlinkage int vprintk(const char *fmt, va_list args) · line 868

Quoted 10 lines

kernel/printk.c +2 -1

+atomic_t mce_entry;
 /*
  * console_sem protects the console_drivers list, and also
  * provides serialisation for access to the entire console

@@ -864,7 +865,8 @@ asmlinkage int vprintk(const char *fmt, va_list args)
 		 * recursion and return - but flag the recursion so that
 		 * it can be printed at the next appropriate moment:
 		 */
-		if (!oops_in_progress && !lockdep_recursing(current)) {
+		if (!oops_in_progress && !atomic_read(&mce_entry)

This is leaking x86-specific (MCE) stuff in generic kernel code. I think it would be more appropriate to add a in_hw_error() helper or similar and define it on each arch. I can very well imagine other architectures would like to print hw error info too...

From: ShuoX Liu <[email protected]>

When MCE happens in printk, we ignore recursion_bug to make sure MCE logs printed out.

According to Boris' suggestion, we add some helper functions.
1) hw_error_enter: Call it when specific arch begins to process a hardware error.
2) hw_error_exit: Call it when specific arch finishes the processing of a hardware error.
3) in_hw_error():indicates whether HW error handling is in processing.

Each arch could call the helpers in their arch-dependent HW error handlers.

Signed-off-by Yanmin Zhang <[email protected]>

Signed-off-by ShuoX Liu <[email protected]>

---
 arch/x86/include/asm/mce.h       |    2 --
 arch/x86/kernel/cpu/mcheck/mce.c |    6 ++----
 include/linux/cpu.h              |   17 +++++++++++++++++
 kernel/cpu.c                     |    3 +++
 kernel/printk.c                  |    3 ++-
 5 files changed, 24 insertions(+), 7 deletions(-)

arch/x86/include/asm/mce.h +0 -2

index 441520e..aeda4cc 100644
@@ -187,8 +187,6 @@ int mce_available(struct cpuinfo_x86 *c);
 DECLARE_PER_CPU(unsigned, mce_exception_count);
 DECLARE_PER_CPU(unsigned, mce_poll_count);
 
-extern atomic_t mce_entry;
-
 typedef DECLARE_BITMAP(mce_banks_t, MAX_NR_BANKS);
 DECLARE_PER_CPU(mce_banks_t, mce_poll_banks);
 

arch/x86/kernel/cpu/mcheck/mce.c +2 -4

index 2afcbd2..aaf41d2 100644
@@ -61,8 +61,6 @@ int mce_disabled __read_mostly;
 
 #define SPINUNIT 100	/* 100ns */
 
-atomic_t mce_entry;
-
 DEFINE_PER_CPU(unsigned, mce_exception_count);
 
 /*

@@ -1015,7 +1013,7 @@ void do_machine_check(struct pt_regs *regs, long error_code)
 	DECLARE_BITMAP(toclear, MAX_NR_BANKS);
 	char *msg = "Unknown";
 
-	atomic_inc(&mce_entry);
+	hw_error_enter();
 
 	this_cpu_inc(mce_exception_count);
 

@@ -1143,7 +1141,7 @@ void do_machine_check(struct pt_regs *regs, long error_code)
 		mce_report_event(regs);
 	mce_wrmsrl(MSR_IA32_MCG_STATUS, 0);
 out:
-	atomic_dec(&mce_entry);
+	hw_error_exit();
 	sync_core();
 }
 EXPORT_SYMBOL_GPL(do_machine_check);

include/linux/cpu.h +6 -0

index 7230bb5..beb56d0 100644
@@ -210,4 +210,21 @@ static inline int disable_nonboot_cpus(void) { return 0; }
 static inline void enable_nonboot_cpus(void) {}
 #endif /* !CONFIG_PM_SLEEP_SMP */
 
+/* HW error handle status helpers */
+extern atomic_t hw_error;
+static inline void hw_error_enter(void)
+{
+	atomic_inc(&hw_error);
+}

Quoted 12 lines

include/linux/cpu.h +12 -0

+/* HW error handle status helpers */
+extern atomic_t hw_error;
+static inline void hw_error_enter(void)
+{
+	atomic_inc(&hw_error);
+}
+
+static inline void hw_error_exit(void)
+{
+	atomic_dec(&hw_error);
+}
+

include/linux/cpu.h static inline int disable_nonboot_cpus(void) { return 0; } · line 228

Quoted 4 lines

include/linux/cpu.h +4 -0

+static inline int in_hw_error(void)
+{
+	return atomic_read(&hw_error);
+}

Shouldn't those be generic empty functions and each arch implement their own with the stuff they want to do on the respective architecture when they get a hardware error?

Quoted 12 lines · On Thu, 24 May 2012 08:11:45 +0200 Borislav Petkov <[email protected]> wrote:

Quoted 12 lines

include/linux/cpu.h +12 -0

+/* HW error handle status helpers */
+extern atomic_t hw_error;
+static inline void hw_error_enter(void)
+{
+	atomic_inc(&hw_error);
+}
+
+static inline void hw_error_exit(void)
+{
+	atomic_dec(&hw_error);
+}
+

Quoted 8 lines

include/linux/cpu.h static inline int disable_nonboot_cpus(void) { return 0; } · line 228

Quoted 4 lines

include/linux/cpu.h +4 -0

+static inline int in_hw_error(void)
+{
+	return atomic_read(&hw_error);
+}

Shouldn't those be generic empty functions and each arch implement their own with the stuff they want to do on the respective architecture when they get a hardware error?

This code needs documentation.

Specifically, it should clearly explain (and hence define) what a "hardware error" *is*, and for what purpose this code exists.

Because as it stands, this interface is hopelessly vague. Once one sees that it is *specifically* used for handling mce within a printk, it all makes sense.

And with that understanding comes the realisation that the interface is poorly named. It will not be used for any purpose other than adjusting printk() behavior so it should mention printk() in its name and in its comments and probably it should all be moved into printk.h.

Futhermore, this code is not really related to MCE or hardware or anything else. It is simply a way in which callers can suppress printk()'s recursion check. Callers are free to use it for reasons other than "hardware errors".

And once all that is done, and this interface becomes part of printk() then no, there is no need to add per-arch hooks. An arch can call into printk_recursion_check_disable() and printk_recursion_chack_enable() - nice and simple.

IOW, the title of this patch should be

[patch 1/2] printk: add interface for disabling recursion check [patch 2/2] x86 mce: use new printk recursion disabling interface

From: ShuoX Liu <[email protected]>

Disable printk recursion to make sure MCE logs printed out.

Signed-off-by Yanmin Zhang <[email protected]>

Signed-off-by ShuoX Liu <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

arch/x86/kernel/cpu/mcheck/mce.c +1 -0

index 2afcbd2..365c35d 100644
@@ -1015,6 +1015,7 @@ void do_machine_check(struct pt_regs *regs, long error_code)
 	DECLARE_BITMAP(toclear, MAX_NR_BANKS);
 	char *msg = "Unknown";
 
+	printk_recursion_check_disable();
 	atomic_inc(&mce_entry);
 
 	this_cpu_inc(mce_exception_count);

Quoted 21 lines · ShuoX Liu

From: ShuoX Liu <[email protected]>

Disable printk recursion to make sure MCE logs printed out.

Signed-off-by Yanmin Zhang <[email protected]>

Signed-off-by ShuoX Liu <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

arch/x86/kernel/cpu/mcheck/mce.c +1 -0

index 2afcbd2..365c35d 100644
@@ -1015,6 +1015,7 @@ void do_machine_check(struct pt_regs *regs, long error_code)
 	DECLARE_BITMAP(toclear, MAX_NR_BANKS);
 	char *msg = "Unknown";
 
+	printk_recursion_check_disable();
 	atomic_inc(&mce_entry);
 

arch/x86/kernel/cpu/mcheck/mce.c void do_machine_check(struct pt_regs *regs, long error_code) · line 1148

Quoted 6 lines

arch/x86/kernel/cpu/mcheck/mce.c +1 -0

 	this_cpu_inc(mce_exception_count);

@@ -1144,6 +1145,7 @@ void do_machine_check(struct pt_regs *regs, long error_code)
 	mce_wrmsrl(MSR_IA32_MCG_STATUS, 0);
 out:
 	atomic_dec(&mce_entry);
+	printk_recursion_check_enable();

Looks like those should be at the beginning and the end of print_mce() - do_machine_check() could exit without printing an MCE and disabling the recursion check then is superfluous, methinks.

Thanks.

Quoted 21 lines · ShuoX Liu

From: ShuoX Liu <[email protected]>

Disable printk recursion to make sure MCE logs printed out.

Signed-off-by Yanmin Zhang <[email protected]>

Signed-off-by ShuoX Liu <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

arch/x86/kernel/cpu/mcheck/mce.c +1 -0

index 2afcbd2..365c35d 100644
@@ -1015,6 +1015,7 @@ void do_machine_check(struct pt_regs *regs, long error_code)
 	DECLARE_BITMAP(toclear, MAX_NR_BANKS);
 	char *msg = "Unknown";
 
+	printk_recursion_check_disable();
 	atomic_inc(&mce_entry);
 

arch/x86/kernel/cpu/mcheck/mce.c void do_machine_check(struct pt_regs *regs, long error_code) · line 1148

Quoted 6 lines

arch/x86/kernel/cpu/mcheck/mce.c +1 -0

 	this_cpu_inc(mce_exception_count);

@@ -1144,6 +1145,7 @@ void do_machine_check(struct pt_regs *regs, long error_code)
 	mce_wrmsrl(MSR_IA32_MCG_STATUS, 0);
 out:
 	atomic_dec(&mce_entry);
+	printk_recursion_check_enable();

Looks like those should be at the beginning and the end of print_mce() - do_machine_check() could exit without printing an MCE and disabling the recursion check then is superfluous, methinks.

Thanks.

Boris,
I checked code and found some other functions in do_machine_check() also would printk something. Such as add_taint(). So i think we'd better place the recursion check at the beginning and the end of do_machine_check(). Also more printks later(maybe) added will benefit from this. Do you agree?

Quoted 6 lines · ShuoX Liu

Boris,
I checked code and found some other functions in do_machine_check() also would printk something. Such as add_taint(). So i think we'd better place the recursion check at the beginning and the end of do_machine_check(). Also more printks later(maybe) added will benefit from this. Do you agree?

I'm not sure we want to disable printk recursion for add_taint() - it doesn't spit out any useful information wrt MCE so we could ignore it.

Btw, I forgot to ask: this printk recursion disabling, do you have a real usecase where you don't get the MCE info in dmesg and with your patch it works or is this purely hypothetical?

From: ShuoX Liu <[email protected]>

Disable printk recursion to make sure MCE logs printed out.

Signed-off-by Yanmin Zhang <[email protected]>

Signed-off-by ShuoX Liu <[email protected]>

---
We hit it when running a MTBF testing on a Android atom mobile.

---
 arch/x86/kernel/cpu/mcheck/mce.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

arch/x86/kernel/cpu/mcheck/mce.c +1 -0

index 2afcbd2..906e838 100644
@@ -306,6 +306,7 @@ static void mce_panic(char *msg, struct mce *final, char *exp)
 {
 	int i, apei_err = 0;
 
+	printk_recursion_check_disable();
 	if (!fake_panic) {
 		/*
 		 * Make sure only one CPU runs in machine check panic

A couple of things here.

a) mce_panic() has a "return" statement deep inside. So we return from mce_panic() with the recursion check disabled. whoops.

b) adding a nice comment is nice.

kernel/printk.c +4 -0

+void printk_recursion_check_enable(void)
+{
+	atomic_dec(&recursion_check_disabled);
+}

Is it worth a BUG_ON() in here to check that recursion_check_disabled is >=1 before blindly decrementing it? Or is this interface so simple that nobody would ever get this wrong?

From: ShuoX Liu <[email protected]>

Disable printk recursion to make sure MCE logs printed out.

Signed-off-by Yanmin Zhang <[email protected]>

Signed-off-by ShuoX Liu <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

arch/x86/kernel/cpu/mcheck/mce.c +1 -0

index 2afcbd2..365c35d 100644
@@ -1015,6 +1015,7 @@ void do_machine_check(struct pt_regs *regs, long error_code)
 	DECLARE_BITMAP(toclear, MAX_NR_BANKS);
 	char *msg = "Unknown";
 
+	printk_recursion_check_disable();
 	atomic_inc(&mce_entry);
 
 	this_cpu_inc(mce_exception_count);

Quoted 21 lines · ShuoX Liu

From: ShuoX Liu <[email protected]>

Disable printk recursion to make sure MCE logs printed out.

Signed-off-by Yanmin Zhang <[email protected]>

Signed-off-by ShuoX Liu <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

arch/x86/kernel/cpu/mcheck/mce.c +1 -0

index 2afcbd2..365c35d 100644
@@ -1015,6 +1015,7 @@ void do_machine_check(struct pt_regs *regs, long error_code)
 	DECLARE_BITMAP(toclear, MAX_NR_BANKS);
 	char *msg = "Unknown";
 
+	printk_recursion_check_disable();
 	atomic_inc(&mce_entry);
 

arch/x86/kernel/cpu/mcheck/mce.c void do_machine_check(struct pt_regs *regs, long error_code) · line 1149

Quoted 7 lines

arch/x86/kernel/cpu/mcheck/mce.c +1 -0

 	this_cpu_inc(mce_exception_count);

@@ -1144,6 +1145,7 @@ void do_machine_check(struct pt_regs *regs, long error_code)
 	mce_wrmsrl(MSR_IA32_MCG_STATUS, 0);
 out:
 	atomic_dec(&mce_entry);
+	printk_recursion_check_enable();
 	sync_core();

This is still adding the recursion check things in do_machine_check instead of in print_mce.

Also, this commit message above needs more explanation why we want to disable the recursion check on an MCE, maybe an example...

You are seeing an old patch. The new patches V4 are:
https://lkml.org/lkml/2012/6/3/176
https://lkml.org/lkml/2012/6/3/177

The new patches change mce_panic instead of do_machine_check.

Quoted 2 lines

Also, this commit message above needs more explanation why we want to disable the recursion check on an MCE, maybe an example...

V4 doesn't have the comment. We would add it in V5.

From: ShuoX Liu <[email protected]>

On x86 machines, some times MCE happens just when kernel calls printk to output some log info to serial console, while usually MCE module in kernel is used to print out some hardware error information, such like bad cache or bad memory bank. That causes printk recursion and printk would omit MCE printk output.

We hit it when running MTBF testing on Android ATOM mobiles.

Here in mce_panic, we choose to disable printk recursion to make sure MCE logs printed out.

Signed-off-by Yanmin Zhang <[email protected]>

Signed-off-by ShuoX Liu <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

arch/x86/kernel/cpu/mcheck/mce.c +1 -0

index 2afcbd2..906e838 100644
@@ -306,6 +306,7 @@ static void mce_panic(char *msg, struct mce *final, char *exp)
 {
 	int i, apei_err = 0;
 
+	printk_recursion_check_disable();
 	if (!fake_panic) {
 		/*
 		 * Make sure only one CPU runs in machine check panic

Quoted 27 lines · ShuoX Liu

From: ShuoX Liu <[email protected]>

On x86 machines, some times MCE happens just when kernel calls printk to output some log info to serial console, while usually MCE module in kernel is used to print out some hardware error information, such like bad cache or bad memory bank. That causes printk recursion and printk would omit MCE printk output.

We hit it when running MTBF testing on Android ATOM mobiles.

Here in mce_panic, we choose to disable printk recursion to make sure MCE logs printed out.

Signed-off-by Yanmin Zhang <[email protected]>

Signed-off-by ShuoX Liu <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

arch/x86/kernel/cpu/mcheck/mce.c +0 -0

index 2afcbd2..906e838 100644
@@ -306,6 +306,7 @@ static void mce_panic(char *msg, struct mce *final, char *exp)
 {
 	int i, apei_err = 0;
 

arch/x86/kernel/cpu/mcheck/mce.c static void mce_panic(char *msg, struct mce *final, char *exp) · line 364

Quoted 9 lines

arch/x86/kernel/cpu/mcheck/mce.c +2 -0

+	printk_recursion_check_disable();
 	if (!fake_panic) {
 		/*
 		 * Make sure only one CPU runs in machine check panic

@@ -360,6 +361,7 @@ static void mce_panic(char *msg, struct mce *final, char *exp)
 		panic(msg);
 	} else
 		pr_emerg(HW_ERR "Fake kernel panic: %s\n", msg);
+	printk_recursion_check_enable();

Ok, let me ask this again: why not disable the printk recursion check in the function that actually _prints_ the MCE, i.e. print_mce() instead of here in mce_panic() which does a whole bunch of other stuff and it can also return without printing any MCE to dmesg?

Are you interested in seeing the printk's from mce_panic? Why?

From: ShuoX Liu <[email protected]>

On x86 machines, some times MCE happens just when kernel calls printk to output some log info to serial console, while usually MCE module in kernel is used to print out some hardware error information, such like bad cache or bad memory bank. That causes printk recursion and printk would omit MCE printk output.

We hit it when running MTBF testing on Android ATOM mobiles.

Here in mce_panic, we choose to disable printk recursion to make sure MCE logs printed out.

Quoted 8 lines · ShuoX Liu

From: ShuoX Liu <[email protected]>

On x86 machines, some times MCE happens just when kernel calls printk to output some log info to serial console, while usually MCE module in kernel is used to print out some hardware error information, such like bad cache or bad memory bank. That causes printk recursion and printk would omit MCE printk output.

Quoted 4 lines

We hit it when running MTBF testing on Android ATOM mobiles.

Here in mce_panic, we choose to disable printk recursion to make sure MCE logs printed out.

Just a minor nitpick: this should say "print_mce" or you can simply remove the whole sentence - commit message is fine without it too.

From: ShuoX Liu <[email protected]>

On x86 machines, some times MCE happens just when kernel calls printk to output some log info to serial console, while usually MCE module in kernel is used to print out some hardware error information, such like bad cache or bad memory bank. That causes printk recursion and printk would omit MCE printk output.

We hit it when running MTBF testing on Android ATOM mobiles.

Here in print_mce, we choose to disable printk recursion to make sure MCE logs printed out.

Signed-off-by Yanmin Zhang <[email protected]>

Signed-off-by ShuoX Liu <[email protected]>

From: ShuoX Liu <[email protected]>

On x86 machines, some times MCE happens just when kernel calls printk to output some log info to serial console, while usually MCE module in kernel is used to print out some hardware error information, such like bad cache or bad memory bank. That causes printk recursion and printk would omit MCE printk output.

We hit it when running MTBF testing on Android ATOM mobiles.

Here in print_mce, we choose to disable printk recursion to make sure MCE logs printed out.

Signed-off-by Yanmin Zhang <[email protected]>

Signed-off-by ShuoX Liu <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

arch/x86/kernel/cpu/mcheck/mce.c +1 -0

index 2afcbd2..6056e94 100644
@@ -242,6 +242,7 @@ static void print_mce(struct mce *m)
 {
 	int ret = 0;
 
+	printk_recursion_check_disable();
 	pr_emerg(HW_ERR "CPU %d: Machine Check Exception: %Lx Bank %d: %016Lx\n",
 	       m->extcpu, m->mcgstatus, m->bank, m->status);
 

@@ -275,10 +276,13 @@ static void print_mce(struct mce *m)
 	 * (if the CPU has an implementation for that)

Quoted 41 lines · ShuoX Liu

From: ShuoX Liu <[email protected]>

On x86 machines, some times MCE happens just when kernel calls printk to output some log info to serial console, while usually MCE module in kernel is used to print out some hardware error information, such like bad cache or bad memory bank. That causes printk recursion and printk would omit MCE printk output.

We hit it when running MTBF testing on Android ATOM mobiles.

Here in print_mce, we choose to disable printk recursion to make sure MCE logs printed out.

Signed-off-by Yanmin Zhang <[email protected]>

Signed-off-by ShuoX Liu <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

arch/x86/kernel/cpu/mcheck/mce.c +4 -1

index 2afcbd2..6056e94 100644
@@ -242,6 +242,7 @@ static void print_mce(struct mce *m)
 {
 	int ret = 0;
 
+	printk_recursion_check_disable();
 	pr_emerg(HW_ERR "CPU %d: Machine Check Exception: %Lx Bank %d: %016Lx\n",
 	       m->extcpu, m->mcgstatus, m->bank, m->status);
 

@@ -275,10 +276,13 @@ static void print_mce(struct mce *m)
 	 * (if the CPU has an implementation for that)
 	 */
 	ret = atomic_notifier_call_chain(&x86_mce_decoder_chain, 0, m);
-	if (ret == NOTIFY_STOP)
+	if (ret == NOTIFY_STOP) {
+		printk_recursion_check_enable();
 		return;
+	}
 

arch/x86/kernel/cpu/mcheck/mce.c static void print_mce(struct mce *m) · line 285

Quoted 2 lines

arch/x86/kernel/cpu/mcheck/mce.c +1 -0

 	pr_emerg_ratelimited(HW_ERR "Run the above through 'mcelog --ascii'\n");
+	printk_recursion_check_enable();

Ok, this looks useful and it solves a real problem, but I'd prefer a better interface: instead of exposing the guts of printk to drivers in an unsafe manner (and allowing them to keep printk in an unsafe state indefinitely), shouldn't we instead introduce printk_emergency() (and variants) that just disable the recursion check, do the printk and then enable them?

That already does bust_spinlocks() which already increments the oops_in_progress thing, so WTF are you doing anyway?

From: ShuoX Liu <[email protected]>

With some special scenario, such as Machine Check Exception happened in printk, we want to bypass printk recursion check to printk some important information. So we add these interfaces of printk.

1) printk_recursion_check_disable() for disabling recursion check
2) printk_recursion_check_enable() for enabling recursion check

Signed-off-by Yanmin Zhang <[email protected]>

Signed-off-by ShuoX Liu <[email protected]>

---
 include/linux/printk.h |    3 +++
 kernel/printk.c        |   17 ++++++++++++++++-

 2 files changed, 19 insertions(+), 1 deletions(-)

include/linux/printk.h +3 -0

index 1bec2f7..da48ec7 100644
@@ -42,6 +42,9 @@ static inline void console_verbose(void)
                console_loglevel = 15;
  }
 
+void printk_recursion_check_disable(void);
+void printk_recursion_check_enable(void);
+
  struct va_format {
        const char *fmt;
        va_list *va;

kernel/printk.c +16 -2

index 32462d2..0580f67 100644
@@ -78,6 +78,19 @@ int console_printk[4] = {
  int oops_in_progress;
  EXPORT_SYMBOL(oops_in_progress);
 
+static atomic_t recursion_check_disabled = ATOMIC_INIT(0);
+
+void printk_recursion_check_disable(void)
+{
+       atomic_inc(&recursion_check_disabled);
+}
+
+void printk_recursion_check_enable(void)
+{
+       WARN_ON(atomic_read(&recursion_check_disabled) < 1);
+       atomic_dec(&recursion_check_disabled);
+}
+
  /*
  * console_sem protects the console_drivers list, and also
  * provides serialisation for access to the entire console

@@ -1295,7 +1308,9 @@ asmlinkage int vprintk_emit(int facility, int level,
                 * recursion and return - but flag the recursion so that
                 * it can be printed at the next appropriate moment:
                 */
-               if (!oops_in_progress && !lockdep_recursing(current)) {
+               if (!atomic_read(&recursion_check_disabled)
+                       && !oops_in_progress
+                       && !lockdep_recursing(current)) {
                        recursion_bug = 1;
                        goto out_restore_irqs;
                }
--

1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Should the two function be export by EXPORT_SYMBOL? The the other module can use it.

From: ShuoX Liu <[email protected]>

With some special scenario, such as Machine Check Exception happened in printk, we want to bypass printk recursion check to printk some important information. So we add these interfaces of printk.

1) printk_recursion_check_disable() for disabling recursion check
2) printk_recursion_check_enable() for enabling recursion check

Signed-off-by Yanmin Zhang <[email protected]>

Signed-off-by ShuoX Liu <[email protected]>