[PATCH 1/1] efi_loader: CloseProtocol in efi_fmp_find

[Heinrich Schuchardt]

On 10/11/22 02:49, AKASHI Takahiro wrote:
> 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.

Typically an agent handle is used to relate to a driver exposing the 
driver binding protocol. The root node does not expose the driver 
binding protocol.

Why would you want to create an open protocol information entry here?

Do you think anything with the code after the patch is wrong?

Best regards

Heinrich

> 
>> 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 at 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
>>