Always print panic message on current console

Always print panic message on current console

[Bernhard Walle]

The kernel offers with TIOCL_GETKMSGREDIRECT ioctl() the possibility to
redirect the kernel messages to a specific console.

However, since it's not possible to switch to the kernel message console after
a panic(), it would be nice if the kernel would print the panic message on the
current console.

As suggested by Andrew, this patch series adds a new interface to access
the global kmsg_redirect variable by a function to be able to use it in
code where CONFIG_VT_CONSOLE is not set (kernel/panic.c).

The patch series makes kmsg_redirect a static variable and all kernel
code outside of vt.c accesses that variable by the vt_set_kmsg_redirect()
and vt_get_kmsg_redirect() functions.


Signed-off-by: Bernhard Walle <bernhard@bwalle.de>

---
 drivers/char/vt.c      |   41 ++++++++++++++++++++++++++++++++++++++++-
 include/linux/tty.h    |    2 --
 include/linux/vt.h     |   18 ++++++++++++++++++
 kernel/panic.c         |    4 ++++
 kernel/power/console.c |    8 ++++----
 5 files changed, 66 insertions(+), 7 deletions(-)

[PATCH 1/5] Only define/declare kmsg_redirect when CONFIG_VT_CONSOLE is set

[Bernhard Walle]

Since the variable is only used when CONFIG_VT_CONSOLE is set, we can save some
bytes of memory when CONFIG_VT_CONSOLE is not set.


Signed-off-by: Bernhard Walle <bernhard@bwalle.de>
---
 drivers/char/vt.c   |    2 ++
 include/linux/tty.h |    2 ++
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/char/vt.c b/drivers/char/vt.c
index 0c80c68..c3b1a86 100644
--- a/drivers/char/vt.c
+++ b/drivers/char/vt.c
@@ -187,7 +187,9 @@ static DECLARE_WORK(console_work, console_callback);
 int fg_console;
 int last_console;
 int want_console = -1;
+#ifdef CONFIG_VT_CONSOLE
 int kmsg_redirect;
+#endif
 
 /*
  * For each existing display, we have a pointer to console currently visible
diff --git a/include/linux/tty.h b/include/linux/tty.h
index f0f43d0..91bb1df 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -340,7 +340,9 @@ extern void tty_write_flush(struct tty_struct *);
 
 extern struct ktermios tty_std_termios;
 
+#ifdef CONFIG_VT_CONSOLE
 extern int kmsg_redirect;
+#endif
 
 extern void console_init(void);
 extern int vcs_init(void);
-- 
1.6.3.3

Re: [PATCH 1/5] Only define/declare kmsg_redirect when CONFIG_VT_CONSOLE is set

[Alan Cox]

On Sat, 10 Oct 2009 22:51:06 +0200
Bernhard Walle <bernhard@bwalle.de> wrote:

> Since the variable is only used when CONFIG_VT_CONSOLE is set, we can save some
> bytes of memory when CONFIG_VT_CONSOLE is not set.

Its four bytes.. its not worth the ifdef mess

[PATCH 2/5] Add setter/getter interface for kmsg_redirect

[Bernhard Walle]

To be able to use kmsg_redirect in code when CONFIG_VT_CONSOLE is not set and to avoid
code like

        #ifdef CONFIG_VT_CONSOLE
                kmsg_redirect = foo;
        #endif

we add a function pair vt_get_kmsg_redirect()/vt_set_kmsg_redirect() that just
returns or sets that variable, but is defined to do nothing in case CONFIG_VT_CONSOLE
is not set.


Signed-off-by: Bernhard Walle <bernhard@bwalle.de>
---
 drivers/char/vt.c  |   37 +++++++++++++++++++++++++++++++++++++
 include/linux/vt.h |   18 ++++++++++++++++++
 2 files changed, 55 insertions(+), 0 deletions(-)

diff --git a/drivers/char/vt.c b/drivers/char/vt.c
index c3b1a86..01c4a1c 100644
--- a/drivers/char/vt.c
+++ b/drivers/char/vt.c
@@ -2428,6 +2428,43 @@ struct tty_driver *console_driver;
 
 #ifdef CONFIG_VT_CONSOLE
 
+/**
+ * vt_get_kmsg_redirect() - Returns the virtual console number for kernel
+ *                          messages
+ *
+ * By default, the kernel messages are always printed on the current virtual
+ * console. However, the user may modify that default with the
+ * TIOCL_SETKMSGREDIRECT ioctl call.
+ *
+ * This funciton returns the virtual console number where kernel messages are
+ * redirected to. 0 means no redirection (i.e. always printed on the currently
+ * active console).
+ *
+ * When the kernel is compiled without CONFIG_VT_CONSOLE, this function also
+ * returns 0.
+ */
+int vt_get_kmsg_redirect(void)
+{
+	return kmsg_redirect;
+}
+
+/**
+ * vt_set_kmsg_redirect() - Sets the virtual console number for kernel messages
+ * @vt:		The number of the virtual terminal.
+ *
+ * See also: vt_get_kmsg_redirect().
+ *
+ * This function allows to modify the virtual console for kernel messages from
+ * kernel code. Calling vt_set_kmsg_redirect(0) restores the default.
+ *
+ * When the kernel is compiled without CONFIG_VT_CONSOLE, this function does
+ * nothing.
+ */
+void vt_set_kmsg_redirect(int vt)
+{
+	kmsg_redirect = vt;
+}
+
 /*
  *	Console on virtual terminal
  *
diff --git a/include/linux/vt.h b/include/linux/vt.h
index 7afca0d..fa87e2a 100644
--- a/include/linux/vt.h
+++ b/include/linux/vt.h
@@ -84,4 +84,22 @@ struct vt_setactivate {
 
 #define VT_SETACTIVATE	0x560F	/* Activate and set the mode of a console */
 
