On 5/8/24 14:04, Dan Carpenter wrote:
On Wed, May 08, 2024 at 07:24:01PM +0800, Weizhao Ouyang wrote:
diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
index de0d49ebebda..919e3cba071b 100644
--- a/lib/efi_loader/efi_capsule.c
+++ b/lib/efi_loader/efi_capsule.c
@@ -922,6 +922,105 @@ static bool device_is_present_and_system_part(struct
efi_device_path *dp)
return true;
}
+/**
+ * get_esp_from_boot_option_file_path - get the expand device path
+ *
+ * Get a possible efi system partition by expanding a boot option
+ * file path.
+ *
+ * @boot_dev The device path pointing to a boot option
+ * Return: The full ESP device path or NULL if fail
+ */
+static struct efi_device_path *get_esp_from_boot_option_file_path(struct
efi_device_path *boot_dev)
+{
+ efi_status_t ret = EFI_SUCCESS;
+ efi_handle_t handle;
+ struct efi_device_path *dp = boot_dev;
+ struct efi_device_path *full_path = NULL;
+
+ ret =
EFI_CALL(efi_locate_device_path(&efi_simple_file_system_protocol_guid,
+ &dp,
+ &handle));
+ if (ret != EFI_SUCCESS)
+ ret = EFI_CALL(efi_locate_device_path(&efi_block_io_guid, &dp,
&handle));
+
+ /* Expand Media Device Path */
+ if (ret == EFI_SUCCESS && EFI_DP_TYPE(dp, END, END)) {
Flip this around. Always do failure handling. Never do success
handling. full_path is always NULL. Just return that. Delete
the variable.
if (ret != EFI_SUCCESS ||
!EFI_DP_TYPE(dp, END, END))
return NULL;
+ struct efi_device_path *temp_dp;
+ struct efi_block_io *block_io;
+ void *buffer;
+ efi_handle_t *simple_file_system_handle;
+ efi_uintn_t number_handles, index;
+ u32 size;
+ u32 temp_size;
+
+ temp_dp = boot_dev;
+ ret =
EFI_CALL(efi_locate_device_path(&efi_simple_file_system_protocol_guid,
+ &temp_dp,
+ &handle));
+ /*
+ * For device path pointing to simple file system, it only
expands to one
+ * full path
+ */
+ if (ret == EFI_SUCCESS && EFI_DP_TYPE(temp_dp, END, END)) {
"Always" was hyperbole. This success handling is fine.
+ if (device_is_present_and_system_part(temp_dp))
+ return temp_dp;
+ }
+
+ /*
+ * For device path only pointing to the removable device
handle, try to
+ * search all its children handles
+ */
+ ret = EFI_CALL(efi_locate_device_path(&efi_block_io_guid, &temp_dp,
&handle));
ret is not used. Delete.
Errors should be handled and not ignored.
+ EFI_CALL(efi_connect_controller(handle, NULL, NULL, true));
+
+ /* Align with edk2, issue a dummy read to the device to check
the device change */
+ ret = EFI_CALL(efi_handle_protocol(handle, &efi_block_io_guid,
(void **)&block_io));
+ if (ret == EFI_SUCCESS) {
if (ret != EFI_SUCCESS)
return NULL;
+ buffer = memalign(block_io->media->io_align,
block_io->media->block_size);
+ if (buffer) {
if (!buffer)
return NULL;
+ ret = EFI_CALL(block_io->read_blocks(block_io,
+
block_io->media->media_id,
+ 0,
+
block_io->media->block_size,
+ buffer));
Delete the unused "ret = " assignment.
ditto
Best regards
Heinrich
+ free(buffer);
+ } else {
+ return full_path;
+ }
+ } else {
+ return full_path;
+ }
+
+ /* detect the default boot file from removable media */
+ size = efi_dp_size(boot_dev) - sizeof(struct efi_device_path);
+ EFI_CALL(efi_locate_handle_buffer(BY_PROTOCOL,
+
&efi_simple_file_system_protocol_guid,
+ NULL,
+ &number_handles,
+ &simple_file_system_handle));
+ for (index = 0; index < number_handles; index++) {
+
EFI_CALL(efi_handle_protocol(simple_file_system_handle[index],
+ &efi_guid_device_path,
+ (void **)&temp_dp));
+
+ log_debug("Search ESP %pD\n", temp_dp);
+ temp_size = efi_dp_size(temp_dp) - sizeof(struct
efi_device_path);
+ if (size <= temp_size && !memcmp(temp_dp, boot_dev,
size)) {
+ if (device_is_present_and_system_part(temp_dp))
{
You could combine these conditions:
if (size <= temp_size &&
memcmp(temp_dp, boot_dev, size) == 0 &&
device_is_present_and_system_part(temp_dp)) {
efi_free_pool(simple_file_system_handle);
return temp_dp;
}
+
efi_free_pool(simple_file_system_handle);
+ return temp_dp;
+ }
+ }
+ }
+
+ if (simple_file_system_handle)
+ efi_free_pool(simple_file_system_handle);
+ }
+
+ return full_path;
+}
+
/**
* find_boot_device - identify the boot device
*
@@ -938,6 +1037,7 @@ static efi_status_t find_boot_device(void)
int i, num;
struct efi_simple_file_system_protocol *volume;
struct efi_device_path *boot_dev = NULL;
+ struct efi_device_path *full_path = NULL;
No need to initialize this to NULL, it disables static checker warnings
for uninitialized variables so it can lead to bugs.
efi_status_t ret;
/* find active boot device in BootNext */
@@ -961,8 +1061,14 @@ static efi_status_t find_boot_device(void)
if (device_is_present_and_system_part(boot_dev)) {
goto found;
} else {
- efi_free_pool(boot_dev);
- boot_dev = NULL;
+ full_path =
get_esp_from_boot_option_file_path(boot_dev);
+ if (full_path) {
+ boot_dev = full_path;
Later, we free full_path but do we ever free the original boot_dev?
+ goto found;
+ } else {
+ efi_free_pool(boot_dev);
+ boot_dev = NULL;
+ }
Better to delete indenting where you can:
full_path =
get_esp_from_boot_option_file_path(boot_dev);
if (full_path) {
boot_dev = full_path;
goto found;
}
efi_free_pool(boot_dev);
boot_dev = NULL;
regards,
dan carpenter
