Problem with nbcon console and amba-pl011 serial port

Problem with nbcon console and amba-pl011 serial port

[Michael Kelley]

I'm seeing a problem in the panic path of an ARM64 VM on Hyper-V
in the Azure public cloud.  Here's the output from the VM's serial port:

# taskset -c 4 /bin/echo c >/proc/sysrq-trigger

[
** replaying previous printk message **
[   51.616656] sysrq: Trigger a crash
[   51.616689] Kernel panic - not syncing: sysrq triggered crash
[   51.624212] CPU: 4 UID: 0 PID: 2278 Comm: echo Tainted: G            E       6.15.0-rc7-next-20250521 #1 VOLUNTARY 
[   51.630165] Tainted: [E]=UNSIGNED_MODULE
[   51.632331] Hardware name: Microsoft Corporation Virtual Machine/Virtual Machine, BIOS Hyper-V UEFI Release v4.1 09/28/2024
[   51.638771] Call trace:
[   51.640179]  show_stack+0x20/0x38 (C)
[   51.642488]  dump_stack_lvl+0xc8/0xf8
[   51.644638]  dump_stack+0x18/0x28
[   51.646654]  panic+0x384/0x478
[   51.648371]  sysrq_handle_crash+0x20/0x28
[   51.650815]  __handle_sysrq+0xdc/0x2b8
[   51.653080]  write_sysrq_trigger+0x124/0x240
[   51.655508]  proc_reg_write+0xa4/0x100
[   51.657917]  vfs_write+0xd8/0x3e0
[   51.659836]  ksys_write+0x74/0x110
[   51.661735]  __arm64_sys_write+0x24/0x38
[   51.663967]  invoke_syscall+0x6c/0xf8
[   51.666025]  el0_svc_common.constprop.0+0xc8/0xf0
[   51.668771]  do_el0_svc+0x24/0x38
[   51.670713]  el0_svc+0x40/0x198
[   51.672509]  el0t_64_sync_handler+0xc8/0xd0
[   51.675170]  el0t_64_sync+0x1b0/0x1b8
[   51.677351] SMP: stopping secondary CPUs
[   52.728175] SMP: failed to stop secondary CPUs 2
[   52.731229] Kernel Offset: 0x5706ebce0000 from 0xffff800080000000
[   52.734528] PHYS_OFFSET: 0x0
[   52.736115] CPU features: 0x2000,400007c0,02110ca1,5401faab
[   52.739275] Memory Limit: none
[   52.803615] ---[ end Kernel panic - not syncing: sysrq triggered crash ]---

The problem is the failure to stop secondary CPU 2.  (The CPU # that fails
to stop varies from run-to-run.) It is mostly reproducible, but not always. I
bisected to commit 2eb2608618ce ("serial: amba-pl011: Implement nbcon
console") in the 6.15 kernel. Further custom logging shows the details of
why the problem can happen. Here are the steps:

1. The "echo" command ends up in __handle_sysrq(), which outputs the
"sysrq: Trigger a crash" message using pr_info().  I always ran the "echo"
command on CPU 4 just for consistency in my testing/debugging -- there's
nothing special about CPU 4.

2. The "pr/ttyAMA0" kernel thread handles the outputting of the message.
nbcon_kthread_func() calls nbcon_emit_one() with the "use_atomic" parameter
set to false. nbcon_emit_one() in turn calls nbcon_emit_next_record() with
the console spin lock held and interrupts disabled. nbcon_emit_next_record()
then calls pl011_console_write_thread(). The latter has a "for" loop to output
each character of the message, and my custom logging shows that it is indeed
outputting the string "[   51.616656] sysrq: Trigger a crash".

3. While "pr/ttyAMA0" is doing its thing, __handle_sysrq() calls
sysrq_handle_crash(), which calls panic(). After some preliminaries, panic()
outputs the message "Kernel panic - not syncing: sysrq triggered crash"
using pr_emerg().

4. pr_emerg() has a high logging level, and it effectively steals the console
from the "pr/ttyAMA0" task, which I believe is intentional in the nbcon design.
Down in pl011_console_write_thread(), the "pr/ttyAMA0" task is doing
nbcon_enter_unsafe() and nbcon_exit_unsafe() around each character
that it outputs.  When pr_emerg() steals the console, nbcon_exit_unsafe()
returns 0, so the "for" loop exits. pl011_console_write_thread() then
enters a busy "while" loop waiting to reclaim the console. It's doing this
busy "while" loop with interrupts disabled, and because of the panic,
it never succeeds. Whatever CPU is running "pr/ttyAMA0" is effectively
stuck at this point.

5. Meanwhile panic() continues, calling panic_other_cpus_shutdown(). On
ARM64, other CPUs are stopped by sending them an IPI. Each CPU receives
the IPI and calls the PSCI function to stop itself. But the CPU running
"pr/ttyAMA0" is looping forever with interrupts disabled, so it never
processes the IPI and it never stops. ARM64 doesn't have a true NMI that
can override the looping with interrupts disabled, so there's no way to
stop that CPU.

6. The failure to stop the "pr/ttyAMA0" CPU then causes downstream
problems, such as when loading and running a kdump kernel.

The problem is timing dependent.  In some cases, the "pr/ttyAMA0"
thread is evidently able to get the message out before panic() calls
pr_emerg(). In my case running as a guest on Hyper-V, the pl011 device
in the guest VM is emulated by Hyper-V. Each pl011 register access
traps to the hypervisor, which slows things down and probably makes
the problem more likely. But from what I can see, the underlying
race condition exists regardless.

At this point, I just wanted to surface the problem. I don't have any
specific ideas on how to fix it, as my knowledge of nbcon is limited
to what I've learned in figuring out the failure path.

Toshiyuki -- what are your thoughts how to fix this?

Michael Kelley

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

[Ryo Takakura]

Hi!

On Tue, 3 Jun 2025 03:18:23 +0000, Michael Kelley wrote:
>The problem is timing dependent.  In some cases, the "pr/ttyAMA0"
>thread is evidently able to get the message out before panic() calls
>pr_emerg(). In my case running as a guest on Hyper-V, the pl011 device
>in the guest VM is emulated by Hyper-V. Each pl011 register access
>traps to the hypervisor, which slows things down and probably makes
>the problem more likely. But from what I can see, the underlying
>race condition exists regardless.

I believe the issue is not amba-pl011 specific, so maybe John and Petr
better be Cced as well.

Sincerely,
Ryo Takakura

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

[Toshiyuki Sato (Fujitsu)]

Hi Michael,

Thanks for the incident report and problem analysis.

> 
> I'm seeing a problem in the panic path of an ARM64 VM on Hyper-V
> in the Azure public cloud.  Here's the output from the VM's serial port:
> 
> # taskset -c 4 /bin/echo c >/proc/sysrq-trigger
> 
> [
> ** replaying previous printk message **
> [   51.616656] sysrq: Trigger a crash
> [   51.616689] Kernel panic - not syncing: sysrq triggered crash
> [   51.624212] CPU: 4 UID: 0 PID: 2278 Comm: echo Tainted: G            E
> 6.15.0-rc7-next-20250521 #1 VOLUNTARY
> [   51.630165] Tainted: [E]=UNSIGNED_MODULE
> [   51.632331] Hardware name: Microsoft Corporation Virtual Machine/Virtual
> Machine, BIOS Hyper-V UEFI Release v4.1 09/28/2024
> [   51.638771] Call trace:
> [   51.640179]  show_stack+0x20/0x38 (C)
> [   51.642488]  dump_stack_lvl+0xc8/0xf8
> [   51.644638]  dump_stack+0x18/0x28
> [   51.646654]  panic+0x384/0x478
> [   51.648371]  sysrq_handle_crash+0x20/0x28
> [   51.650815]  __handle_sysrq+0xdc/0x2b8
> [   51.653080]  write_sysrq_trigger+0x124/0x240
> [   51.655508]  proc_reg_write+0xa4/0x100
> [   51.657917]  vfs_write+0xd8/0x3e0
> [   51.659836]  ksys_write+0x74/0x110
> [   51.661735]  __arm64_sys_write+0x24/0x38
> [   51.663967]  invoke_syscall+0x6c/0xf8
> [   51.666025]  el0_svc_common.constprop.0+0xc8/0xf0
> [   51.668771]  do_el0_svc+0x24/0x38
> [   51.670713]  el0_svc+0x40/0x198
> [   51.672509]  el0t_64_sync_handler+0xc8/0xd0
> [   51.675170]  el0t_64_sync+0x1b0/0x1b8
> [   51.677351] SMP: stopping secondary CPUs
> [   52.728175] SMP: failed to stop secondary CPUs 2
> [   52.731229] Kernel Offset: 0x5706ebce0000 from 0xffff800080000000
> [   52.734528] PHYS_OFFSET: 0x0
> [   52.736115] CPU features: 0x2000,400007c0,02110ca1,5401faab
> [   52.739275] Memory Limit: none
> [   52.803615] ---[ end Kernel panic - not syncing: sysrq triggered crash ]---
> 
> The problem is the failure to stop secondary CPU 2.  (The CPU # that fails
> to stop varies from run-to-run.) It is mostly reproducible, but not always. I
> bisected to commit 2eb2608618ce ("serial: amba-pl011: Implement nbcon
> console") in the 6.15 kernel. Further custom logging shows the details of
> why the problem can happen. Here are the steps:
> 
> 1. The "echo" command ends up in __handle_sysrq(), which outputs the
> "sysrq: Trigger a crash" message using pr_info().  I always ran the "echo"
> command on CPU 4 just for consistency in my testing/debugging -- there's
> nothing special about CPU 4.
> 
> 2. The "pr/ttyAMA0" kernel thread handles the outputting of the message.
> nbcon_kthread_func() calls nbcon_emit_one() with the "use_atomic" parameter
> set to false. nbcon_emit_one() in turn calls nbcon_emit_next_record() with
> the console spin lock held and interrupts disabled. nbcon_emit_next_record()
> then calls pl011_console_write_thread(). The latter has a "for" loop to output
> each character of the message, and my custom logging shows that it is indeed
> outputting the string "[   51.616656] sysrq: Trigger a crash".
> 
> 3. While "pr/ttyAMA0" is doing its thing, __handle_sysrq() calls
> sysrq_handle_crash(), which calls panic(). After some preliminaries, panic()
> outputs the message "Kernel panic - not syncing: sysrq triggered crash"
> using pr_emerg().
> 
> 4. pr_emerg() has a high logging level, and it effectively steals the console
> from the "pr/ttyAMA0" task, which I believe is intentional in the nbcon design.
> Down in pl011_console_write_thread(), the "pr/ttyAMA0" task is doing
> nbcon_enter_unsafe() and nbcon_exit_unsafe() around each character
> that it outputs.  When pr_emerg() steals the console, nbcon_exit_unsafe()
> returns 0, so the "for" loop exits. pl011_console_write_thread() then
> enters a busy "while" loop waiting to reclaim the console. It's doing this
> busy "while" loop with interrupts disabled, and because of the panic,
> it never succeeds. Whatever CPU is running "pr/ttyAMA0" is effectively
> stuck at this point.
> 
> 5. Meanwhile panic() continues, calling panic_other_cpus_shutdown(). On
> ARM64, other CPUs are stopped by sending them an IPI. Each CPU receives
> the IPI and calls the PSCI function to stop itself. But the CPU running
> "pr/ttyAMA0" is looping forever with interrupts disabled, so it never
> processes the IPI and it never stops. ARM64 doesn't have a true NMI that
> can override the looping with interrupts disabled, so there's no way to
> stop that CPU.
> 
> 6. The failure to stop the "pr/ttyAMA0" CPU then causes downstream
> problems, such as when loading and running a kdump kernel.
> 
> The problem is timing dependent.  In some cases, the "pr/ttyAMA0"
> thread is evidently able to get the message out before panic() calls
> pr_emerg(). In my case running as a guest on Hyper-V, the pl011 device
> in the guest VM is emulated by Hyper-V. Each pl011 register access
> traps to the hypervisor, which slows things down and probably makes
> the problem more likely. But from what I can see, the underlying
> race condition exists regardless.
> 
> At this point, I just wanted to surface the problem. I don't have any
> specific ideas on how to fix it, as my knowledge of nbcon is limited
> to what I've learned in figuring out the failure path.
> 
> Toshiyuki -- what are your thoughts how to fix this?
> 
> Michael Kelley

I believe your assumption is correct.
Using nbcon-compatible serial console driver on an architecture
that does not have NMI will likely cause the same issue.
(I think imx, pl011 and sifive which is under development have this problem)

After reproducing the issue, 
I plan to try a workaround that forcibly terminates the nbcon_reacquire_nobuf
loop in pl011_console_write_thread if other_cpu_in_panic is true.
Please comment if you have any other ideas.

Regards,
Toshiyuki Sato

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

[John Ogness]

Hi Toshiyuki,

On 2025-06-03, "Toshiyuki Sato (Fujitsu)" <fj6611ie@fujitsu.com> wrote:
>> 4. pr_emerg() has a high logging level, and it effectively steals the console
>> from the "pr/ttyAMA0" task, which I believe is intentional in the nbcon design.
>> Down in pl011_console_write_thread(), the "pr/ttyAMA0" task is doing
>> nbcon_enter_unsafe() and nbcon_exit_unsafe() around each character
>> that it outputs.  When pr_emerg() steals the console, nbcon_exit_unsafe()
>> returns 0, so the "for" loop exits. pl011_console_write_thread() then
>> enters a busy "while" loop waiting to reclaim the console. It's doing this
>> busy "while" loop with interrupts disabled, and because of the panic,
>> it never succeeds. Whatever CPU is running "pr/ttyAMA0" is effectively
>> stuck at this point.
>> 
>> 5. Meanwhile panic() continues, calling panic_other_cpus_shutdown(). On
>> ARM64, other CPUs are stopped by sending them an IPI. Each CPU receives
>> the IPI and calls the PSCI function to stop itself. But the CPU running
>> "pr/ttyAMA0" is looping forever with interrupts disabled, so it never
>> processes the IPI and it never stops. ARM64 doesn't have a true NMI that
>> can override the looping with interrupts disabled, so there's no way to
>> stop that CPU.
>> 
>> 6. The failure to stop the "pr/ttyAMA0" CPU then causes downstream
>> problems, such as when loading and running a kdump kernel.

[...]

> After reproducing the issue, 
> I plan to try a workaround that forcibly terminates the nbcon_reacquire_nobuf
> loop in pl011_console_write_thread if other_cpu_in_panic is true.
> Please comment if you have any other ideas.

For panic, if it is OK to leave uap->clk enabled and not restore REG_CR,
then it should be fine to just return. But only for panic.

So something like:

	while (!nbcon_enter_unsafe(wctxt)) {
		if (other_cpu_in_panic())
			return;
		nbcon_reacquire_nobuf(wctxt);
	}

(And other_cpu_in_panic() will need to be made generally available.)

John Ogness

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

[John Ogness]

On 2025-06-03, Michael Kelley <mhklinux@outlook.com> wrote:
> The problem is the failure to stop secondary CPU 2.  (The CPU # that fails
> to stop varies from run-to-run.) It is mostly reproducible, but not always. I
> bisected to commit 2eb2608618ce ("serial: amba-pl011: Implement nbcon
> console") in the 6.15 kernel.

Unrelated to this particular report, I am looking at commit 2eb2608618ce
("serial: amba-pl011: Implement nbcon console") and I do not think it
implements atomic printing correctly.

pl011_console_write_atomic() assumes uap->clk is disabled when it is
called. However, if it took over ownership from the printing kthread,
the uap->clk is already enabled. And then after printing its line it
disables uap->clk, even though the interrupted printing kthread expects
uap->clk to still be enabled once it regains ownership.

The atomic printing needs to track if the clock is enabled or disabled
and act accordingly. I suppose something like this:

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 11d65097578cd..914449b46b95b 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2520,11 +2520,14 @@ pl011_console_write_atomic(struct console *co, struct nbcon_write_context *wctxt
 {
 	struct uart_amba_port *uap = amba_ports[co->index];
 	unsigned int old_cr = 0;
+	bool old_enabled;
 
 	if (!nbcon_enter_unsafe(wctxt))
 		return;
 
-	clk_enable(uap->clk);
+	old_enabled = __clk_is_enabled(uap->clk);
+	if (!old_enabled)
+		clk_enable(uap->clk);
 
 	if (!uap->vendor->always_enabled) {
 		old_cr = pl011_read(uap, REG_CR);
@@ -2542,7 +2545,8 @@ pl011_console_write_atomic(struct console *co, struct nbcon_write_context *wctxt
 	if (!uap->vendor->always_enabled)
 		pl011_write(old_cr, uap, REG_CR);
 
-	clk_disable(uap->clk);
+	if (!old_enabled)
+		clk_disable(uap->clk);
 
 	nbcon_exit_unsafe(wctxt);
 }

I am guessing that it is allowed to use __clk_is_enabled() for this
purpose. Otherwise it can be tracked as a bool in struct uart_amba_port.

John Ogness

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

[Toshiyuki Sato (Fujitsu)]

Hi John,

Thank you for your help with this issue.

> On 2025-06-03, Michael Kelley <mhklinux@outlook.com> wrote:
> > The problem is the failure to stop secondary CPU 2.  (The CPU # that fails
> > to stop varies from run-to-run.) It is mostly reproducible, but not always. I
> > bisected to commit 2eb2608618ce ("serial: amba-pl011: Implement nbcon
> > console") in the 6.15 kernel.
> 
> Unrelated to this particular report, I am looking at commit 2eb2608618ce
> ("serial: amba-pl011: Implement nbcon console") and I do not think it
> implements atomic printing correctly.
> 
> pl011_console_write_atomic() assumes uap->clk is disabled when it is
> called. However, if it took over ownership from the printing kthread,
> the uap->clk is already enabled. And then after printing its line it
> disables uap->clk, even though the interrupted printing kthread expects
> uap->clk to still be enabled once it regains ownership.
> 
> The atomic printing needs to track if the clock is enabled or disabled
> and act accordingly. I suppose something like this:
> 
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 11d65097578cd..914449b46b95b 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -2520,11 +2520,14 @@ pl011_console_write_atomic(struct console *co,
> struct nbcon_write_context *wctxt
>  {
>  	struct uart_amba_port *uap = amba_ports[co->index];
>  	unsigned int old_cr = 0;
> +	bool old_enabled;
> 
>  	if (!nbcon_enter_unsafe(wctxt))
>  		return;
> 
> -	clk_enable(uap->clk);
> +	old_enabled = __clk_is_enabled(uap->clk);
> +	if (!old_enabled)
> +		clk_enable(uap->clk);
> 
>  	if (!uap->vendor->always_enabled) {
>  		old_cr = pl011_read(uap, REG_CR);
> @@ -2542,7 +2545,8 @@ pl011_console_write_atomic(struct console *co, struct
> nbcon_write_context *wctxt
>  	if (!uap->vendor->always_enabled)
>  		pl011_write(old_cr, uap, REG_CR);
> 
> -	clk_disable(uap->clk);
> +	if (!old_enabled)
> +		clk_disable(uap->clk);
> 
>  	nbcon_exit_unsafe(wctxt);
>  }
> 
> I am guessing that it is allowed to use __clk_is_enabled() for this
> purpose. Otherwise it can be tracked as a bool in struct uart_amba_port.
> 
> John Ogness

I believe the Common Clock Framework manages the enable count for clocks.
Specifically, uap->clk->core->enable_count is incremented by clk_enable
and decremented by clk_disable.
Wouldn't the clock remain enabled until enable_count reaches 0?

Regards, 
Toshiyuki Sato

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

[John Ogness]

On 2025-06-04, "Toshiyuki Sato (Fujitsu)" <fj6611ie@fujitsu.com> wrote:
>> Unrelated to this particular report, I am looking at commit 2eb2608618ce
>> ("serial: amba-pl011: Implement nbcon console") and I do not think it
>> implements atomic printing correctly.
>> 
>> pl011_console_write_atomic() assumes uap->clk is disabled when it is
>> called. However, if it took over ownership from the printing kthread,
>> the uap->clk is already enabled. And then after printing its line it
>> disables uap->clk, even though the interrupted printing kthread expects
>> uap->clk to still be enabled once it regains ownership.
>
> I believe the Common Clock Framework manages the enable count for clocks.
> Specifically, uap->clk->core->enable_count is incremented by clk_enable
> and decremented by clk_disable.
> Wouldn't the clock remain enabled until enable_count reaches 0?

You are correct. I wasn't aware that the clock framework had a usage
counter to allow recursive calls. Sorry for the noise.

John

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

[Russell King (Oracle)]

On Wed, Jun 04, 2025 at 09:50:03AM +0206, John Ogness wrote:
> You are correct. I wasn't aware that the clock framework had a usage
> counter to allow recursive calls. Sorry for the noise.

It's documented:

/**
 * clk_disable - inform the system when the clock source is no longer required.
 * @clk: clock source
 *
 * Inform the system that a clock source is no longer required by
 * a driver and may be shut down.
 *
 * May be called from atomic contexts.
 *
 * Implementation detail: if the clock source is shared between
 * multiple drivers, clk_enable() calls must be balanced by the
 * same number of clk_disable() calls for the clock source to be
 * disabled.
 */

similarly for clk_unprepare(). It has to be this way if you think about
the fact that clock sources are not unique to clock consumers, otherwise
the first clock consumer to disable a clock will disrupt other
consumers of that same clock.

It's been that way since I created the clk API, which predates CCF.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[John Ogness]

Hi Petr,

On 2025-06-03, John Ogness <john.ogness@linutronix.de> wrote:
> On 2025-06-03, "Toshiyuki Sato (Fujitsu)" <fj6611ie@fujitsu.com> wrote:
>>> 4. pr_emerg() has a high logging level, and it effectively steals the console
>>> from the "pr/ttyAMA0" task, which I believe is intentional in the nbcon design.
>>> Down in pl011_console_write_thread(), the "pr/ttyAMA0" task is doing
>>> nbcon_enter_unsafe() and nbcon_exit_unsafe() around each character
>>> that it outputs.  When pr_emerg() steals the console, nbcon_exit_unsafe()
>>> returns 0, so the "for" loop exits. pl011_console_write_thread() then
>>> enters a busy "while" loop waiting to reclaim the console. It's doing this
>>> busy "while" loop with interrupts disabled, and because of the panic,
>>> it never succeeds. Whatever CPU is running "pr/ttyAMA0" is effectively
>>> stuck at this point.
>>> 
>>> 5. Meanwhile panic() continues, calling panic_other_cpus_shutdown(). On
>>> ARM64, other CPUs are stopped by sending them an IPI. Each CPU receives
>>> the IPI and calls the PSCI function to stop itself. But the CPU running
>>> "pr/ttyAMA0" is looping forever with interrupts disabled, so it never
>>> processes the IPI and it never stops. ARM64 doesn't have a true NMI that
>>> can override the looping with interrupts disabled, so there's no way to
>>> stop that CPU.
>>> 
>>> 6. The failure to stop the "pr/ttyAMA0" CPU then causes downstream
>>> problems, such as when loading and running a kdump kernel.
>
> [...]
>
>> After reproducing the issue, 
>> I plan to try a workaround that forcibly terminates the nbcon_reacquire_nobuf
>> loop in pl011_console_write_thread if other_cpu_in_panic is true.
>> Please comment if you have any other ideas.
>
> For panic, if it is OK to leave uap->clk enabled and not restore REG_CR,
> then it should be fine to just return. But only for panic.
>
> So something like:
>
> 	while (!nbcon_enter_unsafe(wctxt)) {
> 		if (other_cpu_in_panic())
> 			return;
> 		nbcon_reacquire_nobuf(wctxt);
> 	}

Actually this is not enough because there is also a loop inside
nbcon_reacquire_nobuf().

nbcon_reacquire_nobuf() needs to return an error for the panic case
because it will never succeed. This is the only case where it will never
succeed. Should we use a bool? Or return some code like -EPERM?

So the above code becomes:

 	while (!nbcon_enter_unsafe(wctxt)) {
 		if (!nbcon_reacquire_nobuf(wctxt))
 			return;
 	}

We should also add __must_check to the prototype.

Thoughts?

John

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

[Toshiyuki Sato (Fujitsu)]

Hi Michael, John,

> Hi Petr,
> 
> On 2025-06-03, John Ogness <john.ogness@linutronix.de> wrote:
> > On 2025-06-03, "Toshiyuki Sato (Fujitsu)" <fj6611ie@fujitsu.com> wrote:
> >>> 4. pr_emerg() has a high logging level, and it effectively steals the console
> >>> from the "pr/ttyAMA0" task, which I believe is intentional in the nbcon
> design.
> >>> Down in pl011_console_write_thread(), the "pr/ttyAMA0" task is doing
> >>> nbcon_enter_unsafe() and nbcon_exit_unsafe() around each character
> >>> that it outputs.  When pr_emerg() steals the console, nbcon_exit_unsafe()
> >>> returns 0, so the "for" loop exits. pl011_console_write_thread() then
> >>> enters a busy "while" loop waiting to reclaim the console. It's doing this
> >>> busy "while" loop with interrupts disabled, and because of the panic,
> >>> it never succeeds. Whatever CPU is running "pr/ttyAMA0" is effectively
> >>> stuck at this point.
> >>>
> >>> 5. Meanwhile panic() continues, calling panic_other_cpus_shutdown(). On
> >>> ARM64, other CPUs are stopped by sending them an IPI. Each CPU receives
> >>> the IPI and calls the PSCI function to stop itself. But the CPU running
> >>> "pr/ttyAMA0" is looping forever with interrupts disabled, so it never
> >>> processes the IPI and it never stops. ARM64 doesn't have a true NMI that
> >>> can override the looping with interrupts disabled, so there's no way to
> >>> stop that CPU.
> >>>
> >>> 6. The failure to stop the "pr/ttyAMA0" CPU then causes downstream
> >>> problems, such as when loading and running a kdump kernel.
> >
> > [...]
> >
> >> After reproducing the issue,
> >> I plan to try a workaround that forcibly terminates the nbcon_reacquire_nobuf
> >> loop in pl011_console_write_thread if other_cpu_in_panic is true.
> >> Please comment if you have any other ideas.
> >
> > For panic, if it is OK to leave uap->clk enabled and not restore REG_CR,
> > then it should be fine to just return. But only for panic.
> >
> > So something like:
> >
> > 	while (!nbcon_enter_unsafe(wctxt)) {
> > 		if (other_cpu_in_panic())
> > 			return;
> > 		nbcon_reacquire_nobuf(wctxt);
> > 	}
> 
> Actually this is not enough because there is also a loop inside
> nbcon_reacquire_nobuf().
> 
> nbcon_reacquire_nobuf() needs to return an error for the panic case
> because it will never succeed. This is the only case where it will never
> succeed. Should we use a bool? Or return some code like -EPERM?
> 
> So the above code becomes:
> 
>  	while (!nbcon_enter_unsafe(wctxt)) {
>  		if (!nbcon_reacquire_nobuf(wctxt))
>  			return;
>  	}
> 
> We should also add __must_check to the prototype.
> 
> Thoughts?
> 
> John

Thanks for the suggestion, John.

This is a proposed fix to force termination by returning false from
nbcon_reacquire_nobuf when a panic occurs within pl011_console_write_thread.
 (I believe this is similar to what John suggested in his previous reply.)

While I couldn't reproduce the issue using sysrq-trigger in my environment
 (It seemed that the panic was being executed before the thread processing),
I did observe nbcon_reacquire_nobuf failing to complete when injecting an
NMI (SError) during pl011_console_write_thread. 
Applying this fix seems to have resolved the "SMP: failed to stop secondary
CPUs" issue.

This patch is for test.
Modifications to imx and other drivers, as well as adding __must_check, 
will likely be required.

Michael, could you please test this fix in your environment?

Regards,
Toshiyuki Sato

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 11d650975..c3a2b22e6 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2577,8 +2577,10 @@ pl011_console_write_thread(struct console *co, struct nbcon_write_context *wctxt
 		}
 	}
 
-	while (!nbcon_enter_unsafe(wctxt))
-		nbcon_reacquire_nobuf(wctxt);
+	while (!nbcon_enter_unsafe(wctxt)) {
+		if (!nbcon_reacquire_nobuf(wctxt))
+			return;
+	}
 
 	while ((pl011_read(uap, REG_FR) ^ uap->vendor->inv_fr) & uap->vendor->fr_busy)
 		cpu_relax();
diff --git a/include/linux/console.h b/include/linux/console.h
index 8f10d0a85..ae278b875 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -604,14 +604,14 @@ extern void nbcon_cpu_emergency_exit(void);
 extern bool nbcon_can_proceed(struct nbcon_write_context *wctxt);
 extern bool nbcon_enter_unsafe(struct nbcon_write_context *wctxt);
 extern bool nbcon_exit_unsafe(struct nbcon_write_context *wctxt);
-extern void nbcon_reacquire_nobuf(struct nbcon_write_context *wctxt);
+extern bool nbcon_reacquire_nobuf(struct nbcon_write_context *wctxt);
 #else
 static inline void nbcon_cpu_emergency_enter(void) { }
 static inline void nbcon_cpu_emergency_exit(void) { }
 static inline bool nbcon_can_proceed(struct nbcon_write_context *wctxt) { return false; }
 static inline bool nbcon_enter_unsafe(struct nbcon_write_context *wctxt) { return false; }
 static inline bool nbcon_exit_unsafe(struct nbcon_write_context *wctxt) { return false; }
-static inline void nbcon_reacquire_nobuf(struct nbcon_write_context *wctxt) { }
+static inline bool nbcon_reacquire_nobuf(struct nbcon_write_context *wctxt) { }
 #endif
 
 extern int console_set_on_cmdline;
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index fd12efcc4..ec016bb24 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -909,14 +909,18 @@ EXPORT_SYMBOL_GPL(nbcon_exit_unsafe);
  * output buffer because that has been lost. This function cannot be used to
  * resume printing.
  */
-void nbcon_reacquire_nobuf(struct nbcon_write_context *wctxt)
+bool 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)) {
 		cpu_relax();
+		if (other_cpu_in_panic())
+			return false;
+	}
 
 	nbcon_write_context_set_buf(wctxt, NULL, 0);
+	return true;
 }
 EXPORT_SYMBOL_GPL(nbcon_reacquire_nobuf);

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

[John Ogness]

On 2025-06-04, "Toshiyuki Sato (Fujitsu)" <fj6611ie@fujitsu.com> wrote:
> This is a proposed fix to force termination by returning false from
> nbcon_reacquire_nobuf when a panic occurs within pl011_console_write_thread.
>  (I believe this is similar to what John suggested in his previous reply.)
>
> While I couldn't reproduce the issue using sysrq-trigger in my environment
>  (It seemed that the panic was being executed before the thread processing),
> I did observe nbcon_reacquire_nobuf failing to complete when injecting an
> NMI (SError) during pl011_console_write_thread. 
> Applying this fix seems to have resolved the "SMP: failed to stop secondary
> CPUs" issue.
>
> This patch is for test.
> Modifications to imx and other drivers, as well as adding __must_check, 
> will likely be required.
>
> Michael, could you please test this fix in your environment?
>
> Regards,
> Toshiyuki Sato
>
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 11d650975..c3a2b22e6 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -2577,8 +2577,10 @@ pl011_console_write_thread(struct console *co, struct nbcon_write_context *wctxt
>  		}
>  	}
>  
> -	while (!nbcon_enter_unsafe(wctxt))
> -		nbcon_reacquire_nobuf(wctxt);
> +	while (!nbcon_enter_unsafe(wctxt)) {

I realize this is just a test patch. But for the real patch, there needs
to be some comment here about bailing out. On panic, the driver is
leaving the clock enabled and not restoring REG_CR. At a quick glance
this appears to be a bug. So the comment is needed to make it clear that
the driver is exiting on purpose without proper completion. And perhaps
also mentioning why this is safe during panic. Thanks.

> +		if (!nbcon_reacquire_nobuf(wctxt))
> +			return;
> +	}
>  
>  	while ((pl011_read(uap, REG_FR) ^ uap->vendor->inv_fr) & uap->vendor->fr_busy)
>  		cpu_relax();

John Ogness

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

[Petr Mladek]

On Wed 2025-06-04 04:11:10, Toshiyuki Sato (Fujitsu) wrote:
> > On 2025-06-03, John Ogness <john.ogness@linutronix.de> wrote:
> > > On 2025-06-03, "Toshiyuki Sato (Fujitsu)" <fj6611ie@fujitsu.com> wrote:
> > >>> 4. pr_emerg() has a high logging level, and it effectively steals the console
> > >>> from the "pr/ttyAMA0" task, which I believe is intentional in the nbcon
> > design.
> > >>> Down in pl011_console_write_thread(), the "pr/ttyAMA0" task is doing
> > >>> nbcon_enter_unsafe() and nbcon_exit_unsafe() around each character
> > >>> that it outputs.  When pr_emerg() steals the console, nbcon_exit_unsafe()
> > >>> returns 0, so the "for" loop exits. pl011_console_write_thread() then
> > >>> enters a busy "while" loop waiting to reclaim the console. It's doing this
> > >>> busy "while" loop with interrupts disabled, and because of the panic,
> > >>> it never succeeds.

I am a bit surprised that it never succeeds. The panic CPU takes over
the ownership but it releases it when the messages are flushed. And
the original owner should be able to reacquire it in this case.

But maybe, this does not work well on virtualized systems when the
virtualized CPUs do not run at the same time.

> > >>> Whatever CPU is running "pr/ttyAMA0" is effectively
> > >>> stuck at this point.
> > >>>
> > >>> 5. Meanwhile panic() continues, calling panic_other_cpus_shutdown(). On
> > >>> ARM64, other CPUs are stopped by sending them an IPI. Each CPU receives
> > >>> the IPI and calls the PSCI function to stop itself. But the CPU running
> > >>> "pr/ttyAMA0" is looping forever with interrupts disabled, so it never
> > >>> processes the IPI and it never stops. ARM64 doesn't have a true NMI that
> > >>> can override the looping with interrupts disabled, so there's no way to
> > >>> stop that CPU.
> > >>>
> > >>> 6. The failure to stop the "pr/ttyAMA0" CPU then causes downstream
> > >>> problems, such as when loading and running a kdump kernel.

Thanks a lot for this great analyze. It makes sense. It must have been
hard to put all the pieces together.

> > >
> > > [...]
> > >

> This is a proposed fix to force termination by returning false from
> nbcon_reacquire_nobuf when a panic occurs within pl011_console_write_thread.
>  (I believe this is similar to what John suggested in his previous reply.)
> 
> While I couldn't reproduce the issue using sysrq-trigger in my environment
>  (It seemed that the panic was being executed before the thread processing),
> I did observe nbcon_reacquire_nobuf failing to complete when injecting an
> NMI (SError) during pl011_console_write_thread. 
> Applying this fix seems to have resolved the "SMP: failed to stop secondary
> CPUs" issue.
> 
> This patch is for test.
> Modifications to imx and other drivers, as well as adding __must_check, 
> will likely be required.
> 
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 11d650975..c3a2b22e6 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -2577,8 +2577,10 @@ pl011_console_write_thread(struct console *co, struct nbcon_write_context *wctxt
>  		}
>  	}
>  
> -	while (!nbcon_enter_unsafe(wctxt))
> -		nbcon_reacquire_nobuf(wctxt);
> +	while (!nbcon_enter_unsafe(wctxt)) {
> +		if (!nbcon_reacquire_nobuf(wctxt))
> +			return;
> +	}
>  
>  	while ((pl011_read(uap, REG_FR) ^ uap->vendor->inv_fr) & uap->vendor->fr_busy)
>  		cpu_relax();
> diff --git a/include/linux/console.h b/include/linux/console.h
> index 8f10d0a85..ae278b875 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -604,14 +604,14 @@ extern void nbcon_cpu_emergency_exit(void);
>  extern bool nbcon_can_proceed(struct nbcon_write_context *wctxt);
>  extern bool nbcon_enter_unsafe(struct nbcon_write_context *wctxt);
>  extern bool nbcon_exit_unsafe(struct nbcon_write_context *wctxt);
> -extern void nbcon_reacquire_nobuf(struct nbcon_write_context *wctxt);
> +extern bool nbcon_reacquire_nobuf(struct nbcon_write_context *wctxt);
>  #else
>  static inline void nbcon_cpu_emergency_enter(void) { }
>  static inline void nbcon_cpu_emergency_exit(void) { }
>  static inline bool nbcon_can_proceed(struct nbcon_write_context *wctxt) { return false; }
>  static inline bool nbcon_enter_unsafe(struct nbcon_write_context *wctxt) { return false; }
>  static inline bool nbcon_exit_unsafe(struct nbcon_write_context *wctxt) { return false; }
> -static inline void nbcon_reacquire_nobuf(struct nbcon_write_context *wctxt) { }
> +static inline bool nbcon_reacquire_nobuf(struct nbcon_write_context *wctxt) { }
>  #endif
>  
>  extern int console_set_on_cmdline;
> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
> index fd12efcc4..ec016bb24 100644
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -909,14 +909,18 @@ EXPORT_SYMBOL_GPL(nbcon_exit_unsafe);
>   * output buffer because that has been lost. This function cannot be used to
>   * resume printing.
>   */

Please, add a comment explaining the new return value. Something
like:

 * Return:	True when the context reacquired the owner ship. The caller
 *		might try entering the unsafe state and restore the original
 *		console device setting. It just must not access the output
 *		buffer anymore.
 *
 *		False when another CPU is in panic() and a busy waiting for
 *		the ownership is not safe. It might prevent stopping this
 *		CPU and create further complications, especially on virtualized
 *		systems without proper NMI. The caller should bail out
 *		immediately without touching the console device or
 *		the output buffer.

It is very long. But I think that a good explanation might safe people
troubles in the future.

> -void nbcon_reacquire_nobuf(struct nbcon_write_context *wctxt)
> +bool 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)) {
>  		cpu_relax();
> +		if (other_cpu_in_panic())
> +			return false;
> +	}
>  
>  	nbcon_write_context_set_buf(wctxt, NULL, 0);

It would make sense to set the NULL buffer even when returning "false".

> +	return true;
>  }
>  EXPORT_SYMBOL_GPL(nbcon_reacquire_nobuf);

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

[John Ogness]

On 2025-06-04, Petr Mladek <pmladek@suse.com> wrote:
> On Wed 2025-06-04 04:11:10, Toshiyuki Sato (Fujitsu) wrote:
>> > On 2025-06-03, John Ogness <john.ogness@linutronix.de> wrote:
>> > > On 2025-06-03, "Toshiyuki Sato (Fujitsu)" <fj6611ie@fujitsu.com> wrote:
>> > >>> 4. pr_emerg() has a high logging level, and it effectively steals the console
>> > >>> from the "pr/ttyAMA0" task, which I believe is intentional in the nbcon
>> > design.
>> > >>> Down in pl011_console_write_thread(), the "pr/ttyAMA0" task is doing
>> > >>> nbcon_enter_unsafe() and nbcon_exit_unsafe() around each character
>> > >>> that it outputs.  When pr_emerg() steals the console, nbcon_exit_unsafe()
>> > >>> returns 0, so the "for" loop exits. pl011_console_write_thread() then
>> > >>> enters a busy "while" loop waiting to reclaim the console. It's doing this
>> > >>> busy "while" loop with interrupts disabled, and because of the panic,
>> > >>> it never succeeds.
>
> I am a bit surprised that it never succeeds. The panic CPU takes over
> the ownership but it releases it when the messages are flushed. And
> the original owner should be able to reacquire it in this case.

The problem is that other_cpu_in_panic() will return true forever, which
will cause _all_ acquires to fail forever. Originally we did allow
non-panic to take over again after panic releases ownership. But IIRC we
removed that capability because it allowed us to reduce a lot of
complexity. And now nbcon_waiter_matches() relies on "Lower priorities
are ignored during panic() until reboot."

John Ogness

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

[Petr Mladek]

On Wed 2025-06-04 13:56:34, John Ogness wrote:
> On 2025-06-04, Petr Mladek <pmladek@suse.com> wrote:
> > On Wed 2025-06-04 04:11:10, Toshiyuki Sato (Fujitsu) wrote:
> >> > On 2025-06-03, John Ogness <john.ogness@linutronix.de> wrote:
> >> > > On 2025-06-03, "Toshiyuki Sato (Fujitsu)" <fj6611ie@fujitsu.com> wrote:
> >> > >>> 4. pr_emerg() has a high logging level, and it effectively steals the console
> >> > >>> from the "pr/ttyAMA0" task, which I believe is intentional in the nbcon
> >> > design.
> >> > >>> Down in pl011_console_write_thread(), the "pr/ttyAMA0" task is doing
> >> > >>> nbcon_enter_unsafe() and nbcon_exit_unsafe() around each character
> >> > >>> that it outputs.  When pr_emerg() steals the console, nbcon_exit_unsafe()
> >> > >>> returns 0, so the "for" loop exits. pl011_console_write_thread() then
> >> > >>> enters a busy "while" loop waiting to reclaim the console. It's doing this
> >> > >>> busy "while" loop with interrupts disabled, and because of the panic,
> >> > >>> it never succeeds.
> >
> > I am a bit surprised that it never succeeds. The panic CPU takes over
> > the ownership but it releases it when the messages are flushed. And
> > the original owner should be able to reacquire it in this case.
> 
> The problem is that other_cpu_in_panic() will return true forever, which
> will cause _all_ acquires to fail forever. Originally we did allow
> non-panic to take over again after panic releases ownership. But IIRC we
> removed that capability because it allowed us to reduce a lot of
> complexity. And now nbcon_waiter_matches() relies on "Lower priorities
> are ignored during panic() until reboot."

Great catch! I forgot it. And it explains everything.

It would be nice to mention this in the commit message or
in the comment above nbcon_reacquire_nobuf().

My updated prosal of the comment is:

 * Return:	True when the context reacquired the owner ship. The caller
 *		might try entering the unsafe state and restore the original
 *		console device setting. It must not access the output buffer
 *		anymore.
 *
 *		False when another CPU is in panic(). nbcon_try_acquire()
 *		would never succeed and the infinite loop would	prevent
 *		stopping this CPU on architectures without proper NMI.
 *		The caller should bail out immediately without
 *		touching the console device or the output buffer.

Best Regards,
Petr

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

[Toshiyuki Sato (Fujitsu)]

Hi John and Petr,

> On Wed 2025-06-04 13:56:34, John Ogness wrote:
> > On 2025-06-04, Petr Mladek <pmladek@suse.com> wrote:
> > > On Wed 2025-06-04 04:11:10, Toshiyuki Sato (Fujitsu) wrote:
> > >> > On 2025-06-03, John Ogness <john.ogness@linutronix.de> wrote:
> > >> > > On 2025-06-03, "Toshiyuki Sato (Fujitsu)" <fj6611ie@fujitsu.com> wrote:
> > >> > >>> 4. pr_emerg() has a high logging level, and it effectively steals the console
> > >> > >>> from the "pr/ttyAMA0" task, which I believe is intentional in the nbcon
> > >> > design.
> > >> > >>> Down in pl011_console_write_thread(), the "pr/ttyAMA0" task is doing
> > >> > >>> nbcon_enter_unsafe() and nbcon_exit_unsafe() around each character
> > >> > >>> that it outputs.  When pr_emerg() steals the console, nbcon_exit_unsafe()
> > >> > >>> returns 0, so the "for" loop exits. pl011_console_write_thread() then
> > >> > >>> enters a busy "while" loop waiting to reclaim the console. It's doing this
> > >> > >>> busy "while" loop with interrupts disabled, and because of the panic,
> > >> > >>> it never succeeds.
> > >
> > > I am a bit surprised that it never succeeds. The panic CPU takes over
> > > the ownership but it releases it when the messages are flushed. And
> > > the original owner should be able to reacquire it in this case.
> >
> > The problem is that other_cpu_in_panic() will return true forever, which
> > will cause _all_ acquires to fail forever. Originally we did allow
> > non-panic to take over again after panic releases ownership. But IIRC we
> > removed that capability because it allowed us to reduce a lot of
> > complexity. And now nbcon_waiter_matches() relies on "Lower priorities
> > are ignored during panic() until reboot."
> 
> Great catch! I forgot it. And it explains everything.
> 
> It would be nice to mention this in the commit message or
> in the comment above nbcon_reacquire_nobuf().
> 
> My updated prosal of the comment is:
> 
>  * Return:	True when the context reacquired the owner ship. The caller
>  *		might try entering the unsafe state and restore the original
>  *		console device setting. It must not access the output buffer
>  *		anymore.
>  *
>  *		False when another CPU is in panic(). nbcon_try_acquire()
>  *		would never succeed and the infinite loop would	prevent
>  *		stopping this CPU on architectures without proper NMI.
>  *		The caller should bail out immediately without
>  *		touching the console device or the output buffer.
> 
> Best Regards,
> Petr

Thank you for your comments and suggestions.

After consideration, 
I believe that there is no problem with forcibly terminating the process when 
nbcon_reacquire_nobuf returns false at the pl011 driver level, 
as in the test patch.

It feels a bit harsh that a thread which started processing before the panic
and then transferred ownership to an atomic operation isn't allowed to perform
cleanup during panic handling or the grace period before the CPU halts.

I would like to hear your opinion on this.
If nbcon_reacquire_nobuf() could acquire ownership even after the panic, 
then driver-side modifications might not be necessary. 
(The responsibility to complete the process without hindering the panic process
would still remain.)

Would it be difficult to make an exception to the rule,
  "Lower priorities are ignored during panic() until reboot,"
depending on the situation?

This is just my personal opinion.
I think it would be good to get feedback from other UART driver developers as well.

Regards,
Toshiyuki Sato

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

[Petr Mladek]

On Thu 2025-06-05 05:27:56, Toshiyuki Sato (Fujitsu) wrote:
> Hi John and Petr,
> 
> > On Wed 2025-06-04 13:56:34, John Ogness wrote:
> > > On 2025-06-04, Petr Mladek <pmladek@suse.com> wrote:
> > > > On Wed 2025-06-04 04:11:10, Toshiyuki Sato (Fujitsu) wrote:
> > > >> > On 2025-06-03, John Ogness <john.ogness@linutronix.de> wrote:
> > > >> > > On 2025-06-03, "Toshiyuki Sato (Fujitsu)" <fj6611ie@fujitsu.com> wrote:
> > > >> > >>> 4. pr_emerg() has a high logging level, and it effectively steals the console
> > > >> > >>> from the "pr/ttyAMA0" task, which I believe is intentional in the nbcon
> > > >> > design.
> > > >> > >>> Down in pl011_console_write_thread(), the "pr/ttyAMA0" task is doing
> > > >> > >>> nbcon_enter_unsafe() and nbcon_exit_unsafe() around each character
> > > >> > >>> that it outputs.  When pr_emerg() steals the console, nbcon_exit_unsafe()
> > > >> > >>> returns 0, so the "for" loop exits. pl011_console_write_thread() then
> > > >> > >>> enters a busy "while" loop waiting to reclaim the console. It's doing this
> > > >> > >>> busy "while" loop with interrupts disabled, and because of the panic,
> > > >> > >>> it never succeeds.
> > > >
> > > > I am a bit surprised that it never succeeds. The panic CPU takes over
> > > > the ownership but it releases it when the messages are flushed. And
> > > > the original owner should be able to reacquire it in this case.
> > >
> > > The problem is that other_cpu_in_panic() will return true forever, which
> > > will cause _all_ acquires to fail forever. Originally we did allow
> > > non-panic to take over again after panic releases ownership. But IIRC we
> > > removed that capability because it allowed us to reduce a lot of
> > > complexity. And now nbcon_waiter_matches() relies on "Lower priorities
> > > are ignored during panic() until reboot."
> > 
> > Great catch! I forgot it. And it explains everything.
> > 
> > It would be nice to mention this in the commit message or
> > in the comment above nbcon_reacquire_nobuf().
> > 
> > My updated prosal of the comment is:
> > 
> >  * Return:	True when the context reacquired the owner ship. The caller
> >  *		might try entering the unsafe state and restore the original
> >  *		console device setting. It must not access the output buffer
> >  *		anymore.
> >  *
> >  *		False when another CPU is in panic(). nbcon_try_acquire()
> >  *		would never succeed and the infinite loop would	prevent
> >  *		stopping this CPU on architectures without proper NMI.
> >  *		The caller should bail out immediately without
> >  *		touching the console device or the output buffer.
> > 
> > Best Regards,
> > Petr
> 
> Thank you for your comments and suggestions.
> 
> After consideration, 
> I believe that there is no problem with forcibly terminating the process when 
> nbcon_reacquire_nobuf returns false at the pl011 driver level, 
> as in the test patch.
> 
> It feels a bit harsh that a thread which started processing before the panic
> and then transferred ownership to an atomic operation isn't allowed to perform
> cleanup during panic handling or the grace period before the CPU halts.
> 
> I would like to hear your opinion on this.
> If nbcon_reacquire_nobuf() could acquire ownership even after the panic, 
> then driver-side modifications might not be necessary. 
> (The responsibility to complete the process without hindering the panic process
> would still remain.)
> 
> Would it be difficult to make an exception to the rule,
>   "Lower priorities are ignored during panic() until reboot,"
> depending on the situation?

Good question.

The following two problems came to my mind:

1. As John, pointed out, the fact that non-panic CPUs are not
   able to acquire the context allowed to simplify the implementation.

   I think that it is primary about nbcon_waiter_matches(),
   nbcon_owner_matches(), and the related logic. It was documented by
   the commit 8c9dab2c55ad7 ("printk: nbcon: Clarify rules of
   the owner/waiter matching").

   But it seems that nbcon_owner_matches() is safe even without the rule.
   The race is prevented either by disabling interrupts and preemption
   or by taking device_lock().

   The rule prevents a race in nbcon_waiter_matches(). But it seems
   that in the worst case, more CPUs might end up busy waiting.
   And it would be acceptable during panic().

   So, this need not be a big problem in the end.


2. If we allowed non-panic() CPUs to acquire the ownership, it would
   increase the risk that the panic CPU will not be able to
   flush the messages.

   But maybe, the problem is only when the architecture supports
   proper NMI and non-panic CPUs might be stopped anywhere.

   It should be less problem on architectures without proper NMI
   where the non-panic CPU could not be stopped in the problematic
   situation.

   So, maybe, we could relax the rule on architectures without
   proper NMI.


The question is if it is worth it. Is the clean up really important?

Note that the clean up will never be guaranteed on architectures with
a proper NMI. They would stop the non-panic CPUs, including the printk
kthread, anywhere.

And I guess that the console devices will be initialized after the
reboot anyway.

Best Regards,
Petr

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

[Toshiyuki Sato (Fujitsu)]

Hi Petr,

Thanks for your comment.

> On Thu 2025-06-05 05:27:56, Toshiyuki Sato (Fujitsu) wrote:
> > Hi John and Petr,
> >
> > > On Wed 2025-06-04 13:56:34, John Ogness wrote:
> > > > On 2025-06-04, Petr Mladek <pmladek@suse.com> wrote:
> > > > > On Wed 2025-06-04 04:11:10, Toshiyuki Sato (Fujitsu) wrote:
> > > > >> > On 2025-06-03, John Ogness <john.ogness@linutronix.de> wrote:
> > > > >> > > On 2025-06-03, "Toshiyuki Sato (Fujitsu)" <fj6611ie@fujitsu.com> wrote:
> > > > >> > >>> 4. pr_emerg() has a high logging level, and it effectively steals the console
> > > > >> > >>> from the "pr/ttyAMA0" task, which I believe is intentional in the nbcon
> > > > >> > design.
> > > > >> > >>> Down in pl011_console_write_thread(), the "pr/ttyAMA0" task is doing
> > > > >> > >>> nbcon_enter_unsafe() and nbcon_exit_unsafe() around each character
> > > > >> > >>> that it outputs.  When pr_emerg() steals the console, nbcon_exit_unsafe()
> > > > >> > >>> returns 0, so the "for" loop exits. pl011_console_write_thread() then
> > > > >> > >>> enters a busy "while" loop waiting to reclaim the console. It's doing this
> > > > >> > >>> busy "while" loop with interrupts disabled, and because of the panic,
> > > > >> > >>> it never succeeds.
> > > > >
> > > > > I am a bit surprised that it never succeeds. The panic CPU takes over
> > > > > the ownership but it releases it when the messages are flushed. And
> > > > > the original owner should be able to reacquire it in this case.
> > > >
> > > > The problem is that other_cpu_in_panic() will return true forever, which
> > > > will cause _all_ acquires to fail forever. Originally we did allow
> > > > non-panic to take over again after panic releases ownership. But IIRC we
> > > > removed that capability because it allowed us to reduce a lot of
> > > > complexity. And now nbcon_waiter_matches() relies on "Lower priorities
> > > > are ignored during panic() until reboot."
> > >
> > > Great catch! I forgot it. And it explains everything.
> > >
> > > It would be nice to mention this in the commit message or
> > > in the comment above nbcon_reacquire_nobuf().
> > >
> > > My updated prosal of the comment is:
> > >
> > >  * Return:	True when the context reacquired the owner ship. The caller
> > >  *		might try entering the unsafe state and restore the original
> > >  *		console device setting. It must not access the output buffer
> > >  *		anymore.
> > >  *
> > >  *		False when another CPU is in panic(). nbcon_try_acquire()
> > >  *		would never succeed and the infinite loop would	prevent
> > >  *		stopping this CPU on architectures without proper NMI.
> > >  *		The caller should bail out immediately without
> > >  *		touching the console device or the output buffer.
> > >
> > > Best Regards,
> > > Petr
> >
> > Thank you for your comments and suggestions.
> >
> > After consideration,
> > I believe that there is no problem with forcibly terminating the process when
> > nbcon_reacquire_nobuf returns false at the pl011 driver level,
> > as in the test patch.
> >
> > It feels a bit harsh that a thread which started processing before the panic
> > and then transferred ownership to an atomic operation isn't allowed to perform
> > cleanup during panic handling or the grace period before the CPU halts.
> >
> > I would like to hear your opinion on this.
> > If nbcon_reacquire_nobuf() could acquire ownership even after the panic,
> > then driver-side modifications might not be necessary.
> > (The responsibility to complete the process without hindering the panic process
> > would still remain.)
> >
> > Would it be difficult to make an exception to the rule,
> >   "Lower priorities are ignored during panic() until reboot,"
> > depending on the situation?
> 
> Good question.
> 
> The following two problems came to my mind:
> 
> 1. As John, pointed out, the fact that non-panic CPUs are not
>    able to acquire the context allowed to simplify the implementation.
> 
>    I think that it is primary about nbcon_waiter_matches(),
>    nbcon_owner_matches(), and the related logic. It was documented by
>    the commit 8c9dab2c55ad7 ("printk: nbcon: Clarify rules of
>    the owner/waiter matching").
> 
>    But it seems that nbcon_owner_matches() is safe even without the rule.
>    The race is prevented either by disabling interrupts and preemption
>    or by taking device_lock().
> 
>    The rule prevents a race in nbcon_waiter_matches(). But it seems
>    that in the worst case, more CPUs might end up busy waiting.
>    And it would be acceptable during panic().
> 
>    So, this need not be a big problem in the end.
> 
> 
> 2. If we allowed non-panic() CPUs to acquire the ownership, it would
>    increase the risk that the panic CPU will not be able to
>    flush the messages.
> 
>    But maybe, the problem is only when the architecture supports
>    proper NMI and non-panic CPUs might be stopped anywhere.
> 
>    It should be less problem on architectures without proper NMI
>    where the non-panic CPU could not be stopped in the problematic
>    situation.
> 
>    So, maybe, we could relax the rule on architectures without
>    proper NMI.
> 
> 
> The question is if it is worth it. Is the clean up really important?
> 
> Note that the clean up will never be guaranteed on architectures with
> a proper NMI. They would stop the non-panic CPUs, including the printk
> kthread, anywhere.
> 
> And I guess that the console devices will be initialized after the
> reboot anyway.
> 

Understood.

At least for pl011, a forced shutdown does not cause any problems.
(Since clk remains enabled and CR maintains a transmittable state,
it does not hinder the panic handling process.)

I'll create a revised patch based on the patch I posted for testing this time.
Please allow me some time.

When I submit the patch,
I would appreciate your continued assistance with reviews and other aspects.

Regards,
Toshiyuki Sato

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

[John Ogness]

On 2025-06-05, Petr Mladek <pmladek@suse.com> wrote:
> The question is if it is worth it. Is the clean up really important?

I must admit that I am not happy about encouraging the proposed solution
so far (i.e. expecting driver authors to create special unsafe code in
the panic situation). It leads down the "hope and pray" path that nbcon
was designed to fix.

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.

Thoughts?

John Ogness

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
@@ -208,6 +208,7 @@ extern bool nbcon_device_try_acquire(struct console *con);
 extern void nbcon_device_release(struct console *con);
 void nbcon_atomic_flush_unsafe(void);
 bool pr_flush(int timeout_ms, bool reset_on_progress);
+void nbcon_panic_allow_reacquire_set(bool value);
 #else
 static inline __printf(1, 0)
 int vprintk(const char *s, va_list args)
@@ -321,6 +322,10 @@ static inline bool pr_flush(int timeout_ms, bool reset_on_progress)
 	return true;
 }
 
+static inline void nbcon_panic_allow_reacquire_set(bool value)
+{
+}
+
 #endif
 
 bool this_cpu_in_panic(void);
diff --git a/kernel/panic.c b/kernel/panic.c
index b0b9a8bf4560d..8f572630c9f7e 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -292,6 +292,12 @@ static void panic_other_cpus_shutdown(bool crash_kexec)
 		panic_triggering_all_cpu_backtrace = false;
 	}
 
+	/*
+	 * Temporarily allow non-panic CPUs to finish any nbcon cleanup
+	 * in case they were interrupted due to the panic.
+	 */
+	nbcon_panic_allow_reacquire_set(true);
+
 	/*
 	 * Note that smp_send_stop() is the usual SMP shutdown function,
 	 * which unfortunately may not be hardened to work in a panic
@@ -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);
 }
 
 /**
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)
 			return -EPERM;
 
 		if (ctxt->prio <= cur->prio || ctxt->prio <= cur->req_prio)
@@ -568,7 +569,7 @@ static struct printk_buffers panic_nbcon_pbufs;
  * in an unsafe state. Otherwise, on success the caller may assume
  * the console is not in an unsafe state.
  */
-static bool nbcon_context_try_acquire(struct nbcon_context *ctxt)
+static bool nbcon_context_try_acquire(struct nbcon_context *ctxt, bool ignore_other_cpu_in_panic)
 {
 	unsigned int cpu = smp_processor_id();
 	struct console *con = ctxt->console;
@@ -577,7 +578,7 @@ static bool nbcon_context_try_acquire(struct nbcon_context *ctxt)
 
 	nbcon_state_read(con, &cur);
 try_again:
-	err = nbcon_context_try_acquire_direct(ctxt, &cur);
+	err = nbcon_context_try_acquire_direct(ctxt, &cur, ignore_other_cpu_in_panic);
 	if (err != -EBUSY)
 		goto out;
 
@@ -892,6 +893,12 @@ bool nbcon_exit_unsafe(struct nbcon_write_context *wctxt)
 }
 EXPORT_SYMBOL_GPL(nbcon_exit_unsafe);
 
+static bool nbcon_panic_allow_reacquire;
+void nbcon_panic_allow_reacquire_set(bool value)
+{
+	nbcon_panic_allow_reacquire = value;
+}
+
 /**
  * nbcon_reacquire_nobuf - Reacquire a console after losing ownership
  *				while printing
@@ -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)))
 		cpu_relax();
 
 	nbcon_write_context_set_buf(wctxt, NULL, 0);
@@ -1101,7 +1108,7 @@ static bool nbcon_emit_one(struct nbcon_write_context *wctxt, bool use_atomic)
 		cant_migrate();
 	}
 
-	if (!nbcon_context_try_acquire(ctxt))
+	if (!nbcon_context_try_acquire(ctxt, false))
 		goto out;
 
 	/*
@@ -1486,7 +1493,7 @@ static int __nbcon_atomic_flush_pending_con(struct console *con, u64 stop_seq,
 	ctxt->prio			= nbcon_get_default_prio();
 	ctxt->allow_unsafe_takeover	= allow_unsafe_takeover;
 
-	if (!nbcon_context_try_acquire(ctxt))
+	if (!nbcon_context_try_acquire(ctxt, false))
 		return -EPERM;
 
 	while (nbcon_seq_read(con) < stop_seq) {
@@ -1784,7 +1791,7 @@ bool nbcon_device_try_acquire(struct console *con)
 	ctxt->console	= con;
 	ctxt->prio	= NBCON_PRIO_NORMAL;
 
-	if (!nbcon_context_try_acquire(ctxt))
+	if (!nbcon_context_try_acquire(ctxt, false))
 		return false;
 
 	if (!nbcon_context_enter_unsafe(ctxt))

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

[John Ogness]

On 2025-06-06, John Ogness <john.ogness@linutronix.de> wrote:
> On 2025-06-05, Petr Mladek <pmladek@suse.com> wrote:
>> The question is if it is worth it. Is the clean up really important?
>
> I must admit that I am not happy about encouraging the proposed solution
> so far (i.e. expecting driver authors to create special unsafe code in
> the panic situation). It leads down the "hope and pray" path that nbcon
> was designed to fix.
>
> 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.

Thinking about it some more, since it does not involve printing, why not
just always allow reacquiring directly in panic?

This simplifies the diff significantly.

John

diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index d60596777d278..26c229b7f56ea 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)
 			return -EPERM;
 
 		if (ctxt->prio <= cur->prio || ctxt->prio <= cur->req_prio)
@@ -568,7 +569,7 @@ static struct printk_buffers panic_nbcon_pbufs;
  * in an unsafe state. Otherwise, on success the caller may assume
  * the console is not in an unsafe state.
  */
-static bool nbcon_context_try_acquire(struct nbcon_context *ctxt)
+static bool nbcon_context_try_acquire(struct nbcon_context *ctxt, bool ignore_other_cpu_in_panic)
 {
 	unsigned int cpu = smp_processor_id();
 	struct console *con = ctxt->console;
@@ -577,7 +578,7 @@ static bool nbcon_context_try_acquire(struct nbcon_context *ctxt)
 
 	nbcon_state_read(con, &cur);
 try_again:
-	err = nbcon_context_try_acquire_direct(ctxt, &cur);
+	err = nbcon_context_try_acquire_direct(ctxt, &cur, ignore_other_cpu_in_panic);
 	if (err != -EBUSY)
 		goto out;
 
@@ -913,7 +914,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, true))
 		cpu_relax();
 
 	nbcon_write_context_set_buf(wctxt, NULL, 0);
@@ -1101,7 +1102,7 @@ static bool nbcon_emit_one(struct nbcon_write_context *wctxt, bool use_atomic)
 		cant_migrate();
 	}
 
-	if (!nbcon_context_try_acquire(ctxt))
+	if (!nbcon_context_try_acquire(ctxt, false))
 		goto out;
 
 	/*
@@ -1486,7 +1487,7 @@ static int __nbcon_atomic_flush_pending_con(struct console *con, u64 stop_seq,
 	ctxt->prio			= nbcon_get_default_prio();
 	ctxt->allow_unsafe_takeover	= allow_unsafe_takeover;
 
-	if (!nbcon_context_try_acquire(ctxt))
+	if (!nbcon_context_try_acquire(ctxt, false))
 		return -EPERM;
 
 	while (nbcon_seq_read(con) < stop_seq) {
@@ -1784,7 +1785,7 @@ bool nbcon_device_try_acquire(struct console *con)
 	ctxt->console	= con;
 	ctxt->prio	= NBCON_PRIO_NORMAL;
 
-	if (!nbcon_context_try_acquire(ctxt))
+	if (!nbcon_context_try_acquire(ctxt, false))
 		return false;
 
 	if (!nbcon_context_enter_unsafe(ctxt))

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

[Petr Mladek]

On Fri 2025-06-06 12:25:35, John Ogness wrote:
> On 2025-06-05, Petr Mladek <pmladek@suse.com> wrote:
> > The question is if it is worth it. Is the clean up really important?
> 
> I must admit that I am not happy about encouraging the proposed solution
> so far

I am not super happy either. But let me play the devil advocate
for a bit longer ;-)

> (i.e. expecting driver authors to create special unsafe code in
> the panic situation).

The "usafe code" sounds too strong to me. The console driver is
supposed to "do nothing" and just leave when nbcon_reacquire() fails.


> It leads down the "hope and pray" path that nbcon
> was designed to fix.

My understanding is that we "hope and pray" to show the messages
on the console. And it should work because the ownership was
lost only in a safe state.

If the ownership was lost in the unsafe state then it might
be even bigger gamble to do anything in parallel.

Of course, another panic priority is to provide a crash dump
or reboot. But it hopefully should not depend on a console
driver clean up.

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


>  /**
> 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))

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

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

Best Regards,
Petr

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

[John Ogness]

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

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

[Michael Kelley]

From: Toshiyuki Sato (Fujitsu) <fj6611ie@fujitsu.com> Sent: Tuesday, June 3, 2025 9:11 PM
> 
> Hi Michael, John,
> 

[snip]

> 
> This is a proposed fix to force termination by returning false from
> nbcon_reacquire_nobuf when a panic occurs within pl011_console_write_thread.
>  (I believe this is similar to what John suggested in his previous reply.)
> 
> While I couldn't reproduce the issue using sysrq-trigger in my environment
>  (It seemed that the panic was being executed before the thread processing),
> I did observe nbcon_reacquire_nobuf failing to complete when injecting an
> NMI (SError) during pl011_console_write_thread.
> Applying this fix seems to have resolved the "SMP: failed to stop secondary
> CPUs" issue.
> 
> This patch is for test.
> Modifications to imx and other drivers, as well as adding __must_check,
> will likely be required.
> 
> Michael, could you please test this fix in your environment?

I've tested the fix in my primary environment (ARM64 VM in the Azure cloud),
and I've seen no failures to stop a CPU. I kept my custom logging in place, so I
could confirm that the problem path is still happening, and the fix recovers from
the problem path. So the good results are not due to just a timing change. The
"pr/ttyAMA0" task is still looping forever trying to get ownership of the console,
but it is doing so at a higher level in nbcon_kthread_func() and in calling
nbcon_emit_one(), and interrupts are enabled for part of the loop.

Full disclosure: I have a secondary environment, also an ARM64 VM in the
Azure cloud, but running on an older version of Hyper-V. In this environment
I see the same custom logging results, and the "pr/ttyAMA0" task is indeed
looping with interrupts enabled. But for some reason, the CPU doesn't stop
in response to IPI_CPU_STOP. I don't see any evidence that this failure to stop
is due to the Linux pl011 driver or nbcon. This older version of Hyper-V has a
known problem in pl011 UART emulation, and I have a theory on how that
problem may be causing the failure to stop. It will take me some time to
investigate further, but based on what I know now, that investigation should
not hold up this fix.

Michael

> 
> Regards,
> Toshiyuki Sato
> 
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 11d650975..c3a2b22e6 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -2577,8 +2577,10 @@ pl011_console_write_thread(struct console *co, struct nbcon_write_context *wctxt
>  		}
>  	}
> 
> -	while (!nbcon_enter_unsafe(wctxt))
> -		nbcon_reacquire_nobuf(wctxt);
> +	while (!nbcon_enter_unsafe(wctxt)) {
> +		if (!nbcon_reacquire_nobuf(wctxt))
> +			return;
> +	}
> 
>  	while ((pl011_read(uap, REG_FR) ^ uap->vendor->inv_fr) & uap->vendor->fr_busy)
>  		cpu_relax();
> diff --git a/include/linux/console.h b/include/linux/console.h
> index 8f10d0a85..ae278b875 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -604,14 +604,14 @@ extern void nbcon_cpu_emergency_exit(void);
>  extern bool nbcon_can_proceed(struct nbcon_write_context *wctxt);
>  extern bool nbcon_enter_unsafe(struct nbcon_write_context *wctxt);
>  extern bool nbcon_exit_unsafe(struct nbcon_write_context *wctxt);
> -extern void nbcon_reacquire_nobuf(struct nbcon_write_context *wctxt);
> +extern bool nbcon_reacquire_nobuf(struct nbcon_write_context *wctxt);
>  #else
>  static inline void nbcon_cpu_emergency_enter(void) { }
>  static inline void nbcon_cpu_emergency_exit(void) { }
>  static inline bool nbcon_can_proceed(struct nbcon_write_context *wctxt) { return false; }
>  static inline bool nbcon_enter_unsafe(struct nbcon_write_context *wctxt) { return false; }
>  static inline bool nbcon_exit_unsafe(struct nbcon_write_context *wctxt) { return false; }
> -static inline void nbcon_reacquire_nobuf(struct nbcon_write_context *wctxt) { }
> +static inline bool nbcon_reacquire_nobuf(struct nbcon_write_context *wctxt) { }
>  #endif
> 
>  extern int console_set_on_cmdline;
> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
> index fd12efcc4..ec016bb24 100644
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -909,14 +909,18 @@ EXPORT_SYMBOL_GPL(nbcon_exit_unsafe);
>   * output buffer because that has been lost. This function cannot be used to
>   * resume printing.
>   */
> -void nbcon_reacquire_nobuf(struct nbcon_write_context *wctxt)
> +bool 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)) {
>  		cpu_relax();
> +		if (other_cpu_in_panic())
> +			return false;
> +	}
> 
>  	nbcon_write_context_set_buf(wctxt, NULL, 0);
> +	return true;
>  }
>  EXPORT_SYMBOL_GPL(nbcon_reacquire_nobuf);

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

