[PATCH v7 4/5]: core: Add kernel message dumper to call on oopses and panics

[PATCH v7 4/5]: core: Add kernel message dumper to call on oopses and panics

[Simon Kagstrom]

The core functionality is implemented as per Linus suggestion from

  http://lists.infradead.org/pipermail/linux-mtd/2009-October/027620.html

(with the dump_kmsg implementation by Linus). A struct kmsg_dumper has
been added which contains a callback to dump the kernel log buffers on
crashes. The dump_kmsg function gets called from oops_exit() and panic()
and invokes this callbacks with the crash reason.

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
Reviewed-by: Anders Grafstrom <anders.grafstrom@netinsight.net>
---
ChangeLog:

 * (The other patches are sent only to linux-mtd since they are really
   just mtdoops-specific now)
 * (Linus) remove the priv pointer in the structure and
   let users determine the address of their own structure
   with container_of

 include/linux/kmsg_dump.h |   36 +++++++++++++++
 kernel/panic.c            |    3 +
 kernel/printk.c           |  104 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 143 insertions(+), 0 deletions(-)
 create mode 100644 include/linux/kmsg_dump.h

diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h
new file mode 100644
index 0000000..d7c9a4c
--- /dev/null
+++ b/include/linux/kmsg_dump.h
@@ -0,0 +1,36 @@
+/*
+ * linux/include/kmsg_dump.h
+ *
+ * Copyright (C) 2009 Net Insight AB
+ *
+ * Author: Simon Kagstrom <simon.kagstrom@netinsight.net>
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file COPYING in the main directory of this archive
+ * for more details.
+ */
+#ifndef _LINUX_KMSG_DUMP_H
+#define _LINUX_KMSG_DUMP_H
+
+#include <linux/list.h>
+
+enum kmsg_dump_reason {
+	kmsg_dump_oops,
+	kmsg_dump_panic,
+};
+
+struct kmsg_dumper {
+	void (*dump)(struct kmsg_dumper *dumper, enum kmsg_dump_reason reason,
+			const char *s1, unsigned long l1,
+			const char *s2, unsigned long l2);
+	struct list_head list;
+	int registered;
+};
+
+void dump_kmsg(enum kmsg_dump_reason reason);
+
+int register_kmsg_dumper(struct kmsg_dumper *dumper);
+
+void unregister_kmsg_dumper(struct kmsg_dumper *dumper);
+
+#endif /* _LINUX_DUMP_DEVICE_H */
diff --git a/kernel/panic.c b/kernel/panic.c
index c0b33b8..3a5a93f 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -10,6 +10,7 @@
  */
 #include <linux/debug_locks.h>
 #include <linux/interrupt.h>
+#include <linux/kmsg_dump.h>
 #include <linux/kallsyms.h>
 #include <linux/notifier.h>
 #include <linux/module.h>
@@ -76,6 +77,7 @@ NORET_TYPE void panic(const char * fmt, ...)
 	dump_stack();
 #endif
 
+	dump_kmsg(kmsg_dump_panic);
 	/*
 	 * If we have crashed and we have a crash kernel loaded let it handle
 	 * everything else.
@@ -341,6 +343,7 @@ void oops_exit(void)
 {
 	do_oops_enter_exit();
 	print_oops_end_marker();
+	dump_kmsg(kmsg_dump_oops);
 }
 
 #ifdef WANT_WARN_ON_SLOWPATH
diff --git a/kernel/printk.c b/kernel/printk.c
index f38b07f..9c4d9cb 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -33,6 +33,8 @@
 #include <linux/bootmem.h>
 #include <linux/syscalls.h>
 #include <linux/kexec.h>
+#include <linux/kmsg_dump.h>
+#include <linux/spinlock.h>
 
 #include <asm/uaccess.h>
 
@@ -1405,3 +1407,105 @@ bool printk_timed_ratelimit(unsigned long *caller_jiffies,
 }
 EXPORT_SYMBOL(printk_timed_ratelimit);
 #endif
+
+static LIST_HEAD(dump_list);
+static DEFINE_SPINLOCK(dump_list_lock);
+
+/**
+ *	register_dump_device - register a kernel log dumper.
+ *	@dump: pointer to the dump structure
+ *	@priv: private data for the structure
+ *
+ *	Adds a kernel log dumper to the system. The dump callback in the
+ *	structure will be called when the kernel oopses or panics and must be
+ *	set. Returns zero on success and -EINVAL or -EBUSY otherwise.
+ */
+int register_kmsg_dumper(struct kmsg_dumper *dumper)
+{
+	unsigned long flags;
+
+	/* The dump callback needs to be set */
+	if (!dumper->dump)
+		return -EINVAL;
+
+	/* Don't allow registering multiple times */
+	if (dumper->registered)
+		return -EBUSY;
+
+	dumper->registered = 1;
+
+	spin_lock_irqsave(&dump_list_lock, flags);
+	list_add(&dumper->list, &dump_list);
+	spin_unlock_irqrestore(&dump_list_lock, flags);
+	return 0;
+}
+EXPORT_SYMBOL(register_kmsg_dumper);
+
+/**
+ *	unregister_dump_device - unregister a dumpdevice.
+ *	@dump: pointer to the dump structure
+ *
+ *	Removes a dump device from the system.
+ */
+void unregister_kmsg_dumper(struct kmsg_dumper *dumper)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&dump_list_lock, flags);
+	list_del(&dumper->list);
+	spin_unlock_irqrestore(&dump_list_lock, flags);
+}
+EXPORT_SYMBOL(unregister_kmsg_dumper);
+
+static const char *kmsg_reasons[] = {
+	[kmsg_dump_oops]	= "oops",
+	[kmsg_dump_panic]	= "panic",
+};
+
+static const char *kmsg_to_str(enum kmsg_dump_reason reason)
+{
+	if (reason > ARRAY_SIZE(kmsg_reasons) || reason < 0)
+		return "unknown";
+
+	return kmsg_reasons[reason];
+}
+
+/**
+ *	dump_kmsg - dump kernel log to kernel message dumpers.
+ *	@reason: the reason (oops, panic etc) for dumping
+ *
+ *	Iterate through each of the dump devices and call the oops/panic
+ *	callbacks with the log buffer.
+ */
+void dump_kmsg(enum kmsg_dump_reason reason)
+{
+	unsigned long len = ACCESS_ONCE(log_end);
+	struct kmsg_dumper *dumper;
+	const char *s1, *s2;
+	unsigned long l1, l2;
+
+	s1 = "";
+	l1 = 0;
+	s2 = log_buf;
+	l2 = len;
+
+	/* Have we rotated around the circular buffer? */
+	if (len > log_buf_len) {
+		unsigned long pos = len & LOG_BUF_MASK;
+
+		s1 = log_buf + pos;
+		l1 = log_buf_len - pos;
+
+		s2 = log_buf;
+		l2 = pos;
+	}
+
+	if (!spin_trylock(&dump_list_lock)) {
+		printk(KERN_ERR "dump_kmsg: dump list lock is held during %s, skipping dump\n",
+				kmsg_to_str(reason));
+		return;
+	}
+	list_for_each_entry(dumper, &dump_list, list)
+		dumper->dump(dumper, reason, s1, l1, s2, l2);
+	spin_unlock(&dump_list_lock);
+}
-- 
1.6.0.4

Re: [PATCH v7 4/5]: core: Add kernel message dumper to call on oopses and panics

[Ingo Molnar]

* Simon Kagstrom <simon.kagstrom@netinsight.net> wrote:

> --- /dev/null
> +++ b/include/linux/kmsg_dump.h

> +enum kmsg_dump_reason {
> +	kmsg_dump_oops,
> +	kmsg_dump_panic,
> +};

Please capitalize constants - lower case suggests it's some sort of 
variable which it is not.

> +void dump_kmsg(enum kmsg_dump_reason reason);
> +
> +int register_kmsg_dumper(struct kmsg_dumper *dumper);
> +
> +void unregister_kmsg_dumper(struct kmsg_dumper *dumper);

Please rename these APIs to be more in line with how we name new kernel 
APIs. Something like:

  kmsg_dump()
  kmsg_dumper_register()
  kmsg_dumper_unregister()

(we start with the subsystem as a prefix, then we go from more generic 
to less generic words.)

Might even make sense to name them all kmsg_dump:

  kmsg_dump()
  kmsg_dump_register()
  kmsg_dump_unregister()

Thanks,

	Ingo

[PATCH v8 4/5] core: Add kernel message dumper to call on oopses and panics

[Simon Kagstrom]

The core functionality is implemented as per Linus suggestion from

  http://lists.infradead.org/pipermail/linux-mtd/2009-October/027620.html

(with the kmsg_dump implementation by Linus). A struct kmsg_dumper has
been added which contains a callback to dump the kernel log buffers on
crashes. The kmsg_dump function gets called from oops_exit() and panic()
and invokes this callbacks with the crash reason.

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
Reviewed-by: Anders Grafstrom <anders.grafstrom@netinsight.net>
---
ChangeLog:
        * (Ingo Molnar) rename functions to new standard
	* (Ingo Molnar) use capitals for constants

 include/linux/kmsg_dump.h |   36 +++++++++++++++
 kernel/panic.c            |    3 +
 kernel/printk.c           |  104 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 143 insertions(+), 0 deletions(-)
 create mode 100644 include/linux/kmsg_dump.h

diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h
new file mode 100644
index 0000000..4fb9d29
--- /dev/null
+++ b/include/linux/kmsg_dump.h
@@ -0,0 +1,36 @@
+/*
+ * linux/include/kmsg_dump.h
+ *
+ * Copyright (C) 2009 Net Insight AB
+ *
+ * Author: Simon Kagstrom <simon.kagstrom@netinsight.net>
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file COPYING in the main directory of this archive
+ * for more details.
+ */
+#ifndef _LINUX_KMSG_DUMP_H
+#define _LINUX_KMSG_DUMP_H
+
+#include <linux/list.h>
+
+enum kmsg_dump_reason {
+	KMSG_DUMP_OOPS,
+	KMSG_DUMP_PANIC,
+};
+
+struct kmsg_dumper {
+	void (*dump)(struct kmsg_dumper *dumper, enum kmsg_dump_reason reason,
+			const char *s1, unsigned long l1,
+			const char *s2, unsigned long l2);
+	struct list_head list;
+	int registered;
+};
+
+void kmsg_dump(enum kmsg_dump_reason reason);
+
+int kmsg_dump_register(struct kmsg_dumper *dumper);
+
+void kmsg_dump_unregister(struct kmsg_dumper *dumper);
+
+#endif /* _LINUX_DUMP_DEVICE_H */
diff --git a/kernel/panic.c b/kernel/panic.c
index c0b33b8..763296d 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -10,6 +10,7 @@
  */
 #include <linux/debug_locks.h>
 #include <linux/interrupt.h>
