Hi Heinrich, On Tue, 26 Dec 2023 at 10:43, Heinrich Schuchardt <[email protected]> wrote: > > > > Am 26. Dezember 2023 02:13:08 MEZ schrieb Masahisa Kojima > <[email protected]>: > >Hi Heinrich, > > > >On Mon, 25 Dec 2023 at 19:05, Heinrich Schuchardt <[email protected]> wrote: > >> > >> On 12/25/23 05:43, Masahisa Kojima wrote: > >> > efi_delete_handle() calls efi_purge_handle(), then it finally > >> > frees the efi handle. > >> > Both diskobj and handle variables in efi_disk_remove() have > >> > the same pointer, we can not access diskobj->dp after calling > >> > efi_delete_handle(). > >> > > >> > This commit saves the struct efi_device_path pointer before > >> > calling efi_delete_handle(). This commit also fixes the > >> > missing free for volume member in struct efi_disk_obj. > >> > > >> > Signed-off-by: Masahisa Kojima <[email protected]> > >> > --- > >> > lib/efi_loader/efi_disk.c | 12 +++++++++--- > >> > 1 file changed, 9 insertions(+), 3 deletions(-) > >> > > >> > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > >> > index a2f8b531a3..415d8601ba 100644 > >> > --- a/lib/efi_loader/efi_disk.c > >> > +++ b/lib/efi_loader/efi_disk.c > >> > @@ -701,7 +701,9 @@ int efi_disk_remove(void *ctx, struct event *event) > >> > struct udevice *dev = event->data.dm.dev; > >> > efi_handle_t handle; > >> > struct blk_desc *desc; > >> > + struct efi_device_path *dp = NULL; > >> > struct efi_disk_obj *diskobj = NULL; > >> > + struct efi_simple_file_system_protocol *volume = NULL; > >> > efi_status_t ret; > >> > > >> > if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle)) > >> > @@ -722,14 +724,18 @@ int efi_disk_remove(void *ctx, struct event *event) > >> > return 0; > >> > } > >> > > >> > + if (diskobj) { > >> > >> > >> diskobj = container_of(handle, struct efi_disk_obj, header); > >> > >> can be replaced by > >> > >> diskobj = handle > >> > >> We should check the handle immediately after dev_tag_get_ptr() and > >> return 0 if the handle == NULL. > > > >OK, I will try to improve the efi_disk_remove() function. > > > >> > >> > + dp = diskobj->dp; > >> > + volume = diskobj->volume; > >> > + } > >> > + > >> > ret = efi_delete_handle(handle); > >> > >> We must not delete the handle in case of UCLASS_EFI_LOADER. > >> > >> Instead of calling efi_delete_handle we should uninstall all protocols > >> that we have installed. If no protocol is left the handle will go away. > >> > >> To make the protocols to delete tractable they should be opened with > >> BY_DRIVER. > > > >efi_delete_handle() calls efi_remove_all_protocols(), and frees the efi > >handle > >if no protocol is left on the handle. > >So I think efi_delete_handle() does all the required processes mentioned > >above. > > Given an EFI application like iPXE that provides a handle with the block IO > protocol implemented by the application: Why should U-Boot remove the block > IO protocol or the device path protocol when the application calls > DisconnectController()?
Yes, but the current implementation does the same. So in the case of UCLASS_EFI_LOADER, we need to immediately return without calling efi_delete_handle(), is my understanding correct? I checked lib/efi_selftest/efi_selftest_block_device.c implementation, the teadown() process uninstalls the all protocol_interface then the EFI handle goes away if no protocol is left. Thanks, Masahisa Kojima > > Best regards > > Heinrich > > > > >> > >> When a partition is removed we must close the protocol interfaces opened > >> with EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER (cf. efi_disk_add_dev()). > > > >OK, I will fix this. > > > >Thanks, > >Masahisa Kojima > > > >> > >> Best regards > >> > >> Heinrich > >> > >> > /* Do not delete DM device if there are still EFI drivers > >> > attached. */ > >> > if (ret != EFI_SUCCESS) > >> > return -1; > >> > > >> > - if (diskobj) > >> > - efi_free_pool(diskobj->dp); > >> > - > >> > + efi_free_pool(dp); > >> > + free(volume); > >> > dev_tag_del(dev, DM_TAG_EFI); > >> > > >> > return 0; > >>
