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. > + 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. > + 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