Re: [PATCH printk v2 23/26] panic: Mark emergency section in warn

[Petr Mladek]

On Sun 2024-02-18 20:03:23, John Ogness wrote:
> From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> 
> Mark the full contents of __warn() as an emergency section. In
> this section, the CPU will not perform console output for the
> printk() calls. Instead, a flushing of the console output is
> triggered when exiting the emergency section.
> 
> Co-developed-by: John Ogness <john.ogness@xxxxxxxxxxxxx>
> Signed-off-by: John Ogness <john.ogness@xxxxxxxxxxxxx>
> Signed-off-by: Thomas Gleixner (Intel) <tglx@xxxxxxxxxxxxx>

It will change the behavior immediately even before having
any nbcon console. But it makes sense to be consistent.
Let's try it ;-)

Reviewed-by: Petr Mladek <pmladek@xxxxxxxx>

Some comments below just to document my thoughts.

> ---
>  kernel/panic.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/kernel/panic.c b/kernel/panic.c
> index 86813305510f..d30d261f9246 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -667,6 +667,8 @@ struct warn_args {
>  void __warn(const char *file, int line, void *caller, unsigned taint,
>  	    struct pt_regs *regs, struct warn_args *args)
>  {
> +	nbcon_cpu_emergency_enter();

This will disable preemtion. But it should not cause any big problems.
The messages should be stored quickly when the consoles are not called.

>  	disable_trace_on_warning();
>  
>  	if (file)
> @@ -697,6 +699,8 @@ void __warn(const char *file, int line, void *caller, unsigned taint,

There is called check_panic_on_warn() in this context. It might call
panic(). I first thought that printk() would still defer the consoles.
But it actually won't because vprintk_emit() checks NBCON_PRIO_EMERGENCY.

All is good in the end. I just feel that the rules, when vprintk_emit()
flushes the consoles, are pretty complicated. I am a bit nervous that
anyone could break it in the future. Well, I can't think of any
approach which would make it more regression-proof.

The only way might be to do not be so perfect and simplify the logic.
Well, the current logic makes perfect sense. Let's try it.
Maybe, I am just tired today ;-)

>  
>  	/* Just a warning, don't kill lockdep. */
>  	add_taint(taint, LOCKDEP_STILL_OK);
> +
> +	nbcon_cpu_emergency_exit();
>  }
>  

Best Regards,
Petr