[Toshiyuki Sato (Fujitsu)]

Hi Michael,

> From: Michael Kelley <mhklinux@outlook.com>
> Sent: Thursday, June 5, 2025 11:49 AM
> > Hi Michael, John,
> >
> 
> [snip]
> 
> >
> > This is a proposed fix to force termination by returning false from
> > nbcon_reacquire_nobuf when a panic occurs within pl011_console_write_thread.
> >  (I believe this is similar to what John suggested in his previous
> > reply.)
> >
> > While I couldn't reproduce the issue using sysrq-trigger in my
> > environment  (It seemed that the panic was being executed before the
> > thread processing), I did observe nbcon_reacquire_nobuf failing to
> > complete when injecting an NMI (SError) during pl011_console_write_thread.
> > Applying this fix seems to have resolved the "SMP: failed to stop
> > secondary CPUs" issue.
> >
> > This patch is for test.
> > Modifications to imx and other drivers, as well as adding
> > __must_check, will likely be required.
> >
> > Michael, could you please test this fix in your environment?
> 
> I've tested the fix in my primary environment (ARM64 VM in the Azure cloud), and I've seen no failures to stop a CPU. I kept my
> custom logging in place, so I could confirm that the problem path is still happening, and the fix recovers from the problem path.
> So the good results are not due to just a timing change. The "pr/ttyAMA0" task is still looping forever trying to get ownership
> of the console, but it is doing so at a higher level in nbcon_kthread_func() and in calling nbcon_emit_one(), and interrupts are
> enabled for part of the loop.
> 
> Full disclosure: I have a secondary environment, also an ARM64 VM in the Azure cloud, but running on an older version of
> Hyper-V. In this environment I see the same custom logging results, and the "pr/ttyAMA0" task is indeed looping with
> interrupts enabled. But for some reason, the CPU doesn't stop in response to IPI_CPU_STOP. I don't see any evidence that this
> failure to stop is due to the Linux pl011 driver or nbcon. This older version of Hyper-V has a known problem in pl011 UART
> emulation, and I have a theory on how that problem may be causing the failure to stop. It will take me some time to investigate
> further, but based on what I know now, that investigation should not hold up this fix.
> 
> Michael

