[PATCH 0/3] Use nmi_panic() in panic on NMI case

[PATCH 0/3] Use nmi_panic() in panic on NMI case

[Hidehiro Kawai]

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.

---
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: Export panic_cpu and nmi_panic_self_stop
      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          |   12 ++++++++++--
 kernel/panic.c                    |    2 ++
 3 files changed, 13 insertions(+), 3 deletions(-)


-- 
Hidehiro Kawai
Hitachi, Ltd. Research & Development Group

[PATCH 1/3] panic: Export panic_cpu and nmi_panic_self_stop

[Hidehiro Kawai]

Export panic_cpu and nmi_panic_self_stop symbols for modules which
use nmi_panic() macro.

Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Michal Hocko <mhocko@suse.com>
Cc: HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com>
Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: Tejun Heo <tj@kernel.org>
---
 kernel/panic.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/panic.c b/kernel/panic.c
index d96469d..f4e8035 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -69,8 +69,10 @@ void __weak nmi_panic_self_stop(struct pt_regs *regs)
 {
 	panic_smp_self_stop();
 }
+EXPORT_SYMBOL(nmi_panic_self_stop);
 
 atomic_t panic_cpu = ATOMIC_INIT(PANIC_CPU_INVALID);
+EXPORT_SYMBOL(panic_cpu);
 
 /**
  *	panic - halt the system

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

[Hidehiro Kawai]

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.

ipmi_watchdog driver can call panic() from NMI handler, so replace
it with nmi_panic().

Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
Cc: Corey Minyard <minyard@acm.org>
Cc: openipmi-developer@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;

[PATCH 3/3] hpwdt: Use nmi_panic() when kernel panics in NMI handler

[Hidehiro Kawai]

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.

hpwdt driver can call panic() from NMI handler, so replace it with
nmi_panic().

Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
Cc: Thomas Mingarelli <thomas.mingarelli@hpe.com>
Cc: Wim Van Sebroeck <wim@iguana.be>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: linux-watchdog@vger.kernel.org
---
 drivers/watchdog/hpwdt.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 92443c3..fd0486f 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -496,11 +496,12 @@ 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, "
+			nmi_panic(regs, "An NMI occurred, "
 				"but unable to determine source.\n");
+			goto 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,6 +509,13 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
 		"3. OA Forward Progress Log\n"
 		"4. iLO Event Log");
 
+handled:
+	/*
+	 * Returning from nmi_panic() means this CPU was processing panic()
+	 * before NMI.  Return NMI_HANDLED and go back to panic routines.
+	 */
+	return NMI_HANDLED;
+
 out:
 	return NMI_DONE;
 }

Re: [PATCH 2/3] ipmi/watchdog: Use nmi_panic() when kernel panics in NMI handler

[Corey Minyard]

Sure, this is a good idea.

Acked-by: Corey Minyard <cminyard@mvista.com>

Note that nmi_panic() came in commit 1717f2096b5 (panic, x86: Fix
re-entrance problem due to panic on NMI) and then the regs field
was added in the commit you reference.

Do you want me to add this to the IPMI queue or do you have another
way to get this patch into the kernel?

-corey

On 02/29/2016 07:50 PM, Hidehiro Kawai wrote:
> 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.
>
> ipmi_watchdog driver can call panic() from NMI handler, so replace
> it with nmi_panic().
>
> Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
> Cc: Corey Minyard <minyard@acm.org>
> Cc: openipmi-developer@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;
>
>

Re: [PATCH 3/3] hpwdt: Use nmi_panic() when kernel panics in NMI handler

[Guenter Roeck]

On 02/29/2016 05:50 PM, Hidehiro Kawai wrote:
> 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.
>
> hpwdt driver can call panic() from NMI handler, so replace it with
> nmi_panic().
>
> Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
> Cc: Thomas Mingarelli <thomas.mingarelli@hpe.com>
> Cc: Wim Van Sebroeck <wim@iguana.be>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: linux-watchdog@vger.kernel.org
> ---
>   drivers/watchdog/hpwdt.c |   12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> index 92443c3..fd0486f 100644
> --- a/drivers/watchdog/hpwdt.c
> +++ b/drivers/watchdog/hpwdt.c
> @@ -496,11 +496,12 @@ 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, "
> +			nmi_panic(regs, "An NMI occurred, "
>   				"but unable to determine source.\n");

