On 09/07/26 22:01, Ritesh Harjani (IBM) wrote:
Sourabh Jain <[email protected]> writes:
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!
Fixes: 69472ffa6575 ("watchdog/pseries-wdt: initial support for H_WATCHDOG-based
watchdog timers")
Reported-by: Mahesh Kumar G <[email protected]>
Signed-off-by: Sourabh Jain <[email protected]>
---
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.c
/*
* 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_WATCHDOG
+
+ rc = plpar_hcall_norets_notrace(H_WATCHDOG, 0x200UL, -1);
- 0x200 is hardcoded.
- -1 is hardcoded.
- I think it's return value is long.
+ if (rc != H_SUCCESS && rc != H_NOOP)
+ pr_warn("crash: failed to stop watchdogs\n");
Let's print rc as well.
+ }
+}
+
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.
...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
-ritesh
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