+#include <linux/kmsg_dump.h>
 #include <linux/kallsyms.h>
 #include <linux/notifier.h>
 #include <linux/module.h>
@@ -76,6 +77,7 @@ NORET_TYPE void panic(const char * fmt, ...)
 	dump_stack();
 #endif
 
+	kmsg_dump(KMSG_DUMP_PANIC);
 	/*
 	 * If we have crashed and we have a crash kernel loaded let it handle
 	 * everything else.
@@ -341,6 +343,7 @@ void oops_exit(void)
 {
 	do_oops_enter_exit();
 	print_oops_end_marker();
+	kmsg_dump(KMSG_DUMP_OOPS);
 }
 
 #ifdef WANT_WARN_ON_SLOWPATH
diff --git a/kernel/printk.c b/kernel/printk.c
index f38b07f..960406a 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -33,6 +33,8 @@
 #include <linux/bootmem.h>
 #include <linux/syscalls.h>
 #include <linux/kexec.h>
+#include <linux/kmsg_dump.h>
+#include <linux/spinlock.h>
 
 #include <asm/uaccess.h>
 
@@ -1405,3 +1407,105 @@ bool printk_timed_ratelimit(unsigned long *caller_jiffies,
 }
 EXPORT_SYMBOL(printk_timed_ratelimit);
 #endif
+
+static LIST_HEAD(dump_list);
+static DEFINE_SPINLOCK(dump_list_lock);
+
+/**
+ *	kmsg_dump_register - register a kernel log dumper.
+ *	@dump: pointer to the kmsg_dumper structure
+ *	@priv: private data for the structure
+ *
+ *	Adds a kernel log dumper to the system. The dump callback in the
+ *	structure will be called when the kernel oopses or panics and must be
+ *	set. Returns zero on success and -EINVAL or -EBUSY otherwise.
+ */
+int kmsg_dump_register(struct kmsg_dumper *dumper)
+{
+	unsigned long flags;
+
+	/* The dump callback needs to be set */
+	if (!dumper->dump)
+		return -EINVAL;
+
+	/* Don't allow registering multiple times */
+	if (dumper->registered)
+		return -EBUSY;
+
+	dumper->registered = 1;
+
+	spin_lock_irqsave(&dump_list_lock, flags);
+	list_add(&dumper->list, &dump_list);
+	spin_unlock_irqrestore(&dump_list_lock, flags);
+	return 0;
+}
+EXPORT_SYMBOL(kmsg_dump_register);
+
+/**
+ *	kmsg_dump_unregister - unregister a kmsg dumper.
+ *	@dump: pointer to the kmsg_dumper structure
+ *
+ *	Removes a dump device from the system.
+ */
+void kmsg_dump_unregister(struct kmsg_dumper *dumper)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&dump_list_lock, flags);
+	list_del(&dumper->list);
+	spin_unlock_irqrestore(&dump_list_lock, flags);
+}
+EXPORT_SYMBOL(kmsg_dump_unregister);
+
+static const char *kmsg_reasons[] = {
+	[KMSG_DUMP_OOPS]	= "oops",
+	[KMSG_DUMP_PANIC]	= "panic",
+};
+
+static const char *kmsg_to_str(enum kmsg_dump_reason reason)
+{
+	if (reason > ARRAY_SIZE(kmsg_reasons) || reason < 0)
+		return "unknown";
+
+	return kmsg_reasons[reason];
+}
+
+/**
+ *	dump_kmsg - dump kernel log to kernel message dumpers.
+ *	@reason: the reason (oops, panic etc) for dumping
+ *
+ *	Iterate through each of the dump devices and call the oops/panic
+ *	callbacks with the log buffer.
+ */
+void kmsg_dump(enum kmsg_dump_reason reason)
+{
+	unsigned long len = ACCESS_ONCE(log_end);
+	struct kmsg_dumper *dumper;
+	const char *s1, *s2;
+	unsigned long l1, l2;
+
+	s1 = "";
+	l1 = 0;
+	s2 = log_buf;
+	l2 = len;
+
+	/* Have we rotated around the circular buffer? */
+	if (len > log_buf_len) {
+		unsigned long pos = len & LOG_BUF_MASK;
+
+		s1 = log_buf + pos;
+		l1 = log_buf_len - pos;
+
+		s2 = log_buf;
+		l2 = pos;
+	}
+
+	if (!spin_trylock(&dump_list_lock)) {
+		printk(KERN_ERR "dump_kmsg: dump list lock is held during %s, skipping dump\n",
+				kmsg_to_str(reason));
+		return;
+	}
+	list_for_each_entry(dumper, &dump_list, list)
+		dumper->dump(dumper, reason, s1, l1, s2, l2);
+	spin_unlock(&dump_list_lock);
+}
-- 
1.6.0.4

Re: [PATCH v8 4/5] core: Add kernel message dumper to call on oopses and panics

[Ingo Molnar]

* Simon Kagstrom <simon.kagstrom@netinsight.net> wrote:

> The core functionality is implemented as per Linus suggestion from
> 
>   http://lists.infradead.org/pipermail/linux-mtd/2009-October/027620.html
> 
> (with the kmsg_dump implementation by Linus). A struct kmsg_dumper has
> been added which contains a callback to dump the kernel log buffers on
> crashes. The kmsg_dump function gets called from oops_exit() and panic()
> and invokes this callbacks with the crash reason.
> 
> Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
> Reviewed-by: Anders Grafstrom <anders.grafstrom@netinsight.net>

The general structure looks very nice now! Assuming my review comments 
below are addressed all patches are:

  Reviewed-by: Ingo Molnar <mingo@elte.hu>

> diff --git a/kernel/printk.c b/kernel/printk.c
> index f38b07f..960406a 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -33,6 +33,8 @@
>  #include <linux/bootmem.h>
>  #include <linux/syscalls.h>
>  #include <linux/kexec.h>
> +#include <linux/kmsg_dump.h>
> +#include <linux/spinlock.h>

( Small nit: in theory the spinlock.h include should not be needed as 
  printk.c already uses spinlocks and gets the types via mutex.h. )

>  
>  #include <asm/uaccess.h>
>  
> @@ -1405,3 +1407,105 @@ bool printk_timed_ratelimit(unsigned long *caller_jiffies,
>  }
>  EXPORT_SYMBOL(printk_timed_ratelimit);
>  #endif
> +
> +static LIST_HEAD(dump_list);
> +static DEFINE_SPINLOCK(dump_list_lock);

Please switch it around to be:

  static DEFINE_SPINLOCK(dump_list_lock);

  static LIST_HEAD(dump_list);

as the lock will be cacheline aligned on SMP, so the list head can come 
after it 'for free'.

If it's the other way around we'll use 8/16 more .data bytes on average.

> +
> +/**
> + *	kmsg_dump_register - register a kernel log dumper.
> + *	@dump: pointer to the kmsg_dumper structure
> + *	@priv: private data for the structure
> + *
> + *	Adds a kernel log dumper to the system. The dump callback in the
> + *	structure will be called when the kernel oopses or panics and must be
> + *	set. Returns zero on success and -EINVAL or -EBUSY otherwise.
> + */
> +int kmsg_dump_register(struct kmsg_dumper *dumper)
> +{
> +	unsigned long flags;
> +
> +	/* The dump callback needs to be set */
> +	if (!dumper->dump)
> +		return -EINVAL;
> +
> +	/* Don't allow registering multiple times */
> +	if (dumper->registered)
> +		return -EBUSY;
> +
> +	dumper->registered = 1;
> +
> +	spin_lock_irqsave(&dump_list_lock, flags);
> +	list_add(&dumper->list, &dump_list);
> +	spin_unlock_irqrestore(&dump_list_lock, flags);
> +	return 0;
> +}
> +EXPORT_SYMBOL(kmsg_dump_register);

There's a race here: dumper->registered should be set to 1 inside the 
spinlock - to make the register/unregister API SMP safe.

It probably doesnt matter much in practice right now (as the dumper will 
be registered during bootup and unregistered during shutdown), but still 
- it could matter to modular loading of multiple dumpers at once, in the 
future.

Also, the check for ->registered should be done inside too.

Plus a style nit: please put a newline before the 'return 0' - it looks 
more symmetric and separates the return from other code flow.

> +
> +/**
> + *	kmsg_dump_unregister - unregister a kmsg dumper.
> + *	@dump: pointer to the kmsg_dumper structure
> + *
> + *	Removes a dump device from the system.
> + */
> +void kmsg_dump_unregister(struct kmsg_dumper *dumper)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&dump_list_lock, flags);
> +	list_del(&dumper->list);
> +	spin_unlock_irqrestore(&dump_list_lock, flags);
> +}
> +EXPORT_SYMBOL(kmsg_dump_unregister);

I'd suggest for this API to use an error return as well, and to do it 
safely - i.e. any combination of these APIs should result in a safe 
result.

Right now a call sequence kmsg_dump_register() + kmsg_dump_unregister() 
+ kmsg_dump_register() will corrupt memory.

> +
> +static const char *kmsg_reasons[] = {
> +	[KMSG_DUMP_OOPS]	= "oops",
> +	[KMSG_DUMP_PANIC]	= "panic",
> +};

Should be 'const char const' for max constness.

> +static const char *kmsg_to_str(enum kmsg_dump_reason reason)
> +{
> +	if (reason > ARRAY_SIZE(kmsg_reasons) || reason < 0)
> +		return "unknown";

That should be ">=" i guess, for the check to be correct.

> +
> +	return kmsg_reasons[reason];
> +}
> +
> +/**
> + *	dump_kmsg - dump kernel log to kernel message dumpers.
> + *	@reason: the reason (oops, panic etc) for dumping
> + *
> + *	Iterate through each of the dump devices and call the oops/panic
> + *	callbacks with the log buffer.
> + */
> +void kmsg_dump(enum kmsg_dump_reason reason)
> +{
> +	unsigned long len = ACCESS_ONCE(log_end);
> +	struct kmsg_dumper *dumper;
> +	const char *s1, *s2;
> +	unsigned long l1, l2;
> +
> +	s1 = "";
> +	l1 = 0;
> +	s2 = log_buf;
> +	l2 = len;
> +
> +	/* Have we rotated around the circular buffer? */
> +	if (len > log_buf_len) {
> +		unsigned long pos = len & LOG_BUF_MASK;
> +
> +		s1 = log_buf + pos;
> +		l1 = log_buf_len - pos;
> +
> +		s2 = log_buf;
> +		l2 = pos;
> +	}
> +
> +	if (!spin_trylock(&dump_list_lock)) {
> +		printk(KERN_ERR "dump_kmsg: dump list lock is held during %s, skipping dump\n",
> +				kmsg_to_str(reason));
> +		return;
> +	}
> +	list_for_each_entry(dumper, &dump_list, list)
> +		dumper->dump(dumper, reason, s1, l1, s2, l2);
> +	spin_unlock(&dump_list_lock);
> +}

