[PATCH 1/2] watchdog, hpwdt: Remove priority option for NMI callback

[PATCH 1/2] watchdog, hpwdt: Remove priority option for NMI callback

[Don Zickus]

The NMI_UNKNOWN bucket only allows for one function to register to it.
The reason for that is because only functions which can not determine
if the NMI belongs to them or not should register and would like to
assume/swallow any NMI they see.

As a result it doesn't make sense to let more than one function like this
register.  In fact, letting a second function fail allows us to know that
more than one function is going to swallow NMIs on the current system.
This is better than silently being ignored.

Therefore hpwdt's priority mechanism doesn't make sense any more.  They
will be always first on the NMI_UNKNOWN queue, if they register.

Removing this parameter cleans up the code and simplifies things for the
next patch which changes how nmis are registered.

Cc: Thomas Mingarelli <thomas.mingarelli@hp.com>
Cc: Wim Van Sebroeck <wim@iguana.be>
Signed-off-by: Don Zickus <dzickus@redhat.com>
---
 drivers/watchdog/hpwdt.c |   19 ++++---------------
 1 files changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 3c166d3..deee02c8 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -145,7 +145,6 @@ struct cmn_registers {
 
 static unsigned int hpwdt_nmi_decoding;
 static unsigned int allow_kdump;
-static unsigned int priority;		/* hpwdt at end of die_notify list */
 static unsigned int is_icru;
 static DEFINE_SPINLOCK(rom_lock);
 static void *cru_rom_addr;
@@ -730,13 +729,9 @@ static int __devinit hpwdt_init_nmi_decoding(struct pci_dev *dev)
 	}
 
 	/*
-	 * If the priority is set to 1, then we will be put first on the
-	 * die notify list to handle a critical NMI. The default is to
-	 * be last so other users of the NMI signal can function.
+	 * Only one function can register for NMI_UNKNOWN
 	 */
-	retval = register_nmi_handler(NMI_UNKNOWN, hpwdt_pretimeout,
-					(priority) ? NMI_FLAG_FIRST : 0,
-					"hpwdt");
+	retval = register_nmi_handler(NMI_UNKNOWN, hpwdt_pretimeout, 0, "hpwdt");
 	if (retval != 0) {
 		dev_warn(&dev->dev,
 			"Unable to register a die notifier (err=%d).\n",
@@ -747,10 +742,8 @@ static int __devinit hpwdt_init_nmi_decoding(struct pci_dev *dev)
 
 	dev_info(&dev->dev,
 			"HP Watchdog Timer Driver: NMI decoding initialized"
-			", allow kernel dump: %s (default = 0/OFF)"
-			", priority: %s (default = 0/LAST).\n",
-			(allow_kdump == 0) ? "OFF" : "ON",
-			(priority == 0) ? "LAST" : "FIRST");
+			", allow kernel dump: %s (default = 0/OFF)\n",
+			(allow_kdump == 0) ? "OFF" : "ON");
 	return 0;
 }
 
@@ -888,10 +881,6 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
 #ifdef CONFIG_HPWDT_NMI_DECODING
 module_param(allow_kdump, int, 0);
 MODULE_PARM_DESC(allow_kdump, "Start a kernel dump after NMI occurs");
-
-module_param(priority, int, 0);
-MODULE_PARM_DESC(priority, "The hpwdt driver handles NMIs first or last"
-		" (default = 0/Last)\n");
 #endif /* !CONFIG_HPWDT_NMI_DECODING */
 
 module_init(hpwdt_init);
-- 
1.7.7.6

[PATCH 2/2] x86,nmi: Fix page faults by nmiaction if kmemcheck is enabled

[Don Zickus]

From: Li Zhong <zhong@linux.vnet.ibm.com>

This patch tries to fix the problem of page fault exception caused by
accessing nmiaction structure in nmi if kmemcheck is enabled.

If kmemcheck is enabled, the memory allocated through slab are in pages
that are marked non-present, so that some checks could be done in the
page fault handling code ( e.g. whether the memory is read before
written to ).
As nmiaction is allocated in this way, so it resides in a non-present
page. Then there is a page fault while the nmi code accessing the
nmiaction structure, which would then cause a warning by
WARN_ON_ONCE(in_nmi()) in kmemcheck_fault(), called by do_page_fault().

