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

[Mete Durlu]

On 30/07/2026 13:54, Heiko Carstens wrote:
> 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@xxxxxxxxxxxxx>
> > Signed-off-by: Mete Durlu <meted@xxxxxxxxxxxxx>
> > ---
> >   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.

panic timeout can be set during boot or compile time as you said, so it
can be used to determine what will happen to a system if it panics
during boot along with after boot. Since panic timeout covers a larger
area I thought it should get precedence.
Being able to choose what will happen on panic before boot is a super
power IMO and would help immensely if one would like to boot an untested
kernel via kexec for example.

> 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.

For that to happen users have to "misconfigure" the system and try to
use both panic_timeout and a custom "on_panic" action. The same goes for
kdump, when kdump is configured "on_panic" actions are ignored silently
and system always dumps on panic.

> 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.

This I wasn't aware of, I don't understand how it can deadlock on
console messages. I will look this up.

> 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.

ifdef'ed code for s390 is unreachable as the code called by panic
notifiers aka "on_panic_trigger" do the same regardless of the
action it is configured to and stops with disabled_wait().

If that should be the way, I can remove the ifdef s390 chunk
from vpanic and propose the panic_timeout changes in a separate
patch.

> > 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.

My thought process was to put some marker to dmesg for users
to figure out what happened and why their system didn't respect
to their "on_panic" action. I know this is subobtimal but
simply failing the write to "on_panic" doesn't provide a good
solution IMO as users can first write to "on_panic" and then
set a value to panic_timeout from sysctl.

Regardless, I think I can remove semantic changes for s390
from this patchset and submit them from a different patch series
like you suggested.

Thank you for your input Heiko!