Re: [PATCH printk v1 1/1] printk: nbcon: Allow unsafe write_atomic() for panic

[Petr Mladek <pmladek@suse.com>]

On Wed 2025-09-17 14:53:26, John Ogness wrote:
> On 2025-09-16, Petr Mladek <pmladek@suse.com> wrote:
> >> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
> >> index 646801813415..8c2966b85ac3 100644
> >> --- a/kernel/printk/nbcon.c
> >> +++ b/kernel/printk/nbcon.c
> >> @@ -972,14 +972,18 @@ static bool nbcon_emit_next_record(struct nbcon_write_context *wctxt, bool use_a
> >>  	/*
> >>  	 * This function should never be called for consoles that have not
> >>  	 * implemented the necessary callback for writing: i.e. legacy
> >> -	 * consoles and, when atomic, nbcon consoles with no write_atomic().
> >> +	 * consoles and, when atomic, nbcon consoles with no write_atomic()
> >> +	 * or an unsafe write_atomic() without allowing unsafe takeovers.
> >>  	 * Handle it as if ownership was lost and try to continue.
> >>  	 *
> >>  	 * Note that for nbcon consoles the write_thread() callback is
> >>  	 * mandatory and was already checked in nbcon_alloc().
> >>  	 */
> >> -	if (WARN_ON_ONCE((use_atomic && !con->write_atomic) ||
> >> -			 !(console_srcu_read_flags(con) & CON_NBCON))) {
> >> +	if (WARN_ON_ONCE(!(console_srcu_read_flags(con) & CON_NBCON) ||
> >> +			 (use_atomic &&
> >> +			  (!con->write_atomic ||
> >> +			   (!ctxt->allow_unsafe_takeover &&
> >> +			    (console_srcu_read_flags(con) & CON_NBCON_ATOMIC_UNSAFE)))))) {
> >
> > The condition seems to be correct. But it is evil. I wonder whether
> > it would make sense to replace this with:
> >
> > 	flags = console_srcu_read_flags(con);
> >
> > 	if (WARN_ON_ONCE(!(flags & CON_NBCON) ||
> > 			 !console_is_usable(con, flags, use_atomic, ctxt->allow_unsafe_takeover))) {
> >
> >
> > Note that I have added the 4th parameter intentionally, see below.
> 
> ...
> 
> > It would be more reliable when the check was integrated into
> > console_is_usable(). I guess that you did not do it because
> > it added another parameter...
> 
> Not all console_is_usable() call sites have a printing context. That is
> why I only added the checks only to the actual ->write_atomic() paths
> that were possible via nbcon_atomic_flush_unsafe().

I see. But I still believe that it fits well into console_is_usable().
It is similar to the "use_atomic" parameter which depends on the
context as well. We could guess the context most of the time,
so that we hardcode the "use_atomic" value, ...


> > Or maybe, we could define @allow_unsafe_takeover via a global variable,
> > e.g. panic_nbcon_allow_unsafe_takeover. And it might be valid
> > only on the panic CPU, e.g.
> >
> > static inline
> > bool nbcon_allow_unsafe_takeover(void)
> > {
> > 	return panic_on_this_cpu() && panic_nbcon_allow_unsafe_takeover;
> > }
> >
> > It is a kind of hack. But it might be better than the 4th parameter.
> > And it would simplify few other APIs.
> 
> After weighing the pros/cons I think that a global variable makes the
> most sense. It will simplify internal APIs and provide all
> console_is_usable() users a correct value. And the end result is no
> different than what we do now.
> 
> We could also keep its setting inside nbcon_atomic_flush_unsafe() so
> that the variable remains a printk-internal variable.

Sounds good to me.

Best Regards,
Petr