Re: [PATCH] ppc/fadump: invoke kmsg_dump in fadump panic path

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

Hello Shivang,

On 07/04/26 11:13, Shivang Upadhyay wrote:
> fadump is registered in panic_notifier_list and gets triggered
> before kmsg_dump_desc() in the panic path. As a result,
> kmsg_dumpers (e.g. pstore) are not executed.

Can you emphasize why it is important to call kmsg_dump_desc()?
Because fadump captures the full memory dump anyway.


> Invoke kmsg_dump_desc() from the fadump handler to ensure
> kmsg_dumpers are called during panic.

Can you please add some command output from before and after
this patch series? It will make it easier to understand the impact of
the changes introduced.


>
> Cc: Hari Bathini <hbathini@linux.ibm.com>
> Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
> Cc: Mahesh Salgaonkar <mahesh@linux.ibm.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> Reported-by: Shirisha G <shirisha@linux.ibm.com>
> Suggested-by: Sourabh Jain <sourabhjain@linux.ibm.com>
> Signed-off-by: Shivang Upadhyay <shivangu@linux.ibm.com>
> ---
>   arch/powerpc/kernel/setup-common.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
> index b1761909c23f..c329b071643d 100644
> --- a/arch/powerpc/kernel/setup-common.c
> +++ b/arch/powerpc/kernel/setup-common.c
> @@ -68,6 +68,7 @@
>   #include <asm/kasan.h>
>   #include <asm/mce.h>
>   #include <asm/systemcfg.h>
> +#include <linux/kmsg_dump.h>
>   
>   #include "setup.h"
>   
> @@ -748,6 +749,8 @@ static int ppc_panic_fadump_handler(struct notifier_block *this,
>   	 * If firmware-assisted dump has been registered then trigger
>   	 * its callback and let the firmware handles everything else.
>   	 */
> +	kmsg_dump_desc(KMSG_DUMP_PANIC, (char *)ptr);

Can you move the kmsg_dump_desc() function before the comment?
The comment was for crash_fadump() function.

Yes, panic() passes the reason for the panic as a string via ptr. So it 
is good
to pass the same to the message dumpers.

As sashiko suggested:
https://sashiko.dev/#/patchset/20260407054341.308710-1-shivangu%40linux.ibm.com 


I think we should call kmsg_dump_desc() only when fadump is registered, 
to avoid calling
it multiple times. If fadump is not registered, this can be called twice 
in the panic path,
first here and then again in the vpanic() function. Checkout 
should_fadump_crash() function.

The second point from Sashiko is about introducing something like 
crash_kexec_post_notifiers
to give users the option to call message dumpers on the vpanic path, or 
on the other two
paths as well (system reset and die) when fadump is configured.

I think this is not really needed because there are already existing 
crash paths hitting crash_fadump
(system reset and die) on which kmsg_dump_desc() is called directly.

> +
>   	crash_fadump(NULL, ptr);
>

As of now, crash_fadump() (which initiates memory dump
collection) is called through three different paths:

1. System Reset
2. die/oops
3. panic

We call kmsg_dump() -> kmsg_dump_desc() to invoke all message
dumpers on the system reset and die/oops paths. The only
remaining path where kmsg_dump_desc() is not called is the
panic path.

panic() -> vpanic() -> panic notifier call chain -> crash_fadump()

I think it is good to fix this path as well. Thanks for the fix.

- Sourabh Jain