Use nmi_panic() in panic on NMI case
v3 cover Use nmi_panic() in panic on NMI case
commit 1717f2096b54 ("panic, x86: Fix re-entrance problem due to panic on NMI") and commit 58c5661f2144 ("panic, x86: Allow CPUs to save registers even if looping in NMI context") introduced nmi_panic() which prevents concurrent/recursive execution of panic(). It also saves registers for the crash dump on x86.
However, there are some cases where NMI handlers still use panic(). This patch set partially replaces them with nmi_panic() in those cases.
Changes since v2 (https://lkml.org/lkml/2016/3/2/173):
- Make nmi_panic receive a single string instead of printf style args (PATCH 1/3)
Changes since v1 (https://lkml.org/lkml/2016/2/29/858):
- Replace nmi_panic() macro with a function version instead of exporting symbols referred by the macro (PATCH 1/3)
- Improve the patch descriptions (PATCH 2/3 and 3/3)
- Do small cleanups (PATCH 3/3)
---
Even if applying this patch set, some NMI or similar handlers (e.g. MCE handler) remains to use panic(). This is because I can't test them well and actual problems won't happen. For example, the possibility that normal panic and panic on MCE happen simultaneously is very low.
Hidehiro Kawai (3):
panic: Change nmi_panic from macro to function ipmi/watchdog: Use nmi_panic() when kernel panics in NMI handler
hpwdt: Use nmi_panic() when kernel panics in NMI handler
drivers/char/ipmi/ipmi_watchdog.c | 2 +- drivers/watchdog/hpwdt.c | 11 +++++------ include/linux/kernel.h | 21 +-------------------- kernel/panic.c | 20 ++++++++++++++++++++ 4 files changed, 27 insertions(+), 27 deletions(-)
Signature · 2 lines
Hidehiro Kawai Hitachi, Ltd. Research & Development Group
PATCH v3 1/3 panic: Change nmi_panic from macro to function
Change nmi_panic() macro to a normal function for the portability. Also, export it for modules.
Changes since v2:
- Make nmi_panic receive a single string instead of printf style args
--- include/linux/kernel.h | 21 +-------------------- kernel/panic.c | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 20 deletions(-)
include/linux/kernel.h +1 -20
@@ -255,7 +255,7 @@ extern long (*panic_blink)(int state); __printf(1, 2)void panic (const char *fmt, ...)__noreturn __cold ; -void nmi_panic_self_stop (struct pt_regs *); +void nmi_panic (struct pt_regs *regs,const char *msg);extern void oops_enter (void );extern void oops_exit (void );void print_oops_end_marker (void );
@@ -455,25 +455,6 @@ extern atomic_t panic_cpu;#define PANIC_CPU_INVALID -1/* -* A variant of panic() called from NMI context. We return if we've already -* panicked on this CPU. If another CPU already panicked, loop in -* nmi_panic_self_stop() which can provide architecture dependent code such -* as saving register state for crash dump. -*/ -#define nmi_panic (regs, fmt, ...) \ -do { \ - int old_cpu, cpu; \ - \ - cpu = raw_smp_processor_id(); \ - old_cpu = atomic_cmpxchg(&panic_cpu, PANIC_CPU_INVALID, cpu); \ - \ - if (old_cpu == PANIC_CPU_INVALID) \ - panic(fmt, ##__VA_ARGS__); \ - else if (old_cpu != cpu) \ - nmi_panic_self_stop(regs); \ -} while (0) - -/* * Only to be used by arch init code. If the user over-wrote the default * CONFIG_PANIC_TIMEOUT, honor it. */
kernel/panic.c +20 -0
@@ -72,6 +72,26 @@ void __weak nmi_panic_self_stop(struct pt_regs *regs)atomic_t panic_cpu =ATOMIC_INIT (PANIC_CPU_INVALID ); +/* +* A variant of panic() called from NMI context. We return if we've already +* panicked on this CPU. If another CPU already panicked, loop in +* nmi_panic_self_stop() which can provide architecture dependent code such +* as saving register state for crash dump. +*/ +void nmi_panic (struct pt_regs *regs,const char *msg) +{ +int old_cpu, cpu; + + cpu =raw_smp_processor_id (); + old_cpu =atomic_cmpxchg (&panic_cpu,PANIC_CPU_INVALID , cpu); + +if (old_cpu ==PANIC_CPU_INVALID ) +panic ("%s" , msg); +else if (old_cpu != cpu) +nmi_panic_self_stop (regs); +} +EXPORT_SYMBOL (nmi_panic); +/** * panic - halt the system * @fmt: The text string to print
Re: [v3 PATCH 1/3] panic: Change nmi_panic from macro to function
Quoted 1 line · Hidehiro Kawai
Change
nmi_panic()macro to a normal function for the portability.
portability?
Quoted 19 lines
Also, export it for modules.
Changes since v2:
- Makenmi_panicreceive a single string instead of printf style argsSigned-off-by Hidehiro Kawai <[email protected]>Cc Andrew Morton <[email protected]>Cc Borislav Petkov <[email protected]>Cc Michal Nazarewicz <[email protected]>Cc Michal Hocko <[email protected]>Cc Rasmus Villemoes <[email protected]>Cc Nicolas Iooss <[email protected]>Cc Javi Merino <[email protected]>Cc Gobinda Charan Maji <[email protected]>Cc "Steven Rostedt (Red Hat)" <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Vitaly Kuznetsov <[email protected]>Cc HATAYAMA Daisuke <[email protected]>Cc Tejun Heo <[email protected]>
Quoted 4 lines
--- include/linux/kernel.h | 21 +-------------------- kernel/panic.c | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 20 deletions(-)
Signature · 4 lines
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.RE: [v3 PATCH 1/3] panic: Change nmi_panic from macro to function
Quoted 5 lines
From: Borislav Petkov [mailto:[email protected]]
Quoted 1 line · Hidehiro Kawai
Change
nmi_panic()macro to a normal function for the portability.portability?
I wanted to say encapsulating things into a function makes modules only have to know about the function. Modules don't need to know all things in the macro. But I thought again, and `portability' was not appropriate.
However, this patch set has been queued into -mm now. So I'll leave this if not necessary.
Regards,
Hidehiro Kawai
Quoted 31 lines
Quoted 19 lines
Also, export it for modules.
Changes since v2:
- Makenmi_panicreceive a single string instead of printf style argsSigned-off-by Hidehiro Kawai <[email protected]>Cc Andrew Morton <[email protected]>Cc Borislav Petkov <[email protected]>Cc Michal Nazarewicz <[email protected]>Cc Michal Hocko <[email protected]>Cc Rasmus Villemoes <[email protected]>Cc Nicolas Iooss <[email protected]>Cc Javi Merino <[email protected]>Cc Gobinda Charan Maji <[email protected]>Cc "Steven Rostedt (Red Hat)" <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Vitaly Kuznetsov <[email protected]>Cc HATAYAMA Daisuke <[email protected]>Cc Tejun Heo <[email protected]>Quoted 4 lines
--- include/linux/kernel.h | 21 +-------------------- kernel/panic.c | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 20 deletions(-)Acked-by Borislav Petkov <[email protected]>--
Regards/Gruss,
Boris.ECO tip #101: Trim your mails when you reply.
Re: [v3 PATCH 1/3] panic: Change nmi_panic from macro to function
Quoted 6 lines · 河合英宏 / KAWAI，HIDEHIRO
Quoted 5 lines
From: Borislav Petkov [mailto:[email protected]]
Quoted 1 line · Hidehiro Kawai
Change
nmi_panic()macro to a normal function for the portability.portability?
Quoted 7 lines
I wanted to say encapsulating things into a function makes modules only have to know about the function. Modules don't need to know all things in the macro. But I thought again, and `portability' was not appropriate.
However, this patch set has been queued into -mm now. So I'll leave this if not necessary.
Unless Andrew is willing to do a quick "quilt header -e" on your patch and paste in a corrected commit message text which you give him...
:-)
Signature · 8 lines
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.htmlRe: [v3 PATCH 1/3] panic: Change nmi_panic from macro to function
In reply to Hidehiro KawaiQuoted 2 lines · Hidehiro Kawai
Change
nmi_panic()macro to a normal function for the portability. Also, export it for modules.
I guess you wanted to say
"
Change nmi_panic() from macro to a normal function so that it can be exported to modules. At least ipmi and hpwdt watchdogs can be compiled as a module and need to panic from the NMI context.
"
Quoted 4 lines
Changes since v2:
- Makenmi_panicreceive a single string instead of printf style args
Quoted 14 lines
Signed-off-by Hidehiro Kawai <[email protected]>Cc Andrew Morton <[email protected]>Cc Borislav Petkov <[email protected]>Cc Michal Nazarewicz <[email protected]>Cc Michal Hocko <[email protected]>Cc Rasmus Villemoes <[email protected]>Cc Nicolas Iooss <[email protected]>Cc Javi Merino <[email protected]>Cc Gobinda Charan Maji <[email protected]>Cc "Steven Rostedt (Red Hat)" <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Vitaly Kuznetsov <[email protected]>Cc HATAYAMA Daisuke <[email protected]>Cc Tejun Heo <[email protected]>
For the change itself
Quoted 77 lines
--- include/linux/kernel.h | 21 +-------------------- kernel/panic.c | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 20 deletions(-)include/linux/kernel.h +1
-20@@ -255,7 +255,7 @@ extern long (*panic_blink)(int state); __printf(1, 2)void panic (const char *fmt, ...)__noreturn __cold ; -void nmi_panic_self_stop (struct pt_regs *); +void nmi_panic (struct pt_regs *regs,const char *msg);extern void oops_enter (void );extern void oops_exit (void );void print_oops_end_marker (void );@@ -455,25 +455,6 @@ extern atomic_t panic_cpu;#define PANIC_CPU_INVALID -1/* -* A variant of panic() called from NMI context. We return if we've already -* panicked on this CPU. If another CPU already panicked, loop in -* nmi_panic_self_stop() which can provide architecture dependent code such -* as saving register state for crash dump. -*/ -#define nmi_panic (regs, fmt, ...) \ -do { \ - int old_cpu, cpu; \ - \ - cpu = raw_smp_processor_id(); \ - old_cpu = atomic_cmpxchg(&panic_cpu, PANIC_CPU_INVALID, cpu); \ - \ - if (old_cpu == PANIC_CPU_INVALID) \ - panic(fmt, ##__VA_ARGS__); \ - else if (old_cpu != cpu) \ - nmi_panic_self_stop(regs); \ -} while (0) - -/* * Only to be used by arch init code. If the user over-wrote the default * CONFIG_PANIC_TIMEOUT, honor it. */ kernel/panic.c +20
-0@@ -72,6 +72,26 @@ void __weak nmi_panic_self_stop(struct pt_regs *regs)atomic_t panic_cpu =ATOMIC_INIT (PANIC_CPU_INVALID ); +/* +* A variant of panic() called from NMI context. We return if we've already +* panicked on this CPU. If another CPU already panicked, loop in +* nmi_panic_self_stop() which can provide architecture dependent code such +* as saving register state for crash dump. +*/ +void nmi_panic (struct pt_regs *regs,const char *msg) +{ +int old_cpu, cpu; + + cpu =raw_smp_processor_id (); + old_cpu =atomic_cmpxchg (&panic_cpu,PANIC_CPU_INVALID , cpu); + +if (old_cpu ==PANIC_CPU_INVALID ) +panic ("%s" , msg); +else if (old_cpu != cpu) +nmi_panic_self_stop (regs); +} +EXPORT_SYMBOL (nmi_panic); +/** * panic - halt the system * @fmt: The text string to print
Signature · 2 lines
Michal Hocko SUSE Labs
Re: [v3 PATCH 1/3] panic: Change nmi_panic from macro to function
In reply to Hidehiro KawaiQuoted 10 lines · Hidehiro Kawai
Change
nmi_panic()macro to a normal function for the portability. Also, export it for modules.Changes since v2:
- Makenmi_panicreceive a single string instead of printf style argsSigned-off-by Hidehiro Kawai <[email protected]>Cc Andrew Morton <[email protected]>Cc Borislav Petkov <[email protected]>Cc Michal Nazarewicz <[email protected]>
Code does what the commit advertises so
Quoted 87 lines
Cc Michal Hocko <[email protected]>Cc Rasmus Villemoes <[email protected]>Cc Nicolas Iooss <[email protected]>Cc Javi Merino <[email protected]>Cc Gobinda Charan Maji <[email protected]>Cc "Steven Rostedt (Red Hat)" <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Vitaly Kuznetsov <[email protected]>Cc HATAYAMA Daisuke <[email protected]>Cc Tejun Heo <[email protected]>--- include/linux/kernel.h | 21 +------------------- kernel/panic.c | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 20 deletions(-)include/linux/kernel.h +1
-20@@ -255,7 +255,7 @@ extern long (*panic_blink)(int state); __printf(1, 2)void panic (const char *fmt, ...)__noreturn __cold ; -void nmi_panic_self_stop (struct pt_regs *); +void nmi_panic (struct pt_regs *regs,const char *msg);extern void oops_enter (void );extern void oops_exit (void );void print_oops_end_marker (void );@@ -455,25 +455,6 @@ extern atomic_t panic_cpu;#define PANIC_CPU_INVALID -1/* -* A variant of panic() called from NMI context. We return if we've already -* panicked on this CPU. If another CPU already panicked, loop in -* nmi_panic_self_stop() which can provide architecture dependent code such -* as saving register state for crash dump. -*/ -#define nmi_panic (regs, fmt, ...) \ -do { \ - int old_cpu, cpu; \ - \ - cpu = raw_smp_processor_id(); \ - old_cpu = atomic_cmpxchg(&panic_cpu, PANIC_CPU_INVALID, cpu); \ - \ - if (old_cpu == PANIC_CPU_INVALID) \ - panic(fmt, ##__VA_ARGS__); \ - else if (old_cpu != cpu) \ - nmi_panic_self_stop(regs); \ -} while (0) - -/* * Only to be used by arch init code. If the user over-wrote the default * CONFIG_PANIC_TIMEOUT, honor it. */ kernel/panic.c +20
-0@@ -72,6 +72,26 @@ void __weak nmi_panic_self_stop(struct pt_regs *regs)atomic_t panic_cpu =ATOMIC_INIT (PANIC_CPU_INVALID ); +/* +* A variant of panic() called from NMI context. We return if we've already +* panicked on this CPU. If another CPU already panicked, loop in +* nmi_panic_self_stop() which can provide architecture dependent code such +* as saving register state for crash dump. +*/ +void nmi_panic (struct pt_regs *regs,const char *msg) +{ +int old_cpu, cpu; + + cpu =raw_smp_processor_id (); + old_cpu =atomic_cmpxchg (&panic_cpu,PANIC_CPU_INVALID , cpu); + +if (old_cpu ==PANIC_CPU_INVALID ) +panic ("%s" , msg); +else if (old_cpu != cpu) +nmi_panic_self_stop (regs); +} +EXPORT_SYMBOL (nmi_panic); +/** * panic - halt the system * @fmt: The text string to print
Signature · 7 lines
Best regards ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ «If at first you don’t succeed, give up skydiving» -- To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v3 2/3 ipmi/watchdog: Use nmi_panic() when kernel panics in NMI handler
In reply to Hidehiro Kawaicommit 1717f2096b54 ("panic, x86: Fix re-entrance problem due to panic on NMI") introduced nmi_panic() which prevents concurrent and recursive execution of panic(). It also saves registers for the crash dump on x86 by later commit 58c5661f2144 ("panic, x86: Allow CPUs to save registers even if looping in NMI context").
ipmi_watchdog driver can call panic() from NMI handler, so replace it with nmi_panic().
--- drivers/char/ipmi/ipmi_watchdog.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/char/ipmi/ipmi_watchdog.c +1 -1
@@ -1140,7 +1140,7 @@ ipmi_nmi(unsigned int val, struct pt_regs *regs)the timer. So do so. */ pretimeout_since_last_heartbeat =1 ;if (atomic_inc_and_test (&preop_panic_excl)) -panic (PFX "pre-timeout" ); +nmi_panic (regs,PFX "pre-timeout" ); }return NMI_HANDLED ;
PATCH v3 3/3 hpwdt: Use nmi_panic() when kernel panics in NMI handler
In reply to Hidehiro Kawaicommit 1717f2096b54 ("panic, x86: Fix re-entrance problem due to panic on NMI") introduced nmi_panic() which prevents concurrent and recursive execution of panic(). It also saves registers for the crash dump on x86 by later commit 58c5661f2144 ("panic, x86: Allow CPUs to save registers even if looping in NMI context").
hpwdt driver can call panic() from NMI handler, so replace it with nmi_panic(). Also, do some cleanups.
Changes since V1:
- Use direct return instead of goto
- Combine the panic message string into a single line
--- drivers/watchdog/hpwdt.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
drivers/watchdog/hpwdt.c +5 -6
@@ -483,7 +483,7 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)static int die_nmi_called;if (!hpwdt_nmi_decoding) - goto out; +return NMI_DONE ;spin_lock_irqsave (&rom_lock, rom_pl);if (!die_nmi_called && !is_icru && !is_uefi)
@@ -496,11 +496,11 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)if (!is_icru && !is_uefi) {if (cmn_regs.u1.ral ==0 ) { -panic ("An NMI occurred, " -"but unable to determine source.\n" ); +nmi_panic (regs,"An NMI occurred, but unable to determine source.\n" ); +return NMI_HANDLED ; } } -panic ("An NMI occurred. Depending on your system the reason " +nmi_panic (regs,"An NMI occurred. Depending on your system the reason " "for the NMI is logged in any one of the following " "resources:\n" "1. Integrated Management Log (IML)\n"
@@ -508,8 +508,7 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)"3. OA Forward Progress Log\n" "4. iLO Event Log"); -out: - return NMI_DONE; + return NMI_HANDLED; }#endif /* CONFIG_HPWDT_NMI_DECODING */
Re: [v3 PATCH 3/3] hpwdt: Use nmi_panic() when kernel panics in NMI handler
Quoted 9 lines · Hidehiro Kawai
commit
1717f2096b54("panic, x86: Fix re-entrance problem due to panic on NMI") introducednmi_panic()which prevents concurrent and recursive execution ofpanic(). It also saves registers for the crash dump on x86 by later commit58c5661f2144("panic, x86: Allow CPUs to save registers even if looping in NMI context").hpwdt driver can call
panic()from NMI handler, so replace it withnmi_panic(). Also, do some cleanups.
Quoted 8 lines
Changes since V1:
- Use direct return instead of goto
- Combine the panic message string into a single lineSigned-off-by Hidehiro Kawai <[email protected]>Cc Thomas Mingarelli <[email protected]>Cc Wim Van Sebroeck <[email protected]>Cc Guenter Roeck <[email protected]>
Quoted 1 line
In case you resend the series, please drop this Cc: from the commit logs. Please see usage of Cc: in Documentation/SubmittingPatches.
Thanks,
Guenter
Quoted 45 lines
--- drivers/watchdog/hpwdt.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)drivers/watchdog/hpwdt.c +5
-6@@ -483,7 +483,7 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)static int die_nmi_called;if (!hpwdt_nmi_decoding) - goto out; +return NMI_DONE ;spin_lock_irqsave (&rom_lock, rom_pl);if (!die_nmi_called && !is_icru && !is_uefi)@@ -496,11 +496,11 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)if (!is_icru && !is_uefi) {if (cmn_regs.u1.ral ==0 ) { -panic ("An NMI occurred, " -"but unable to determine source.\n" ); +nmi_panic (regs,"An NMI occurred, but unable to determine source.\n" ); +return NMI_HANDLED ; } } -panic ("An NMI occurred. Depending on your system the reason " +nmi_panic (regs,"An NMI occurred. Depending on your system the reason " "for the NMI is logged in any one of the following " "resources:\n" "1. Integrated Management Log (IML)\n"@@ -508,8 +508,7 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)"3. OA Forward Progress Log\n" "4. iLO Event Log"); -out: - return NMI_DONE; + return NMI_HANDLED; }#endif /* CONFIG_HPWDT_NMI_DECODING */
RE: [Openipmi-developer] [v3 PATCH 3/3] hpwdt: Use nmi_panic() when kernel panics in NMI handler
In reply to Hidehiro KawaiQuoted 6 lines
From: Phil Pokorny [mailto:[email protected]]
While you are in
drivers/watchdog/hpwdt.creplacingpanic()withnmi_panic()can you also fix a few more of the broken strings? The Linux style guide specfiically says NOT to break strings that are user visible because it breaks "grep". So:
Ahh, yes. I should have fixed this as well as the former case. But this patch set has already been queued into -mm tree, so I'll leave this patch if the fix is not important.
Regards,
Hidehiro Kawai
Quoted 25 lines
drivers/watchdog/hpwdt.c +1
-1-panic ("An NMI occurred. Depending on your system the reason " +nmi_panic (regs,"An NMI occurred. Depending on your system the reason " "for the NMI is logged in any one of the following " "resources:\n" "1. Integrated Management Log (IML)\n"Should probably be:
(quoted diff) +1
-3- panic("An NMI occurred. Depending on your system the reason " - "for the NMI is logged in any one of the following " - "resources:\n" + nmi_panic(regs, "An NMI occurred. Depending on your system thereason for the NMI is logged in any one of the following resources:\n"
"1. Integrated Management Log (IML)\n"So it's okay to break on "\n", but not in the middle of a line.
--
Philip Pokorny, RHCE
Chief Technology Officer
PENGUIN COMPUTING, Inc
www.penguincomputing.comChanging the world through technical innovation