> > +/**
> > + * efi_memmap_free - Free memory pointed by new_memmap.map
> > + * @new_memmap: Structure that describes EFI memory map.
> > + *
> > + * Memory is freed depending on the type of allocation performed.
> > + */
> > +static void __init efi_memmap_free(struct efi_memory_map new_memmap)
>
> ^^ Its not very clear what you mean by the term 'new_memmap' here.
> Also can we pass a pointer to struct efi_memory_map to this function rather
> than passing the entire struct.
Sure! I will change it.
> > +static void __init __efi_memmap_unmap(struct efi_memory_map
> > +new_memmap) {
> > + if (!new_memmap.late) {
> > + unsigned long size;
> > +
> > + size = new_memmap.desc_size * new_memmap.nr_map;
> > + early_memunmap(new_memmap.map, size);
>
> Nitpick: May be we can avoid the extra local variable size and directly pass
> 'new_memmap.desc_size * new_memmap.nr_map' while calling
> early_memunmap(). I think the code would still be readable and the calculation
> seems self-explanatory.
Yes, we could do that but that would make early_memunmap() exceed 80 characters
limit.
Personally, I feel single line code is more readable when compared to split
lines
and this local variable would anyways be gone once the function returns.
> > +/**
> > + * Unmap and free existing EFI Memory Map i.e. efi.memmap */ void
> > +__init efi_memmap_unmap_and_free(void) {
> > + if (!efi_enabled(EFI_MEMMAP)) {
> > + WARN(1, "EFI_MEMMAP is not enabled");
>
> Nitpick: Do we really need a WARN() here? May be we can do away with the
> same.
My thought behind adding a WARN() is to let the developer know that he's using
the
API in wrong scenario i.e. trying to unmap/free memory map when it's not
present.
If we just return, the developer might think that his code isn't buggy when he
tries to
unmap an non-existent EFI memory map.
Regards,
Sai