Thank you for testing the patch.
I'm concerned about the thread looping...

Regards,
Toshiyuki Sato

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

[John Ogness]

On 2025-06-05, "Toshiyuki Sato (Fujitsu)" <fj6611ie@fujitsu.com> wrote:
>> I've tested the fix in my primary environment (ARM64 VM in the Azure cloud), and I've seen no failures to stop a CPU. I kept my
>> custom logging in place, so I could confirm that the problem path is still happening, and the fix recovers from the problem path.
>> So the good results are not due to just a timing change. The "pr/ttyAMA0" task is still looping forever trying to get ownership
>> of the console, but it is doing so at a higher level in nbcon_kthread_func() and in calling nbcon_emit_one(), and interrupts are
>> enabled for part of the loop.
>> 
>> Full disclosure: I have a secondary environment, also an ARM64 VM in the Azure cloud, but running on an older version of
>> Hyper-V. In this environment I see the same custom logging results, and the "pr/ttyAMA0" task is indeed looping with
>> interrupts enabled. But for some reason, the CPU doesn't stop in response to IPI_CPU_STOP. I don't see any evidence that this
>> failure to stop is due to the Linux pl011 driver or nbcon. This older version of Hyper-V has a known problem in pl011 UART
>> emulation, and I have a theory on how that problem may be causing the failure to stop. It will take me some time to investigate
>> further, but based on what I know now, that investigation should not hold up this fix.
>> 
>> Michael
>
> Thank you for testing the patch.
> I'm concerned about the thread looping...