( Might make sense to use _irqsave()/_irqrestore() variants here - so 
  that if an IRQ comes in and panics too we dont recurse. The trylock 
  protects us above, but we are already non-preempt here - going irqsafe 
  is even better i guess. )

	Ingo

[PATCH v9 4/5] core: Add kernel message dumper to call on oopses and panics

[Simon Kagstrom]

The core functionality is implemented as per Linus suggestion from

  http://lists.infradead.org/pipermail/linux-mtd/2009-October/027620.html

(with the kmsg_dump implementation by Linus). A struct kmsg_dumper has
been added which contains a callback to dump the kernel log buffers on
crashes. The kmsg_dump function gets called from oops_exit() and panic()
and invokes this callbacks with the crash reason.

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
Reviewed-by: Anders Grafstrom <anders.grafstrom@netinsight.net>
Reviewed-by: Ingo Molnar <mingo@elte.hu>
---
ChangeLog:
	* (Arjan van de Ven): Use EXPORT_SYMBOL_GPL
	* (Ingo Molnar): Switch list/spinlock definition order
	* (Ingo Molnar): Set/check ->registered within spinlocked region
	* (Ingo Molnar): Return error from unregister and check that the
	  dumper really is registered
	* (Ingo Molnar): Maximise constness and correct array check
	* (Ingo Molnar): Use _irqsave/_irqrestore also in kmsg_dump
	* (Ingo Molnar): Style fixes
	* (Anders Grafström): Correct kerneldoc names

 include/linux/kmsg_dump.h |   36 ++++++++++++++
 kernel/panic.c            |    3 +
 kernel/printk.c           |  116 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 155 insertions(+), 0 deletions(-)
 create mode 100644 include/linux/kmsg_dump.h

diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h
new file mode 100644
index 0000000..64296f3
--- /dev/null
+++ b/include/linux/kmsg_dump.h
@@ -0,0 +1,36 @@
+/*
+ * linux/include/kmsg_dump.h
+ *
+ * Copyright (C) 2009 Net Insight AB
+ *
+ * Author: Simon Kagstrom <simon.kagstrom@netinsight.net>
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file COPYING in the main directory of this archive
+ * for more details.
+ */
+#ifndef _LINUX_KMSG_DUMP_H
+#define _LINUX_KMSG_DUMP_H
+
+#include <linux/list.h>
+
+enum kmsg_dump_reason {
+	KMSG_DUMP_OOPS,
+	KMSG_DUMP_PANIC,
+};
+
+struct kmsg_dumper {
+	void (*dump)(struct kmsg_dumper *dumper, enum kmsg_dump_reason reason,
+			const char *s1, unsigned long l1,
+			const char *s2, unsigned long l2);
+	struct list_head list;
+	int registered;
+};
+
+void kmsg_dump(enum kmsg_dump_reason reason);
+
+int kmsg_dump_register(struct kmsg_dumper *dumper);
+
+int kmsg_dump_unregister(struct kmsg_dumper *dumper);
+
+#endif /* _LINUX_DUMP_DEVICE_H */
diff --git a/kernel/panic.c b/kernel/panic.c
index c0b33b8..763296d 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -10,6 +10,7 @@
  */
 #include <linux/debug_locks.h>
 #include <linux/interrupt.h>
+#include <linux/kmsg_dump.h>
 #include <linux/kallsyms.h>
 #include <linux/notifier.h>
 #include <linux/module.h>
@@ -76,6 +77,7 @@ NORET_TYPE void panic(const char * fmt, ...)
 	dump_stack();
 #endif
 