This message should really be in a single line.
Sure, strictly speaking that should be done in a separate patch,
but since you touch the line you might as well fix it.

> +			goto handled;

The goto is unnecessary. Just 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,6 +509,13 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
>   		"3. OA Forward Progress Log\n"
>   		"4. iLO Event Log");
>
> +handled:
> +	/*
> +	 * Returning from nmi_panic() means this CPU was processing panic()
> +	 * before NMI.  Return NMI_HANDLED and go back to panic routines.
> +	 */

I don't think this comment adds much if any value.

> +	return NMI_HANDLED;
> +
>   out:

Any goto to this label does no longer serve a useful purpose (if it ever served one),
and should be replaced with "return NMI_DONE;"

>   	return NMI_DONE;
>   }
>
>
>

Re: [PATCH 2/3] ipmi/watchdog: Use nmi_panic() when kernel panics in NMI handler

[Guenter Roeck]

On 02/29/2016 05:50 PM, Hidehiro Kawai wrote:
> 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.
>
> ipmi_watchdog driver can call panic() from NMI handler, so replace
> it with nmi_panic().
>
> Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
> Cc: Corey Minyard <minyard@acm.org>
> Cc: openipmi-developer@lists.sourceforge.net

Acked-by: Guenter Roeck <linux@roeck-us.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;
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

RE: [PATCH 3/3] hpwdt: Use nmi_panic() when kernel panics in NMI handler

[河合英宏 / KAWAI，HIDEHIRO]

Hi Guenter,

Thanks for the review.

> On 02/29/2016 05:50 PM, Hidehiro Kawai wrote:
> > 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.
> >
> > hpwdt driver can call panic() from NMI handler, so replace it with
> > nmi_panic().
> >
> > Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
> > Cc: Thomas Mingarelli <thomas.mingarelli@hpe.com>
> > Cc: Wim Van Sebroeck <wim@iguana.be>
> > Cc: Guenter Roeck <linux@roeck-us.net>
> > Cc: linux-watchdog@vger.kernel.org
> > ---
> >   drivers/watchdog/hpwdt.c |   12 ++++++++++--
> >   1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> > index 92443c3..fd0486f 100644
> > --- a/drivers/watchdog/hpwdt.c
> > +++ b/drivers/watchdog/hpwdt.c
> > @@ -496,11 +496,12 @@ 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, "
> > +			nmi_panic(regs, "An NMI occurred, "
> >   				"but unable to determine source.\n");
> 
> This message should really be in a single line.
> Sure, strictly speaking that should be done in a separate patch,
> but since you touch the line you might as well fix it.

OK, I'll combine them.

> > +			goto handled;
> 
> The goto is unnecessary. Just return NMI_HANDLED.

Sure, I'll do that.

> 
> >   		}
> >   	}
> > -	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,6 +509,13 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
> >   		"3. OA Forward Progress Log\n"
> >   		"4. iLO Event Log");
> >
> > +handled:
> > +	/*
> > +	 * Returning from nmi_panic() means this CPU was processing panic()
> > +	 * before NMI.  Return NMI_HANDLED and go back to panic routines.
> > +	 */
> 
> I don't think this comment adds much if any value.

So, I remove this comment.  NMI_HANDLED would be self-explanatory.

> > +	return NMI_HANDLED;
> > +
> >   out:
> 
> Any goto to this label does no longer serve a useful purpose (if it ever served one),
> and should be replaced with "return NMI_DONE;"

I'll fix it.  It will become a bit simpler.

Thanks,

--
Hidehiro Kawai
Hitachi, Ltd. Research & Development Group

