Re: Problem with nbcon console and amba-pl011 serial port

John Ogness
2025-06-06T16:58:24Z

On 2025-06-06, Petr Mladek <pmladek@suse.com> wrote:
>> What if during non-panic-CPU shutdown, we allow reacquires to succeed
>> only for _direct_ acquires? The below diff shows how this could be
>> implemented. Since it only supports direct acquires, it does not violate
>> any state rules. And also, since it only involves the reacquire, there
>> is no ugly battling for console printing between the panic and non-panic
>> CPUs.
>
> Interesting idea. I thought a lot about it, see below.
>
>
>> diff --git a/include/linux/printk.h b/include/linux/printk.h
>> index 5b462029d03c1..d58ebdc8170b3 100644
>> --- a/include/linux/printk.h
>> +++ b/include/linux/printk.h
>> diff --git a/kernel/panic.c b/kernel/panic.c
>> index b0b9a8bf4560d..8f572630c9f7e 100644
>> --- a/kernel/panic.c
>> +++ b/kernel/panic.c
>> @@ -304,6 +310,8 @@ static void panic_other_cpus_shutdown(bool crash_kexec)
>> smp_send_stop();
>> else
>> crash_smp_send_stop();
>> +
>> + nbcon_panic_allow_reacquire_set(false);
>> }
>
> I have two concerns here:
>
> 1. I wonder whether this is reliable enough. It seems that
> smp_send_stop() waits at least 1 sec until the CPUs
> get stopped. But is this enough on virtualized systems?
>
> 2. It might increase a risk when CPUs are stopped using NMI.
> The change would allow a non-panic CPU to reacquire the ownership
> and enter _unsafe_ section right before being stopped by NMI.
>
>
> The 1st problem might be avoided by allowing the reacquire all
> the time unless an "unsafe" takeover happened.
>
> The 2nd problem is worse. But allowing the reacquire all the time
> might actually help as well.
>
> Note that the information about the "unsafe_takeover" is stored
> in struct console so that we even won't need a new global
> variable.

That is a good idea. We should add unsafe_takeover as a condition as
well. That can only happen way after the smp_send_stop() anyway. But it
means that only the atomic printing code would ever need to worry about
unsafe_takeover (assuming a driver were to implement some sort of
handling of it).

>> /**
>> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
>> index d60596777d278..d960cb8a05558 100644
>> --- a/kernel/printk/nbcon.c
>> +++ b/kernel/printk/nbcon.c
>> @@ -235,7 +235,8 @@ static void nbcon_seq_try_update(struct nbcon_context *ctxt, u64 new_seq)
>> * the handover acquire method.
>> */
>> static int nbcon_context_try_acquire_direct(struct nbcon_context *ctxt,
>> - struct nbcon_state *cur)
>> + struct nbcon_state *cur,
>> + bool ignore_other_cpu_in_panic)
>> {
>> unsigned int cpu = smp_processor_id();
>> struct console *con = ctxt->console;
>> @@ -249,7 +250,7 @@ static int nbcon_context_try_acquire_direct(struct nbcon_context *ctxt,
>> * nbcon_waiter_matches(). In particular, the assumption that
>> * lower priorities are ignored during panic.
>> */
>> - if (other_cpu_in_panic())
>> + if (other_cpu_in_panic() && !ignore_other_cpu_in_panic)
>
> If you agree with allowing the reacquire all the time then I would
> rename the parameter to @is_reacquire and do something like:
>
> if (other_cpu_in_panic() &&
> (!is_reacquire || cur->unsafe_takeover))

Looks fine to me.

>> return -EPERM;
>>
>> if (ctxt->prio <= cur->prio || ctxt->prio <= cur->req_prio)
>> @@ -913,7 +920,7 @@ void nbcon_reacquire_nobuf(struct nbcon_write_context *wctxt)
>> {
>> struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
>>
>> - while (!nbcon_context_try_acquire(ctxt))
>> + while (!nbcon_context_try_acquire(ctxt, READ_ONCE(nbcon_panic_allow_reacquire)))
>
> And here it would be:
>
> while (!nbcon_context_try_acquire(ctxt, true))

Right.

>> cpu_relax();
>>
>> nbcon_write_context_set_buf(wctxt, NULL, 0);
>
>
> Summary:
>
> I open to give this alternative approach a chance when we allow the
> reacquire all the time. It might work well. And it won't require
> any special "panic" handling in all console drivers.

Agreed. Thanks for being open about this approach. I will put together
an official patch.

John