Re: [PATCH 1/1] efi_loader: CloseProtocol in efi_fmp_find

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

The commit message is not accurate.

On Fri, Oct 07, 2022 at 04:06:23PM +0200, Heinrich Schuchardt wrote:
> The CloseProtocol() boot service requires a handle as first argument.
> Passing the protocol interface is incorrect.

Correct, but

> CloseProtocol() only has an effect if called with a non-zero value for
> agent_handle. HandleProtocol() uses an opaque agent_handle when invoking
> OpenProtocol() (currently NULL).

No. OpenProtocol() is called with efi_root as an agent handle.
So, calling CloseProtocol() is a right thing at the end.

> Therefore HandleProtocol() should be
> avoided.
> 
> * Replace the LocateHandle() call by efi_search_protocol().

LocateHandle() -> efi_handle_protocol()

So you could have fixed this way:
    EFI_CALL(efi_close_protocol(handle, ..., &efi_root, NULL);

I preferred to use EFI_CALL() over this file as you can see.

-Takahiro Akashi

> * Remove the CloseProtocol() call.
> 
> Fixes: 8d99026f0697 ("efi_loader: capsule: support firmware update")
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  lib/efi_loader/efi_capsule.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> index b6bd2d6af8..397e393a18 100644
> --- a/lib/efi_loader/efi_capsule.c
> +++ b/lib/efi_loader/efi_capsule.c
> @@ -159,12 +159,14 @@ efi_fmp_find(efi_guid_t *image_type, u8 image_index, u64 instance,
>  	efi_status_t ret;
>  
>  	for (i = 0, handle = handles; i < no_handles; i++, handle++) {
> -		ret = EFI_CALL(efi_handle_protocol(
> -				*handle,
> -				&efi_guid_firmware_management_protocol,
> -				(void **)&fmp));
> +		struct efi_handler *fmp_handler;
> +
> +		ret = efi_search_protocol(
> +				*handle, &efi_guid_firmware_management_protocol,
> +				&fmp_handler);
>  		if (ret != EFI_SUCCESS)
>  			continue;
> +		fmp = fmp_handler->protocol_interface;
>  
>  		/* get device's image info */
>  		info_size = 0;
> @@ -215,10 +217,6 @@ efi_fmp_find(efi_guid_t *image_type, u8 image_index, u64 instance,
>  skip:
>  		efi_free_pool(package_version_name);
>  		free(image_info);
> -		EFI_CALL(efi_close_protocol(
> -				(efi_handle_t)fmp,
> -				&efi_guid_firmware_management_protocol,
> -				NULL, NULL));
>  		if (found)
>  			return fmp;
>  	}
> -- 
> 2.37.2
>