Re: [PATCH 1/1] powerpc/crash: stop watchdogs before booting kdump kernel

[Ritesh Harjani (IBM) <ritesh.list@gmail.com>]

Sourabh Jain <sourabhjain@linux.ibm.com> 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 <mahe657@linux.ibm.com>
> Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com>
> ---
>  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