Re: [PATCH v2 2/3] s390: Implement arch_do_panic

[Bradley Morgan]

Mete,

> -#endif
> -#if defined(CONFIG_S390)
> -	disabled_wait();
> -#endif

removal is fine, this was unreachable anyway. the notifier ran
do_panic() which ends in stop_run(), so nothing ever came back to hit
this disabled_wait().

> +void arch_do_panic(void);
> +#define arch_do_panic arch_do_panic

with the weak function i suggested on patch 1, this whole asm/setup.h
hunk disappears and the patch shrinks to exactly the parts that mean
something:

diff --git a/arch/s390/kernel/ipl.c b/arch/s390/kernel/ipl.c
--- a/arch/s390/kernel/ipl.c
+++ b/arch/s390/kernel/ipl.c
@@ -2115,7 +2115,7 @@ static ssize_t on_panic_store(struct kobject *kobj,
 }
 static struct kobj_attribute on_panic_attr = __ATTR_RW(on_panic);
 
-static void do_panic(void)
+void arch_do_panic(void)
 {
 	lgr_info_log();
 	on_panic_trigger.action->fn(&on_panic_trigger);
@@ -2331,18 +2331,6 @@ static int __init vmcmd_on_poff_setup(char *str)
 }
 __setup("vmpoff=", vmcmd_on_poff_setup);
 
-static int on_panic_notify(struct notifier_block *self,
-			   unsigned long event, void *data)
-{
-	do_panic();
-	return NOTIFY_OK;
-}
-
-static struct notifier_block on_panic_nb = {
-	.notifier_call = on_panic_notify,
-	.priority = INT_MIN,
-};
-
 void __init setup_ipl(void)
 {
 	BUILD_BUG_ON(sizeof(struct ipl_parameter_block) != PAGE_SIZE);
@@ -2375,7 +2363,6 @@ void __init setup_ipl(void)
 		/* We have no info to copy */
 		break;
 	}
-	atomic_notifier_chain_register(&panic_notifier_list, &on_panic_nb);
 }
 
 void __no_stack_protector s390_reset_system(void)
diff --git a/kernel/panic.c b/kernel/panic.c
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -755,9 +755,6 @@ void vpanic(const char *fmt, va_list args)
 		pr_emerg("Press Stop-A (L1-A) from sun keyboard or send break\n"
 			 "twice on console to return to the boot prom\n");
 	}
-#endif
-#if defined(CONFIG_S390)
-	disabled_wait();
 #endif
 	pr_emerg("---[ end Kernel panic - not syncing: %s ]---\n", buf);
 
same deal, suggestion only, no SoB from me, fold it in under yours.
check whether linux/panic_notifier.h is still needed in ipl.c after
the notifier goes away.

> This now means that the panic_timeout value takes precedence over user
> defined on_panic behavior.

this is the part i want to poke at. distros routinely ship panic=N on
the cmdline. with this change, anyone who configured on_panic=dump on
such a system silently stops getting dumps, because
emergency_restart() runs first and never returns. a reboot instead of
the dump you explicitly asked for is strictly worse. if you and Sven
want that semantic, fine, but spell it out properly and give the s390
docs for on_panic a line about the new precedence.

also worth saying in the changelog what this actually wins: today the
INT_MIN notifier never returns, so sys_info, kmsg_dump, the post
notifier crash_kexec path, the console flushes and the end banner
never execute on s390 once on_panic fires. after this patch they all
do. thats the headline and the changelog undersells it.

> Fixes: ff6b8ea68f4b ("[S390] ipl/dump on panic.")

What's broken? What's buggy? I'm sure this is just a behaviour 
change, could you either

1: Drop the fixes tag
2: Put in the changelog What's broken and why?


s/now allow/now allows/ in the changelog..
Thanks!