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

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

On 09/07/26 22:01, 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. 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...

That's a good idea. I wasn't aware of this crash handler.

The main reason I wanted to stop the watchdog as soon as the kernel
enters the architecture-specific crash code is that, on PowerPC, the
crash path sends IPIs to all other CPUs and waits for their response
before continuing. Because of this, I thought it would be better to
stop the watchdog as early as possible.

I knew there was an IPI timeout, but I just checked and it's set to
10 seconds. See crash_kexec_prepare_cpus() in crash.c.

The crash handler is called after the IPI wait. So, in theory, the watchdog
timeout could occur before the IPI timeout. But I think that's a very 
unlikely
scenario, though. So I think disabling the watchdog from the crash handler
is a reasonable approach.

Please share your thoughts.

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

Yes, it is better to keep the watchdog definitions in a common header 
instead
of duplicating them in multiple places.

> I think you get the idea. Can you try this way and let me know if this works?

Sure. Thanks for the review.

- Sourabh Jain

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