Hi Petr, Thanks for your review.
On Wed, Feb 20, 2019 at 02:43:44PM +0100, Petr Mladek wrote: > On Fri 2019-02-15 13:56:54, Feng Tang wrote: > > Hi all, > > > > On Tue, Dec 11, 2018 at 09:32:30AM +0100, Petr Mladek wrote: > > > On Mon 2018-12-10 10:49:22, Kees Cook wrote: > > > > On Mon, Dec 10, 2018 at 10:17 AM Steven Rostedt <[email protected]> > > > > wrote: > > > > > > > > > > On Fri, 7 Dec 2018 17:51:19 +0800 > > > > > Feng Tang <[email protected]> wrote: > > > > > > > > > > > When kernel panic happens, it will first print the panic call stack, > > > > > > then the ending msg like: > > > > > > > > > > > > [ 35.743249] ---[ end Kernel panic - not syncing: Fatal exception > > > > > > [ 35.749975] ------------[ cut here ]------------ > > > > > > > > > > > > The above message are very useful for debugging. > > > > > > > > > > > > But if system is configured to not reboot on panic, say the > > > > > > "panic_timeout" > > > > > > parameter equals 0, it will likely print out many noisy message like > > > > > > WARN() call stack for each and every CPU except the panic one, > > > > > > messages > > > > > > like below: > > > > So currently, there are 2 proposals: > > 1. this v4 patch of "panic_keep_irq_on" flag (default off to be same > > as the current kernel behavior) > > 2. Petr's suggestion of adding a flag to suppress printk after enterring > > late panic phase (blinking time), while keeping the sysrq printk > > working. > > > > Following is the draft patch based on Petr's suggestion: > > > > Please review, thanks. I'm fine with both solutions. > > > > - Feng > > > > diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c > > index 1f03078..8921fed 100644 > > --- a/drivers/tty/sysrq.c > > +++ b/drivers/tty/sysrq.c > > @@ -528,6 +528,11 @@ void __handle_sysrq(int key, bool check_mask) > > struct sysrq_key_op *op_p; > > int orig_log_level; > > int i; > > + int old_val; > > + > > + /* save the old panic printk flag */ > > The comment is not needed. It is obvious. ok, will remove. > > > + old_val = panic_suppress_printk; > > s/old_val/orig_panic_suppress_printk/ to follow > the naming of orig_log_level. Ok. > > > + panic_suppress_printk = 1; > > We want to enable the messages in sysrq. This should be: > > panic_suppress_printk = 0; Yes, will do. > > > rcu_sysrq_start(); > > rcu_read_lock(); > > @@ -574,6 +579,8 @@ void __handle_sysrq(int key, bool check_mask) > > } > > rcu_read_unlock(); > > rcu_sysrq_end(); > > + > > + panic_suppress_printk = old_val; > > } > > > > void handle_sysrq(int key) > > diff --git a/kernel/panic.c b/kernel/panic.c > > index f121e6b..0cd3a1b 100644 > > --- a/kernel/panic.c > > +++ b/kernel/panic.c > > @@ -326,6 +328,7 @@ void panic(const char *fmt, ...) > > } > > #endif > > pr_emerg("---[ end Kernel panic - not syncing: %s ]---\n", buf); > > /* Do not scroll important messages with errors from blinking code. */ Will add the comment, with one minor chanage that the noisy messages may also come from other places. > > > + panic_suppress_printk = 1; > > local_irq_enable(); > > for (i = 0; ; i += PANIC_TIMER_STEP) { > > touch_softlockup_watchdog(); > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > > index d3d1703..c27bbf5 100644 > > --- a/kernel/printk/printk.c > > +++ b/kernel/printk/printk.c > > @@ -1987,6 +1987,9 @@ asmlinkage __visible int printk(const char *fmt, ...) > > va_list args; > > int r; > > /* Suppress messages from panic blinking code. */ Ditto. > > + if (unlikely(panic_suppress_printk)) > > + return 0; > > This should go to vprintk_emit() so that it works for all > printk() interfaces. Ok, will do. Thanks, Feng > > > + > > va_start(args, fmt); > > r = vprintk_func(fmt, args); > > va_end(args); > > Best Regards, > Petr
