Re: [PATCH v2 3/3] 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. 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 <mahe657@linux.ibm.com>
> Suggested-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com>
> ---
>  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":
 *       PAPR says use -1 (all ones) to stop all watchdogs.
 */
#define PSERIES_WDT_NUM_ALL	((unsigned long)-1)

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

>  #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.


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