RE: [PATCH 3/3] hpwdt: Use nmi_panic() when kernel panics in NMI handler

[河合英宏 / KAWAI，HIDEHIRO]

Hi Guenter,

Thanks for the review.

> On 02/29/2016 05:50 PM, Hidehiro Kawai wrote:
> > 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.
> >
> > hpwdt driver can call panic() from NMI handler, so replace it with
> > nmi_panic().
> >
> > Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
> > Cc: Thomas Mingarelli <thomas.mingarelli@hpe.com>
> > Cc: Wim Van Sebroeck <wim@iguana.be>
> > Cc: Guenter Roeck <linux@roeck-us.net>
> > Cc: linux-watchdog@vger.kernel.org
> > ---
> >   drivers/watchdog/hpwdt.c |   12 ++++++++++--
> >   1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> > index 92443c3..fd0486f 100644
> > --- a/drivers/watchdog/hpwdt.c
> > +++ b/drivers/watchdog/hpwdt.c
> > @@ -496,11 +496,12 @@ 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, "
> > +			nmi_panic(regs, "An NMI occurred, "
> >   				"but unable to determine source.\n");
> 
> This message should really be in a single line.
> Sure, strictly speaking that should be done in a separate patch,
> but since you touch the line you might as well fix it.

OK, I'll combine them.

> > +			goto handled;
> 
> The goto is unnecessary. Just return NMI_HANDLED.

Sure, I'll do that.

> 
> >   		}
> >   	}
> > -	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,6 +509,13 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
> >   		"3. OA Forward Progress Log\n"
> >   		"4. iLO Event Log");
> >
> > +handled:
> > +	/*
> > +	 * Returning from nmi_panic() means this CPU was processing panic()
> > +	 * before NMI.  Return NMI_HANDLED and go back to panic routines.
> > +	 */
> 
> I don't think this comment adds much if any value.

So, I remove this comment.  NMI_HANDLED would be self-explanatory.

> > +	return NMI_HANDLED;
> > +
> >   out:
> 
> Any goto to this label does no longer serve a useful purpose (if it ever served one),
> and should be replaced with "return NMI_DONE;"

I'll fix it.  It will become a bit simpler.

Thanks,

--
Hidehiro Kawai
Hitachi, Ltd. Research & Development Group

RE: [PATCH 2/3] ipmi/watchdog: Use nmi_panic() when kernel panics in NMI handler

[河合英宏 / KAWAI，HIDEHIRO]

Hi Corey,

Thanks for the review.

> Sure, this is a good idea.
> 
> Acked-by: Corey Minyard <cminyard@mvista.com>
> 
> Note that nmi_panic() came in commit 1717f2096b5 (panic, x86: Fix
> re-entrance problem due to panic on NMI) and then the regs field
> was added in the commit you reference.

Yes. So, I'll change the description to more proper one.

> Do you want me to add this to the IPMI queue or do you have another
> way to get this patch into the kernel?

I don't have another way, and I don't know how cross-subsystem
patch set should be handled.

I think it would be better this patch set is managed by one person
because both PATCH 2/3 and 3/3 depend on 1/3.

Thanks,

--
Hidehiro Kawai
Hitachi, Ltd. Research & Development Group

RE: [PATCH 2/3] ipmi/watchdog: Use nmi_panic() when kernel panics in NMI handler

[河合英宏 / KAWAI，HIDEHIRO]

Hi Corey,

Thanks for the review.

> Sure, this is a good idea.
> 
> Acked-by: Corey Minyard <cminyard@mvista.com>
> 
> Note that nmi_panic() came in commit 1717f2096b5 (panic, x86: Fix
> re-entrance problem due to panic on NMI) and then the regs field
> was added in the commit you reference.

Yes. So, I'll change the description to more proper one.

> Do you want me to add this to the IPMI queue or do you have another
> way to get this patch into the kernel?

I don't have another way, and I don't know how cross-subsystem
patch set should be handled.

I think it would be better this patch set is managed by one person
because both PATCH 2/3 and 3/3 depend on 1/3.

