Skip to content

Commit

Permalink
kernel/panic: Drop unblank_screen call
Browse files Browse the repository at this point in the history
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 b61312d ("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 e3e8a75 ("[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 c7c3f05 ("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.

Cc: Greg Kroah-Hartman <[email protected]>
Cc: Jiri Slaby <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: "Ilpo Järvinen" <[email protected]>
Cc: Tetsuo Handa <[email protected]>
Cc: Xuezhi Zhang <[email protected]>
Cc: Yangxi Xiang <[email protected]>
Cc: nick black <[email protected]>
Cc: Petr Mladek <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Luis Chamberlain <[email protected]>
Cc: "Guilherme G. Piccoli" <[email protected]>
Cc: Marco Elver <[email protected]>
Cc: John Ogness <[email protected]>
Cc: Sebastian Andrzej Siewior <[email protected]>
Cc: David Gow <[email protected]>
Cc: tangmeng <[email protected]>
Cc: Tiezhu Yang <[email protected]>
Cc: Chris Wilson <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
Acked-by: Sebastian Andrzej Siewior <[email protected]>
Signed-off-by: Daniel Vetter <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Greg Kroah-Hartman <[email protected]>
  • Loading branch information
danvet authored and gregkh committed Sep 1, 2022
1 parent bf98ef6 commit 0a64ce6
Show file tree
Hide file tree
Showing 4 changed files with 2 additions and 8 deletions.
3 changes: 2 additions & 1 deletion drivers/tty/vt/vt.c
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -4448,7 +4449,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);
}
Expand Down
1 change: 0 additions & 1 deletion include/linux/vt_kern.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
3 changes: 0 additions & 3 deletions kernel/panic.c
Original file line number Diff line number Diff line change
Expand Up @@ -329,9 +329,6 @@ void panic(const char *fmt, ...)
if (_crash_kexec_post_notifiers)
__crash_kexec(NULL);

#ifdef CONFIG_VT
unblank_screen();
#endif
console_unblank();

/*
Expand Down
3 changes: 0 additions & 3 deletions lib/bust_spinlocks.c
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down

0 comments on commit 0a64ce6

Please sign in to comment.