Re: [PATCH] powerpc/fadump: update kernel logs before fadump crash begins

[Christophe Leroy <christophe.leroy@csgroup.eu>]

Hi,

Le 06/06/2020 à 06:45, Sourabh Jain a écrit :
> When we hit the fadump crash via the panic path the pstore update is
> missing. This is observed when commit 8341f2f222d7 ("sysrq: Use panic()
> to force a crash") changed the sysrq-trigger to take panic path instead
> of die path.
> 
> The PPC panic event handler addresses the system panic in two different
> ways based on the system configuration. It first allows the FADump (if
> configured) to handle the kernel panic else forwards the call to platform
> specific panic function. Now pstore update is missing only if FADump
> handles the kernel panic, the platform-specific panic function do update
> the pstore by calling panic_flush_kmsg_end function.
> 
> The simplest approach to handle this issue is to add pstore update in PPC
> panic handler before FADump handles the panic. But this leads to multiple
> pstore updates in case FADump is not configured and platform-specific
> panic function serves the kernel panic.
> 
> Hence the function panic_flush_kmsg_end (used by the platform-specific
> panic function to update the kernel logs) is split into two functions, one
> will update the pstore (called in ppc panic event handler) and others will
> flush the kmsg on the console (called in platform specific panic function).

Is this patch still relevant ?

It is still awaiting in patchwork and I see it has produced sparse 
warnings 
(https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20200606044506.44551-1-sourabhjain@linux.ibm.com/)

In addition, it doesn't apply anymore so I'll discard it. Please 
re-submit if still needed.

Christophe



> 
> Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com>
> ---
>   arch/powerpc/include/asm/bug.h         |  2 ++
>   arch/powerpc/kernel/setup-common.c     |  1 +
>   arch/powerpc/kernel/traps.c            | 12 +++++++++++-
>   arch/powerpc/platforms/ps3/setup.c     |  2 +-
>   arch/powerpc/platforms/pseries/setup.c |  2 +-
>   5 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
> index 338f36cd9934..9268551a69bc 100644
> --- a/arch/powerpc/include/asm/bug.h
> +++ b/arch/powerpc/include/asm/bug.h
> @@ -118,6 +118,8 @@ extern void _exception_pkey(struct pt_regs *, unsigned long, int);
>   extern void die(const char *, struct pt_regs *, long);
>   extern bool die_will_crash(void);
>   extern void panic_flush_kmsg_start(void);
> +extern void panic_flush_kmsg_dump(void);
> +extern void panic_flush_kmsg_console(void);
>   extern void panic_flush_kmsg_end(void);
>   #endif /* !__ASSEMBLY__ */
>   
> diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
> index 7f8c890360fe..2d546a9e8bb1 100644
> --- a/arch/powerpc/kernel/setup-common.c
> +++ b/arch/powerpc/kernel/setup-common.c
> @@ -699,6 +699,7 @@ static int ppc_panic_event(struct notifier_block *this,
>   	 * want interrupts to be hard disabled.
>   	 */
>   	hard_irq_disable();
> +	panic_flush_kmsg_dump();
>   
>   	/*
>   	 * If firmware-assisted dump has been registered then trigger
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index 82a3438300fd..bb6bc19992b3 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -169,15 +169,25 @@ extern void panic_flush_kmsg_start(void)
>   	bust_spinlocks(1);
>   }
>   
> -extern void panic_flush_kmsg_end(void)
> +extern void panic_flush_kmsg_dump(void)
>   {
>   	printk_safe_flush_on_panic();
>   	kmsg_dump(KMSG_DUMP_PANIC);
> +}
> +
> +extern void panic_flush_kmsg_console(void)
> +{
>   	bust_spinlocks(0);
>   	debug_locks_off();
>   	console_flush_on_panic(CONSOLE_FLUSH_PENDING);
>   }
>   
> +extern void panic_flush_kmsg_end(void)
> +{
> +	panic_flush_kmsg_dump();
> +	panic_flush_kmsg_console();
> +}
> +
>   static unsigned long oops_begin(struct pt_regs *regs)
>   {
>   	int cpu;
> diff --git a/arch/powerpc/platforms/ps3/setup.c b/arch/powerpc/platforms/ps3/setup.c
> index b29368931c56..f96ba34284a1 100644
> --- a/arch/powerpc/platforms/ps3/setup.c
> +++ b/arch/powerpc/platforms/ps3/setup.c
> @@ -101,7 +101,7 @@ static void ps3_panic(char *str)
>   	printk("   System does not reboot automatically.\n");
>   	printk("   Please press POWER button.\n");
>   	printk("\n");
> -	panic_flush_kmsg_end();
> +	panic_flush_kmsg_console();
>   
>   	while(1)
>   		lv1_pause(1);
> diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
> index 0c8421dd01ab..66ecb88c4b8e 100644
> --- a/arch/powerpc/platforms/pseries/setup.c
> +++ b/arch/powerpc/platforms/pseries/setup.c
> @@ -788,7 +788,7 @@ static void __init pSeries_setup_arch(void)
>   
>   static void pseries_panic(char *str)
>   {
> -	panic_flush_kmsg_end();
> +	panic_flush_kmsg_console();
>   	rtas_os_term(str);
>   }
>