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

[Heiko Carstens <hca@linux.ibm.com>]

On Thu, Jul 30, 2026 at 11:23:28AM +0200, Mete Durlu wrote:
> s390 has a custom panic handler which carries out user specified actions
> during a panic scenario. This handler is invoked via the panic_notifier
> call chain and executed before panic_timeout value is evaluated in
> common code.
> 
> Use arch_do_panic() hook to invoke arch specific panic handling instead
> of using panic_notifier call chain. By reordering s390's panic handler
> allow more information to be printed during a panic.
> The execution order of panic handlers now allows for user specified
> panic_timeout value to be taken into account. This fixes the broken
> "panic" kernel parameter for s390, earlier it was just ignored
> inexplicibly.
> 
> This now means that the panic_timeout value takes precedence over user
> defined on_panic behavior defined via "chshut" or writing to
> /sys/firmware/shutdown_actions/on_panic.
> 
> Fixes: ff6b8ea68f4b ("[S390] ipl/dump on panic.")
> Suggested-by: Sven Schnelle <svens@linux.ibm.com>
> Signed-off-by: Mete Durlu <meted@linux.ibm.com>
> ---
>  arch/s390/kernel/ipl.c | 19 +++++--------------
>  kernel/panic.c         |  3 ---
>  2 files changed, 5 insertions(+), 17 deletions(-)

So, finally I took a closer look :)

Question: why is it desirable that panic_timeout takes precedence? The result
of this change is quite surprising: if anybody (e.g. a distribution) sets
CONFIG_PANIC_TIMEOUT to a non-zero value this completely breaks "on_panic"
behaviour on s390.

I could understand if this change would result in a larger timeout and
additional information being printed, but not that it breaks existing and
actually designed and desired behaviour.

This change also makes it more likely that the system deadlocks on console
messages, before the actual arch_do_panic() is called, if I'm not mistaken.
Which would also be a regression.

What I like about this patch set is that it removes architecture dependent
ifdefs from common code. But the side effects are very questionable.

The "obvious" cleanup would be to move only the existing ifdef'ed code
into arch_do_panic(), and only then provide semantical changes, which
wouldn't need to be part of such a cleanup series.

> diff --git a/arch/s390/kernel/ipl.c b/arch/s390/kernel/ipl.c
> index 3c346b02ceb9..6a5fa9213450 100644
> --- a/arch/s390/kernel/ipl.c
> +++ b/arch/s390/kernel/ipl.c
> @@ -2111,11 +2111,15 @@ static ssize_t on_panic_store(struct kobject *kobj,
>  			      struct kobj_attribute *attr,
>  			      const char *buf, size_t len)
>  {
> +	if (panic_timeout) {
> +		pr_warn("on_panic action will be ignored in favor of panic timeout (panic=%d)",
> +			panic_timeout);
> +	}
>  	return set_trigger(buf, &on_panic_trigger, len);
>  }

I'm wondering why AI doesn't complain about this user trigger-able warning
message. This is not good. *If* we go this way, writing to this attribute
should simply fail, instead of giving the user the impression that something
has been configured, which would actually do something.