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

[Shivang Upadhyay]

Hi Sourabh,

On Tue, 2026-04-07 at 21:00 +0530, Sourabh Jain wrote:
> 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.
> 

Sure. So pstore kmsg_dump can be useful in the cases where we fail to
reboot system with kdump/fadump. Because pstore kmsg dump is collected
during the crashing kernel, it can be *only* information that we
collect from that system.

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

To test for this case, we just made sure that pstore kmsg-dump file's
time stamp was getting updated or not. Currently in fadump's crash path
pstore isn't collecting any logs.

> 
> > 
> > Cc: Hari Bathini <hbathini@xxxxxxxxxxxxx>
> > Cc: Madhavan Srinivasan <maddy@xxxxxxxxxxxxx>
> > Cc: Mahesh Salgaonkar <mahesh@xxxxxxxxxxxxx>
> > Cc: Michael Ellerman <mpe@xxxxxxxxxxxxxx>
> > Cc: Ritesh Harjani (IBM) <ritesh.list@xxxxxxxxx>
> > Reported-by: Shirisha G <shirisha@xxxxxxxxxxxxx>
> > Suggested-by: Sourabh Jain <sourabhjain@xxxxxxxxxxxxx>
> > Signed-off-by: Shivang Upadhyay <shivangu@xxxxxxxxxxxxx>
> > ---
> >   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.
> 
Ack.
> 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.
> 

Sure, ill find a new place to call this.

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

Yeah, the other paths should be covered by this line in crash path here
[1].

> I think it is good to fix this path as well. Thanks for the fix.
> 
> - Sourabh Jain


Thanks,
~Shivang.

[1] http://elixir.bootlin.com/linux/v6.15.6/source/kernel/panic.c#L387