+	kmsg_dump(KMSG_DUMP_PANIC);
 	/*
 	 * If we have crashed and we have a crash kernel loaded let it handle
 	 * everything else.
@@ -341,6 +343,7 @@ void oops_exit(void)
 {
 	do_oops_enter_exit();
 	print_oops_end_marker();
+	kmsg_dump(KMSG_DUMP_OOPS);
 }
 
 #ifdef WANT_WARN_ON_SLOWPATH
diff --git a/kernel/printk.c b/kernel/printk.c
index f38b07f..d363306 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -33,6 +33,7 @@
 #include <linux/bootmem.h>
 #include <linux/syscalls.h>
 #include <linux/kexec.h>
+#include <linux/kmsg_dump.h>
 
 #include <asm/uaccess.h>
 
@@ -1405,3 +1406,118 @@ bool printk_timed_ratelimit(unsigned long *caller_jiffies,
 }
 EXPORT_SYMBOL(printk_timed_ratelimit);
 #endif
+
+static DEFINE_SPINLOCK(dump_list_lock);
+static LIST_HEAD(dump_list);
+
+/**
+ *	kmsg_dump_register - register a kernel log dumper.
+ *	@dump: pointer to the kmsg_dumper structure
+ *
+ *	Adds a kernel log dumper to the system. The dump callback in the
+ *	structure will be called when the kernel oopses or panics and must be
+ *	set. Returns zero on success and -EINVAL or -EBUSY otherwise.
+ */
+int kmsg_dump_register(struct kmsg_dumper *dumper)
+{
+	unsigned long flags;
+
+	/* The dump callback needs to be set */
+	if (!dumper->dump)
+		return -EINVAL;
+
+	spin_lock_irqsave(&dump_list_lock, flags);
+
+	/* Don't allow registering multiple times */
+	if (dumper->registered) {
+		spin_unlock_irqrestore(&dump_list_lock, flags);
+
+		return -EBUSY;
+	}
+
+	dumper->registered = 1;
+	list_add(&dumper->list, &dump_list);
+	spin_unlock_irqrestore(&dump_list_lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(kmsg_dump_register);
+
+/**
+ *	kmsg_dump_unregister - unregister a kmsg dumper.
+ *	@dump: pointer to the kmsg_dumper structure
+ *
+ *	Removes a dump device from the system.
+ */
+int kmsg_dump_unregister(struct kmsg_dumper *dumper)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&dump_list_lock, flags);
+	if (!dumper->registered) {
+		spin_unlock_irqrestore(&dump_list_lock, flags);
+
+		return -EINVAL;
+	}
+
+	dumper->registered = 0;
+	list_del(&dumper->list);
+	spin_unlock_irqrestore(&dump_list_lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(kmsg_dump_unregister);
+
+static const char const *kmsg_reasons[] = {
+	[KMSG_DUMP_OOPS]	= "oops",
+	[KMSG_DUMP_PANIC]	= "panic",
+};
+
+static const char *kmsg_to_str(enum kmsg_dump_reason reason)
+{
+	if (reason >= ARRAY_SIZE(kmsg_reasons) || reason < 0)
+		return "unknown";
+
+	return kmsg_reasons[reason];
+}
+
+/**
+ *	kmsg_dump - dump kernel log to kernel message dumpers.
+ *	@reason: the reason (oops, panic etc) for dumping
+ *
+ *	Iterate through each of the dump devices and call the oops/panic
+ *	callbacks with the log buffer.
+ */
+void kmsg_dump(enum kmsg_dump_reason reason)
+{
+	unsigned long len = ACCESS_ONCE(log_end);
+	struct kmsg_dumper *dumper;
+	const char *s1, *s2;
+	unsigned long l1, l2;
+	unsigned long flags;
+
+	s1 = "";
+	l1 = 0;
+	s2 = log_buf;
+	l2 = len;
+
+	/* Have we rotated around the circular buffer? */
+	if (len > log_buf_len) {
+		unsigned long pos = len & LOG_BUF_MASK;
+
+		s1 = log_buf + pos;
+		l1 = log_buf_len - pos;
+
+		s2 = log_buf;
+		l2 = pos;
+	}
+
+	if (!spin_trylock_irqsave(&dump_list_lock, flags)) {
+		printk(KERN_ERR "dump_kmsg: dump list lock is held during %s, skipping dump\n",
+				kmsg_to_str(reason));
+		return;
+	}
+	list_for_each_entry(dumper, &dump_list, list)
+		dumper->dump(dumper, reason, s1, l1, s2, l2);
+	spin_unlock_irqrestore(&dump_list_lock, flags);
+}
-- 
1.6.0.4

Re: [PATCH v9 4/5] core: Add kernel message dumper to call on oopses and panics

[Ingo Molnar]

* Simon Kagstrom <simon.kagstrom@netinsight.net> wrote:

/**
 *	kmsg_dump_register - register a kernel log dumper.
 *	@dump: pointer to the kmsg_dumper structure
 *
 *	Adds a kernel log dumper to the system. The dump callback in the
 *	structure will be called when the kernel oopses or panics and must be
 *	set. Returns zero on success and -EINVAL or -EBUSY otherwise.
 */

Small detail - please use the KernelDoc style used in existing printk.c 
facilities, i.e. use a single space instead of a tab:

/**
 * kmsg_dump_register - register a kernel log dumper.
 * @dump: pointer to the kmsg_dumper structure
 *
 * Adds a kernel log dumper to the system. The dump callback in the
 * structure will be called when the kernel oopses or panics and must be
 * set. Returns zero on success and -EINVAL or -EBUSY otherwise.
 */

(in the other functions as well)

	Ingo

Re: [PATCH v9 4/5] core: Add kernel message dumper to call on oopses and panics

[Artem Bityutskiy]

On Fri, 2009-10-16 at 10:09 +0200, Ingo Molnar wrote:
> * Simon Kagstrom <simon.kagstrom@netinsight.net> wrote:
> 
> /**
>  *	kmsg_dump_register - register a kernel log dumper.
>  *	@dump: pointer to the kmsg_dumper structure
>  *
>  *	Adds a kernel log dumper to the system. The dump callback in the
>  *	structure will be called when the kernel oopses or panics and must be
>  *	set. Returns zero on success and -EINVAL or -EBUSY otherwise.
>  */
> 
> Small detail - please use the KernelDoc style used in existing printk.c 
> facilities, i.e. use a single space instead of a tab:
> 
> /**
>  * kmsg_dump_register - register a kernel log dumper.
>  * @dump: pointer to the kmsg_dumper structure
>  *
>  * Adds a kernel log dumper to the system. The dump callback in the
>  * structure will be called when the kernel oopses or panics and must be
>  * set. Returns zero on success and -EINVAL or -EBUSY otherwise.
>  */

And then %-EINVAL and %-EBUSY.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

[PATCH v10 4/5] core: Add kernel message dumper to call on oopses and panics

[Simon Kagstrom]

The core functionality is implemented as per Linus suggestion from

  http://lists.infradead.org/pipermail/linux-mtd/2009-October/027620.html

(with the kmsg_dump implementation by Linus). A struct kmsg_dumper has
been added which contains a callback to dump the kernel log buffers on
crashes. The kmsg_dump function gets called from oops_exit() and panic()
and invokes this callbacks with the crash reason.

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
Reviewed-by: Anders Grafstrom <anders.grafstrom@netinsight.net>
Reviewed-by: Ingo Molnar <mingo@elte.hu>
---
ChangeLog:
	* (Ingo Molnar): tabs->spaces in KernelDoc comments
	* (Artem Bityutskiy): %-EINVAL
	* (me): Add KernelDoc for the structure as well

 include/linux/kmsg_dump.h |   44 +++++++++++++++++
 kernel/panic.c            |    3 +
 kernel/printk.c           |  117 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 164 insertions(+), 0 deletions(-)
 create mode 100644 include/linux/kmsg_dump.h

diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h
new file mode 100644
index 0000000..27d3aec
--- /dev/null
+++ b/include/linux/kmsg_dump.h
@@ -0,0 +1,44 @@
+/*
+ * linux/include/kmsg_dump.h
+ *
+ * Copyright (C) 2009 Net Insight AB
+ *
+ * Author: Simon Kagstrom <simon.kagstrom@netinsight.net>
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file COPYING in the main directory of this archive
+ * for more details.
+ */
+#ifndef _LINUX_KMSG_DUMP_H
+#define _LINUX_KMSG_DUMP_H
+
+#include <linux/list.h>
+
+enum kmsg_dump_reason {
+	KMSG_DUMP_OOPS,
+	KMSG_DUMP_PANIC,
+};
+
+/**
+ * struct kmsg_dumper - kernel crash message dumper structure
+ * @dump:	The callback which gets called on crashes. The buffer is passed
+ * 		as two sections, where s1 (length l1) contains the older
+ * 		messages and s2 (length l2) contains the newer.
+ * @list:	Entry in the dumper list (private)
+ * @registered:	Flag that specifies if this is already registered
+ */
+struct kmsg_dumper {
+	void (*dump)(struct kmsg_dumper *dumper, enum kmsg_dump_reason reason,
+			const char *s1, unsigned long l1,
+			const char *s2, unsigned long l2);
+	struct list_head list;
+	int registered;
+};
+
+void kmsg_dump(enum kmsg_dump_reason reason);
+
+int kmsg_dump_register(struct kmsg_dumper *dumper);
+
+int kmsg_dump_unregister(struct kmsg_dumper *dumper);
+
+#endif /* _LINUX_DUMP_DEVICE_H */
diff --git a/kernel/panic.c b/kernel/panic.c
index c0b33b8..763296d 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -10,6 +10,7 @@
  */
 #include <linux/debug_locks.h>
 #include <linux/interrupt.h>
+#include <linux/kmsg_dump.h>
 #include <linux/kallsyms.h>
 #include <linux/notifier.h>
 #include <linux/module.h>
@@ -76,6 +77,7 @@ NORET_TYPE void panic(const char * fmt, ...)
 	dump_stack();
 #endif
 
+	kmsg_dump(KMSG_DUMP_PANIC);
 	/*
 	 * If we have crashed and we have a crash kernel loaded let it handle
 	 * everything else.
@@ -341,6 +343,7 @@ void oops_exit(void)
 {
 	do_oops_enter_exit();
 	print_oops_end_marker();
+	kmsg_dump(KMSG_DUMP_OOPS);
 }
 
 #ifdef WANT_WARN_ON_SLOWPATH
diff --git a/kernel/printk.c b/kernel/printk.c
index f38b07f..59116be 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -33,6 +33,7 @@
 #include <linux/bootmem.h>
 #include <linux/syscalls.h>
 #include <linux/kexec.h>
+#include <linux/kmsg_dump.h>
 
 #include <asm/uaccess.h>
 
@@ -1405,3 +1406,119 @@ bool printk_timed_ratelimit(unsigned long *caller_jiffies,
 }
 EXPORT_SYMBOL(printk_timed_ratelimit);
 #endif
+
+static DEFINE_SPINLOCK(dump_list_lock);
+static LIST_HEAD(dump_list);
+
+/**
+ * kmsg_dump_register - register a kernel log dumper.
+ * @dump: pointer to the kmsg_dumper structure
+ *
+ * Adds a kernel log dumper to the system. The dump callback in the
+ * structure will be called when the kernel oopses or panics and must be
+ * set. Returns zero on success and %-EINVAL or %-EBUSY otherwise.
+ */
+int kmsg_dump_register(struct kmsg_dumper *dumper)
+{
+	unsigned long flags;
+
+	/* The dump callback needs to be set */
+	if (!dumper->dump)
+		return -EINVAL;
+
+	spin_lock_irqsave(&dump_list_lock, flags);
+
+	/* Don't allow registering multiple times */
+	if (dumper->registered) {
+		spin_unlock_irqrestore(&dump_list_lock, flags);
+
+		return -EBUSY;
+	}
+
+	dumper->registered = 1;
+	list_add(&dumper->list, &dump_list);
+	spin_unlock_irqrestore(&dump_list_lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(kmsg_dump_register);
+
+/**
+ * kmsg_dump_unregister - unregister a kmsg dumper.
+ * @dump: pointer to the kmsg_dumper structure
+ *
+ * Removes a dump device from the system. Returns zero on success and
+ * %-EINVAL otherwise.
+ */
+int kmsg_dump_unregister(struct kmsg_dumper *dumper)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&dump_list_lock, flags);
+	if (!dumper->registered) {
+		spin_unlock_irqrestore(&dump_list_lock, flags);
+
+		return -EINVAL;
+	}
+
+	dumper->registered = 0;
+	list_del(&dumper->list);
+	spin_unlock_irqrestore(&dump_list_lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(kmsg_dump_unregister);
+
+static const char const *kmsg_reasons[] = {
+	[KMSG_DUMP_OOPS]	= "oops",
+	[KMSG_DUMP_PANIC]	= "panic",
+};
+
+static const char *kmsg_to_str(enum kmsg_dump_reason reason)
+{
+	if (reason >= ARRAY_SIZE(kmsg_reasons) || reason < 0)
+		return "unknown";
+
+	return kmsg_reasons[reason];
+}
+
+/**
+ * kmsg_dump - dump kernel log to kernel message dumpers.
+ * @reason: the reason (oops, panic etc) for dumping
+ *
+ * Iterate through each of the dump devices and call the oops/panic
+ * callbacks with the log buffer.
+ */
+void kmsg_dump(enum kmsg_dump_reason reason)
+{
+	unsigned long len = ACCESS_ONCE(log_end);
+	struct kmsg_dumper *dumper;
+	const char *s1, *s2;
+	unsigned long l1, l2;
+	unsigned long flags;
+
+	s1 = "";
+	l1 = 0;
+	s2 = log_buf;
+	l2 = len;
+
+	/* Have we rotated around the circular buffer? */
+	if (len > log_buf_len) {
+		unsigned long pos = len & LOG_BUF_MASK;
+
+		s1 = log_buf + pos;
+		l1 = log_buf_len - pos;
+
+		s2 = log_buf;
+		l2 = pos;
+	}
+
+	if (!spin_trylock_irqsave(&dump_list_lock, flags)) {
+		printk(KERN_ERR "dump_kmsg: dump list lock is held during %s, skipping dump\n",
+				kmsg_to_str(reason));
+		return;
+	}
+	list_for_each_entry(dumper, &dump_list, list)
+		dumper->dump(dumper, reason, s1, l1, s2, l2);
+	spin_unlock_irqrestore(&dump_list_lock, flags);
+}
-- 
1.6.0.4

Re: [PATCH v10 4/5] core: Add kernel message dumper to call on oopses and panics

[Ingo Molnar]

* Simon Kagstrom <simon.kagstrom@netinsight.net> wrote:

> +int kmsg_dump_register(struct kmsg_dumper *dumper)
> +{
> +	unsigned long flags;
> +
> +	/* The dump callback needs to be set */
> +	if (!dumper->dump)
> +		return -EINVAL;
> +
> +	spin_lock_irqsave(&dump_list_lock, flags);
> +
> +	/* Don't allow registering multiple times */
> +	if (dumper->registered) {
> +		spin_unlock_irqrestore(&dump_list_lock, flags);
> +
> +		return -EBUSY;
> +	}
> +
> +	dumper->registered = 1;
> +	list_add(&dumper->list, &dump_list);
> +	spin_unlock_irqrestore(&dump_list_lock, flags);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(kmsg_dump_register);


i dont want to bikeshed paint this but this sequence caught my eyes. We 
generally do flatter and clearer locking sequences:

int kmsg_dump_register(struct kmsg_dumper *dumper)
{
	unsigned long flags;
	int err = -EBUSY;

	/* The dump callback needs to be set */
	if (!dumper->dump)
		return -EINVAL;

	spin_lock_irqsave(&dump_list_lock, flags);

	/* Don't allow registering multiple times */
	if (!dumper->registered) {
		dumper->registered = 1;
		list_add_tail(&dumper->list, &dump_list);
		err = 0;
	}

	spin_unlock_irqrestore(&dump_list_lock, flags);

	return err;
}
EXPORT_SYMBOL_GPL(kmsg_dump_register);

(warning: untested, written in mail editor)

Same goes for kmsg_dump_unregister().

	Ingo

Re: [PATCH v10 4/5] core: Add kernel message dumper to call on oopses and panics

[Artem Bityutskiy]

On Fri, 2009-10-16 at 12:10 +0200, Ingo Molnar wrote:
> * Simon Kagstrom <simon.kagstrom@netinsight.net> wrote:
> 
> > +int kmsg_dump_register(struct kmsg_dumper *dumper)
> > +{
> > +	unsigned long flags;
> > +
> > +	/* The dump callback needs to be set */
> > +	if (!dumper->dump)
> > +		return -EINVAL;
> > +
> > +	spin_lock_irqsave(&dump_list_lock, flags);
> > +
> > +	/* Don't allow registering multiple times */
> > +	if (dumper->registered) {
> > +		spin_unlock_irqrestore(&dump_list_lock, flags);
> > +
> > +		return -EBUSY;
> > +	}
> > +
> > +	dumper->registered = 1;
> > +	list_add(&dumper->list, &dump_list);
> > +	spin_unlock_irqrestore(&dump_list_lock, flags);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(kmsg_dump_register);
> 
> 
> i dont want to bikeshed paint this but this sequence caught my eyes. We 
> generally do flatter and clearer locking sequences:
> 
> int kmsg_dump_register(struct kmsg_dumper *dumper)
> {
> 	unsigned long flags;
> 	int err = -EBUSY;
> 
> 	/* The dump callback needs to be set */
> 	if (!dumper->dump)
> 		return -EINVAL;
> 
> 	spin_lock_irqsave(&dump_list_lock, flags);
> 
> 	/* Don't allow registering multiple times */
> 	if (!dumper->registered) {
> 		dumper->registered = 1;
> 		list_add_tail(&dumper->list, &dump_list);
> 		err = 0;
> 	}
> 
> 	spin_unlock_irqrestore(&dump_list_lock, flags);
> 
> 	return err;
> }
> EXPORT_SYMBOL_GPL(kmsg_dump_register);

And while we are on it, I think these extra lines before and after
spinlocks are unneeded and even a bit annoying :-)

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH v10 4/5] core: Add kernel message dumper to call on oopses and panics

[Ingo Molnar]

* Artem Bityutskiy <dedekind1@gmail.com> wrote:

> On Fri, 2009-10-16 at 12:10 +0200, Ingo Molnar wrote:
> > * Simon Kagstrom <simon.kagstrom@netinsight.net> wrote:
> > 
> > > +int kmsg_dump_register(struct kmsg_dumper *dumper)
> > > +{
> > > +	unsigned long flags;
> > > +
> > > +	/* The dump callback needs to be set */
> > > +	if (!dumper->dump)
> > > +		return -EINVAL;
> > > +
> > > +	spin_lock_irqsave(&dump_list_lock, flags);
> > > +
> > > +	/* Don't allow registering multiple times */
> > > +	if (dumper->registered) {
> > > +		spin_unlock_irqrestore(&dump_list_lock, flags);
> > > +
> > > +		return -EBUSY;
> > > +	}
> > > +
> > > +	dumper->registered = 1;
> > > +	list_add(&dumper->list, &dump_list);
> > > +	spin_unlock_irqrestore(&dump_list_lock, flags);
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(kmsg_dump_register);
> > 
> > 
> > i dont want to bikeshed paint this but this sequence caught my eyes. We 
> > generally do flatter and clearer locking sequences:
> > 
> > int kmsg_dump_register(struct kmsg_dumper *dumper)
> > {
> > 	unsigned long flags;
> > 	int err = -EBUSY;
> > 
> > 	/* The dump callback needs to be set */
> > 	if (!dumper->dump)
> > 		return -EINVAL;
> > 
> > 	spin_lock_irqsave(&dump_list_lock, flags);
> > 
> > 	/* Don't allow registering multiple times */
> > 	if (!dumper->registered) {
> > 		dumper->registered = 1;
> > 		list_add_tail(&dumper->list, &dump_list);
> > 		err = 0;
> > 	}
> > 
> > 	spin_unlock_irqrestore(&dump_list_lock, flags);
> > 
> > 	return err;
> > }
> > EXPORT_SYMBOL_GPL(kmsg_dump_register);
> 
> And while we are on it, I think these extra lines before and after 
> spinlocks are unneeded and even a bit annoying :-)

To me they increase readability quite a bit as it allows me to 
concentrate on just the inner critical section without the distraction 
of the lock/unlock sequence. YMMV.

	Ingo

[PATCH v11 4/5] core: Add kernel message dumper to call on oopses and panics

[Simon Kagstrom]

The core functionality is implemented as per Linus suggestion from

  http://lists.infradead.org/pipermail/linux-mtd/2009-October/027620.html

(with the kmsg_dump implementation by Linus). A struct kmsg_dumper has
been added which contains a callback to dump the kernel log buffers on
crashes. The kmsg_dump function gets called from oops_exit() and panic()
and invokes this callbacks with the crash reason.

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
Reviewed-by: Anders Grafstrom <anders.grafstrom@netinsight.net>
Reviewed-by: Ingo Molnar <mingo@elte.hu>
---
The bikeshed paint is drying:

ChangeLog:
	* (Ingo Molnar): Flatten lock use
	* (Artem, Aaro): Fix style issues

 include/linux/kmsg_dump.h |   44 ++++++++++++++++++
 kernel/panic.c            |    3 +
 kernel/printk.c           |  112 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 159 insertions(+), 0 deletions(-)
 create mode 100644 include/linux/kmsg_dump.h

diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h
new file mode 100644
index 0000000..7f089ec
--- /dev/null
+++ b/include/linux/kmsg_dump.h
@@ -0,0 +1,44 @@
+/*
+ * linux/include/kmsg_dump.h
+ *
+ * Copyright (C) 2009 Net Insight AB
+ *
+ * Author: Simon Kagstrom <simon.kagstrom@netinsight.net>
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file COPYING in the main directory of this archive
+ * for more details.
+ */
+#ifndef _LINUX_KMSG_DUMP_H
+#define _LINUX_KMSG_DUMP_H
+
+#include <linux/list.h>
+
+enum kmsg_dump_reason {
+	KMSG_DUMP_OOPS,
+	KMSG_DUMP_PANIC,
+};
+
+/**
+ * struct kmsg_dumper - kernel crash message dumper structure
+ * @dump:	The callback which gets called on crashes. The buffer is passed
+ * 		as two sections, where s1 (length l1) contains the older
+ * 		messages and s2 (length l2) contains the newer.
+ * @list:	Entry in the dumper list (private)
+ * @registered:	Flag that specifies if this is already registered
+ */
+struct kmsg_dumper {
+	void (*dump)(struct kmsg_dumper *dumper, enum kmsg_dump_reason reason,
+			const char *s1, unsigned long l1,
+			const char *s2, unsigned long l2);
+	struct list_head list;
+	int registered;
+};
+
+void kmsg_dump(enum kmsg_dump_reason reason);
+
+int kmsg_dump_register(struct kmsg_dumper *dumper);
+
+int kmsg_dump_unregister(struct kmsg_dumper *dumper);
+
+#endif /* _LINUX_KMSG_DUMP_H */
diff --git a/kernel/panic.c b/kernel/panic.c
index c0b33b8..763296d 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -10,6 +10,7 @@
  */
 #include <linux/debug_locks.h>
 #include <linux/interrupt.h>
+#include <linux/kmsg_dump.h>
 #include <linux/kallsyms.h>
 #include <linux/notifier.h>
 #include <linux/module.h>
@@ -76,6 +77,7 @@ NORET_TYPE void panic(const char * fmt, ...)
 	dump_stack();
 #endif
 
+	kmsg_dump(KMSG_DUMP_PANIC);
 	/*
 	 * If we have crashed and we have a crash kernel loaded let it handle
 	 * everything else.
@@ -341,6 +343,7 @@ void oops_exit(void)
 {
 	do_oops_enter_exit();
 	print_oops_end_marker();
+	kmsg_dump(KMSG_DUMP_OOPS);
 }
 
 #ifdef WANT_WARN_ON_SLOWPATH
diff --git a/kernel/printk.c b/kernel/printk.c
index f38b07f..f711b99 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -33,6 +33,7 @@
 #include <linux/bootmem.h>
 #include <linux/syscalls.h>
 #include <linux/kexec.h>
+#include <linux/kmsg_dump.h>
 
 #include <asm/uaccess.h>
 
@@ -1405,3 +1406,114 @@ bool printk_timed_ratelimit(unsigned long *caller_jiffies,
 }
 EXPORT_SYMBOL(printk_timed_ratelimit);
 #endif
+
+static DEFINE_SPINLOCK(dump_list_lock);
+static LIST_HEAD(dump_list);
+
+/**
+ * kmsg_dump_register - register a kernel log dumper.
+ * @dump: pointer to the kmsg_dumper structure
+ *
+ * Adds a kernel log dumper to the system. The dump callback in the
+ * structure will be called when the kernel oopses or panics and must be
+ * set. Returns zero on success and %-EINVAL or %-EBUSY otherwise.
+ */
+int kmsg_dump_register(struct kmsg_dumper *dumper)
+{
+	unsigned long flags;
+	int err = -EBUSY;
+
+	/* The dump callback needs to be set */
+	if (!dumper->dump)
+		return -EINVAL;
+
+	spin_lock_irqsave(&dump_list_lock, flags);
+	/* Don't allow registering multiple times */
+	if (!dumper->registered) {
+		dumper->registered = 1;
+		list_add_tail(&dumper->list, &dump_list);
+		err = 0;
+	}
+	spin_unlock_irqrestore(&dump_list_lock, flags);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(kmsg_dump_register);
+
+/**
+ * kmsg_dump_unregister - unregister a kmsg dumper.
+ * @dump: pointer to the kmsg_dumper structure
+ *
+ * Removes a dump device from the system. Returns zero on success and
+ * %-EINVAL otherwise.
+ */
+int kmsg_dump_unregister(struct kmsg_dumper *dumper)
+{
+	unsigned long flags;
+	int err = -EINVAL;
+
+	spin_lock_irqsave(&dump_list_lock, flags);
+	if (dumper->registered) {
+		dumper->registered = 0;
+		list_del(&dumper->list);
+		err = 0;
+	}
+	spin_unlock_irqrestore(&dump_list_lock, flags);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(kmsg_dump_unregister);
+
+static const char const *kmsg_reasons[] = {
+	[KMSG_DUMP_OOPS]	= "oops",
+	[KMSG_DUMP_PANIC]	= "panic",
+};
+
+static const char *kmsg_to_str(enum kmsg_dump_reason reason)
+{
+	if (reason >= ARRAY_SIZE(kmsg_reasons) || reason < 0)
+		return "unknown";
+
+	return kmsg_reasons[reason];
+}
+
+/**
+ * kmsg_dump - dump kernel log to kernel message dumpers.
+ * @reason: the reason (oops, panic etc) for dumping
+ *
+ * Iterate through each of the dump devices and call the oops/panic
+ * callbacks with the log buffer.
+ */
+void kmsg_dump(enum kmsg_dump_reason reason)
+{
+	unsigned long len = ACCESS_ONCE(log_end);
+	struct kmsg_dumper *dumper;
+	const char *s1, *s2;
+	unsigned long l1, l2;
+	unsigned long flags;
+
+	s1 = "";
+	l1 = 0;
+	s2 = log_buf;
+	l2 = len;
+
+	/* Have we rotated around the circular buffer? */
+	if (len > log_buf_len) {
+		unsigned long pos = len & LOG_BUF_MASK;
+
+		s1 = log_buf + pos;
+		l1 = log_buf_len - pos;
+
+		s2 = log_buf;
+		l2 = pos;
+	}
+
+	if (!spin_trylock_irqsave(&dump_list_lock, flags)) {
+		printk(KERN_ERR "dump_kmsg: dump list lock is held during %s, skipping dump\n",
+				kmsg_to_str(reason));
+		return;
+	}
+	list_for_each_entry(dumper, &dump_list, list)
+		dumper->dump(dumper, reason, s1, l1, s2, l2);
+	spin_unlock_irqrestore(&dump_list_lock, flags);
+}
-- 
1.6.0.4

Re: [PATCH v11 4/5] core: Add kernel message dumper to call on oopses and panics

[Artem Bityutskiy]

On Fri, 2009-10-16 at 14:09 +0200, Simon Kagstrom wrote:
> The core functionality is implemented as per Linus suggestion from
> 
>   http://lists.infradead.org/pipermail/linux-mtd/2009-October/027620.html
> 
> (with the kmsg_dump implementation by Linus). A struct kmsg_dumper has
> been added which contains a callback to dump the kernel log buffers on
> crashes. The kmsg_dump function gets called from oops_exit() and panic()
> and invokes this callbacks with the crash reason.
> 
> Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
> Reviewed-by: Anders Grafstrom <anders.grafstrom@netinsight.net>
> Reviewed-by: Ingo Molnar <mingo@elte.hu>
> ---
> The bikeshed paint is drying:
> 
> ChangeLog:
> 	* (Ingo Molnar): Flatten lock use
> 	* (Artem, Aaro): Fix style issues
> 
>  include/linux/kmsg_dump.h |   44 ++++++++++++++++++
>  kernel/panic.c            |    3 +
>  kernel/printk.c           |  112 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 159 insertions(+), 0 deletions(-)
>  create mode 100644 include/linux/kmsg_dump.h

I wonder, via which tree this should go in. We are going to have mtdoops
depend on this. Should we go one of these ways:

   * this patch goes to one of Ingo's tip trees, so we can pull it and
     work on top.
   * we have Ingo's / Linus' acks, and this goes via the MTD tree.
?

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH v11 4/5] core: Add kernel message dumper to call on oopses and panics

[Ingo Molnar]

* Artem Bityutskiy <dedekind1@gmail.com> wrote:

> On Fri, 2009-10-16 at 14:09 +0200, Simon Kagstrom wrote:
> > The core functionality is implemented as per Linus suggestion from
> > 
> >   http://lists.infradead.org/pipermail/linux-mtd/2009-October/027620.html
> > 
> > (with the kmsg_dump implementation by Linus). A struct kmsg_dumper has
> > been added which contains a callback to dump the kernel log buffers on
> > crashes. The kmsg_dump function gets called from oops_exit() and panic()
> > and invokes this callbacks with the crash reason.
> > 
> > Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
> > Reviewed-by: Anders Grafstrom <anders.grafstrom@netinsight.net>
> > Reviewed-by: Ingo Molnar <mingo@elte.hu>
> > ---
> > The bikeshed paint is drying:
> > 
> > ChangeLog:
> > 	* (Ingo Molnar): Flatten lock use
> > 	* (Artem, Aaro): Fix style issues
> > 
> >  include/linux/kmsg_dump.h |   44 ++++++++++++++++++
> >  kernel/panic.c            |    3 +
> >  kernel/printk.c           |  112 +++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 159 insertions(+), 0 deletions(-)
> >  create mode 100644 include/linux/kmsg_dump.h
> 
> I wonder, via which tree this should go in. We are going to have mtdoops
> depend on this. Should we go one of these ways:
> 
>    * this patch goes to one of Ingo's tip trees, so we can pull it and
>      work on top.
>    * we have Ingo's / Linus' acks, and this goes via the MTD tree.
> ?

Either way is good to me. Linus, do you have any preferences?

	Ingo

Re: [PATCH v11 4/5] core: Add kernel message dumper to call on oopses and panics

[Linus Torvalds]

On Mon, 19 Oct 2009, Ingo Molnar wrote:
> > 
> > I wonder, via which tree this should go in. We are going to have mtdoops
> > depend on this. Should we go one of these ways:
> > 
> >    * this patch goes to one of Ingo's tip trees, so we can pull it and
> >      work on top.
> >    * we have Ingo's / Linus' acks, and this goes via the MTD tree.
> > ?
> 
> Either way is good to me. Linus, do you have any preferences?

Either works for me.

		Linus

Re: [PATCH v11 4/5] core: Add kernel message dumper to call on oopses and panics

[Simon Kagstrom]

On Thu, 22 Oct 2009 08:33:11 +0900 (JST)
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> > > I wonder, via which tree this should go in. We are going to have mtdoops
> > > depend on this. Should we go one of these ways:
> > > 
> > >    * this patch goes to one of Ingo's tip trees, so we can pull it and
> > >      work on top.
> > >    * we have Ingo's / Linus' acks, and this goes via the MTD tree.
> > > ?
> > 
> > Either way is good to me. Linus, do you have any preferences?
> 
> Either works for me.

I would put it all through Artems MTD tree then so that we keep the new
API and the user of it close together then.

Thanks to everyone for the reviewing and suggestions!

// Simon

Re: [PATCH v11 4/5] core: Add kernel message dumper to call on oopses and panics

[Artem Bityutskiy]

On Thu, 2009-10-22 at 08:25 +0200, Simon Kagstrom wrote:
> On Thu, 22 Oct 2009 08:33:11 +0900 (JST)
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > > > I wonder, via which tree this should go in. We are going to have mtdoops
> > > > depend on this. Should we go one of these ways:
> > > > 
> > > >    * this patch goes to one of Ingo's tip trees, so we can pull it and
> > > >      work on top.
> > > >    * we have Ingo's / Linus' acks, and this goes via the MTD tree.
> > > > ?
> > > 
> > > Either way is good to me. Linus, do you have any preferences?
> > 
> > Either works for me.
> 
> I would put it all through Artems MTD tree then so that we keep the new
> API and the user of it close together then.
> 
> Thanks to everyone for the reviewing and suggestions!
> 

It's dwmw2's tree actually. I just act as his secretary :-)

David, could you push this patch to your tree? Ingo's and Linus' acks
are there, AFAIU.

Simon, I'll look at your other mtdoops patches as well a bit later.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH v11 4/5] core: Add kernel message dumper to call on oopses and panics

[Ingo Molnar]

* Artem Bityutskiy <dedekind1@gmail.com> wrote:

> On Thu, 2009-10-22 at 08:25 +0200, Simon Kagstrom wrote:
> > On Thu, 22 Oct 2009 08:33:11 +0900 (JST)
> > Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > 
> > > > > I wonder, via which tree this should go in. We are going to have mtdoops
> > > > > depend on this. Should we go one of these ways:
> > > > > 
> > > > >    * this patch goes to one of Ingo's tip trees, so we can pull it and
> > > > >      work on top.
> > > > >    * we have Ingo's / Linus' acks, and this goes via the MTD tree.
> > > > > ?
> > > > 
> > > > Either way is good to me. Linus, do you have any preferences?
> > > 
> > > Either works for me.
> > 
> > I would put it all through Artems MTD tree then so that we keep the new
> > API and the user of it close together then.
> > 
> > Thanks to everyone for the reviewing and suggestions!
> > 
> 
> It's dwmw2's tree actually. I just act as his secretary :-)
> 
> David, could you push this patch to your tree? Ingo's and Linus' acks 
> are there, AFAIU.

Yeah, the latest bits also have my:

Acked-by: Ingo Molnar <mingo@elte.hu>

	Ingo

Re: [PATCH v11 4/5] core: Add kernel message dumper to call on oopses and panics

[Shargorodsky Atal (EXT-Teleca/Helsinki)]

Hi all,

On Thu, 2009-10-22 at 08:25 +0200, ext Simon Kagstrom wrote:

> Thanks to everyone for the reviewing and suggestions!
> 

I have a couple of questions:

1. If somebody writes a module that uses dumper for uploading the
oopses/panics logs via some pay-per-byte medium,  since he has no way
to know in a module if the panic_on_oops flag is set, he'll have
to upload both oops and the following panic, because he does not
know for sure that the panic was caused by the oops. Hence he
pays twice for the same information, right?

I can think of a couple of way to figure it out in the module
itself, but I could not think of any clean way to do it.


2. We tried to use panic notifiers mechanism to print additional
information that we want to see in mtdoops logs and it worked well,
but having the kmsg_dump(KMSG_DUMP_PANIC) before the
atomic_notifier_call_chain() breaks this functionality.
Can we the call kmsg_dump() after the notifiers had been invoked?


Thanks,
Atal.

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

Re: [PATCH v11 4/5] core: Add kernel message dumper to call on oopses and panics

[Artem Bityutskiy]

On Fri, 2009-10-23 at 18:53 +0300, Shargorodsky Atal
(EXT-Teleca/Helsinki) wrote:
> Hi all,
> 
> On Thu, 2009-10-22 at 08:25 +0200, ext Simon Kagstrom wrote:
> 
> > Thanks to everyone for the reviewing and suggestions!
> > 
> 
> I have a couple of questions:
> 
> 1. If somebody writes a module that uses dumper for uploading the
> oopses/panics logs via some pay-per-byte medium,  since he has no way
> to know in a module if the panic_on_oops flag is set, he'll have
> to upload both oops and the following panic, because he does not
> know for sure that the panic was caused by the oops. Hence he
> pays twice for the same information, right?

Looks like a valid point to me. Indeed, in this case we will first call
'kmsg_dump(KMSG_DUMP_OOPS)' from 'oops_exit()', and then call it from
'panic()'. And the dumper may dump the same information, which is not
nice.

I've looked briefly and tried to figure out how to fix this. But I
cannot find an clean way. And I was confused by the die/oops/etc code.
My question is, why the code does not work the following way instead?

diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index 2d8a371..8f322c7 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -235,13 +235,12 @@ void __kprobes oops_end(unsigned long flags, struct pt_regs *regs, int signr)
        raw_local_irq_restore(flags);
        oops_exit();

-       if (!signr)
-               return;
        if (in_interrupt())
                panic("Fatal exception in interrupt");
        if (panic_on_oops)
                panic("Fatal exception");
-       do_exit(signr);
+       if (signr)
+               do_exit(signr);
 }

 int __kprobes __die(const char *str, struct pt_regs *regs, long err)

If the code worked like this, I think the problem indicated by Atal
could be easily fixed.

> 2. We tried to use panic notifiers mechanism to print additional
> information that we want to see in mtdoops logs and it worked well,
> but having the kmsg_dump(KMSG_DUMP_PANIC) before the
> atomic_notifier_call_chain() breaks this functionality.
> Can we the call kmsg_dump() after the notifiers had been invoked?

Atal, I think you should just attach your patch, it is easier to express
what you mean.

But for me it looks like atomic_notifier_call_chain() can be moved up.

Anyway, please, show your patch.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH v11 4/5] core: Add kernel message dumper to call on oopses and panics

[Shargorodsky Atal (EXT-Teleca/Helsinki)]

On Sat, 2009-10-24 at 19:05 +0200, Artem Bityutskiy wrote:
> On Fri, 2009-10-23 at 18:53 +0300, Shargorodsky Atal
> (EXT-Teleca/Helsinki) wrote:
> > Hi all,
> > 
> > On Thu, 2009-10-22 at 08:25 +0200, ext Simon Kagstrom wrote:
> > 
> > > Thanks to everyone for the reviewing and suggestions!
> > > 
> > 
> > I have a couple of questions:
> > 
> > 1. If somebody writes a module that uses dumper for uploading the
> > oopses/panics logs via some pay-per-byte medium,  since he has no way
> > to know in a module if the panic_on_oops flag is set, he'll have
> > to upload both oops and the following panic, because he does not
> > know for sure that the panic was caused by the oops. Hence he
> > pays twice for the same information, right?
> 
> Looks like a valid point to me. Indeed, in this case we will first call
> 'kmsg_dump(KMSG_DUMP_OOPS)' from 'oops_exit()', and then call it from
> 'panic()'. And the dumper may dump the same information, which is not
> nice.
> 
> I've looked briefly and tried to figure out how to fix this. But I
> cannot find an clean way. And I was confused by the die/oops/etc code.
> My question is, why the code does not work the following way instead?
> 
> diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
> index 2d8a371..8f322c7 100644
> --- a/arch/x86/kernel/dumpstack.c
> +++ b/arch/x86/kernel/dumpstack.c
> @@ -235,13 +235,12 @@ void __kprobes oops_end(unsigned long flags, struct pt_regs *regs, int signr)
>         raw_local_irq_restore(flags);
>         oops_exit();
> 
> -       if (!signr)
> -               return;
>         if (in_interrupt())
>                 panic("Fatal exception in interrupt");
>         if (panic_on_oops)
>                 panic("Fatal exception");
> -       do_exit(signr);
> +       if (signr)
> +               do_exit(signr);
>  }
> 
>  int __kprobes __die(const char *str, struct pt_regs *regs, long err)
> 
> If the code worked like this, I think the problem indicated by Atal
> could be easily fixed.
> 

