* Sai Praneeth Prakhya <sai.praneeth.prak...@intel.com> wrote:

>       if (efi_enabled(EFI_MEMMAP)) {
> +             /*
> +              * efi_clean_memmap() uses memblock_phys_alloc() to allocate
> +              * memory for new EFI memmap and hence will work only after
> +              * e820__memblock_setup()
> +              */
> +             efi_clean_memmap();
>               efi_fake_memmap();
>               efi_find_mirror();
>               efi_esrt_init();

I'd also suggest a namespace cleanup before we do any material 
modifications:

'efi_esrt_init()' is the proper pattern to follow, it's prefixed by efi_, 
then followed by the more generic subsystem (_esrt) and then by the 
functionality (_init). This is a good, hierarchical, top-down 
nomenclature that makes it easy to grep for ESRT functionality by typing 
'git grep esrt'.

The same is not true of the memmap functionality: 'git grep efi_memmap_' 
doesn't do the right thing.

So I think this should be renamed to:

        efi_memmap_clean()
        efi_memmap_insert()
        efi_memmap_free()
        efi_memmap_print()
        efi_memmap_fake()
        etc.

While at it, there's another area that could be improved:

 - efi_memmap_fake() is a bit weird naming: it's not really 'fake', 
   presumably the specified table is still very much real, otherwise it 
   won't result in a working kernel.

   What that functionality *really* is about is user-defined entries. Why 
   not name it in such a fashion? efi_memmap_init_user_defined() or so?

Thanks,

        Ingo

Reply via email to