[RFC,1/2] efi_loader: define internal implementations of install/uninstallmultiple

A following patch is cleaning up the core EFI code trying to remove
sequences of efi_create_handle, efi_add_protocol.

Although this works fine there's a problem with the latter since it is
usually combined with efi_delete_handle() which blindly removes all
protocols on a handle and deletes the handle.  We should try to adhere to
the EFI spec which only deletes a handle if the last instance of a protocol
has been removed.  So let's fix this by replacing all callsites of
efi_create_handle(), efi_add_protocol() , efi_delete_handle() with
Install/UninstallMultipleProtocol.

In order to do that redefine functions that can be used by the U-Boot
proper internally and add '_ext' variants that will be used from the
EFI API

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 include/efi.h                    |   2 +
 include/efi_loader.h             |   6 +-
 lib/efi_loader/efi_boottime.c    | 180 ++++++++++++++++++++++++-------
 lib/efi_loader/efi_capsule.c     |  15 +--
 lib/efi_loader/efi_console.c     |  14 +--
 lib/efi_loader/efi_disk.c        |  10 +-
 lib/efi_loader/efi_load_initrd.c |  15 ++-
 lib/efi_loader/efi_root_node.c   |  44 ++++----
 8 files changed, 197 insertions(+), 89 deletions(-)

Hi Ilias,

On Wed, Oct 05, 2022 at 06:26:01PM +0300, Ilias Apalodimas wrote:
> A following patch is cleaning up the core EFI code trying to remove
> sequences of efi_create_handle, efi_add_protocol.
> 
> Although this works fine there's a problem with the latter since it is
> usually combined with efi_delete_handle() which blindly removes all
> protocols on a handle and deletes the handle.  We should try to adhere to
> the EFI spec which only deletes a handle if the last instance of a protocol
> has been removed.  So let's fix this by replacing all callsites of
> efi_create_handle(), efi_add_protocol() , efi_delete_handle() with
> Install/UninstallMultipleProtocol.
> 
> In order to do that redefine functions that can be used by the U-Boot
> proper internally and add '_ext' variants that will be used from the
> EFI API

Our practice so far is to use '_int' postfix for internal interfaces
and not use any frill for external interfaces.

-Takahiro Akashi

> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>  include/efi.h                    |   2 +
>  include/efi_loader.h             |   6 +-
>  lib/efi_loader/efi_boottime.c    | 180 ++++++++++++++++++++++++-------
>  lib/efi_loader/efi_capsule.c     |  15 +--
>  lib/efi_loader/efi_console.c     |  14 +--
>  lib/efi_loader/efi_disk.c        |  10 +-
>  lib/efi_loader/efi_load_initrd.c |  15 ++-
>  lib/efi_loader/efi_root_node.c   |  44 ++++----
>  8 files changed, 197 insertions(+), 89 deletions(-)
> 
> diff --git a/include/efi.h b/include/efi.h
> index 6159f34ad2be..42f4e58a917e 100644
> --- a/include/efi.h
> +++ b/include/efi.h
> @@ -37,12 +37,14 @@
>  #define EFIAPI __attribute__((ms_abi))
>  #define efi_va_list __builtin_ms_va_list
>  #define efi_va_start __builtin_ms_va_start
> +#define efi_va_copy __builtin_ms_va_copy
>  #define efi_va_arg __builtin_va_arg
>  #define efi_va_end __builtin_ms_va_end
>  #else
>  #define EFIAPI asmlinkage
>  #define efi_va_list va_list
>  #define efi_va_start va_start
> +#define efi_va_copy va_copy
>  #define efi_va_arg va_arg
>  #define efi_va_end va_end
>  #endif /* __x86_64__ */
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index ad01395b39c3..2b294d64efd0 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -655,8 +655,10 @@ efi_status_t efi_remove_protocol(const efi_handle_t handle,
>  /* Delete all protocols from a handle */
>  efi_status_t efi_remove_all_protocols(const efi_handle_t handle);
>  /* Install multiple protocol interfaces */
> -efi_status_t EFIAPI efi_install_multiple_protocol_interfaces
> -				(efi_handle_t *handle, ...);
> +efi_status_t EFIAPI
> +efi_install_multiple_protocol_interfaces(efi_handle_t *handle, ...);
> +efi_status_t EFIAPI
> +efi_uninstall_multiple_protocol_interfaces(efi_handle_t handle, ...);
>  /* Get handles that support a given protocol */
>  efi_status_t EFIAPI efi_locate_handle_buffer(
>  			enum efi_locate_search_type search_type,
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 1bfd094e89f8..aeb8b27dc676 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -2590,35 +2590,31 @@ found:
>  }
>  
>  /**
> - * efi_install_multiple_protocol_interfaces() - Install multiple protocol
> + * __efi_install_multiple_protocol_interfaces() - Install multiple protocol
>   *                                              interfaces
>   * @handle: handle on which the protocol interfaces shall be installed
> - * @...:    NULL terminated argument list with pairs of protocol GUIDS and
> - *          interfaces
> - *
> - * This function implements the MultipleProtocolInterfaces service.
> + * @argptr: va_list of args
>   *
> - * See the Unified Extensible Firmware Interface (UEFI) specification for
> - * details.
> + * Core functionality of efi_install_multiple_protocol_interfaces
> + * Must not be called directly
>   *
>   * Return: status code
>   */
> -efi_status_t EFIAPI efi_install_multiple_protocol_interfaces
> -				(efi_handle_t *handle, ...)
> +static efi_status_t EFIAPI
> +__efi_install_multiple_protocol_interfaces(efi_handle_t *handle,
> +					   efi_va_list argptr)
>  {
> -	EFI_ENTRY("%p", handle);
> -
> -	efi_va_list argptr;
>  	const efi_guid_t *protocol;
>  	void *protocol_interface;
>  	efi_handle_t old_handle;
>  	efi_status_t r = EFI_SUCCESS;
>  	int i = 0;
> +	efi_va_list argptr_copy;
>  
>  	if (!handle)
> -		return EFI_EXIT(EFI_INVALID_PARAMETER);
> +		return EFI_INVALID_PARAMETER;
>  
> -	efi_va_start(argptr, handle);
> +	efi_va_copy(argptr_copy, argptr);
>  	for (;;) {
>  		protocol = efi_va_arg(argptr, efi_guid_t*);
>  		if (!protocol)
> @@ -2646,52 +2642,103 @@ efi_status_t EFIAPI efi_install_multiple_protocol_interfaces
>  			break;
>  		i++;
>  	}
> -	efi_va_end(argptr);
> -	if (r == EFI_SUCCESS)
> -		return EFI_EXIT(r);
> +	if (r == EFI_SUCCESS) {
> +		efi_va_end(argptr_copy);
> +		return r;
> +	}
>  
>  	/* If an error occurred undo all changes. */
> -	efi_va_start(argptr, handle);
>  	for (; i; --i) {
> -		protocol = efi_va_arg(argptr, efi_guid_t*);
> -		protocol_interface = efi_va_arg(argptr, void*);
> +		protocol = efi_va_arg(argptr_copy, efi_guid_t*);
> +		protocol_interface = efi_va_arg(argptr_copy, void*);
>  		EFI_CALL(efi_uninstall_protocol_interface(*handle, protocol,
>  							  protocol_interface));
>  	}
> -	efi_va_end(argptr);
> +	efi_va_end(argptr_copy);
> +
> +	return r;
>  
> -	return EFI_EXIT(r);
>  }
>  
>  /**
> - * efi_uninstall_multiple_protocol_interfaces() - uninstall multiple protocol
> - *                                                interfaces
> - * @handle: handle from which the protocol interfaces shall be removed
> + * efi_install_multiple_protocol_interfaces() - Install multiple protocol
> + *                                              interfaces
> + * @handle: handle on which the protocol interfaces shall be installed
>   * @...:    NULL terminated argument list with pairs of protocol GUIDS and
>   *          interfaces
>   *
> - * This function implements the UninstallMultipleProtocolInterfaces service.
> + *
> + * This is the function for internal usage in U-Boot. For the API function
> + * implementing the InstallMultipleProtocol service see
> + * efi_install_multiple_protocol_interfaces_ext()
> + *
> + * Return: status code
> + */
> +efi_status_t EFIAPI
> +efi_install_multiple_protocol_interfaces(efi_handle_t *handle, ...)
> +{
> +	efi_status_t r = EFI_SUCCESS;
> +	efi_va_list argptr;
> +
> +	efi_va_start(argptr, handle);
> +	r = __efi_install_multiple_protocol_interfaces(handle, argptr);
> +	efi_va_end(argptr);
> +	return r;
> +}
> +
> +/**
> + * efi_install_multiple_protocol_interfaces_ext() - Install multiple protocol
> + *                                                  interfaces
> + * @handle: handle on which the protocol interfaces shall be installed
> + * @...:    NULL terminated argument list with pairs of protocol GUIDS and
> + *          interfaces
> + *
> + * This function implements the MultipleProtocolInterfaces service.
>   *
>   * See the Unified Extensible Firmware Interface (UEFI) specification for
>   * details.
>   *
>   * Return: status code
>   */
> -static efi_status_t EFIAPI efi_uninstall_multiple_protocol_interfaces(
> -			efi_handle_t handle, ...)
> +static efi_status_t EFIAPI
> +efi_install_multiple_protocol_interfaces_ext(efi_handle_t *handle, ...)
>  {
>  	EFI_ENTRY("%p", handle);
> -
> +	efi_status_t r = EFI_SUCCESS;
>  	efi_va_list argptr;
> +
> +	efi_va_start(argptr, handle);
> +	r = __efi_install_multiple_protocol_interfaces(handle, argptr);
> +	efi_va_end(argptr);
> +	return EFI_EXIT(r);
> +}
> +
> +/**
> + * __efi_uninstall_multiple_protocol_interfaces() - wrapper for uninstall
> + *                                                  multiple protocol
> + *                                                  interfaces
> + * @handle: handle from which the protocol interfaces shall be removed
> + * @argptr: va_list of args
> + *
> + * Core functionality of efi_uninstall_multiple_protocol_interfaces
> + * Must not be called directly
> + *
> + * Return: status code
> + */
> +static efi_status_t EFIAPI
> +__efi_uninstall_multiple_protocol_interfaces(efi_handle_t handle,
> +					     efi_va_list argptr)
> +{
>  	const efi_guid_t *protocol;
>  	void *protocol_interface;
>  	efi_status_t r = EFI_SUCCESS;
>  	size_t i = 0;
> +	efi_va_list argptr_copy;
>  
>  	if (!handle)
> -		return EFI_EXIT(EFI_INVALID_PARAMETER);
> +		return EFI_INVALID_PARAMETER;
>  
> -	efi_va_start(argptr, handle);
> +	efi_va_copy(argptr_copy, argptr);
>  	for (;;) {
>  		protocol = efi_va_arg(argptr, efi_guid_t*);
>  		if (!protocol)
> @@ -2703,29 +2750,82 @@ static efi_status_t EFIAPI efi_uninstall_multiple_protocol_interfaces(
>  			break;
>  		i++;
>  	}
> -	efi_va_end(argptr);
>  	if (r == EFI_SUCCESS) {
>  		/* If the last protocol has been removed, delete the handle. */
>  		if (list_empty(&handle->protocols)) {
>  			list_del(&handle->link);
>  			free(handle);
>  		}
> -		return EFI_EXIT(r);
> +		efi_va_end(argptr_copy);
> +		return r;
>  	}
>  
>  	/* If an error occurred undo all changes. */
> -	efi_va_start(argptr, handle);
>  	for (; i; --i) {
> -		protocol = efi_va_arg(argptr, efi_guid_t*);
> -		protocol_interface = efi_va_arg(argptr, void*);
> +		protocol = efi_va_arg(argptr_copy, efi_guid_t*);
> +		protocol_interface = efi_va_arg(argptr_copy, void*);
>  		EFI_CALL(efi_install_protocol_interface(&handle, protocol,
>  							EFI_NATIVE_INTERFACE,
>  							protocol_interface));
>  	}
> -	efi_va_end(argptr);
> +	efi_va_end(argptr_copy);
>  
>  	/* In case of an error always return EFI_INVALID_PARAMETER */
> -	return EFI_EXIT(EFI_INVALID_PARAMETER);
> +	return EFI_INVALID_PARAMETER;
> +}
> +
> +/**
> + * efi_uninstall_multiple_protocol_interfaces() - uninstall multiple protocol
> + *                                                interfaces
> + * @handle: handle from which the protocol interfaces shall be removed
> + * @...:    NULL terminated argument list with pairs of protocol GUIDS and
> + *          interfaces
> + *
> + * This function implements the UninstallMultipleProtocolInterfaces service.
> + *
> + * This is the function for internal usage in U-Boot. For the API function
> + * implementing the UninstallMultipleProtocolInterfaces service see
> + * efi_uninstall_multiple_protocol_interfaces_ext()
> + *
> + * Return: status code
> + */
> +efi_status_t EFIAPI
> +efi_uninstall_multiple_protocol_interfaces(efi_handle_t handle, ...)
> +{
> +	efi_status_t r = EFI_SUCCESS;
> +	efi_va_list argptr;
> +
> +	efi_va_start(argptr, handle);
> +	r = __efi_uninstall_multiple_protocol_interfaces(handle, argptr);
> +	efi_va_end(argptr);
> +	return r;
> +}
> +
> +/**
> + * efi_uninstall_multiple_protocol_interfaces_ext() - uninstall multiple protocol
> + *                                                    interfaces
> + * @handle: handle from which the protocol interfaces shall be removed
> + * @...:    NULL terminated argument list with pairs of protocol GUIDS and
> + *          interfaces
> + *
> + * This function implements the UninstallMultipleProtocolInterfaces service.
> + *
> + * See the Unified Extensible Firmware Interface (UEFI) specification for
> + * details.
> + *
> + * Return: status code
> + */
> +static efi_status_t EFIAPI
> +efi_uninstall_multiple_protocol_interfaces_ext(efi_handle_t handle, ...)
> +{
> +	EFI_ENTRY("%p", handle);
> +	efi_status_t r = EFI_SUCCESS;
> +	efi_va_list argptr;
> +
> +	efi_va_start(argptr, handle);
> +	r = __efi_uninstall_multiple_protocol_interfaces(handle, argptr);
> +	efi_va_end(argptr);
> +	return EFI_EXIT(r);
>  }
>  
>  /**
> @@ -3785,9 +3885,9 @@ static struct efi_boot_services efi_boot_services = {
>  	.locate_handle_buffer = efi_locate_handle_buffer,
>  	.locate_protocol = efi_locate_protocol,
>  	.install_multiple_protocol_interfaces =
> -			efi_install_multiple_protocol_interfaces,
> +			efi_install_multiple_protocol_interfaces_ext,
>  	.uninstall_multiple_protocol_interfaces =
> -			efi_uninstall_multiple_protocol_interfaces,
> +			efi_uninstall_multiple_protocol_interfaces_ext,
>  	.calculate_crc32 = efi_calculate_crc32,
>  	.copy_mem = efi_copy_mem,
>  	.set_mem = efi_set_mem,
> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> index a6b98f066a0b..b6bd2d6af882 100644
> --- a/lib/efi_loader/efi_capsule.c
> +++ b/lib/efi_loader/efi_capsule.c
> @@ -636,17 +636,18 @@ efi_status_t __weak efi_load_capsule_drivers(void)
>  
>  	if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_FIT)) {
>  		handle = NULL;
> -		ret = EFI_CALL(efi_install_multiple_protocol_interfaces(
> -				&handle, &efi_guid_firmware_management_protocol,
> -				&efi_fmp_fit, NULL));
> +		ret = efi_install_multiple_protocol_interfaces(&handle,
> +							       &efi_guid_firmware_management_protocol,
> +							       &efi_fmp_fit,
> +							       NULL);
>  	}
>  
>  	if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_RAW)) {
>  		handle = NULL;
> -		ret = EFI_CALL(efi_install_multiple_protocol_interfaces(
> -				&handle,
> -				&efi_guid_firmware_management_protocol,
> -				&efi_fmp_raw, NULL));
> +		ret = efi_install_multiple_protocol_interfaces(&handle,
> +							       &efi_guid_firmware_management_protocol,
> +							       &efi_fmp_raw,
> +							       NULL);
>  	}
>  
>  	return ret;
> diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
> index cf9fbd9cb54d..3354b217a9a4 100644
> --- a/lib/efi_loader/efi_console.c
> +++ b/lib/efi_loader/efi_console.c
> @@ -1278,12 +1278,14 @@ efi_status_t efi_console_register(void)
>  	struct efi_device_path *dp;
>  
>  	/* Install protocols on root node */
> -	r = EFI_CALL(efi_install_multiple_protocol_interfaces
> -		     (&efi_root,
> -		      &efi_guid_text_output_protocol, &efi_con_out,
> -		      &efi_guid_text_input_protocol, &efi_con_in,
> -		      &efi_guid_text_input_ex_protocol, &efi_con_in_ex,
> -		      NULL));
> +	r = efi_install_multiple_protocol_interfaces(&efi_root,
> +						     &efi_guid_text_output_protocol,
> +						     &efi_con_out,
> +						     &efi_guid_text_input_protocol,
> +						     &efi_con_in,
> +						     &efi_guid_text_input_ex_protocol,
> +						     &efi_con_in_ex,
> +						     NULL);
>  
>  	/* Create console node and install device path protocols */
>  	if (CONFIG_IS_ENABLED(DM_SERIAL)) {
> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> index 39ea1a68a683..fb96b0508528 100644
> --- a/lib/efi_loader/efi_disk.c
> +++ b/lib/efi_loader/efi_disk.c
> @@ -449,10 +449,12 @@ static efi_status_t efi_disk_add_dev(
>  	 * in this case.
>  	 */
>  	handle = &diskobj->header;
> -	ret = EFI_CALL(efi_install_multiple_protocol_interfaces(
> -			&handle, &efi_guid_device_path, diskobj->dp,
> -			&efi_block_io_guid, &diskobj->ops,
> -			guid, NULL, NULL));
> +	ret = efi_install_multiple_protocol_interfaces(&handle,
> +						       &efi_guid_device_path,
> +						       diskobj->dp,
> +						       &efi_block_io_guid,
> +						       &diskobj->ops, guid,
> +						       NULL, NULL);
>  	if (ret != EFI_SUCCESS)
>  		goto error;
>  
> diff --git a/lib/efi_loader/efi_load_initrd.c b/lib/efi_loader/efi_load_initrd.c
> index 3d6044f76047..87fde3f88c2b 100644
> --- a/lib/efi_loader/efi_load_initrd.c
> +++ b/lib/efi_loader/efi_load_initrd.c
> @@ -208,14 +208,13 @@ efi_status_t efi_initrd_register(void)
>  	if (ret != EFI_SUCCESS)
>  		return ret;
>  
> -	ret = EFI_CALL(efi_install_multiple_protocol_interfaces
> -		       (&efi_initrd_handle,
> -			/* initramfs */
> -			&efi_guid_device_path, &dp_lf2_handle,
> -			/* LOAD_FILE2 */
> -			&efi_guid_load_file2_protocol,
> -			(void *)&efi_lf2_protocol,
> -			NULL));
> +	ret = efi_install_multiple_protocol_interfaces(&efi_initrd_handle,
> +						       /* initramfs */
> +						       &efi_guid_device_path, &dp_lf2_handle,
> +						       /* LOAD_FILE2 */
> +						       &efi_guid_load_file2_protocol,
> +						       (void *)&efi_lf2_protocol,
> +						       NULL);
>  
>  	return ret;
>  }
> diff --git a/lib/efi_loader/efi_root_node.c b/lib/efi_loader/efi_root_node.c
> index 739c6867f412..b4696d54c33d 100644
> --- a/lib/efi_loader/efi_root_node.c
> +++ b/lib/efi_loader/efi_root_node.c
> @@ -49,38 +49,38 @@ efi_status_t efi_root_node_register(void)
>  	dp->end.length = sizeof(struct efi_device_path);
>  
>  	/* Create root node and install protocols */
> -	ret = EFI_CALL(efi_install_multiple_protocol_interfaces
> -			(&efi_root,
> -			 /* Device path protocol */
> -			 &efi_guid_device_path, dp,
> +	ret = efi_install_multiple_protocol_interfaces
> +		(&efi_root,
> +		 /* Device path protocol */
> +		 &efi_guid_device_path, dp,
>  #if CONFIG_IS_ENABLED(EFI_DEVICE_PATH_TO_TEXT)
> -			 /* Device path to text protocol */
> -			 &efi_guid_device_path_to_text_protocol,
> -			 (void *)&efi_device_path_to_text,
> +		 /* Device path to text protocol */
> +		 &efi_guid_device_path_to_text_protocol,
> +		 (void *)&efi_device_path_to_text,
>  #endif
>  #ifdef CONFIG_EFI_DEVICE_PATH_UTIL
> -			 /* Device path utilities protocol */
> -			 &efi_guid_device_path_utilities_protocol,
> -			 (void *)&efi_device_path_utilities,
> +		 /* Device path utilities protocol */
> +		 &efi_guid_device_path_utilities_protocol,
> +		 (void *)&efi_device_path_utilities,
>  #endif
>  #ifdef CONFIG_EFI_DT_FIXUP
> -			 /* Device-tree fix-up protocol */
> -			 &efi_guid_dt_fixup_protocol,
> -			 (void *)&efi_dt_fixup_prot,
> +		 /* Device-tree fix-up protocol */
> +		 &efi_guid_dt_fixup_protocol,
> +		 (void *)&efi_dt_fixup_prot,
>  #endif
>  #if CONFIG_IS_ENABLED(EFI_UNICODE_COLLATION_PROTOCOL2)
> -			 &efi_guid_unicode_collation_protocol2,
> -			 (void *)&efi_unicode_collation_protocol2,
> +		 &efi_guid_unicode_collation_protocol2,
> +		 (void *)&efi_unicode_collation_protocol2,
>  #endif
>  #if CONFIG_IS_ENABLED(EFI_LOADER_HII)
> -			 /* HII string protocol */
> -			 &efi_guid_hii_string_protocol,
> -			 (void *)&efi_hii_string,
> -			 /* HII database protocol */
> -			 &efi_guid_hii_database_protocol,
> -			 (void *)&efi_hii_database,
> +		 /* HII string protocol */
> +		 &efi_guid_hii_string_protocol,
> +		 (void *)&efi_hii_string,
> +		 /* HII database protocol */
> +		 &efi_guid_hii_database_protocol,
> +		 (void *)&efi_hii_database,
>  #endif
> -			 NULL));
> +		 NULL);
>  	efi_root->type = EFI_OBJECT_TYPE_U_BOOT_FIRMWARE;
>  	return ret;
>  }
> -- 
> 2.34.1
>

On 10/5/22 17:26, Ilias Apalodimas wrote:
> A following patch is cleaning up the core EFI code trying to remove
> sequences of efi_create_handle, efi_add_protocol.
>
> Although this works fine there's a problem with the latter since it is
> usually combined with efi_delete_handle() which blindly removes all
> protocols on a handle and deletes the handle.  We should try to adhere to
> the EFI spec which only deletes a handle if the last instance of a protocol
> has been removed.  So let's fix this by replacing all callsites of
> efi_create_handle(), efi_add_protocol() , efi_delete_handle() with
> Install/UninstallMultipleProtocol.
>
> In order to do that redefine functions that can be used by the U-Boot
> proper internally and add '_ext' variants that will be used from the
> EFI API
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>   include/efi.h                    |   2 +
>   include/efi_loader.h             |   6 +-
>   lib/efi_loader/efi_boottime.c    | 180 ++++++++++++++++++++++++-------
>   lib/efi_loader/efi_capsule.c     |  15 +--
>   lib/efi_loader/efi_console.c     |  14 +--
>   lib/efi_loader/efi_disk.c        |  10 +-
>   lib/efi_loader/efi_load_initrd.c |  15 ++-
>   lib/efi_loader/efi_root_node.c   |  44 ++++----
>   8 files changed, 197 insertions(+), 89 deletions(-)
>
> diff --git a/include/efi.h b/include/efi.h
> index 6159f34ad2be..42f4e58a917e 100644
> --- a/include/efi.h
> +++ b/include/efi.h
> @@ -37,12 +37,14 @@
>   #define EFIAPI __attribute__((ms_abi))
>   #define efi_va_list __builtin_ms_va_list
>   #define efi_va_start __builtin_ms_va_start
> +#define efi_va_copy __builtin_ms_va_copy
>   #define efi_va_arg __builtin_va_arg
>   #define efi_va_end __builtin_ms_va_end
>   #else
>   #define EFIAPI asmlinkage
>   #define efi_va_list va_list
>   #define efi_va_start va_start
> +#define efi_va_copy va_copy
>   #define efi_va_arg va_arg
>   #define efi_va_end va_end
>   #endif /* __x86_64__ */
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index ad01395b39c3..2b294d64efd0 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -655,8 +655,10 @@ efi_status_t efi_remove_protocol(const efi_handle_t handle,
>   /* Delete all protocols from a handle */
>   efi_status_t efi_remove_all_protocols(const efi_handle_t handle);
>   /* Install multiple protocol interfaces */
> -efi_status_t EFIAPI efi_install_multiple_protocol_interfaces
> -				(efi_handle_t *handle, ...);
> +efi_status_t EFIAPI
> +efi_install_multiple_protocol_interfaces(efi_handle_t *handle, ...);
> +efi_status_t EFIAPI
> +efi_uninstall_multiple_protocol_interfaces(efi_handle_t handle, ...);
>   /* Get handles that support a given protocol */
>   efi_status_t EFIAPI efi_locate_handle_buffer(
>   			enum efi_locate_search_type search_type,
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 1bfd094e89f8..aeb8b27dc676 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -2590,35 +2590,31 @@ found:
>   }
>
>   /**
> - * efi_install_multiple_protocol_interfaces() - Install multiple protocol
> + * __efi_install_multiple_protocol_interfaces() - Install multiple protocol

Thanks for this cleanup work. The patch is conceptionally correct. Just
the usual nitpicking below.

As Takahiro mentioned, we a have avoided __ prefixes up to now.

>    *                                              interfaces
>    * @handle: handle on which the protocol interfaces shall be installed
> - * @...:    NULL terminated argument list with pairs of protocol GUIDS and
> - *          interfaces
> - *
> - * This function implements the MultipleProtocolInterfaces service.
> + * @argptr: va_list of args
>    *
> - * See the Unified Extensible Firmware Interface (UEFI) specification for
> - * details.
> + * Core functionality of efi_install_multiple_protocol_interfaces
> + * Must not be called directly
>    *
>    * Return: status code
>    */
> -efi_status_t EFIAPI efi_install_multiple_protocol_interfaces
> -				(efi_handle_t *handle, ...)
> +static efi_status_t EFIAPI
> +__efi_install_multiple_protocol_interfaces(efi_handle_t *handle,
> +					   efi_va_list argptr)
>   {
> -	EFI_ENTRY("%p", handle);
> -
> -	efi_va_list argptr;
>   	const efi_guid_t *protocol;
>   	void *protocol_interface;
>   	efi_handle_t old_handle;
>   	efi_status_t r = EFI_SUCCESS;
>   	int i = 0;
> +	efi_va_list argptr_copy;
>
>   	if (!handle)
> -		return EFI_EXIT(EFI_INVALID_PARAMETER);
> +		return EFI_INVALID_PARAMETER;

Please, use a efi_search_obj(handle) to determine if the handle is valid.

	if (!efi_search_obj(handle))
		return EFI_INVALID_PARAMETER;

>
> -	efi_va_start(argptr, handle);
> +	efi_va_copy(argptr_copy, argptr);
>   	for (;;) {
>   		protocol = efi_va_arg(argptr, efi_guid_t*);
>   		if (!protocol)
> @@ -2646,52 +2642,103 @@ efi_status_t EFIAPI efi_install_multiple_protocol_interfaces
>   			break;
>   		i++;
>   	}
> -	efi_va_end(argptr);
> -	if (r == EFI_SUCCESS)
> -		return EFI_EXIT(r);
> +	if (r == EFI_SUCCESS) {
> +		efi_va_end(argptr_copy);

I would prefer a single exit point here:

	goto err;

> +		return r;
> +	}
>
>   	/* If an error occurred undo all changes. */
> -	efi_va_start(argptr, handle);
>   	for (; i; --i) {
> -		protocol = efi_va_arg(argptr, efi_guid_t*);
> -		protocol_interface = efi_va_arg(argptr, void*);
> +		protocol = efi_va_arg(argptr_copy, efi_guid_t*);
> +		protocol_interface = efi_va_arg(argptr_copy, void*);
>   		EFI_CALL(efi_uninstall_protocol_interface(*handle, protocol,
>   							  protocol_interface));

In future we should try to get rid of this EFI_CALL. But that is beyond
the scope of the current patch.

>   	}
> -	efi_va_end(argptr);

err:

> +	efi_va_end(argptr_copy);
> +
> +	return r;
>
> -	return EFI_EXIT(r);
>   }
>
>   /**
> - * efi_uninstall_multiple_protocol_interfaces() - uninstall multiple protocol
> - *                                                interfaces
> - * @handle: handle from which the protocol interfaces shall be removed
> + * efi_install_multiple_protocol_interfaces() - Install multiple protocol
> + *                                              interfaces
> + * @handle: handle on which the protocol interfaces shall be installed
>    * @...:    NULL terminated argument list with pairs of protocol GUIDS and
>    *          interfaces
>    *
> - * This function implements the UninstallMultipleProtocolInterfaces service.
> + *
> + * This is the function for internal usage in U-Boot. For the API function
> + * implementing the InstallMultipleProtocol service see
> + * efi_install_multiple_protocol_interfaces_ext()
> + *
> + * Return: status code
> + */
> +efi_status_t EFIAPI
> +efi_install_multiple_protocol_interfaces(efi_handle_t *handle, ...)
> +{
> +	efi_status_t r = EFI_SUCCESS;

The assigned value is never used.
We tend to call the return value ret.

> +	efi_va_list argptr;
> +
> +	efi_va_start(argptr, handle);
> +	r = __efi_install_multiple_protocol_interfaces(handle, argptr);
> +	efi_va_end(argptr);
> +	return r;
> +}
> +
> +/**
> + * efi_install_multiple_protocol_interfaces_ext() - Install multiple protocol
> + *                                                  interfaces
> + * @handle: handle on which the protocol interfaces shall be installed
> + * @...:    NULL terminated argument list with pairs of protocol GUIDS and
> + *          interfaces
> + *
> + * This function implements the MultipleProtocolInterfaces service.
>    *
>    * See the Unified Extensible Firmware Interface (UEFI) specification for
>    * details.
>    *
>    * Return: status code
>    */
> -static efi_status_t EFIAPI efi_uninstall_multiple_protocol_interfaces(
> -			efi_handle_t handle, ...)
> +static efi_status_t EFIAPI
> +efi_install_multiple_protocol_interfaces_ext(efi_handle_t *handle, ...)
>   {
>   	EFI_ENTRY("%p", handle);
> -
> +	efi_status_t r = EFI_SUCCESS;

The assigned value is never used.

efi_status ret;

>   	efi_va_list argptr;
> +
> +	efi_va_start(argptr, handle);
> +	r = __efi_install_multiple_protocol_interfaces(handle, argptr);
> +	efi_va_end(argptr);
> +	return EFI_EXIT(r);
> +}
> +
> +/**
> + * __efi_uninstall_multiple_protocol_interfaces() - wrapper for uninstall
> + *                                                  multiple protocol
> + *                                                  interfaces
> + * @handle: handle from which the protocol interfaces shall be removed
> + * @argptr: va_list of args
> + *
> + * Core functionality of efi_uninstall_multiple_protocol_interfaces
> + * Must not be called directly
> + *
> + * Return: status code
> + */
> +static efi_status_t EFIAPI
> +__efi_uninstall_multiple_protocol_interfaces(efi_handle_t handle,
> +					     efi_va_list argptr)
> +{
>   	const efi_guid_t *protocol;
>   	void *protocol_interface;
>   	efi_status_t r = EFI_SUCCESS;

We tend to use ret as variable name.

>   	size_t i = 0;
> +	efi_va_list argptr_copy;
>
>   	if (!handle)
> -		return EFI_EXIT(EFI_INVALID_PARAMETER);
> +		return EFI_INVALID_PARAMETER;

	if (!efi_search_obj(handle))
		return EFI_INVALID_PARAMETER;

>
> -	efi_va_start(argptr, handle);
> +	efi_va_copy(argptr_copy, argptr);
>   	for (;;) {
>   		protocol = efi_va_arg(argptr, efi_guid_t*);
>   		if (!protocol)
> @@ -2703,29 +2750,82 @@ static efi_status_t EFIAPI efi_uninstall_multiple_protocol_interfaces(
>   			break;
>   		i++;
>   	}
> -	efi_va_end(argptr);
>   	if (r == EFI_SUCCESS) {
>   		/* If the last protocol has been removed, delete the handle. */
>   		if (list_empty(&handle->protocols)) {
>   			list_del(&handle->link);
>   			free(handle);
>   		}
> -		return EFI_EXIT(r);
> +		efi_va_end(argptr_copy);

Please, use a single exit point.
		goto err;

> +		return r;
>   	}
>
>   	/* If an error occurred undo all changes. */
> -	efi_va_start(argptr, handle);
>   	for (; i; --i) {
> -		protocol = efi_va_arg(argptr, efi_guid_t*);
> -		protocol_interface = efi_va_arg(argptr, void*);
> +		protocol = efi_va_arg(argptr_copy, efi_guid_t*);
> +		protocol_interface = efi_va_arg(argptr_copy, void*);
>   		EFI_CALL(efi_install_protocol_interface(&handle, protocol,
>   							EFI_NATIVE_INTERFACE,
>   							protocol_interface));

We should remove EFI_CALL() here in a future patch.

>   	}
> -	efi_va_end(argptr);

ret = EFI_INVALID_PARAMETER;

err:

> +	efi_va_end(argptr_copy);
>
>   	/* In case of an error always return EFI_INVALID_PARAMETER */
> -	return EFI_EXIT(EFI_INVALID_PARAMETER);
> +	return EFI_INVALID_PARAMETER;

return ret;

> +}
> +
> +/**
> + * efi_uninstall_multiple_protocol_interfaces() - uninstall multiple protocol
> + *                                                interfaces
> + * @handle: handle from which the protocol interfaces shall be removed
> + * @...:    NULL terminated argument list with pairs of protocol GUIDS and
> + *          interfaces
> + *
> + * This function implements the UninstallMultipleProtocolInterfaces service.
> + *
> + * This is the function for internal usage in U-Boot. For the API function
> + * implementing the UninstallMultipleProtocolInterfaces service see
> + * efi_uninstall_multiple_protocol_interfaces_ext()
> + *
> + * Return: status code
> + */
> +efi_status_t EFIAPI
> +efi_uninstall_multiple_protocol_interfaces(efi_handle_t handle, ...)
> +{
> +	efi_status_t r = EFI_SUCCESS;

The assigned value is never used.

We tend to use ret as variable name.

> +	efi_va_list argptr;
> +
> +	efi_va_start(argptr, handle);
> +	r = __efi_uninstall_multiple_protocol_interfaces(handle, argptr);
> +	efi_va_end(argptr);
> +	return r;
> +}
> +
> +/**
> + * efi_uninstall_multiple_protocol_interfaces_ext() - uninstall multiple protocol
> + *                                                    interfaces
> + * @handle: handle from which the protocol interfaces shall be removed
> + * @...:    NULL terminated argument list with pairs of protocol GUIDS and
> + *          interfaces
> + *
> + * This function implements the UninstallMultipleProtocolInterfaces service.
> + *
> + * See the Unified Extensible Firmware Interface (UEFI) specification for
> + * details.
> + *
> + * Return: status code
> + */
> +static efi_status_t EFIAPI
> +efi_uninstall_multiple_protocol_interfaces_ext(efi_handle_t handle, ...)
> +{
> +	EFI_ENTRY("%p", handle);
> +	efi_status_t r = EFI_SUCCESS;

The assigned value is never used.

efi_status_t ret;

> +	efi_va_list argptr;
> +
> +	efi_va_start(argptr, handle);
> +	r = __efi_uninstall_multiple_protocol_interfaces(handle, argptr);
> +	efi_va_end(argptr);
> +	return EFI_EXIT(r);
>   }
>
>   /**
> @@ -3785,9 +3885,9 @@ static struct efi_boot_services efi_boot_services = {
>   	.locate_handle_buffer = efi_locate_handle_buffer,
>   	.locate_protocol = efi_locate_protocol,
>   	.install_multiple_protocol_interfaces =
> -			efi_install_multiple_protocol_interfaces,
> +			efi_install_multiple_protocol_interfaces_ext,
>   	.uninstall_multiple_protocol_interfaces =
> -			efi_uninstall_multiple_protocol_interfaces,
> +			efi_uninstall_multiple_protocol_interfaces_ext,
>   	.calculate_crc32 = efi_calculate_crc32,
>   	.copy_mem = efi_copy_mem,
>   	.set_mem = efi_set_mem,
> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> index a6b98f066a0b..b6bd2d6af882 100644
> --- a/lib/efi_loader/efi_capsule.c
> +++ b/lib/efi_loader/efi_capsule.c
> @@ -636,17 +636,18 @@ efi_status_t __weak efi_load_capsule_drivers(void)
>
>   	if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_FIT)) {
>   		handle = NULL;
> -		ret = EFI_CALL(efi_install_multiple_protocol_interfaces(
> -				&handle, &efi_guid_firmware_management_protocol,
> -				&efi_fmp_fit, NULL));
> +		ret = efi_install_multiple_protocol_interfaces(&handle,
> +							       &efi_guid_firmware_management_protocol,
> +							       &efi_fmp_fit,
> +							       NULL);
>   	}
>
>   	if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_RAW)) {
>   		handle = NULL;
> -		ret = EFI_CALL(efi_install_multiple_protocol_interfaces(
> -				&handle,
> -				&efi_guid_firmware_management_protocol,
> -				&efi_fmp_raw, NULL));
> +		ret = efi_install_multiple_protocol_interfaces(&handle,
> +							       &efi_guid_firmware_management_protocol,
> +							       &efi_fmp_raw,
> +							       NULL);
>   	}
>
>   	return ret;
> diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
> index cf9fbd9cb54d..3354b217a9a4 100644
> --- a/lib/efi_loader/efi_console.c
> +++ b/lib/efi_loader/efi_console.c
> @@ -1278,12 +1278,14 @@ efi_status_t efi_console_register(void)
>   	struct efi_device_path *dp;
>
>   	/* Install protocols on root node */
> -	r = EFI_CALL(efi_install_multiple_protocol_interfaces
> -		     (&efi_root,
> -		      &efi_guid_text_output_protocol, &efi_con_out,
> -		      &efi_guid_text_input_protocol, &efi_con_in,
> -		      &efi_guid_text_input_ex_protocol, &efi_con_in_ex,
> -		      NULL));
> +	r = efi_install_multiple_protocol_interfaces(&efi_root,
> +						     &efi_guid_text_output_protocol,
> +						     &efi_con_out,
> +						     &efi_guid_text_input_protocol,
> +						     &efi_con_in,
> +						     &efi_guid_text_input_ex_protocol,
> +						     &efi_con_in_ex,
> +						     NULL);
>
>   	/* Create console node and install device path protocols */
>   	if (CONFIG_IS_ENABLED(DM_SERIAL)) {
> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> index 39ea1a68a683..fb96b0508528 100644
> --- a/lib/efi_loader/efi_disk.c
> +++ b/lib/efi_loader/efi_disk.c
> @@ -449,10 +449,12 @@ static efi_status_t efi_disk_add_dev(
>   	 * in this case.
>   	 */
>   	handle = &diskobj->header;
> -	ret = EFI_CALL(efi_install_multiple_protocol_interfaces(
> -			&handle, &efi_guid_device_path, diskobj->dp,
> -			&efi_block_io_guid, &diskobj->ops,
> -			guid, NULL, NULL));
> +	ret = efi_install_multiple_protocol_interfaces(&handle,
> +						       &efi_guid_device_path,
> +						       diskobj->dp,
> +						       &efi_block_io_guid,
> +						       &diskobj->ops, guid,
> +						       NULL, NULL);
>   	if (ret != EFI_SUCCESS)
>   		goto error;
>
> diff --git a/lib/efi_loader/efi_load_initrd.c b/lib/efi_loader/efi_load_initrd.c
> index 3d6044f76047..87fde3f88c2b 100644
> --- a/lib/efi_loader/efi_load_initrd.c
> +++ b/lib/efi_loader/efi_load_initrd.c
> @@ -208,14 +208,13 @@ efi_status_t efi_initrd_register(void)
>   	if (ret != EFI_SUCCESS)
>   		return ret;
>
> -	ret = EFI_CALL(efi_install_multiple_protocol_interfaces
> -		       (&efi_initrd_handle,
> -			/* initramfs */
> -			&efi_guid_device_path, &dp_lf2_handle,
> -			/* LOAD_FILE2 */
> -			&efi_guid_load_file2_protocol,
> -			(void *)&efi_lf2_protocol,
> -			NULL));
> +	ret = efi_install_multiple_protocol_interfaces(&efi_initrd_handle,
> +						       /* initramfs */
> +						       &efi_guid_device_path, &dp_lf2_handle,
> +						       /* LOAD_FILE2 */
> +						       &efi_guid_load_file2_protocol,
> +						       (void *)&efi_lf2_protocol,
> +						       NULL);
>
>   	return ret;
>   }
> diff --git a/lib/efi_loader/efi_root_node.c b/lib/efi_loader/efi_root_node.c
> index 739c6867f412..b4696d54c33d 100644
> --- a/lib/efi_loader/efi_root_node.c
> +++ b/lib/efi_loader/efi_root_node.c

Please, put these changes into a separate patch.

Best regards

Heinrich

> @@ -49,38 +49,38 @@ efi_status_t efi_root_node_register(void)
>   	dp->end.length = sizeof(struct efi_device_path);
>
>   	/* Create root node and install protocols */
> -	ret = EFI_CALL(efi_install_multiple_protocol_interfaces
> -			(&efi_root,
> -			 /* Device path protocol */
> -			 &efi_guid_device_path, dp,
> +	ret = efi_install_multiple_protocol_interfaces
> +		(&efi_root,
> +		 /* Device path protocol */
> +		 &efi_guid_device_path, dp,
>   #if CONFIG_IS_ENABLED(EFI_DEVICE_PATH_TO_TEXT)
> -			 /* Device path to text protocol */
> -			 &efi_guid_device_path_to_text_protocol,
> -			 (void *)&efi_device_path_to_text,
> +		 /* Device path to text protocol */
> +		 &efi_guid_device_path_to_text_protocol,
> +		 (void *)&efi_device_path_to_text,
>   #endif
>   #ifdef CONFIG_EFI_DEVICE_PATH_UTIL
> -			 /* Device path utilities protocol */
> -			 &efi_guid_device_path_utilities_protocol,
> -			 (void *)&efi_device_path_utilities,
> +		 /* Device path utilities protocol */
> +		 &efi_guid_device_path_utilities_protocol,
> +		 (void *)&efi_device_path_utilities,
>   #endif
>   #ifdef CONFIG_EFI_DT_FIXUP
> -			 /* Device-tree fix-up protocol */
> -			 &efi_guid_dt_fixup_protocol,
> -			 (void *)&efi_dt_fixup_prot,
> +		 /* Device-tree fix-up protocol */
> +		 &efi_guid_dt_fixup_protocol,
> +		 (void *)&efi_dt_fixup_prot,
>   #endif
>   #if CONFIG_IS_ENABLED(EFI_UNICODE_COLLATION_PROTOCOL2)
> -			 &efi_guid_unicode_collation_protocol2,
> -			 (void *)&efi_unicode_collation_protocol2,
> +		 &efi_guid_unicode_collation_protocol2,
> +		 (void *)&efi_unicode_collation_protocol2,
>   #endif
>   #if CONFIG_IS_ENABLED(EFI_LOADER_HII)
> -			 /* HII string protocol */
> -			 &efi_guid_hii_string_protocol,
> -			 (void *)&efi_hii_string,
> -			 /* HII database protocol */
> -			 &efi_guid_hii_database_protocol,
> -			 (void *)&efi_hii_database,
> +		 /* HII string protocol */
> +		 &efi_guid_hii_string_protocol,
> +		 (void *)&efi_hii_string,
> +		 /* HII database protocol */
> +		 &efi_guid_hii_database_protocol,
> +		 (void *)&efi_hii_database,
>   #endif
> -			 NULL));
> +		 NULL);
>   	efi_root->type = EFI_OBJECT_TYPE_U_BOOT_FIRMWARE;
>   	return ret;
>   }

Hi Heinrich 

[...]

> > diff --git a/lib/efi_loader/efi_load_initrd.c b/lib/efi_loader/efi_load_initrd.c
> > index 3d6044f76047..87fde3f88c2b 100644
> > --- a/lib/efi_loader/efi_load_initrd.c
> > +++ b/lib/efi_loader/efi_load_initrd.c
> > @@ -208,14 +208,13 @@ efi_status_t efi_initrd_register(void)
> >   	if (ret != EFI_SUCCESS)
> >   		return ret;
> > 
> > -	ret = EFI_CALL(efi_install_multiple_protocol_interfaces
> > -		       (&efi_initrd_handle,
> > -			/* initramfs */
> > -			&efi_guid_device_path, &dp_lf2_handle,
> > -			/* LOAD_FILE2 */
> > -			&efi_guid_load_file2_protocol,
> > -			(void *)&efi_lf2_protocol,
> > -			NULL));
> > +	ret = efi_install_multiple_protocol_interfaces(&efi_initrd_handle,
> > +						       /* initramfs */
> > +						       &efi_guid_device_path, &dp_lf2_handle,
> > +						       /* LOAD_FILE2 */
> > +						       &efi_guid_load_file2_protocol,
> > +						       (void *)&efi_lf2_protocol,
> > +						       NULL);
> > 
> >   	return ret;
> >   }
> > diff --git a/lib/efi_loader/efi_root_node.c b/lib/efi_loader/efi_root_node.c
> > index 739c6867f412..b4696d54c33d 100644
> > --- a/lib/efi_loader/efi_root_node.c
> > +++ b/lib/efi_loader/efi_root_node.c
> 
> Please, put these changes into a separate patch.
> 

Shouldn't they go in the same patchset?  We are changing the definition of 
efi_install_multiple_protocol_interfaces() so EFI_ENTRY/EFI_EXIT is not
there anymore and we don't need EFI_CALL to save/restore the gd on
entry/exit.

Thanks
/Ilias

> Best regards
> 
> Heinrich
> 
> > @@ -49,38 +49,38 @@ efi_status_t efi_root_node_register(void)
> >   	dp->end.length = sizeof(struct efi_device_path);
> > 
> >   	/* Create root node and install protocols */
> > -	ret = EFI_CALL(efi_install_multiple_protocol_interfaces
> > -			(&efi_root,
> > -			 /* Device path protocol */
> > -			 &efi_guid_device_path, dp,
> > +	ret = efi_install_multiple_protocol_interfaces
> > +		(&efi_root,
> > +		 /* Device path protocol */
> > +		 &efi_guid_device_path, dp,
> >   #if CONFIG_IS_ENABLED(EFI_DEVICE_PATH_TO_TEXT)
> > -			 /* Device path to text protocol */
> > -			 &efi_guid_device_path_to_text_protocol,
> > -			 (void *)&efi_device_path_to_text,
> > +		 /* Device path to text protocol */
> > +		 &efi_guid_device_path_to_text_protocol,
> > +		 (void *)&efi_device_path_to_text,
> >   #endif
> >   #ifdef CONFIG_EFI_DEVICE_PATH_UTIL
> > -			 /* Device path utilities protocol */
> > -			 &efi_guid_device_path_utilities_protocol,
> > -			 (void *)&efi_device_path_utilities,
> > +		 /* Device path utilities protocol */
> > +		 &efi_guid_device_path_utilities_protocol,
> > +		 (void *)&efi_device_path_utilities,
> >   #endif
> >   #ifdef CONFIG_EFI_DT_FIXUP
> > -			 /* Device-tree fix-up protocol */
> > -			 &efi_guid_dt_fixup_protocol,
> > -			 (void *)&efi_dt_fixup_prot,
> > +		 /* Device-tree fix-up protocol */
> > +		 &efi_guid_dt_fixup_protocol,
> > +		 (void *)&efi_dt_fixup_prot,
> >   #endif
> >   #if CONFIG_IS_ENABLED(EFI_UNICODE_COLLATION_PROTOCOL2)
> > -			 &efi_guid_unicode_collation_protocol2,
> > -			 (void *)&efi_unicode_collation_protocol2,
> > +		 &efi_guid_unicode_collation_protocol2,
> > +		 (void *)&efi_unicode_collation_protocol2,
> >   #endif
> >   #if CONFIG_IS_ENABLED(EFI_LOADER_HII)
> > -			 /* HII string protocol */
> > -			 &efi_guid_hii_string_protocol,
> > -			 (void *)&efi_hii_string,
> > -			 /* HII database protocol */
> > -			 &efi_guid_hii_database_protocol,
> > -			 (void *)&efi_hii_database,
> > +		 /* HII string protocol */
> > +		 &efi_guid_hii_string_protocol,
> > +		 (void *)&efi_hii_string,
> > +		 /* HII database protocol */
> > +		 &efi_guid_hii_database_protocol,
> > +		 (void *)&efi_hii_database,
> >   #endif
> > -			 NULL));
> > +		 NULL);
> >   	efi_root->type = EFI_OBJECT_TYPE_U_BOOT_FIRMWARE;
> >   	return ret;
> >   }
>

Hi Heinrich 

[...]

> > -		return EFI_EXIT(EFI_INVALID_PARAMETER);
> > +		return EFI_INVALID_PARAMETER;
> 
> Please, use a efi_search_obj(handle) to determine if the handle is valid.
> 
> 	if (!efi_search_obj(handle))
> 		return EFI_INVALID_PARAMETER;

We should only check against NULL here, since we need to add a new handle
if it doesn't exist.  In Uninstall that would make sense, but we already
check that in efi_uninstall_protocol().

[...]


Thanks
/Ilias