* Sai Praneeth Prakhya <sai.praneeth.prakhya@xxxxxxxxx> wrote:
> +/**
> + * 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)
> +{
> + phys_addr_t start, end;
> + unsigned long size = new_memmap.nr_map * new_memmap.desc_size;
> + unsigned int order = get_order(size);
> +
> + start = new_memmap.phys_map;
> + end = start + size;
> + if (new_memmap.late) {
> + __free_pages(pfn_to_page(PHYS_PFN(start)), order);
> + return;
> + }
> +
> + if (memblock_free(start, size))
> + pr_err("Failed to free mem from %pa to %pa\n", &start, &end);
Why is this rather large structure passed in by value and not by
reference?
Also, 'new_memmap' naming is confusing - by this time this is a rather
old memmap table we are going to free, right? So naming it 'memmap' would
be fine, right?
In a similar vein the 'old_memmap' function parameter in
efi_memmap_insert() should probably be renamed to 'memmap' too, in a
separate patch. Also, I find efi_memmap_insert() rather confusing to read
- would it be possible to clean it up? Here's the various problems I can
see:
- 'new' is rather confusingly named, and because it's a void its exact
purpose and role is not clear.
- m_start/end_attr could be renamed to mem_start/end/attr - there's very
little point in abbreviating that.
- All around the names do not help readability. Why is the new range
being inserted just named 'mem'? Why is the descriptor table we insert
it into named old_memmap but the actual *new* descriptor table passed
in via an opaque, misleadingly named void pointer named 'buf'?
etc.
This function needs a lot of help to make it reviewable easily, before we
complicate the EFI memory descriptor table code with freeing logic...
Thanks,
Ingo
