Message-Id: <20220830145004.430545-1-daniel.vetter@ffwll.ch>
Date:   Tue, 30 Aug 2022 16:50:04 +0200
From:   Daniel Vetter <daniel.vetter@...ll.ch>
To:     LKML <linux-kernel@...r.kernel.org>
Cc:     DRI Development <dri-devel@...ts.freedesktop.org>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        Daniel Vetter <daniel.vetter@...el.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
        Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>,
        Xuezhi Zhang <zhangxuezhi1@...lpad.com>,
        Yangxi Xiang <xyangxi5@...il.com>,
        nick black <dankamongmen@...il.com>,
        Petr Mladek <pmladek@...e.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Luis Chamberlain <mcgrof@...nel.org>,
        "Guilherme G. Piccoli" <gpiccoli@...lia.com>,
        Marco Elver <elver@...gle.com>,
        John Ogness <john.ogness@...utronix.de>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        David Gow <davidgow@...gle.com>,
        tangmeng <tangmeng@...ontech.com>,
        Tiezhu Yang <yangtiezhu@...ngson.cn>,
        Chris Wilson <chris@...is-wilson.co.uk>
Subject: [PATCH] kernel/panic: Drop unblank_screen call

console_unblank() does this too (called in both places right after),
and with a lot more confidence inspiring approach to locking.

Reconstructing this story is very strange:

In b61312d353da ("oops handling: ensure that any oops is flushed to
the mtdoops console") it is claimed that a printk(" "); flushed out
the console buffer, which was removed in e3e8a75d2acf ("[PATCH]
Extract and use wake_up_klogd()"). In todays kernels this is done way
earlier in console_flush_on_panic with some really nasty tricks. I
didn't bother to fully reconstruct this all, least because the call to
bust_spinlock(0); gets moved every few years, depending upon how the
wind blows (or well, who screamed loudest about the various issue each
call site caused).

Before that commit the only calls to console_unblank() where in s390
arch code.

The other side here is the console->unblank callback, which was
introduced in 2.1.31 for the vt driver. Which predates the
console_unblank() function by a lot, which was added (without users)
in 2.4.14.3. So pretty much impossible to guess at any motivation
here. Also afaict the vt driver is the only (and always was the only)
console driver implementing the unblank callback, so no idea why a
call to console_unblank() was added for the mtdooops driver - the
action actually flushing out the console buffers is done from
console_unlock() only.

Note that as prep for the s390 users the locking was adjusted in
2.5.22 (I couldn't figure out how to properly reference the BK commit
from the historical git trees) from a normal semaphore to a trylock.

Note that a copy of the direct unblank_screen() call was added to
panic() in c7c3f05e341a ("panic: avoid deadlocks in re-entrant console
drivers"), which partially inlined the bust_spinlocks(0); call.

Long story short, I have no idea why the direct call to unblank_screen
survived for so long (the infrastructure to do it properly existed for
years), nor why it wasn't removed when the console_unblank() call was
finally added. But it makes a ton more sense to finally do that than
not - it's just better encapsulation to go through the console
functions instead of doing a direct call, so let's dare. Plus it
really does not make much sense to call the only unblank
implementation there is twice, once without, and once with appropriate
locking.

Signed-off-by: Daniel Vetter <daniel.vetter@...el.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Jiri Slaby <jirislaby@...nel.org>
Cc: Daniel Vetter <daniel.vetter@...ll.ch>
Cc: "Ilpo Järvinen" <ilpo.jarvinen@...ux.intel.com>
Cc: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
Cc: Xuezhi Zhang <zhangxuezhi1@...lpad.com>
Cc: Yangxi Xiang <xyangxi5@...il.com>
Cc: nick black <dankamongmen@...il.com>
Cc: Petr Mladek <pmladek@...e.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>
Cc: Luis Chamberlain <mcgrof@...nel.org>
Cc: "Guilherme G. Piccoli" <gpiccoli@...lia.com>
Cc: Marco Elver <elver@...gle.com>
Cc: John Ogness <john.ogness@...utronix.de>
Cc: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc: David Gow <davidgow@...gle.com>
Cc: tangmeng <tangmeng@...ontech.com>
Cc: Tiezhu Yang <yangtiezhu@...ngson.cn>
Cc: Chris Wilson <chris@...is-wilson.co.uk>
---
 drivers/tty/vt/vt.c     | 3 ++-
 include/linux/vt_kern.h | 1 -
 kernel/panic.c          | 3 ---
 lib/bust_spinlocks.c    | 3 ---
 4 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index a6be32798fad..08498fcf080a 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -154,6 +154,7 @@ static void console_callback(struct work_struct *ignored);
 static void con_driver_unregister_callback(struct work_struct *ignored);
 static void blank_screen_t(struct timer_list *unused);
 static void set_palette(struct vc_data *vc);
+static void unblank_screen(void);
 
 #define vt_get_kmsg_redirect() vt_kmsg_redirect(-1)
 
@@ -4450,7 +4451,7 @@ EXPORT_SYMBOL(do_unblank_screen);
  * call it with 1 as an argument and so force a mode restore... that may kill
  * X or at least garbage the screen but would also make the Oops visible...
  */
-void unblank_screen(void)
+static void unblank_screen(void)
 {
 	do_unblank_screen(0);
 }
diff --git a/include/linux/vt_kern.h b/include/linux/vt_kern.h
index b5ab452fca5b..c1f5aebef170 100644
--- a/include/linux/vt_kern.h
+++ b/include/linux/vt_kern.h
@@ -30,7 +30,6 @@ struct vc_data *vc_deallocate(unsigned int console);
 void reset_palette(struct vc_data *vc);
 void do_blank_screen(int entering_gfx);
 void do_unblank_screen(int leaving_gfx);
-void unblank_screen(void);
 void poke_blanked_console(void);
 int con_font_op(struct vc_data *vc, struct console_font_op *op);
 int con_set_cmap(unsigned char __user *cmap);
diff --git a/kernel/panic.c b/kernel/panic.c
index 1fc3d98417d1..74a7b55851c4 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -330,9 +330,6 @@ void panic(const char *fmt, ...)
 	if (_crash_kexec_post_notifiers)
 		__crash_kexec(NULL);
 
-#ifdef CONFIG_VT
-	unblank_screen();
-#endif
 	console_unblank();
 
 	/*
diff --git a/lib/bust_spinlocks.c b/lib/bust_spinlocks.c
index 8be59f84eaea..bfd53972a4d8 100644
--- a/lib/bust_spinlocks.c
+++ b/lib/bust_spinlocks.c
@@ -22,9 +22,6 @@ void bust_spinlocks(int yes)
 	if (yes) {
 		++oops_in_progress;
 	} else {
-#ifdef CONFIG_VT
-		unblank_screen();
-#endif
 		console_unblank();
 		if (--oops_in_progress == 0)
 			wake_up_klogd();
-- 
2.37.2

Powered by blists - more mailing lists