On 10/11/22 09:35, AKASHI Takahiro wrote:
On Tue, Oct 11, 2022 at 07:58:11AM +0200, Heinrich Schuchardt wrote:
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.
Why can't we, other than a driver, call HandleProtocol() as a convenient way of accessing an interface?
The description of HandleProtocol() clearly says that it is deprecated.
The assumption that the UEFI specification makes in it is example code that you never be able to close a protocol opened with HandleProtocol.
After the first usage of handle protocol the open protocol information with the opaque agent handle will block the protocol interface from ever being removed by the driver exposing it.
The root node does not expose the driver binding protocol.
So do you mean the current implementation of HandleProtocol() is wrong?
Yes. If you ever install a boot time driver, it might remove a protocol interface which is actually still in use.
Why would you want to create an open protocol information entry here?
To access get_image_info() quickly.
This is not related to an open protocol information (see the UEFI spec description of OpenProtocolInformation()).
Best regards
Heinrich
Do you think anything with the code after the patch is wrong?
No reason to replace handle_protocol().
Another example is here: efi_load_image_from_path() efi_handle_protocol(device, guid, (void **)&load_file_protocol)); ... efi_close_protocol(device, guid, efi_root, NULL);
I believe that this function is anything but a driver. I think using HandleProtocol() (or preferably OpenProtocol()) and CloseProtocol() in pair seems totally sane.
-Takahiro Akashi
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@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