+#ifdef CONFIG_VT_CONSOLE
+
+extern void vt_set_kmsg_redirect(int vt);
+extern int  vt_get_kmsg_redirect(void);
+
+#else /* CONFIG_VT_CONSOLE */
+
+static inline void vt_set_kmsg_redirect(int vt)
+{
+}
+
+static inline int vt_get_kmsg_redirect(void)
+{
+    return 0;
+}
+
+#endif /* CONFIG_VT_CONSOLE */
+
 #endif /* _LINUX_VT_H */
-- 
1.6.3.3

Re: [PATCH 2/5] Add setter/getter interface for kmsg_redirect

[Alan Cox]

> +int vt_get_kmsg_redirect(void)
> +{
> +	return kmsg_redirect;
> +}

Explain the locking rules

> +#ifdef CONFIG_VT_CONSOLE
> +
> +extern void vt_set_kmsg_redirect(int vt);
> +extern int  vt_get_kmsg_redirect(void);

Just make them extern inline, its a one liner.

[PATCH 3/5] Use vt_get_kmsg_redirect() and vt_set_kmsg_redirect()

[Bernhard Walle]

Although the only code file where kmsg_redirect is used (power/console.c) is
already protected by CONFIG_VT_CONSOLE, we use the new interface here
to be able to make kmsg_redirect static.


Signed-off-by: Bernhard Walle <bernhard@bwalle.de>
---
 kernel/power/console.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/power/console.c b/kernel/power/console.c
index 5187136..df959d7 100644
--- a/kernel/power/console.c
+++ b/kernel/power/console.c
@@ -6,7 +6,7 @@
 
 #include <linux/vt_kern.h>
 #include <linux/kbd_kern.h>
-#include <linux/console.h>
+#include <linux/vt.h>
 #include <linux/module.h>
 #include "power.h"
 
@@ -21,8 +21,8 @@ int pm_prepare_console(void)
 	if (orig_fgconsole < 0)
 		return 1;
 
-	orig_kmsg = kmsg_redirect;
-	kmsg_redirect = SUSPEND_CONSOLE;
+	orig_kmsg = vt_get_kmsg_redirect();
+	vt_set_kmsg_redirect(SUSPEND_CONSOLE);
 	return 0;
 }
 
@@ -30,7 +30,7 @@ void pm_restore_console(void)
 {
 	if (orig_fgconsole >= 0) {
 		vt_move_to_console(orig_fgconsole, 0);
-		kmsg_redirect = orig_kmsg;
+		vt_set_kmsg_redirect(orig_kmsg);
 	}
 }
 #endif
-- 
1.6.3.3

Re: [PATCH 3/5] Use vt_get_kmsg_redirect() and vt_set_kmsg_redirect()

[Alan Cox]

> -	kmsg_redirect = SUSPEND_CONSOLE;
> +	orig_kmsg = vt_get_kmsg_redirect();
> +	vt_set_kmsg_redirect(SUSPEND_CONSOLE);

