On (22/02/24 15:33), Petr Mladek wrote:
> > My bad! I did not spot the `return` at the end of the new branch.
> >
> > + if (console_flush) {
> > + if (panic_print & PANIC_PRINT_ALL_PRINTK_MSG)
> > + console_flush_on_panic(CONSOLE_REPLAY_ALL);
> > + return;
> > + }
> >
> > Hmm. Yeah, well, that's a bit of a tricky interface now
> >
> > panic()
> > // everything (if corresponding bits set), no console flush
> > panic_print_sys_info(false)
> > ...
> > // console flush only if corresponding bit set
> > panic_print_sys_info(true)
>
> I agree that self-explaining names are always better than true/false.
> It is pity that replay the log is handled in panic_print at all.
>
> I sometimes hide these tricks into wrappers. We could rename:
>
> panic_printk_sys_info() -> panic_print_handler()
>
> and add wrappers:
>
> void panic_print_sys_info()
> {
> panic_printk_handler(false);
> }
>
> void panic_print_log_replay()
> {
> panic_printk_handler(true);
> }
>
> Or just split panic_printk_sys_info() into these two functions.
Agreed. I also tend to think that panic_printk_sys_info() is needed anyway,
just because now we do
debug_locks_off();
console_flush_on_panic(CONSOLE_FLUSH_PENDING);
if (panic_print & PANIC_PRINT_ALL_PRINTK_MSG)
console_flush_on_panic(CONSOLE_REPLAY_ALL);
It probably would be better if we do
debug_locks_off();
if (panic_print & PANIC_PRINT_ALL_PRINTK_MSG)
console_flush_on_panic(CONSOLE_REPLAY_ALL);
else
console_flush_on_panic(CONSOLE_FLUSH_PENDING);
instead.
IOW move console_flush_on_panic() handling out of panic_print_sys_info().
console_flush_on_panic() isn't really related to "print sys info" stuff
that panic_print_sys_info() does.
Something like this may be:
---
static void panic_print_sys_info(void)
{
- if (panic_print & PANIC_PRINT_ALL_PRINTK_MSG)
- console_flush_on_panic(CONSOLE_REPLAY_ALL);
-
if (panic_print & PANIC_PRINT_ALL_CPU_BT)
trigger_all_cpu_backtrace();
@@ -196,6 +193,23 @@ static void panic_print_sys_info(void)
ftrace_dump(DUMP_ALL);
}
+static void panic_console_flush(void)
+{
+ /*
+ * We may have ended up stopping the CPU holding the lock (in
+ * smp_send_stop()) while still having some valuable data in the console
+ * buffer. Try to acquire the lock then release it regardless of the
+ * result. The release will also print the buffers out. Locks debug
+ * should be disabled to avoid reporting bad unlock balance when
+ * panic() is not being callled from OOPS.
+ */
+ debug_locks_off();
+ if (panic_print & PANIC_PRINT_ALL_PRINTK_MSG)
+ console_flush_on_panic(CONSOLE_REPLAY_ALL);
+ else
+ console_flush_on_panic(CONSOLE_FLUSH_PENDING);
+}
+
/**
* panic - halt the system
* @fmt: The text string to print
@@ -329,17 +343,7 @@ void panic(const char *fmt, ...)
#endif
console_unblank();
- /*
- * We may have ended up stopping the CPU holding the lock (in
- * smp_send_stop()) while still having some valuable data in the console
- * buffer. Try to acquire the lock then release it regardless of the
- * result. The release will also print the buffers out. Locks debug
- * should be disabled to avoid reporting bad unlock balance when
- * panic() is not being callled from OOPS.
- */
- debug_locks_off();
- console_flush_on_panic(CONSOLE_FLUSH_PENDING);
-
+ panic_console_flush();
panic_print_sys_info();
if (!panic_blink)
---
> > If everyone is fine then OK.
> >
> > But I _personally_ would look into changing this to something like this:
> >
> > #define EARLY_PANIC_MASK (PANIC_PRINT_FOO | PANIC_PRINT_BAR | ...)
> > #define LATE_PANIC_MASK (PANIC_PRINT_ALL_PRINTK_MSG)
>
> These lists cause merge and backporting conflicts. I vote to avoid
> this approach ;-)
OK :)
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