Thanks,

--
Hidehiro Kawai
Hitachi, Ltd. Research & Development Group

Re: [PATCH 2/3] ipmi/watchdog: Use nmi_panic() when kernel panics in NMI handler

[Guenter Roeck]

On 02/29/2016 08:23 PM, 河合英宏 / KAWAI，HIDEHIRO wrote:
> Hi Corey,
>
> Thanks for the review.
>
>> Sure, this is a good idea.
>>
>> Acked-by: Corey Minyard <cminyard@mvista.com>
>>
>> Note that nmi_panic() came in commit 1717f2096b5 (panic, x86: Fix
>> re-entrance problem due to panic on NMI) and then the regs field
>> was added in the commit you reference.
>
> Yes. So, I'll change the description to more proper one.
>
>> Do you want me to add this to the IPMI queue or do you have another
>> way to get this patch into the kernel?
>
> I don't have another way, and I don't know how cross-subsystem
> patch set should be handled.
>
> I think it would be better this patch set is managed by one person
> because both PATCH 2/3 and 3/3 depend on 1/3.
>

We'll need an Ack from someone with authority over kernel/panic.c.
If someone sends an Ack, the series can go through IPMI (or watchdog).
It might be easier though if Andrew would take the entire series.

Guenter

Re: [PATCH 2/3] ipmi/watchdog: Use nmi_panic() when kernel panics in NMI handler

[Guenter Roeck]

On 02/29/2016 08:23 PM, 河合英宏 / KAWAI，HIDEHIRO wrote:
> Hi Corey,
>
> Thanks for the review.
>
>> Sure, this is a good idea.
>>
>> Acked-by: Corey Minyard <cminyard@mvista.com>
>>
>> Note that nmi_panic() came in commit 1717f2096b5 (panic, x86: Fix
>> re-entrance problem due to panic on NMI) and then the regs field
>> was added in the commit you reference.
>
> Yes. So, I'll change the description to more proper one.
>
>> Do you want me to add this to the IPMI queue or do you have another
>> way to get this patch into the kernel?
>
> I don't have another way, and I don't know how cross-subsystem
> patch set should be handled.
>
> I think it would be better this patch set is managed by one person
> because both PATCH 2/3 and 3/3 depend on 1/3.
>

We'll need an Ack from someone with authority over kernel/panic.c.
If someone sends an Ack, the series can go through IPMI (or watchdog).
It might be easier though if Andrew would take the entire series.

Guenter

Re: [PATCH 1/3] panic: Export panic_cpu and nmi_panic_self_stop

[Borislav Petkov]

On Tue, Mar 01, 2016 at 10:50:37AM +0900, Hidehiro Kawai wrote:
> Export panic_cpu and nmi_panic_self_stop symbols for modules which
> use nmi_panic() macro.
> 
> Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com>
> Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
> Cc: Tejun Heo <tj@kernel.org>
> ---
>  kernel/panic.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/kernel/panic.c b/kernel/panic.c
> index d96469d..f4e8035 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -69,8 +69,10 @@ void __weak nmi_panic_self_stop(struct pt_regs *regs)
>  {
>  	panic_smp_self_stop();
>  }
> +EXPORT_SYMBOL(nmi_panic_self_stop);
>  
>  atomic_t panic_cpu = ATOMIC_INIT(PANIC_CPU_INVALID);
> +EXPORT_SYMBOL(panic_cpu);

Can we make nmi_panic() at least a proper function and export that
instead of exporting all those implementation details...?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

RE: [PATCH 1/3] panic: Export panic_cpu and nmi_panic_self_stop

[河合英宏 / KAWAI，HIDEHIRO]

Hi Borislav,

