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

[John Ogness <john.ogness@linutronix.de>]

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().

> 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.

John