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

[Sourabh Jain <sourabhjain@linux.ibm.com>]

On 13/07/26 10:40, Ritesh Harjani (IBM) wrote:
> 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":

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