The thread would only loop if there is a backlog. But that backlog
should have been flushed atomically by the panic CPU.

Are you able to dump the kernel buffer and see if there are trailing
messages in the kernel buffer that did not get printed? I wonder if the
atomic printing is hanging or something.

John

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

[Michael Kelley]

From: John Ogness <john.ogness@linutronix.de> Sent: Thursday, June 5, 2025 12:43 AM
> 
> On 2025-06-05, "Toshiyuki Sato (Fujitsu)" <fj6611ie@fujitsu.com> wrote:
> >> I've tested the fix in my primary environment (ARM64 VM in the Azure cloud), and
> I've seen no failures to stop a CPU. I kept my
> >> custom logging in place, so I could confirm that the problem path is still happening,
> and the fix recovers from the problem path.
> >> So the good results are not due to just a timing change. The "pr/ttyAMA0" task is still
> looping forever trying to get ownership
> >> of the console, but it is doing so at a higher level in nbcon_kthread_func() and in
> calling nbcon_emit_one(), and interrupts are
> >> enabled for part of the loop.
> >>
> >> Full disclosure: I have a secondary environment, also an ARM64 VM in the Azure
> cloud, but running on an older version of
> >> Hyper-V. In this environment I see the same custom logging results, and the
> "pr/ttyAMA0" task is indeed looping with
> >> interrupts enabled. But for some reason, the CPU doesn't stop in response to
> IPI_CPU_STOP. I don't see any evidence that this
> >> failure to stop is due to the Linux pl011 driver or nbcon. This older version of Hyper-V
> has a known problem in pl011 UART
> >> emulation, and I have a theory on how that problem may be causing the failure to
> stop. It will take me some time to investigate
> >> further, but based on what I know now, that investigation should not hold up this fix.
> >>
> >> Michael
> >
> > Thank you for testing the patch.
> > I'm concerned about the thread looping...
> 
> The thread would only loop if there is a backlog. But that backlog
> should have been flushed atomically by the panic CPU.
> 
> Are you able to dump the kernel buffer and see if there are trailing
> messages in the kernel buffer that did not get printed? I wonder if the
> atomic printing is hanging or something.
> 

Getting back to your question. There are 24 lines of console output
in the panic path with sysrq, up to and including the "SMP: stopping
secondary CPUs" line. The nbcon kthread starts to output the
first line, which is at INFO level. Then the panic() function outputs
the 2nd line at EMERGENCY level and in doing so it takes control
of the console, and re-outputs the 1st line followed by the 2nd line.
The panic function then outputs the remaining 22 lines. What I see
is that in nbcon_kthread_func(), the call to rcuwait_wait_event()
completes about 80,000 times after the panic() path takes control
of the console. That rcuwait_wait_event() stops completing sometime
between when the panic path calls nbcon_emit_next_record() for
the 2nd line and again for the 3rd line. Then nbcon_kthread_func()
remains quiescent as the panic path outputs the remaining lines in
successive calls to nbcon_emit_next_record(). Of course, the other
CPUs then get stopped, and the kthread can't do anything anyway. I
haven't tried to track down all the nuances of the expected behavior,
and my custom tracing has limitations. But maybe the kthread looping
behavior is as expected?

Separately, I see that you have posted a patch that solves the
original problem in a different way. I'll test that tonight or
tomorrow.

Michael