Could as well be, but will not help us much on arch/arm. :)

> > 2. We tried to use panic notifiers mechanism to print additional
> > information that we want to see in mtdoops logs and it worked well,
> > but having the kmsg_dump(KMSG_DUMP_PANIC) before the
> > atomic_notifier_call_chain() breaks this functionality.
> > Can we the call kmsg_dump() after the notifiers had been invoked?
> 
> Atal, I think you should just attach your patch, it is easier to express
> what you mean.
> 
> But for me it looks like atomic_notifier_call_chain() can be moved up.
> 
> Anyway, please, show your patch.
> 

Inlined:


>From 12343c0918853f326b042c6e285b0e184565564f Mon Sep 17 00:00:00 2001
Message-Id:
<12343c0918853f326b042c6e285b0e184565564f.1256223642.git.ext-atal.shargorodsky@nokia.com>
From: Atal Shargorodsky <ext-atal.shargorodsky@nokia.com>
Date: Fri, 2 Oct 2009 17:34:56 +0300
Subject: [PATCH 1/3] debug: introduction of panic info buffer module

The feature of having an option for application to add some data
to be printed at panic could be useful for debugging systems where
the hardware, the kernel, the firmware for supplementary chips, etc.
are under constant development and kernel version does not identify
the exact setup in which the panic occurred.