> From: Borislav Petkov [mailto:bp@alien8.de]
> On Tue, Mar 01, 2016 at 10:50:37AM +0900, Hidehiro Kawai wrote:
> > Export panic_cpu and nmi_panic_self_stop symbols for modules which
> > use nmi_panic() macro.
> >
> > Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Borislav Petkov <bp@suse.de>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Michal Hocko <mhocko@suse.com>
> > Cc: HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com>
> > Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
> > Cc: Tejun Heo <tj@kernel.org>
> > ---
> >  kernel/panic.c |    2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/kernel/panic.c b/kernel/panic.c
> > index d96469d..f4e8035 100644
> > --- a/kernel/panic.c
> > +++ b/kernel/panic.c
> > @@ -69,8 +69,10 @@ void __weak nmi_panic_self_stop(struct pt_regs *regs)
> >  {
> >  	panic_smp_self_stop();
> >  }
> > +EXPORT_SYMBOL(nmi_panic_self_stop);
> >
> >  atomic_t panic_cpu = ATOMIC_INIT(PANIC_CPU_INVALID);
> > +EXPORT_SYMBOL(panic_cpu);
> 
> Can we make nmi_panic() at least a proper function and export that
> instead of exporting all those implementation details...?

The reason I implemented nmi_panic() as a macro is to pass
variable arguments directly to panic().  Fortunately, since all
invocations of nmi_panic() just pass a fixed string, I can change it
to a normal function like:

