Re: [PATCH v5 11/31] powernv/fadump: add fadump support on powernv

From: Hari Bathini <hbathini@linux.ibm.com>
To: Michael Ellerman <mpe@ellerman.id.au>,
	linuxppc-dev <linuxppc-dev@ozlabs.org>
Cc: Ananth N Mavinakayanahalli <ananth@linux.ibm.com>,
	Mahesh J Salgaonkar <mahesh@linux.ibm.com>,
	Nicholas Piggin <npiggin@gmail.com>, Oliver <oohall@gmail.com>,
	Vasant Hegde <hegdevasant@linux.ibm.com>,
	Daniel Axtens <dja@axtens.net>
Subject: Re: [PATCH v5 11/31] powernv/fadump: add fadump support on powernv
Date: Tue, 3 Sep 2019 22:01:18 +0530
Message-ID: <69b1eba6-9ca6-864c-7148-4693e4578339@linux.ibm.com> (raw)
In-Reply-To: <87sgpdr5y6.fsf@mpe.ellerman.id.au>

On 03/09/19 4:40 PM, Michael Ellerman wrote:
> Hari Bathini <hbathini@linux.ibm.com> writes:
>> Add basic callback functions for FADump on PowerNV platform.
> 
> I assume this doesn't actually work yet?
> 
> Does something block it from appearing to work at runtime?

With this patch, "fadump=on" would reserve memory for FADump as support is enabled
but registration with f/w is not yet added. So, it would fail to register...

> 
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index d8dcd88..fc4ecfe 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -566,7 +566,7 @@ config CRASH_DUMP
>>  
>>  config FA_DUMP
>>  	bool "Firmware-assisted dump"
>> -	depends on PPC64 && PPC_RTAS
>> +	depends on PPC64 && (PPC_RTAS || PPC_POWERNV)
>>  	select CRASH_CORE
>>  	select CRASH_DUMP
>>  	help
>> @@ -577,7 +577,8 @@ config FA_DUMP
>>  	  is meant to be a kdump replacement offering robustness and
>>  	  speed not possible without system firmware assistance.
>>  
>> -	  If unsure, say "N"
>> +	  If unsure, say "y". Only special kernels like petitboot may
>> +	  need to say "N" here.
>>  
>>  config IRQ_ALL_CPUS
>>  	bool "Distribute interrupts on all CPUs by default"
>> diff --git a/arch/powerpc/kernel/fadump-common.h b/arch/powerpc/kernel/fadump-common.h
>> index d2c5b16..f6c52d3 100644
>> --- a/arch/powerpc/kernel/fadump-common.h
>> +++ b/arch/powerpc/kernel/fadump-common.h
>> @@ -140,4 +140,13 @@ static inline int rtas_fadump_dt_scan(struct fw_dump *fadump_config, ulong node)
>>  }
>>  #endif
>>  
>> +#ifdef CONFIG_PPC_POWERNV
>> +extern int opal_fadump_dt_scan(struct fw_dump *fadump_config, ulong node);
>> +#else
>> +static inline int opal_fadump_dt_scan(struct fw_dump *fadump_config, ulong node)
>> +{
>> +	return 1;
>> +}
> 
> Extending the strange flat device tree calling convention to these
> functions is not ideal.
> 
> It would be better I think if they just returned bool true/false for
> "found it" / "not found", and then early_init_dt_scan_fw_dump() can
> convert that into the appropriate return value.
> 
>> diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
>> index f7c8073..b8061fb9 100644
>> --- a/arch/powerpc/kernel/fadump.c
>> +++ b/arch/powerpc/kernel/fadump.c
>> @@ -114,6 +114,9 @@ int __init early_init_dt_scan_fw_dump(unsigned long node, const char *uname,
>>  	if (strcmp(uname, "rtas") == 0)
>>  		return rtas_fadump_dt_scan(&fw_dump, node);
>>  
>> +	if (strcmp(uname, "ibm,opal") == 0)
>> +		return opal_fadump_dt_scan(&fw_dump, node);
>> +
> 
> ie this would become:
> 
> 	if (strcmp(uname, "ibm,opal") == 0 && opal_fadump_dt_scan(&fw_dump, node))
>             return 1;
> 

Yeah. Will update accordingly...

Thanks
Hari