The way to supply the string is to write it to debugfs file named
panic_info_buff. Currently the buffer length is limited to 1KB - 1B.

Signed-off-by: Atal Shargorodsky <ext-atal.shargorodsky@nokia.com>
---
 drivers/misc/panic_info_buff.c |   92
++++++++++++++++++++++++++++++++++++++++
 1 files changed, 92 insertions(+), 0 deletions(-)
 create mode 100644 drivers/misc/panic_info_buff.c

diff --git a/drivers/misc/panic_info_buff.c
b/drivers/misc/panic_info_buff.c
new file mode 100644
index 0000000..fa6d2b1
--- /dev/null
+++ b/drivers/misc/panic_info_buff.c
@@ -0,0 +1,92 @@
+/*
+ * Copyright (C) Nokia Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
USA
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/uaccess.h>
+#include <linux/debugfs.h>
+#include <linux/notifier.h>
+
+#define PANIC_BUFFER_MAX_LEN  1024
+static char panic_info_buff[PANIC_BUFFER_MAX_LEN];
+static struct dentry *panic_info_buff_debugfs;
+
+static int panic_info_buff_open(struct inode *inode, struct file *file)
+{
+	return 0;
+}
+
+static ssize_t panic_info_buff_write(struct file *file,
+		const char __user *buf, size_t len, loff_t *off)
+{
+	if (len >= PANIC_BUFFER_MAX_LEN)
+		return -EINVAL;
+	if (copy_from_user(panic_info_buff, buf, len))
+		return -EFAULT;
+	panic_info_buff[len] = '\0';
+	return len;
+}
+
+static struct file_operations panic_info_buff_fops = {
+	.open   = panic_info_buff_open,
+	.write  = panic_info_buff_write,
+	.llseek = no_llseek,
+	.owner  = THIS_MODULE,
+};
+
+static int panic_info_buff_event(struct notifier_block *this,
+	unsigned long event, void *ptr)
+{
+	if (panic_info_buff[0] == '\0')
+		printk(KERN_EMERG "Panic info buffer is empty.\n");
+	else
+		printk(KERN_EMERG "Panic info buffer:\n"
+				"%s\nBuffer total length: %d characters.\n",
+				panic_info_buff,
+				strlen(panic_info_buff));
+	return NOTIFY_OK;
+}
+
+static struct notifier_block panic_info_buff_block = {
+	.notifier_call  = panic_info_buff_event,
+	.priority       = 1,
+};
+
+static int __devinit panic_info_buff_init(void)
+{
+	panic_info_buff_debugfs = debugfs_create_file("panic_info_buff",
+		S_IFREG | S_IWUSR | S_IWGRP,
+		NULL, NULL, &panic_info_buff_fops);
+	atomic_notifier_chain_register(&panic_notifier_list,
+		&panic_info_buff_block);
+	return 0;
+}
+module_init(panic_info_buff_init);
+
+static void __devexit panic_info_buff_exit(void)
+{
+	debugfs_remove(panic_info_buff_debugfs);
+	atomic_notifier_chain_unregister(&panic_notifier_list,
+		&panic_info_buff_block);
+
+}
+module_exit(panic_info_buff_exit);
+
+MODULE_AUTHOR("Nokia Corporation");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("panic_info_buff");
-- 
1.5.4.3

Re: [PATCH v11 4/5] core: Add kernel message dumper to call on oopses and panics

[Simon Kagstrom]

On Fri, 23 Oct 2009 18:53:22 +0300
"Shargorodsky Atal (EXT-Teleca/Helsinki)" <ext-atal.shargorodsky@nokia.com> wrote:

> 1. If somebody writes a module that uses dumper for uploading the
> oopses/panics logs via some pay-per-byte medium,  since he has no way
> to know in a module if the panic_on_oops flag is set, he'll have
> to upload both oops and the following panic, because he does not
> know for sure that the panic was caused by the oops. Hence he
> pays twice for the same information, right?
> 
> I can think of a couple of way to figure it out in the module
> itself, but I could not think of any clean way to do it.

This is correct, and the mtdoops driver has some provisions to handle
this. First, there is a parameter to the module to specify whether
oopses should be dumped at all - I added this for the particular case
that someone has panic_on_oops set.

Second, it does not dump oopses directly anyway, but puts it in a work
queue. That way, if panic_on_oops is set, it will store the panic but
the oops (called from the workqueue) will not get written anyway.

> 2. We tried to use panic notifiers mechanism to print additional
> information that we want to see in mtdoops logs and it worked well,
> but having the kmsg_dump(KMSG_DUMP_PANIC) before the
> atomic_notifier_call_chain() breaks this functionality.
> Can we the call kmsg_dump() after the notifiers had been invoked?

Well, it depends I think. The code currently looks like this:

	kmsg_dump(KMSG_DUMP_PANIC);
	/*
	 * If we have crashed and we have a crash kernel loaded let it handle
	 * everything else.
	 * Do we want to call this before we try to display a message?
	 */
	crash_kexec(NULL);
	[... Comments removed]
	atomic_notifier_call_chain(&panic_notifier_list, 0, buf);

