* Baoquan He <b...@redhat.com> wrote:
> +/*
> + * Returns true if mirror region found (and must have been processed
> + * for slots adding)
> + */
> +static bool process_efi_entries(unsigned long minimum,
> + unsigned long image_size)
Also, please don't break the line in the middle of the prototype.
> +{
> + struct efi_info *e = &boot_params->efi_info;
> + bool efi_mirror_found = false;
> + struct mem_vector region;
> + efi_memory_desc_t *md;
> + unsigned long pmap;
> + char *signature;
> + u32 nr_desc;
> + int i;
> +
> + signature = (char *)&boot_params->efi_info.efi_loader_signature;
This is sloppy too: we already have '&boot_params->efi_info' in 'e', why do you
duplicate it again, why not write 'e->efi_loader_signature'??
> + if (strncmp(signature, EFI32_LOADER_SIGNATURE, 4) &&
> + strncmp(signature, EFI64_LOADER_SIGNATURE, 4))
> + return false;
> +
> +#ifdef CONFIG_X86_32
> + /* Can't handle data above 4GB at this time */
> + if (e->efi_memmap_hi) {
> + warn("Memory map is above 4GB, EFI should be disabled.\n");
> + return false;
This kernel warning is pretty passive-aggressive: please explain what the
problem
is and how it can be resolved.
> + }
> + pmap = e->efi_memmap;
> +#else
> + pmap = (e->efi_memmap | ((__u64)e->efi_memmap_hi << 32));
> +#endif
> +
> + nr_desc = e->efi_memmap_size / e->efi_memdesc_size;
> + for (i = 0; i < nr_desc; i++) {
> + md = (efi_memory_desc_t *)(pmap + (i * e->efi_memdesc_size));
This looks unnecessarily obfuscated: why not initialize 'md' to 'pmap' when
pmap
is calculated and just use md[i]?
> +static inline bool process_efi_entries(unsigned long minimum,
> + unsigned long image_size)
ugly linebreak again ...
Thanks,
Ingo
