Re: [RFC v3 1/3] fadump: Refactor and prepare fadump_cma_init for late init

[Madhavan Srinivasan <maddy@linux.ibm.com>]

On 10/11/24 8:30 PM, Ritesh Harjani (IBM) wrote:
> We anyway don't use any return values from fadump_cma_init(). Since
> fadump_reserve_mem() from where fadump_cma_init() gets called today,
> already has the required checks.
> This patch makes this function return type as void. Let's also handle
> extra cases like return if fadump_supported is false or dump_active, so
> that in later patches we can call fadump_cma_init() separately from
> setup_arch().

Usually patches to this file are posted with title format of

powerpc/fadump:<>


> 
> Acked-by: Hari Bathini <hbathini@linux.ibm.com>
> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> ---
> v2 -> v3: Separated the series into 2 as discussed in v2.
> [v2]: https://lore.kernel.org/linuxppc-dev/cover.1728585512.git.ritesh.list@gmail.com/
> 
>  arch/powerpc/kernel/fadump.c | 23 +++++++++--------------
>  1 file changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
> index a612e7513a4f..162327d66982 100644
> --- a/arch/powerpc/kernel/fadump.c
> +++ b/arch/powerpc/kernel/fadump.c
> @@ -78,27 +78,23 @@ static struct cma *fadump_cma;
>   * But for some reason even if it fails we still have the memory reservation
>   * with us and we can still continue doing fadump.
>   */
> -static int __init fadump_cma_init(void)
> +static void __init fadump_cma_init(void)
>  {
>  	unsigned long long base, size;
>  	int rc;
> 
> -	if (!fw_dump.fadump_enabled)
> -		return 0;
> -
> +	if (!fw_dump.fadump_supported || !fw_dump.fadump_enabled ||
> +			fw_dump.dump_active)
> +		return;

Is these checks even needed here? fadump_reserve_mem() checked for all
these already, also dont see any other caller for fadump_cma_init(). 


>  	/*
>  	 * Do not use CMA if user has provided fadump=nocma kernel parameter.
> -	 * Return 1 to continue with fadump old behaviour.
>  	 */
> -	if (fw_dump.nocma)
> -		return 1;
> +	if (fw_dump.nocma || !fw_dump.boot_memory_size)
> +		return;
> 
>  	base = fw_dump.reserve_dump_area_start;
>  	size = fw_dump.boot_memory_size;
> 
> -	if (!size)
> -		return 0;

So this is the only place where we return 0, which in turn will make the
"ret" in fadump_reserve_mem() as zero forcing to call reserve_crashkernel()
in early_init_devtree().

we are removing it, becos we know "size" here will never be zero?


> -
>  	rc = cma_init_reserved_mem(base, size, 0, "fadump_cma", &fadump_cma);
>  	if (rc) {
>  		pr_err("Failed to init cma area for firmware-assisted dump,%d\n", rc);
> @@ -108,7 +104,7 @@ static int __init fadump_cma_init(void)
>  		 * blocked from production system usage.  Hence return 1,
>  		 * so that we can continue with fadump.
>  		 */
> -		return 1;
> +		return;
>  	}
> 
>  	/*
> @@ -125,10 +121,9 @@ static int __init fadump_cma_init(void)
>  		cma_get_size(fadump_cma),
>  		(unsigned long)cma_get_base(fadump_cma) >> 20,
>  		fw_dump.reserve_dump_area_size);
> -	return 1;
>  }
>  #else
> -static int __init fadump_cma_init(void) { return 1; }
> +static void __init fadump_cma_init(void) { }
>  #endif /* CONFIG_CMA */
> 
>  /*
> @@ -638,7 +633,7 @@ int __init fadump_reserve_mem(void)
>  		pr_info("Reserved %lldMB of memory at %#016llx (System RAM: %lldMB)\n",
>  			(size >> 20), base, (memblock_phys_mem_size() >> 20));
> 
> -		ret = fadump_cma_init();
> +		fadump_cma_init();
>  	}
> 
>  	return ret;
> --
> 2.46.0
>