And moving kdump_msg() after crash_kexec() will make us miss the
message if we have a kexec crash kernel as well. I realise that these
two approaches might be complementary and are not likely to be used at
the same time, but it's still something to think about.

Then again, maybe it's possible to move the panic notifiers above
crash_kexec() as well, which would solve things nicely.

// Simon

Re: [PATCH v11 4/5] core: Add kernel message dumper to call on oopses and panics

[Shargorodsky Atal (EXT-Teleca/Helsinki)]

On Mon, 2009-10-26 at 08:41 +0100, ext Simon Kagstrom wrote:
> On Fri, 23 Oct 2009 18:53:22 +0300
> "Shargorodsky Atal (EXT-Teleca/Helsinki)" <ext-atal.shargorodsky@nokia.com> wrote:
> 
> > 1. If somebody writes a module that uses dumper for uploading the
> > oopses/panics logs via some pay-per-byte medium,  since he has no way
> > to know in a module if the panic_on_oops flag is set, he'll have
> > to upload both oops and the following panic, because he does not
> > know for sure that the panic was caused by the oops. Hence he
> > pays twice for the same information, right?
> > 
> > I can think of a couple of way to figure it out in the module
> > itself, but I could not think of any clean way to do it.
> 
> This is correct, and the mtdoops driver has some provisions to handle
> this. First, there is a parameter to the module to specify whether
> oopses should be dumped at all - I added this for the particular case
> that someone has panic_on_oops set.
> 

