Re: [PATCH 1/1] powerpc/crash: stop watchdogs before booting kdump kernel
From: Sourabh Jain <hidden>
Date: 2026-07-10 06:09:08
Also in:
stable
On 09/07/26 22:01, Ritesh Harjani (IBM) wrote:
Sourabh Jain [off-list ref] writes:quoted
On pseries LPAR systems, watchdog timers configured from userspace can remain active after a kernel panic. During panic triggered crash dump capture, the crashing kernel jumps directly to the kdump kernel without shutting down userspace services. As a result, active watchdogs are not stopped before entering the kdump kernel. If dump capture takes longer than the watchdog timeout, PHYP resets the LPAR before dump collection completes, resulting in dump capture failure. Fix this by issuing the H_WATCHDOG hcall on the crash shutdown path to stop all active watchdogs before booting the kdump kernel.Nice catch!quoted
Fixes: 69472ffa6575 ("watchdog/pseries-wdt: initial support for H_WATCHDOG-based watchdog timers") Reported-by: Mahesh Kumar G <redacted> Signed-off-by: Sourabh Jain <redacted> --- arch/powerpc/kexec/crash.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)diff --git a/arch/powerpc/kexec/crash.c b/arch/powerpc/kexec/crash.c index e6539f213b3d..5651523e3a70 100644 --- a/arch/powerpc/kexec/crash.c +++ b/arch/powerpc/kexec/crash.c@@ -28,6 +28,7 @@ #include <asm/interrupt.h> #include <asm/kexec_ranges.h> #include <asm/crashdump-ppc64.h> +#include <asm/hvcall.h>would be nice, if we could avoid papr specific header into common crash.cquoted
/* * The primary CPU waits a while for all secondary CPUs to enter. This is to@@ -352,6 +353,28 @@ int crash_shutdown_unregister(crash_shutdown_t handler) } EXPORT_SYMBOL(crash_shutdown_unregister); +/** + * stop_watchdogs - Stop active watchdogs before entering kdump kernel + * On pseries LPAR systems, watchdogs configured from userspace remain + * active after a kernel panic because userspace services are not shut + * down on the kdump crash path. If a watchdog expires while the kdump + * kernel is collecting the dump, PHYP resets the LPAR and dump capture + * fails + * + * 0x200UL : watchdog stop operation + * -1 : watchdog number, disable all watchdogs + */ +static void stop_watchdogs(void) +{ + if (firmware_has_feature(FW_FEATURE_LPAR)) { + int rc;ditto. Also I guess this could be FW_FEATURE_WATCHDOGquoted
+ + rc = plpar_hcall_norets_notrace(H_WATCHDOG, 0x200UL, -1);- 0x200 is hardcoded. - -1 is hardcoded. - I think it's return value is long.quoted
+ if (rc != H_SUCCESS && rc != H_NOOP) + pr_warn("crash: failed to stop watchdogs\n");Let's print rc as well.quoted
+ } +} +Looking at the code, we already have a mechanism to register a crash shutdown handler which anyways is getting called from default_machine_crash_shutdown(). So, I think we could use this generic crash handler register mechanism and keep the wdt specific calls within pseries/setup.c file...
That's a good idea. I wasn't aware of this crash handler. The main reason I wanted to stop the watchdog as soon as the kernel enters the architecture-specific crash code is that, on PowerPC, the crash path sends IPIs to all other CPUs and waits for their response before continuing. Because of this, I thought it would be better to stop the watchdog as early as possible. I knew there was an IPI timeout, but I just checked and it's set to 10 seconds. See crash_kexec_prepare_cpus() in crash.c. The crash handler is called after the IPI wait. So, in theory, the watchdog timeout could occur before the IPI timeout. But I think that's a very unlikely scenario, though. So I think disabling the watchdog from the crash handler is a reasonable approach. Please share your thoughts.
quoted hunk ↗ jump to hunk
...How about something like this?diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c index 50b26ed8432d..4e557694d724 100644 --- a/arch/powerpc/platforms/pseries/setup.c +++ b/arch/powerpc/platforms/pseries/setup.c@@ -59,6 +59,7 @@ #include <asm/xics.h> #include <asm/xive.h> #include <asm/papr-sysparm.h> +#include <asm/papr-watchdog.h> #include <asm/ppc-pci.h> #include <asm/i8259.h> #include <asm/udbg.h>@@ -185,14 +186,42 @@ static void __init fwnmi_init(void) #endif }<...> +static void pseries_crash_stop_watchdogs(void) +{ + long rc; + + rc = plpar_hcall_norets_notrace(H_WATCHDOG, PSERIES_WDTF_OP_STOP, + PSERIES_WDT_NUM_ALL); + if (rc != H_SUCCESS && rc != H_NOOP) + pr_warn("Could not stop watchdogs before kdump rc=%ld\n", rc); +} + /* * Affix a device for the first timer to the platform bus if * we have firmware support for the H_WATCHDOG hypercall. */ static __init int pseries_wdt_init(void) { - if (firmware_has_feature(FW_FEATURE_WATCHDOG)) - platform_device_register_simple("pseries-wdt", 0, NULL, 0); + if (!firmware_has_feature(FW_FEATURE_WATCHDOG)) + return 0; + + platform_device_register_simple("pseries-wdt", 0, NULL, 0); + + if (crash_shutdown_register(pseries_crash_stop_watchdogs)) + pr_warn("Could not register watchdog crash shutdown handler\n"); + return 0; } machine_subsys_initcall(pseries, pseries_wdt_init); Note that I added papr-watchdog.h header file in above. I am guessing we can move some definitions from drivers/watchdog/pseries-wdt.c to arch/powerpc/include/asm/papr-watchdog.h in a separate patch before this change.
Yes, it is better to keep the watchdog definitions in a common header instead of duplicating them in multiple places.
I think you get the idea. Can you try this way and let me know if this works?
Sure. Thanks for the review. - Sourabh Jain
-riteshquoted
void default_machine_crash_shutdown(struct pt_regs *regs) { volatile unsigned int i;@@ -360,6 +383,8 @@ void default_machine_crash_shutdown(struct pt_regs *regs) if (TRAP(regs) == INTERRUPT_SYSTEM_RESET) is_via_system_reset = 1; + stop_watchdogs(); + if (IS_ENABLED(CONFIG_SMP)) crash_smp_send_stop(); else-- 2.52.0