int nmi_panic(struct pt_regs *regs, const char *msg)
{
...
	panic("%s", msg);

If people don't mind if that, I'll change it.

Regards,

--
Hidehiro Kawai
Hitachi, Ltd. Research & Development Group

RE: [PATCH 1/3] panic: Export panic_cpu and nmi_panic_self_stop

[河合英宏 / KAWAI，HIDEHIRO]

Hi Borislav,

> From: Borislav Petkov [mailto:bp@alien8.de]
> On Tue, Mar 01, 2016 at 10:50:37AM +0900, Hidehiro Kawai wrote:
> > Export panic_cpu and nmi_panic_self_stop symbols for modules which
> > use nmi_panic() macro.
> >
> > Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Borislav Petkov <bp@suse.de>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Michal Hocko <mhocko@suse.com>
> > Cc: HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com>
> > Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
> > Cc: Tejun Heo <tj@kernel.org>
> > ---
> >  kernel/panic.c |    2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/kernel/panic.c b/kernel/panic.c
> > index d96469d..f4e8035 100644
> > --- a/kernel/panic.c
> > +++ b/kernel/panic.c
> > @@ -69,8 +69,10 @@ void __weak nmi_panic_self_stop(struct pt_regs *regs)
> >  {
> >  	panic_smp_self_stop();
> >  }
> > +EXPORT_SYMBOL(nmi_panic_self_stop);
> >
> >  atomic_t panic_cpu = ATOMIC_INIT(PANIC_CPU_INVALID);
> > +EXPORT_SYMBOL(panic_cpu);
> 
> Can we make nmi_panic() at least a proper function and export that
> instead of exporting all those implementation details...?

The reason I implemented nmi_panic() as a macro is to pass
variable arguments directly to panic().  Fortunately, since all
invocations of nmi_panic() just pass a fixed string, I can change it
to a normal function like:

int nmi_panic(struct pt_regs *regs, const char *msg)
{
...
	panic("%s", msg);

If people don't mind if that, I'll change it.

Regards,

--
Hidehiro Kawai
Hitachi, Ltd. Research & Development Group

Re: [PATCH 1/3] panic: Export panic_cpu and nmi_panic_self_stop

[Michal Hocko]

On Tue 01-03-16 07:48:10, 河合英宏 / KAWAI，HIDEHIRO wrote:
> Hi Borislav,
> 
> > From: Borislav Petkov [mailto:bp@alien8.de]
> > On Tue, Mar 01, 2016 at 10:50:37AM +0900, Hidehiro Kawai wrote:
> > > Export panic_cpu and nmi_panic_self_stop symbols for modules which
> > > use nmi_panic() macro.
> > >
> > > Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
> > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > Cc: Borislav Petkov <bp@suse.de>
> > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > Cc: Michal Hocko <mhocko@suse.com>
> > > Cc: HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com>
> > > Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
> > > Cc: Tejun Heo <tj@kernel.org>
> > > ---
> > >  kernel/panic.c |    2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/kernel/panic.c b/kernel/panic.c
> > > index d96469d..f4e8035 100644
> > > --- a/kernel/panic.c
> > > +++ b/kernel/panic.c
> > > @@ -69,8 +69,10 @@ void __weak nmi_panic_self_stop(struct pt_regs *regs)
> > >  {
> > >  	panic_smp_self_stop();
> > >  }
> > > +EXPORT_SYMBOL(nmi_panic_self_stop);
> > >
> > >  atomic_t panic_cpu = ATOMIC_INIT(PANIC_CPU_INVALID);
> > > +EXPORT_SYMBOL(panic_cpu);
> > 
> > Can we make nmi_panic() at least a proper function and export that
> > instead of exporting all those implementation details...?
> 
> The reason I implemented nmi_panic() as a macro is to pass
> variable arguments directly to panic().  Fortunately, since all
> invocations of nmi_panic() just pass a fixed string, I can change it
> to a normal function like:
> 
> int nmi_panic(struct pt_regs *regs, const char *msg)
> {
> ...
> 	panic("%s", msg);
> 
> If people don't mind if that, I'll change it.

Yeah, I think this is better. If we ever need a variable strings we can
reconsider.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/3] panic: Export panic_cpu and nmi_panic_self_stop

[Michal Hocko]

On Tue 01-03-16 07:48:10, 河合英宏 / KAWAI，HIDEHIRO wrote:
> Hi Borislav,
> 
> > From: Borislav Petkov [mailto:bp@alien8.de]
> > On Tue, Mar 01, 2016 at 10:50:37AM +0900, Hidehiro Kawai wrote:
> > > Export panic_cpu and nmi_panic_self_stop symbols for modules which
> > > use nmi_panic() macro.
> > >
> > > Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
> > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > Cc: Borislav Petkov <bp@suse.de>
> > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > Cc: Michal Hocko <mhocko@suse.com>
> > > Cc: HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com>
> > > Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
> > > Cc: Tejun Heo <tj@kernel.org>
> > > ---
> > >  kernel/panic.c |    2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/kernel/panic.c b/kernel/panic.c
> > > index d96469d..f4e8035 100644
> > > --- a/kernel/panic.c
> > > +++ b/kernel/panic.c
> > > @@ -69,8 +69,10 @@ void __weak nmi_panic_self_stop(struct pt_regs *regs)
> > >  {
> > >  	panic_smp_self_stop();
> > >  }
> > > +EXPORT_SYMBOL(nmi_panic_self_stop);
> > >
> > >  atomic_t panic_cpu = ATOMIC_INIT(PANIC_CPU_INVALID);
> > > +EXPORT_SYMBOL(panic_cpu);
> > 
> > Can we make nmi_panic() at least a proper function and export that
> > instead of exporting all those implementation details...?
> 
> The reason I implemented nmi_panic() as a macro is to pass
> variable arguments directly to panic().  Fortunately, since all
> invocations of nmi_panic() just pass a fixed string, I can change it
> to a normal function like:
> 
> int nmi_panic(struct pt_regs *regs, const char *msg)
> {
> ...
> 	panic("%s", msg);
> 
> If people don't mind if that, I'll change it.

Yeah, I think this is better. If we ever need a variable strings we can
reconsider.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 2/3] ipmi/watchdog: Use nmi_panic() when kernel panics in NMI handler

[Michal Hocko]

On Tue 01-03-16 10:50:39, Hidehiro Kawai wrote:
> 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.
> 
> ipmi_watchdog driver can call panic() from NMI handler, so replace
> it with nmi_panic().
> 
> Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
> Cc: Corey Minyard <minyard@acm.org>
> Cc: openipmi-developer@lists.sourceforge.net

Reviewed-by: Michal Hocko <mhocko@suse.com>

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

-- 
Michal Hocko
SUSE Labs