It takes care of most of the situations, but panic_on_oops
can be changed any time, even after the module is loaded.

While I think that exporting oops_on_panic is a wrong thing to do,
I believe that dumpers differ a bit from the rest of the modules in
that aspect and should be at least hinted about this flag setting.
Does it not make sense?

> Second, it does not dump oopses directly anyway, but puts it in a work
> queue. That way, if panic_on_oops is set, it will store the panic but
> the oops (called from the workqueue) will not get written anyway.
> 

AFAIK, mtdoops does not put oopses in a work queue. And if by any chance
it does, then I think it's wrong and might lead to missed oopses, as
the oops might be because of the work queues themselves, or it might
look to the kernel like some non-fatal fault, but actually it's a
sign of a much more catastrophic failure - IOMMU device garbaging
memory, for instance.

But anyway, I was not talking about mtdoops. In fact, I was not
talking about any particular module, I just described some situation
which looks a bit problematic to me.

> > 2. We tried to use panic notifiers mechanism to print additional
> > information that we want to see in mtdoops logs and it worked well,
> > but having the kmsg_dump(KMSG_DUMP_PANIC) before the
> > atomic_notifier_call_chain() breaks this functionality.
> > Can we the call kmsg_dump() after the notifiers had been invoked?
> 
> Well, it depends I think. The code currently looks like this:
> 
> 	kmsg_dump(KMSG_DUMP_PANIC);
> 	/*
> 	 * If we have crashed and we have a crash kernel loaded let it handle
> 	 * everything else.
> 	 * Do we want to call this before we try to display a message?
> 	 */
> 	crash_kexec(NULL);
> 	[... Comments removed]
> 	atomic_notifier_call_chain(&panic_notifier_list, 0, buf);
> 
> And moving kdump_msg() after crash_kexec() will make us miss the
> message if we have a kexec crash kernel as well. I realise that these
> two approaches might be complementary and are not likely to be used at
> the same time, but it's still something to think about.
> 
> Then again, maybe it's possible to move the panic notifiers above
> crash_kexec() as well, which would solve things nicely.
> 

Which leaves me no choice but just ask the question, as it bothering me
for some time: does anybody know why we try to crash_kexec() at so early
stage?


> // Simon

Re: [PATCH v11 4/5] core: Add kernel message dumper to call on oopses and panics

[Simon Kagstrom]

On Mon, 26 Oct 2009 12:36:33 +0200
"Shargorodsky Atal (EXT-Teleca/Helsinki)" <ext-atal.shargorodsky@nokia.com> wrote:

> > > I can think of a couple of way to figure it out in the module
> > > itself, but I could not think of any clean way to do it.
> > 
> > This is correct, and the mtdoops driver has some provisions to handle
> > this. First, there is a parameter to the module to specify whether
> > oopses should be dumped at all - I added this for the particular case
> > that someone has panic_on_oops set.
> 
> It takes care of most of the situations, but panic_on_oops
> can be changed any time, even after the module is loaded.

Yes, but this parameter is settable at runtime as well by writing
to /sys/module/mtdoops/parameters/dump_oops.

> > Second, it does not dump oopses directly anyway, but puts it in a work
> > queue. That way, if panic_on_oops is set, it will store the panic but
> > the oops (called from the workqueue) will not get written anyway.
> > 
> 
> AFAIK, mtdoops does not put oopses in a work queue. And if by any chance
> it does, then I think it's wrong and might lead to missed oopses, as
> the oops might be because of the work queues themselves, or it might
> look to the kernel like some non-fatal fault, but actually it's a
> sign of a much more catastrophic failure - IOMMU device garbaging
> memory, for instance.

I was referring to my patches to it, sorry. It's in the patch "[PATCH v7 5/5]:
mtdoops: refactor as a kmsg_dumper" (as well as the parameter to dump
oopses at all).

There are other situations which will make dumping problematic as well,
e.g., crashes in the mtd code, so there are certainly some cases which
will be difficult to catch. But in the panic_on_oops case or
oops-in-interrupt, the oops won't be missed and won't be outputted
twice for mtdoops.


Anyway, I understand your problem and agree that it would be good to
fix. Moving up crash_kexec() and the notifiers will at least fix your
second issue. For the double-output-of-oopses, I don't see a good way
to fix it unless relying on the module to correct it like above.

// Simon

Re: [PATCH v11 4/5] core: Add kernel message dumper to call on oopses and panics

[Shargorodsky Atal (EXT-Teleca/Helsinki)]

On Mon, 2009-10-26 at 12:53 +0100, ext Simon Kagstrom wrote:
> On Mon, 26 Oct 2009 12:36:33 +0200
> "Shargorodsky Atal (EXT-Teleca/Helsinki)" <ext-atal.shargorodsky@nokia.com> wrote:
> 
> > > > I can think of a couple of way to figure it out in the module
> > > > itself, but I could not think of any clean way to do it.
> > > 
> > > This is correct, and the mtdoops driver has some provisions to handle
> > > this. First, there is a parameter to the module to specify whether
> > > oopses should be dumped at all - I added this for the particular case
> > > that someone has panic_on_oops set.
> > 
> > It takes care of most of the situations, but panic_on_oops
> > can be changed any time, even after the module is loaded.
> 
> Yes, but this parameter is settable at runtime as well by writing
> to /sys/module/mtdoops/parameters/dump_oops.
> 
> > > Second, it does not dump oopses directly anyway, but puts it in a work
> > > queue. That way, if panic_on_oops is set, it will store the panic but
> > > the oops (called from the workqueue) will not get written anyway.
> > > 
> > 
> > AFAIK, mtdoops does not put oopses in a work queue. And if by any chance
> > it does, then I think it's wrong and might lead to missed oopses, as
> > the oops might be because of the work queues themselves, or it might
> > look to the kernel like some non-fatal fault, but actually it's a
> > sign of a much more catastrophic failure - IOMMU device garbaging
> > memory, for instance.
> 
> I was referring to my patches to it, sorry. It's in the patch "[PATCH v7 5/5]:
> mtdoops: refactor as a kmsg_dumper" (as well as the parameter to dump
> oopses at all).
> 
> There are other situations which will make dumping problematic as well,
> e.g., crashes in the mtd code, so there are certainly some cases which
> will be difficult to catch. But in the panic_on_oops case or
> oops-in-interrupt, the oops won't be missed and won't be outputted
> twice for mtdoops.
> 
> 
> Anyway, I understand your problem and agree that it would be good to
> fix. Moving up crash_kexec() and the notifiers will at least fix your
> second issue. For the double-output-of-oopses, I don't see a good way
> to fix it unless relying on the module to correct it like above.
> 

How about adding KMSG_DUMP_LAST_OOPS_BEFORE_PANIC or something to the
kmsg_dump_reason enum, and making the kmsg_dump look like
kmsg_dump(panic_on_oops ? KMSG_DUMP_LAST_OOPS_BEFORE_PANIC : KMSG_DUMP_OOPS);
in oops_exit. Then let the dumpers decide what they want to do about it.
Just a thought.

And since you have no objections about moving notifiers up, it looks
like the second issue will be resolved, I believe Artem
will take care of it. :)

Thanks a lot,
Atal.

> // Simon

Re: [PATCH v11 4/5] core: Add kernel message dumper to call on oopses and panics

[Simon Kagstrom]

On Mon, 26 Oct 2009 17:13:27 +0200
"Shargorodsky Atal (EXT-Teleca/Helsinki)" <ext-atal.shargorodsky@nokia.com> wrote:

> How about adding KMSG_DUMP_LAST_OOPS_BEFORE_PANIC or something to the
> kmsg_dump_reason enum, and making the kmsg_dump look like
> kmsg_dump(panic_on_oops ? KMSG_DUMP_LAST_OOPS_BEFORE_PANIC : KMSG_DUMP_OOPS);
> in oops_exit. Then let the dumpers decide what they want to do about it.
> Just a thought.

It would also have to take in_interrupt() in account since that will
also lead to a panic.

> And since you have no objections about moving notifiers up, it looks
> like the second issue will be resolved, I believe Artem
> will take care of it. :)

Well, I have no objections to it, but I also know very little of the
reasoning to put it there in the first place, so don't count my vote
for very much here :-)

// Simon

Re: [PATCH v10 4/5] core: Add kernel message dumper to call on oopses and panics

[Aaro Koskinen]

Hello,

Simon Kagstrom wrote:
> +#ifndef _LINUX_KMSG_DUMP_H
> +#define _LINUX_KMSG_DUMP_H
> +
> +#include <linux/list.h>
> +
> +enum kmsg_dump_reason {
> +	KMSG_DUMP_OOPS,
> +	KMSG_DUMP_PANIC,
> +};
> +
> +/**
> + * struct kmsg_dumper - kernel crash message dumper structure
> + * @dump:	The callback which gets called on crashes. The buffer is passed
> + * 		as two sections, where s1 (length l1) contains the older
> + * 		messages and s2 (length l2) contains the newer.
> + * @list:	Entry in the dumper list (private)
> + * @registered:	Flag that specifies if this is already registered
> + */
> +struct kmsg_dumper {
> +	void (*dump)(struct kmsg_dumper *dumper, enum kmsg_dump_reason reason,
> +			const char *s1, unsigned long l1,
> +			const char *s2, unsigned long l2);
> +	struct list_head list;
> +	int registered;
> +};
> +
> +void kmsg_dump(enum kmsg_dump_reason reason);
> +
> +int kmsg_dump_register(struct kmsg_dumper *dumper);
> +
> +int kmsg_dump_unregister(struct kmsg_dumper *dumper);
> +
> +#endif /* _LINUX_DUMP_DEVICE_H */

If you still make a new version of the patch, please correct the
"_LINUX_DUMP_DEVICE_H" comment.

A.