Probably better to have a single

	orig_kmsg = vt_kmsg_redirect(new);

because it makes the locking easy and means a third party can't sneak
between those two and break stuff. This has another advantage - the
variable is now function local and the whole pile of stuff you posted
becomes

#if defined(CONFIG_VT_CONSOLE)

extern inline int vt_kmsg_redirect(int new)
{
	static int kmsg_con;

	if (new != -1)
		xchg(new, &kmsg_con);
	else
		new = kmsg_con;
	return new;
}

#else

extern inline int vt_kmsg_redirect(int new)
{
	return 0;
}

#endif


Optionally adding

#define vt_get_kmsg_redirect vt_kmsg_redirect(-1);


One ifdef, clear locking rules, no globals, no file level statics and for
almost all cases it'll compile down to almost nothing.

Alan

Re: [PATCH 3/5] Use vt_get_kmsg_redirect() and vt_set_kmsg_redirect()

[Alan Cox]

> variable is now function local and the whole pile of stuff you posted
> becomes
> 
> #if defined(CONFIG_VT_CONSOLE)

Umm testing it this seems gcc/ld isn't quite smart enough to get that
right so the function can't be inlined - the rest holds though, it can be
hidden in function just not in the header file.

[PATCH 4/5] Make kmsg_redirect static

[Bernhard Walle]

Now we have vt_get_kmsg_redirect()/vt_set_kmsg_redirect(), so we can make the
variable local to the compilcation unit.


Signed-off-by: Bernhard Walle <bernhard@bwalle.de>
---
 drivers/char/vt.c   |    2 +-
 include/linux/tty.h |    4 ----
 2 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/drivers/char/vt.c b/drivers/char/vt.c
index 01c4a1c..ad1e7af 100644
--- a/drivers/char/vt.c
+++ b/drivers/char/vt.c
@@ -188,7 +188,7 @@ int fg_console;
 int last_console;
 int want_console = -1;
 #ifdef CONFIG_VT_CONSOLE
-int kmsg_redirect;
+static int kmsg_redirect;
 #endif
 
 /*
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 91bb1df..fddafcb 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -340,10 +340,6 @@ extern void tty_write_flush(struct tty_struct *);
 
 extern struct ktermios tty_std_termios;
 
-#ifdef CONFIG_VT_CONSOLE
-extern int kmsg_redirect;
-#endif
-
 extern void console_init(void);
 extern int vcs_init(void);
 
-- 
1.6.3.3

[PATCH 5/5] Always print panic message on current console

[Bernhard Walle]

The kernel offers with TIOCL_GETKMSGREDIRECT ioctl() the possibility to
redirect the kernel messages to a specific console.

However, since it's not possible to switch to the kernel message console after
a panic(), it would be nice if the kernel would print the panic message on the
current console.


Signed-off-by: Bernhard Walle <bernhard@bwalle.de>
---
 kernel/panic.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index 96b45d0..a230459 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -22,6 +22,7 @@
 #include <linux/init.h>
 #include <linux/nmi.h>
 #include <linux/dmi.h>
+#include <linux/vt.h>
 
 int panic_on_oops;
 static unsigned long tainted_mask;
@@ -65,6 +66,9 @@ NORET_TYPE void panic(const char * fmt, ...)
 	 */
 	preempt_disable();
 
+	/* don't redirect the panic message to some hidden console */
+	vt_set_kmsg_redirect(0);
+
 	bust_spinlocks(1);
 	va_start(args, fmt);
 	vsnprintf(buf, sizeof(buf), fmt, args);
-- 
1.6.3.3

Re: [PATCH 5/5] Always print panic message on current console

[Andi Kleen]

Bernhard Walle <bernhard@bwalle.de> writes:

> The kernel offers with TIOCL_GETKMSGREDIRECT ioctl() the possibility to
> redirect the kernel messages to a specific console.
>
> However, since it's not possible to switch to the kernel message console after
> a panic(), it would be nice if the kernel would print the panic message on the
> current console.

The basic idea is good, but you really need to call this in a lot
more places, which print some addition needed information before the panic.
The one liner panic is often not enough to decide what went wrong.
Example are machine checks, oopses, probably more.

In general for the recent work to switch the video mode on panic
we need a generalized hook for this anyways, so you could use
some generalized function.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH 5/5] Always print panic message on current console

[Bernhard Walle]

