Re: [PATCH 3/4] x86/boot: split parsing of boot_params into a helper function

[Ingo Molnar <mingo@kernel.org>]

* Mike Rapoport <rppt@kernel.org> wrote:

> From: "Mike Rapoport (Microsoft)" <rppt@kernel.org>
> 
> Makes setup_arch a bit easier to comprehend.
> 
> No functional changes.
> 
> Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
> ---
>  arch/x86/kernel/setup.c | 72 +++++++++++++++++++++++------------------
>  1 file changed, 41 insertions(+), 31 deletions(-)
> 
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index d299fe5bda25..d4bb9a2e8f15 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -426,6 +426,42 @@ static void __init parse_setup_data(void)
>  	}
>  }
>  
> +static void __init parse_boot_params(void)
> +{
> +	ROOT_DEV = old_decode_dev(boot_params.hdr.root_dev);
> +	screen_info = boot_params.screen_info;
> +	edid_info = boot_params.edid_info;
> +#ifdef CONFIG_X86_32
> +	apm_info.bios = boot_params.apm_bios_info;
> +	ist_info = boot_params.ist_info;
> +#endif
> +	saved_video_mode = boot_params.hdr.vid_mode;
> +	bootloader_type = boot_params.hdr.type_of_loader;
> +	if ((bootloader_type >> 4) == 0xe) {
> +		bootloader_type &= 0xf;
> +		bootloader_type |= (boot_params.hdr.ext_loader_type+0x10) << 4;
> +	}
> +	bootloader_version  = bootloader_type & 0xf;
> +	bootloader_version |= boot_params.hdr.ext_loader_ver << 4;
> +
> +#ifdef CONFIG_BLK_DEV_RAM
> +	rd_image_start = boot_params.hdr.ram_size & RAMDISK_IMAGE_START_MASK;
> +#endif
> +#ifdef CONFIG_EFI
> +	if (!strncmp((char *)&boot_params.efi_info.efi_loader_signature,
> +		     EFI32_LOADER_SIGNATURE, 4)) {
> +		set_bit(EFI_BOOT, &efi.flags);
> +	} else if (!strncmp((char *)&boot_params.efi_info.efi_loader_signature,
> +		     EFI64_LOADER_SIGNATURE, 4)) {
> +		set_bit(EFI_BOOT, &efi.flags);
> +		set_bit(EFI_64BIT, &efi.flags);
> +	}
> +#endif
> +
> +	if (!boot_params.hdr.root_flags)
> +		root_mountflags &= ~MS_RDONLY;
> +}
> +
>  static void __init memblock_x86_reserve_range_setup_data(void)
>  {
>  	struct setup_indirect *indirect;
> @@ -803,35 +839,11 @@ void __init setup_arch(char **cmdline_p)
>  
>  	setup_olpc_ofw_pgd();
>  
> -	ROOT_DEV = old_decode_dev(boot_params.hdr.root_dev);
> -	screen_info = boot_params.screen_info;
> -	edid_info = boot_params.edid_info;
> -#ifdef CONFIG_X86_32
> -	apm_info.bios = boot_params.apm_bios_info;
> -	ist_info = boot_params.ist_info;
> -#endif
> -	saved_video_mode = boot_params.hdr.vid_mode;
> -	bootloader_type = boot_params.hdr.type_of_loader;
> -	if ((bootloader_type >> 4) == 0xe) {
> -		bootloader_type &= 0xf;
> -		bootloader_type |= (boot_params.hdr.ext_loader_type+0x10) << 4;
> -	}
> -	bootloader_version  = bootloader_type & 0xf;
> -	bootloader_version |= boot_params.hdr.ext_loader_ver << 4;
> -
> -#ifdef CONFIG_BLK_DEV_RAM
> -	rd_image_start = boot_params.hdr.ram_size & RAMDISK_IMAGE_START_MASK;
> -#endif
> -#ifdef CONFIG_EFI
> -	if (!strncmp((char *)&boot_params.efi_info.efi_loader_signature,
> -		     EFI32_LOADER_SIGNATURE, 4)) {
> -		set_bit(EFI_BOOT, &efi.flags);
> -	} else if (!strncmp((char *)&boot_params.efi_info.efi_loader_signature,
> -		     EFI64_LOADER_SIGNATURE, 4)) {
> -		set_bit(EFI_BOOT, &efi.flags);
> -		set_bit(EFI_64BIT, &efi.flags);
> -	}
> -#endif
> +	/*
> +	 * Translate the fields of struct boot_param into global variables

s/struct boot_param
 /'struct boot_param'

> +	 * represting these parameters.

Typo...

> +	 */
> +	parse_boot_params();

Also, why not move this explanatory comment to the definition of the new 
helper function?

Thanks,

	Ingo