v2: as Peter suggested, changed the nmiaction to use static storage.

v3: as Peter suggested, use macro to shorten the codes. Also keep the
original usage of register_nmi_handler, so users of this call doesn't
need change.

[simplified wrappers -dcz]

Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
Signed-off-by: Don Zickus <dzickus@redhat.com>
---
 arch/x86/include/asm/nmi.h |   20 ++++++++++++-
 arch/x86/kernel/nmi.c      |   65 ++++---------------------------------------
 2 files changed, 24 insertions(+), 61 deletions(-)

diff --git a/arch/x86/include/asm/nmi.h b/arch/x86/include/asm/nmi.h
index fd3f9f1..2a17e57 100644
--- a/arch/x86/include/asm/nmi.h
+++ b/arch/x86/include/asm/nmi.h
@@ -35,8 +35,24 @@ enum {
 
 typedef int (*nmi_handler_t)(unsigned int, struct pt_regs *);
 
-int register_nmi_handler(unsigned int, nmi_handler_t, unsigned long,
-			 const char *);
+struct nmiaction {
+	struct list_head list;
+	nmi_handler_t handler;
+	unsigned int flags;
+	const char *name;
+};
+
+#define register_nmi_handler(t, fn, fg, n)		\
+({							\
+	static struct nmiaction fn##_na = {		\
+		.handler = (fn),			\
+		.name = (n),				\
+		.flags = (fg),				\
+	};						\
+	__register_nmi_handler((t), &fn##_na);	\
+})
+
+int __register_nmi_handler(unsigned int, struct nmiaction *);
 
 void unregister_nmi_handler(unsigned int, const char *);
 
diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
index 47acaf3..f13302d 100644
--- a/arch/x86/kernel/nmi.c
+++ b/arch/x86/kernel/nmi.c
@@ -31,14 +31,6 @@
 #include <asm/nmi.h>
 #include <asm/x86_init.h>
 
-#define NMI_MAX_NAMELEN	16
-struct nmiaction {
-	struct list_head list;
-	nmi_handler_t handler;
-	unsigned int flags;
-	char *name;
-};
-
 struct nmi_desc {
 	spinlock_t lock;
 	struct list_head head;
@@ -107,11 +99,14 @@ static int notrace __kprobes nmi_handle(unsigned int type, struct pt_regs *regs,
 	return handled;
 }
 
-static int __setup_nmi(unsigned int type, struct nmiaction *action)
+int __register_nmi_handler(unsigned int type, struct nmiaction *action)
 {
 	struct nmi_desc *desc = nmi_to_desc(type);
 	unsigned long flags;
 
+	if (!action->handler)
+		return -EINVAL;
+
 	spin_lock_irqsave(&desc->lock, flags);
 
 	/*
@@ -133,8 +128,9 @@ static int __setup_nmi(unsigned int type, struct nmiaction *action)
 	spin_unlock_irqrestore(&desc->lock, flags);
 	return 0;
 }
+EXPORT_SYMBOL(__register_nmi_handler);
 
-static struct nmiaction *__free_nmi(unsigned int type, const char *name)
+void unregister_nmi_handler(unsigned int type, const char *name)
 {
 	struct nmi_desc *desc = nmi_to_desc(type);
 	struct nmiaction *n;
@@ -157,56 +153,7 @@ static struct nmiaction *__free_nmi(unsigned int type, const char *name)
 
 	spin_unlock_irqrestore(&desc->lock, flags);
 	synchronize_rcu();
-	return (n);
 }
-
-int register_nmi_handler(unsigned int type, nmi_handler_t handler,
-			unsigned long nmiflags, const char *devname)
-{
-	struct nmiaction *action;
-	int retval = -ENOMEM;
-
-	if (!handler)
-		return -EINVAL;
-
-	action = kzalloc(sizeof(struct nmiaction), GFP_KERNEL);
-	if (!action)
-		goto fail_action;
-
-	action->handler = handler;
-	action->flags = nmiflags;
-	action->name = kstrndup(devname, NMI_MAX_NAMELEN, GFP_KERNEL);
-	if (!action->name)
-		goto fail_action_name;
-
-	retval = __setup_nmi(type, action);
-
-	if (retval)
-		goto fail_setup_nmi;
-
-	return retval;
-
-fail_setup_nmi:
-	kfree(action->name);
-fail_action_name:
-	kfree(action);
-fail_action:	
-
-	return retval;
-}
-EXPORT_SYMBOL_GPL(register_nmi_handler);
-
-void unregister_nmi_handler(unsigned int type, const char *name)
-{
-	struct nmiaction *a;
-
-	a = __free_nmi(type, name);
-	if (a) {
-		kfree(a->name);
-		kfree(a);
-	}
-}
-
 EXPORT_SYMBOL_GPL(unregister_nmi_handler);
 
 static notrace __kprobes void
-- 
1.7.7.6

RE: [PATCH 1/2] watchdog, hpwdt: Remove priority option for NMI callback

[Mingarelli, Thomas]

Has this been tested with echo c > /proc/sysrg-trigger and the virtual NMI button via iLO?

-----Original Message-----
From: Don Zickus [mailto:dzickus@redhat.com] 
Sent: Friday, March 09, 2012 10:37 AM
To: x86@kernel.org
Cc: Peter Zijlstra; LKML; Don Zickus; Mingarelli, Thomas; Wim Van Sebroeck
Subject: [PATCH 1/2] watchdog, hpwdt: Remove priority option for NMI callback

The NMI_UNKNOWN bucket only allows for one function to register to it.
The reason for that is because only functions which can not determine
if the NMI belongs to them or not should register and would like to
assume/swallow any NMI they see.

As a result it doesn't make sense to let more than one function like this
register.  In fact, letting a second function fail allows us to know that
more than one function is going to swallow NMIs on the current system.
This is better than silently being ignored.

Therefore hpwdt's priority mechanism doesn't make sense any more.  They
will be always first on the NMI_UNKNOWN queue, if they register.

Removing this parameter cleans up the code and simplifies things for the
next patch which changes how nmis are registered.

Cc: Thomas Mingarelli <thomas.mingarelli@hp.com>
Cc: Wim Van Sebroeck <wim@iguana.be>
Signed-off-by: Don Zickus <dzickus@redhat.com>
---
 drivers/watchdog/hpwdt.c |   19 ++++---------------
 1 files changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 3c166d3..deee02c8 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -145,7 +145,6 @@ struct cmn_registers {
 
 static unsigned int hpwdt_nmi_decoding;
 static unsigned int allow_kdump;
-static unsigned int priority;		/* hpwdt at end of die_notify list */
 static unsigned int is_icru;
 static DEFINE_SPINLOCK(rom_lock);
 static void *cru_rom_addr;
@@ -730,13 +729,9 @@ static int __devinit hpwdt_init_nmi_decoding(struct pci_dev *dev)
 	}
 
 	/*
-	 * If the priority is set to 1, then we will be put first on the
-	 * die notify list to handle a critical NMI. The default is to
-	 * be last so other users of the NMI signal can function.
+	 * Only one function can register for NMI_UNKNOWN
 	 */
-	retval = register_nmi_handler(NMI_UNKNOWN, hpwdt_pretimeout,
-					(priority) ? NMI_FLAG_FIRST : 0,
-					"hpwdt");
+	retval = register_nmi_handler(NMI_UNKNOWN, hpwdt_pretimeout, 0, "hpwdt");
 	if (retval != 0) {
 		dev_warn(&dev->dev,
 			"Unable to register a die notifier (err=%d).\n",
@@ -747,10 +742,8 @@ static int __devinit hpwdt_init_nmi_decoding(struct pci_dev *dev)
 
 	dev_info(&dev->dev,
 			"HP Watchdog Timer Driver: NMI decoding initialized"
-			", allow kernel dump: %s (default = 0/OFF)"
-			", priority: %s (default = 0/LAST).\n",
-			(allow_kdump == 0) ? "OFF" : "ON",
-			(priority == 0) ? "LAST" : "FIRST");
+			", allow kernel dump: %s (default = 0/OFF)\n",
+			(allow_kdump == 0) ? "OFF" : "ON");
 	return 0;
 }
 
@@ -888,10 +881,6 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
 #ifdef CONFIG_HPWDT_NMI_DECODING
 module_param(allow_kdump, int, 0);
 MODULE_PARM_DESC(allow_kdump, "Start a kernel dump after NMI occurs");
-
-module_param(priority, int, 0);
-MODULE_PARM_DESC(priority, "The hpwdt driver handles NMIs first or last"
-		" (default = 0/Last)\n");
 #endif /* !CONFIG_HPWDT_NMI_DECODING */
 
 module_init(hpwdt_init);
-- 
1.7.7.6

Re: [PATCH 1/2] watchdog, hpwdt: Remove priority option for NMI callback

[Don Zickus]

On Fri, Mar 09, 2012 at 04:51:09PM +0000, Mingarelli, Thomas wrote:
> Has this been tested with echo c > /proc/sysrg-trigger and the virtual NMI button via iLO?

No, because I don't know how to do that.  Though I can probably ping Tony
over here to show me how.  But for the most part the functionality hasn't
changed (the priority is always a '0' now).  Looking in
arch/x86/kernel/nmi.c shows that it will spit out a warning if two
functions register against NMI_UNKNOWN.

Cheers,
Don

RE: [PATCH 1/2] watchdog, hpwdt: Remove priority option for NMI callback

[Mingarelli, Thomas]

Ok I will run some tests asap. I may not get to it today.

Tom

-----Original Message-----
From: Don Zickus [mailto:dzickus@redhat.com] 
Sent: Friday, March 09, 2012 11:17 AM
To: Mingarelli, Thomas
Cc: x86@kernel.org; Peter Zijlstra; LKML; Wim Van Sebroeck
Subject: Re: [PATCH 1/2] watchdog, hpwdt: Remove priority option for NMI callback

On Fri, Mar 09, 2012 at 04:51:09PM +0000, Mingarelli, Thomas wrote:
> Has this been tested with echo c > /proc/sysrg-trigger and the virtual NMI button via iLO?

No, because I don't know how to do that.  Though I can probably ping Tony
over here to show me how.  But for the most part the functionality hasn't
changed (the priority is always a '0' now).  Looking in
arch/x86/kernel/nmi.c shows that it will spit out a warning if two
functions register against NMI_UNKNOWN.

Cheers,
Don

RE: [PATCH 1/2] watchdog, hpwdt: Remove priority option for NMI callback

[Mingarelli, Thomas]

Can I register for more than one type? Like NMI_UNKNOWN | NMI_DIE (or something like that)?

-----Original Message-----
From: Don Zickus [mailto:dzickus@redhat.com] 
Sent: Friday, March 09, 2012 11:17 AM
To: Mingarelli, Thomas
Cc: x86@kernel.org; Peter Zijlstra; LKML; Wim Van Sebroeck
Subject: Re: [PATCH 1/2] watchdog, hpwdt: Remove priority option for NMI callback

On Fri, Mar 09, 2012 at 04:51:09PM +0000, Mingarelli, Thomas wrote:
> Has this been tested with echo c > /proc/sysrg-trigger and the virtual NMI button via iLO?

No, because I don't know how to do that.  Though I can probably ping Tony
over here to show me how.  But for the most part the functionality hasn't
changed (the priority is always a '0' now).  Looking in
arch/x86/kernel/nmi.c shows that it will spit out a warning if two
functions register against NMI_UNKNOWN.

Cheers,
Don

RE: [PATCH 1/2] watchdog, hpwdt: Remove priority option for NMI callback

[Peter Zijlstra]

On Fri, 2012-03-09 at 17:28 +0000, Mingarelli, Thomas wrote:
> Can I register for more than one type? Like NMI_UNKNOWN | NMI_DIE (or
> something like that)?

There's only NMI_LOCAL and NMI_UNKNOWN, and you can only use NMI_LOCAL
if you can tell you generated the NMI. See it as one massively shared
interrupt line.

The NMI_UNKNOWN handler is basically the handler of last resort for
those that cannot multiplex the line, hence the restriction of only
having one.

Re: [PATCH 1/2] watchdog, hpwdt: Remove priority option for NMI callback

[Don Zickus]

On Fri, Mar 09, 2012 at 05:28:15PM +0000, Mingarelli, Thomas wrote:
> Can I register for more than one type? Like NMI_UNKNOWN | NMI_DIE (or something like that)?

There are only two queues, NMI_LOCAL and NMI_UNKNOWN.  The difference is
NMI_LOCAL has handlers that can determine if they are the NMI source,
which is needed because _every_ handler is executed on every NMI.
NMI_UNKNOWN is reserved for handlers that just assume the NMI is theirs.

Cheers,
Don