On 13/07/26 10:40, 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. When a panic triggers kdump, the
crashing kernel jumps directly to the kdump kernel without stopping
active watchdogs. As a result, the watchdogs remain active after the
kdump kernel starts.

If dump capture takes longer than the watchdog timeout, PHYP resets the
LPAR before the dump is fully captured, causing dump capture to fail.

Fix this by issuing the `H_WATCHDOG` hcall during the crash shutdown
sequence to stop all active watchdogs before booting the kdump kernel.

Fixes: 69472ffa6575 ("watchdog/pseries-wdt: initial support for H_WATCHDOG-based watchdog timers")
Reported-by: Mahesh Kumar G <redacted>
Suggested-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
Signed-off-by: Sourabh Jain <redacted>
---
  arch/powerpc/include/asm/papr-watchdog.h |  2 ++
  arch/powerpc/platforms/pseries/setup.c   | 18 ++++++++++++++++++
  2 files changed, 20 insertions(+)

diff --git a/arch/powerpc/include/asm/papr-watchdog.h b/arch/powerpc/include/asm/papr-watchdog.h
index fb3a511aa861..84bbe1ddd56f 100644
--- a/arch/powerpc/include/asm/papr-watchdog.h
+++ b/arch/powerpc/include/asm/papr-watchdog.h

@@ -55,4 +55,6 @@
  #define PSERIES_WDTQ_MIN_TIMEOUT(cap)	(((cap) >> 48) & 0xffff)
  #define PSERIES_WDTQ_MAX_NUMBER(cap)	(((cap) >> 32) & 0xffff)
  
+#define PSERIES_WDT_NUM_ALL	((unsigned long)-1)
+

minor nit:

This should be defined at the end of the H_WATCHDOG Input section.
/*
  * H_WATCHDOG Input
  *

<...>

Something like this maybe?

/*
  * R5: "watchdogNumber":

Makes sense. Since this is the third argument to the hypercall,
R5 is the correct register to use in the comment.

  *       PAPR says use -1 (all ones) to stop all watchdogs.
  */
#define PSERIES_WDT_NUM_ALL	((unsigned long)-1)

/*
  * H_WATCHDOG Output
  *
  * R3: Return code
  *
  <...>

quoted

  #endif /* _ASM_POWERPC_CRASHDUMP_PPC64_H */

diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
index bbb2813f8ede..2e40a9dba637 100644
--- a/arch/powerpc/platforms/pseries/setup.c
+++ b/arch/powerpc/platforms/pseries/setup.c

@@ -77,6 +77,7 @@
  #include <asm/dtl.h>
  #include <asm/hvconsole.h>
  #include <asm/setup.h>
+#include <asm/papr-watchdog.h>
  
  #include "pseries.h"
  

@@ -185,6 +186,18 @@ static void __init fwnmi_init(void)
  #endif
  }
  
+#ifdef CONFIG_CRASH_DUMP
+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);
+}
+#endif /* CONFIG_CRASH_DUMP */
+
  /*
   * Affix a device for the first timer to the platform bus if
   * we have firmware support for the H_WATCHDOG hypercall.

@@ -203,6 +216,11 @@ static __init int pseries_wdt_init(void)
  		return PTR_ERR(pseries_wdt_dev);
  	}
  
+#ifdef CONFIG_CRASH_DUMP
+	if (crash_shutdown_register(pseries_crash_stop_watchdogs))
+		pr_warn("Could not register watchdog crash shutdown handler\n");
+#endif
+

minor nit:
I don't think we need any of the #ifdef. All definitions used inside
pseries_crash_stop_watchdogs are already available and
crash_shutdown_register() already exists for !CONFIG_CRASH_DUMP, so we
may as well drop all of the ifdefs.

Yes, the #ifdef is not really needed  because crash_shutdown_register() is
always available.

I removed the #ifdef blocks and built the kernel both with and without
CONFIG_CRASH_DUMP. The kernel built successfully in both cases.

Otherwise LGTM, so feel free to add:
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

Thanks for the review.

- Sourabh Jain