Re: [PATCH v2 1/3] panic: Introduce arch_do_panic

[Bradley Morgan]

Hi Mete,

> +#ifndef arch_do_panic
> +#define arch_do_panic arch_do_panic
> +static inline void arch_do_panic(void) {}
> +#endif

this is fragile. the arch override only kicks in if whatever header
defines the macro happens to be in panic.c's include chain. if it ever
falls out, the empty stub wins silently, the arch version still
compiles as a global nobody calls, and the hook is just dead. no build
break, no warning, nothing.

use a weak function instead. arch defines the strong symbol and thats
it, no macro dance, no header placement contract, and the silent stub
failure cant happen. nobody cares about an indirect call in the panic
path. something like:

diff --git a/include/linux/panic.h b/include/linux/panic.h
--- a/include/linux/panic.h
+++ b/include/linux/panic.h
@@ -13,6 +13,7 @@ __printf(1, 2)
 void panic(const char *fmt, ...) __noreturn __cold;
 __printf(1, 0)
 void vpanic(const char *fmt, va_list args) __noreturn __cold;
+void arch_do_panic(void);
 void nmi_panic(struct pt_regs *regs, const char *msg);
 void check_panic_on_warn(const char *origin);
 extern void oops_enter(void);
diff --git a/kernel/panic.c b/kernel/panic.c
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -567,6 +567,11 @@ static void panic_other_cpus_shutdown(bool crash_kexec)
 		crash_smp_send_stop();
 }
 
+/* Overridden by architectures with a panic handler of last resort. */
+void __weak arch_do_panic(void)
+{
+}
+
 /**
  * vpanic - halt the system
  * @fmt: The text string to print
@@ -756,6 +761,7 @@ void vpanic(const char *fmt, va_list args)
 #endif
 	pr_emerg("---[ end Kernel panic - not syncing: %s ]---\n", buf);
 
+	arch_do_panic();
 	/* Do not scroll important messages printed above */
 	suppress_printk = 1;
 
untested, and no SoB from me, its a suggestion. if you fold it into v3
its your patch under your own SoB as usual.


(Heh, hope my client don't mutilate this)

the prototype in panic.h also means the arch definitions dont
trip -Wmissing-prototypes, which your version dodges with the macro but
only as long as the header keeps getting pulled in.

> 	pr_emerg("---[ end Kernel panic - not syncing: %s ]---\n", buf);
> 
> +	arch_do_panic();

one more thing on the call site: arch code now runs (and prints, on
sparc) after the "end Kernel panic" marker, which until now was the
last line anyone would ever see. tools grep for that as a terminal
marker. probably fine, but say so in the changelog instead of leaving
it implicit.

s/already has/already have/ in the changelog while your at it.

Thanks!