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

>From the example you gave about efi_esrt_init(), the naming of efi memmap 
related functions does look messy to me now.. and yes, a namespace clean up 
might be good.

Regards,
Sai

Reply via email to