* Re: [RFC PATCH v1] panic: Flush unsafe consoles before panic reboot
2026-09-15 10:01 [RFC PATCH v1] panic: Flush unsafe consoles before panic reboot Ryan Roberts
@ 2026-09-15 10:06 ` Ryan Roberts
2026-09-15 18:49 ` Bradley Morgan
2026-09-16 13:08 ` Petr Mladek
1 sibling, 1 reply; 6+ messages in thread
From: Ryan Roberts @ 2026-09-15 10:06 UTC (permalink / raw)
To: Andrew Morton, Petr Mladek, Steven Rostedt, John Ogness,
Sergey Senozhatsky, Rio
Cc: linux-kernel
On 15/09/2026 11:01, Ryan Roberts wrote:
> Netconsole registers with CON_NBCON_ATOMIC_UNSAFE, so its write_atomic()
> callback is only usable during nbcon_atomic_flush_unsafe(). Previously
> this was only called by vpanic() if panic_timeout=0 - i.e. if the system
> was configured not to reboot on panic. So if the system was configured
> to reboot on panic, netconsole would never receive the panic logs.
>
> Move vpanic()'s emergency_restart() call to after the call to
> nbcon_atomic_flush_unsafe() to solve this problem. The downside is that
> potentially unsafe operations are now performed prior to
> emergency_restart() which could theoretically reduce the chances of the
> restart succeeding. But we are already in a panic situation so it could
> be argued that everything is best effort already.
>
> In older kernels (v6.19 and earlier) netconsole is able to print these
> panic logs while the system is configured to reboot on panic. So from a
> user perspective this is a regression caused by commit 7eab73b18630
> ("netconsole: convert to NBCON console infrastructure").
>
> We have automated test systems without BMC access, which rely on these
> two features working together.
>
> Fixes: 7eab73b18630 ("netconsole: convert to NBCON console infrastructure")
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> ---
Oops forgot to add commentary to the bottom:
I've set this as RFC because I'm not sure if the current behaviour is a
deliberate design decision? I guess if we flush the unsafe console, there is a
possibility it deadlocks, preventing the subsequent reboot. If people are
concerned about this, perhaps this needs a cmdline flag?
Thanks,
Ryan
> kernel/panic.c | 35 ++++++++++++++++++-----------------
> 1 file changed, 18 insertions(+), 17 deletions(-)
>
> diff --git a/kernel/panic.c b/kernel/panic.c
> index 213725b612aa..473dc7d13b23 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -732,32 +732,22 @@ void vpanic(const char *fmt, va_list args)
> mdelay(PANIC_TIMER_STEP);
> }
> }
> - if (panic_timeout != 0) {
> - /*
> - * This will not be a clean reboot, with everything
> - * shutting down. But if there is a chance of
> - * rebooting the system it will be rebooted.
> - */
> - if (panic_reboot_mode != REBOOT_UNDEFINED)
> - reboot_mode = panic_reboot_mode;
> - emergency_restart();
> - }
> + if (panic_timeout == 0) {
> #ifdef __sparc__
> - {
> extern int stop_a_enabled;
> /* Make sure the user can actually press Stop-A (L1-A) */
> stop_a_enabled = 1;
> pr_emerg("Press Stop-A (L1-A) from sun keyboard or send break\n"
> - "twice on console to return to the boot prom\n");
> - }
> + "twice on console to return to the boot prom\n");
> #endif
> #if defined(CONFIG_S390)
> - disabled_wait();
> + disabled_wait();
> #endif
> - pr_emerg("---[ end Kernel panic - not syncing: %s ]---\n", buf);
> + pr_emerg("---[ end Kernel panic - not syncing: %s ]---\n", buf);
>
> - /* Do not scroll important messages printed above */
> - suppress_printk = 1;
> + /* Do not scroll important messages printed above */
> + suppress_printk = 1;
> + }
>
> /*
> * The final messages may not have been printed if in a context that
> @@ -767,6 +757,17 @@ void vpanic(const char *fmt, va_list args)
> console_flush_on_panic(CONSOLE_FLUSH_PENDING);
> nbcon_atomic_flush_unsafe();
>
> + if (panic_timeout != 0) {
> + /*
> + * This will not be a clean reboot, with everything
> + * shutting down. But if there is a chance of
> + * rebooting the system it will be rebooted.
> + */
> + if (panic_reboot_mode != REBOOT_UNDEFINED)
> + reboot_mode = panic_reboot_mode;
> + emergency_restart();
> + }
> +
> local_irq_enable();
> for (i = 0; ; i += PANIC_TIMER_STEP) {
> touch_softlockup_watchdog();
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RFC PATCH v1] panic: Flush unsafe consoles before panic reboot
2026-09-15 10:01 [RFC PATCH v1] panic: Flush unsafe consoles before panic reboot Ryan Roberts
2026-09-15 10:06 ` Ryan Roberts
@ 2026-09-16 13:08 ` Petr Mladek
1 sibling, 0 replies; 6+ messages in thread
From: Petr Mladek @ 2026-09-16 13:08 UTC (permalink / raw)
To: Ryan Roberts
Cc: Andrew Morton, Steven Rostedt, John Ogness, Sergey Senozhatsky,
Rio, linux-kernel
On Tue 2026-09-15 11:01:38, Ryan Roberts wrote:
> Netconsole registers with CON_NBCON_ATOMIC_UNSAFE, so its write_atomic()
> callback is only usable during nbcon_atomic_flush_unsafe(). Previously
> this was only called by vpanic() if panic_timeout=0 - i.e. if the system
> was configured not to reboot on panic. So if the system was configured
> to reboot on panic, netconsole would never receive the panic logs.
>
> Move vpanic()'s emergency_restart() call to after the call to
> nbcon_atomic_flush_unsafe() to solve this problem. The downside is that
> potentially unsafe operations are now performed prior to
> emergency_restart() which could theoretically reduce the chances of the
> restart succeeding. But we are already in a panic situation so it could
> be argued that everything is best effort already.
The panic() code is like walking on bumpy roads. It tries to be as
safe as possible. IMHO, it has few important tasks:
+ show debug messages
+ create crash dump (optional)
+ safely reboot (optional)
The debug messages are important but some users might prioritize
reboot over the debug messages. It helps when the panic() is rare
(low likely race) and the system is quickly able to handle the load
after the reboot.
This is why we put nbcon_atomic_flush_unsafe() only into the path
which ends with an infinite loop.
> In older kernels (v6.19 and earlier) netconsole is able to print these
> panic logs while the system is configured to reboot on panic. So from a
> user perspective this is a regression caused by commit 7eab73b18630
> ("netconsole: convert to NBCON console infrastructure").
>
> We have automated test systems without BMC access, which rely on these
> two features working together.
I see. To be precise, I would say that the primary culprit is the
commit 187de7c212e5fa87 ("printk: nbcon: Allow unsafe write_atomic()
for panic"). It adds support for consoles which will be flushed
in panic() only by nbcon_atomic_flush_unsafe().
It is a big difference because printk() tries to flush both
legacy and "safe" NBCON consoles directly in panic(). In addition,
they are explicitely flushed by console_flush_on_panic().
console_flush_on_panic() is called more times in panic().
And it tries to flush legacy consoles the unsafe way even
before emergency_restart().
OK, we should close this gap. But I suggest another solution.
We already have the following functions for flushing NBCON
consoles using write_atomic()
+ nbcon_atomic_flush_pending() - flush CON_NBCON consoles
using con->write_atomic() but only when safe
+ nbcon_atomic_flush_unsafe() - flush CON_NBCON consoles
using con->write_atomic() and allow unsafe takeover.
I suggest to add 3rd variant, for example:
+ nbcon_atomic_unsafe_flush_unsafe() - flush
CON_NBCON_ATOMIC_UNSAFE consoles with con->write_atomic()
and allow unsafe takeover.
And I would call this 3rd variant in console_flush_on_panic(),
something like:
void console_flush_on_panic(enum con_flush_mode mode)
{
[...]
printk_get_console_flush_type(&ft);
if (ft.nbcon_atomic) {
nbcon_atomic_flush_pending();
nbcon_atomic_unsafe_flush_unsafe();
}
/* Flush legacy consoles once allowed, even when dangerous. */
if (legacy_allow_panic_sync)
console_flush_all(false, &next_seq, &handover);
}
But maybe, this is over engineered and we should just call
nbcon_atomic_flush_unsafe() in console_flush_on_panic(), like:
void console_flush_on_panic(enum con_flush_mode mode)
{
[...]
printk_get_console_flush_type(&ft);
if (ft.nbcon_atomic) {
nbcon_atomic_flush_pending();
nbcon_atomic_flush_unsafe();
}
/* Flush legacy consoles once allowed, even when dangerous. */
if (legacy_allow_panic_sync)
console_flush_all(false, &next_seq, &handover);
}
If there there are people really concerned about the reboot
reliability then we might need to make it configurable, e.g.
by adding reliable_panic command line option which would skip
the unsafe flush before reboot.
See below.
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -732,32 +732,22 @@ void vpanic(const char *fmt, va_list args)
> mdelay(PANIC_TIMER_STEP);
> }
> }
> - if (panic_timeout != 0) {
> - /*
> - * This will not be a clean reboot, with everything
> - * shutting down. But if there is a chance of
> - * rebooting the system it will be rebooted.
> - */
> - if (panic_reboot_mode != REBOOT_UNDEFINED)
> - reboot_mode = panic_reboot_mode;
> - emergency_restart();
> - }
> + if (panic_timeout == 0) {
> #ifdef __sparc__
> - {
> extern int stop_a_enabled;
> /* Make sure the user can actually press Stop-A (L1-A) */
> stop_a_enabled = 1;
> pr_emerg("Press Stop-A (L1-A) from sun keyboard or send break\n"
> - "twice on console to return to the boot prom\n");
> - }
> + "twice on console to return to the boot prom\n");
> #endif
> #if defined(CONFIG_S390)
> - disabled_wait();
> + disabled_wait();
> #endif
> - pr_emerg("---[ end Kernel panic - not syncing: %s ]---\n", buf);
> + pr_emerg("---[ end Kernel panic - not syncing: %s ]---\n", buf);
>
> - /* Do not scroll important messages printed above */
> - suppress_printk = 1;
> + /* Do not scroll important messages printed above */
> + suppress_printk = 1;
We should not supress printk when emergency_restart() is going to
be called. It might print some error messages which might explain
why it failed, ...
> + }
>
> /*
> * The final messages may not have been printed if in a context that
> @@ -767,6 +757,17 @@ void vpanic(const char *fmt, va_list args)
> console_flush_on_panic(CONSOLE_FLUSH_PENDING);
> nbcon_atomic_flush_unsafe();
>
> + if (panic_timeout != 0) {
> + /*
> + * This will not be a clean reboot, with everything
> + * shutting down. But if there is a chance of
> + * rebooting the system it will be rebooted.
> + */
> + if (panic_reboot_mode != REBOOT_UNDEFINED)
> + reboot_mode = panic_reboot_mode;
> + emergency_restart();
> + }
> +
> local_irq_enable();
> for (i = 0; ; i += PANIC_TIMER_STEP) {
> touch_softlockup_watchdog();
Best Regards,
Petr
^ permalink raw reply [flat|nested] 6+ messages in thread