Andi Kleen schrieb:
> Bernhard Walle <bernhard@bwalle.de> writes:
> 
>> The kernel offers with TIOCL_GETKMSGREDIRECT ioctl() the possibility to
>> redirect the kernel messages to a specific console.
>>
>> However, since it's not possible to switch to the kernel message console after
>> a panic(), it would be nice if the kernel would print the panic message on the
>> current console.
> 
> The basic idea is good, but you really need to call this in a lot
> more places, which print some addition needed information before the panic.
> The one liner panic is often not enough to decide what went wrong.
> Example are machine checks, oopses, probably more.
> 
> In general for the recent work to switch the video mode on panic
> we need a generalized hook for this anyways, so you could use
> some generalized function.

Is that video switch called in a panic notifier list or does that
introduce a new hook?

One problem is that kexec is executed before the panic notifier. Which
is okay for printk(), but it would be very good if the video switch
would occur before kexec.



Regards,
Bernhard

Re: [PATCH 5/5] Always print panic message on current console

[Jesse Barnes]

On Mon, 12 Oct 2009 20:24:56 +0200
Bernhard Walle <bernhard@bwalle.de> wrote:

> Andi Kleen schrieb:
> > Bernhard Walle <bernhard@bwalle.de> writes:
> > 
> >> The kernel offers with TIOCL_GETKMSGREDIRECT ioctl() the
> >> possibility to redirect the kernel messages to a specific console.
> >>
> >> However, since it's not possible to switch to the kernel message
> >> console after a panic(), it would be nice if the kernel would
> >> print the panic message on the current console.
> > 
> > The basic idea is good, but you really need to call this in a lot
> > more places, which print some addition needed information before
> > the panic. The one liner panic is often not enough to decide what
> > went wrong. Example are machine checks, oopses, probably more.
> > 
> > In general for the recent work to switch the video mode on panic
> > we need a generalized hook for this anyways, so you could use
> > some generalized function.
> 
> Is that video switch called in a panic notifier list or does that
> introduce a new hook?
> 
> One problem is that kexec is executed before the panic notifier. Which
> is okay for printk(), but it would be very good if the video switch
> would occur before kexec.

It's called in the panic notifier atm.  Yeah sounds like kexec vs
notifiers should be reordered a bit (or the current panic notifier
should be split into notify and "yes we actually failed to do anything"
cases).

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 5/5] Always print panic message on current console

[Bernhard Walle]

Jesse Barnes schrieb:
> 
> It's called in the panic notifier atm.  Yeah sounds like kexec vs
> notifiers should be reordered a bit (or the current panic notifier
> should be split into notify and "yes we actually failed to do anything"
> cases).

Well, we had that discussion several times. But maybe the opinion of
some developers changed because now there would be an in-kernel user of
notification before kexec.


Regards,
Bernhard

Re: [PATCH 5/5] Always print panic message on current console

[Jason Wessel]

Bernhard Walle wrote:
> Jesse Barnes schrieb:
>   
>> It's called in the panic notifier atm.  Yeah sounds like kexec vs
>> notifiers should be reordered a bit (or the current panic notifier
>> should be split into notify and "yes we actually failed to do anything"
>> cases).
>>     
>
> Well, we had that discussion several times. But maybe the opinion of
> some developers changed because now there would be an in-kernel user of
> notification before kexec.
>   

The other internal kernel user would be kdb (granted it is not merged to
the mainline at this point).  The prototype that we demonstrated at LPC
hooked the panic notification first and passed the string that was
passed to the panic.

Where KMS is integrated with KDB all the switching is dynamic and you do
get a chance to see what happened as well as interact with the
debugger.  The argument of folks who only care about kexec has always
been they want the first right of anything to get to kexec before
anything could get worse by going down any other code paths.

The kms does not have the choice to dynamically configure the behavior
on panic, where as kdb/kgdb is something you activate if you plan on
debugging (meaning user preference is clear).  Perhaps two notifiers is
the right answer.

Jason.

Re: [PATCH 5/5] Always print panic message on current console

[Andi Kleen]

> It's called in the panic notifier atm.  Yeah sounds like kexec vs
> notifiers should be reordered a bit (or the current panic notifier
> should be split into notify and "yes we actually failed to do anything"
> cases).

I think it needs oops_begin() move over to a generalized panic_begin()
which is used everywhere there are multiple line panics.

oops_begin could do all these things, possibly with a new notifier.

This could also solve a couple of other open issues, like
currently we can have parallel panics deadlocking so doing proper
locking.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.