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 <hbath...@linux.ibm.com>
> Signed-off-by: Ritesh Harjani (IBM) <ritesh.l...@gmail.com>
> ---
> v2 -> v3: Separated the series into 2 as discussed in v2.
> [v2]:
> https://lore.kernel.org/linuxppc-dev/cover.1728585512.git.ritesh.l...@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
>
