[Openipmi-developer] [v3 PATCH 2/3] ipmi/watchdog: Use nmi_panic() when kernel panics in NMI handler

[Hidehiro Kawai]

commit 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().

Signed-off-by: Hidehiro Kawai <***@hitachi.com>
Acked-by: Corey Minyard <***@mvista.com>
Acked-by: Guenter Roeck <***@roeck-us.net>
Reviewed-by: Michal Hocko <***@suse.com>
Cc: openipmi-***@lists.sourceforge.net
---
drivers/char/ipmi/ipmi_watchdog.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/ipmi/ipmi_watchdog.c b/drivers/char/ipmi/ipmi_watchdog.c
index 096f0ce..4facc75 100644
--- a/drivers/char/ipmi/ipmi_watchdog.c
+++ b/drivers/char/ipmi/ipmi_watchdog.c
@@ -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;

[Hidehiro Kawai]

commit 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

Signed-off-by: Hidehiro Kawai <***@hitachi.com>
Cc: Thomas Mingarelli <***@hpe.com>
Cc: Wim Van Sebroeck <***@iguana.be>
Cc: Guenter Roeck <***@roeck-us.net>
Cc: linux-***@vger.kernel.org
---
drivers/watchdog/hpwdt.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 92443c3..a6c8797 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -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 */

[Guenter Roeck]

Post by Hidehiro Kawai
commit 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.
- Use direct return instead of goto
- Combine the panic message string into a single 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

Post by Hidehiro Kawai
---
drivers/watchdog/hpwdt.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 92443c3..a6c8797 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -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");
- return NMI_DONE;
+ return NMI_HANDLED;
}
#endif /* CONFIG_HPWDT_NMI_DECODING */

[河合英宏 / KAWAI，HIDEHIRO]

While you are in drivers/watchdog/hpwdt.c replacing panic() with
nmi_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

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

- 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"
- 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 the
reason 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.com
Changing the world through technical innovation

[Borislav Petkov]

Change nmi_panic() macro to a normal function for the portability.

portability?

Also, export it for modules.
- 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(-)

Acked-by: Borislav Petkov <***@suse.de>

[河合英宏 / KAWAI，HIDEHIRO]

Post by Borislav Petkov

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

Post by Borislav Petkov

Also, export it for modules.
- 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(-)

--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.

[Borislav Petkov]

Post by 河合英宏 / KAWAI，HIDEHIRO

Post by Borislav Petkov

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.

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...

:-)

[Michal Hocko]

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.
"

- Make nmi_panic receive a single string instead of printf style args

For the change itself

---
include/linux/kernel.h | 21 +--------------------
kernel/panic.c | 20 ++++++++++++++++++++
2 files changed, 21 insertions(+), 20 deletions(-)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index f31638c..cbe7d70 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -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.
*/
diff --git a/kernel/panic.c b/kernel/panic.c
index d96469d..8abfc30 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -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

[Michal Nazarewicz]

Change nmi_panic() macro to a normal function for the portability.
Also, export it for modules.
- Make nmi_panic receive a single string instead of printf style args

Code does what the commit advertises so

---
include/linux/kernel.h | 21 +-------------------
kernel/panic.c | 20 ++++++++++++++++++++
2 files changed, 21 insertions(+), 20 deletions(-)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index f31638c..cbe7d70 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -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.
*/
diff --git a/kernel/panic.c b/kernel/panic.c
index d96469d..8abfc30 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -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