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...
...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.
I think you get the idea. Can you try this way and let me know if this works?
-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
