Hi Heinrich,

On Mon, Oct 03, 2022 at 11:44:58AM +0200, Heinrich Schuchardt wrote:

When deleting a device or a handle we must remove the link between the two to avoid dangling references.

Provide function efi_unlink_dev() for this purpose.

Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

include/efi_loader.h | 1 + lib/efi_loader/efi_helper.c | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+)

diff --git a/include/efi_loader.h b/include/efi_loader.h index ad01395b39..5a993b0d2e 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -708,6 +708,7 @@ const char *guid_to_sha_str(const efi_guid_t *guid); int algo_to_len(const char *algo);

int efi_link_dev(efi_handle_t handle, struct udevice *dev); +int efi_unlink_dev(efi_handle_t handle);

/**

• efi_size_in_pages() - convert size in bytes to size in pages

diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c index 8ed564e261..c71e87d118 100644 --- a/lib/efi_loader/efi_helper.c +++ b/lib/efi_loader/efi_helper.c @@ -171,3 +171,22 @@ int efi_link_dev(efi_handle_t handle, struct udevice *dev) handle->dev = dev; return dev_tag_set_ptr(dev, DM_TAG_EFI, handle); }

+/**

• • efi_unlink_dev() - unlink udevice and handle
• • @handle: EFI handle to unlink
• • Return: 0 on success, negative on failure
• */

+int efi_unlink_dev(efi_handle_t handle) +{

• int ret;
• ret = dev_tag_del(handle->dev, DM_TAG_EFI);
• if (ret)

• return ret;
  

Is handle->dev always guaranteed to hold a valid ptr? Would it make sense to add an if here and use this function everywhere instead of sprinkling dev_tag_del() around?

• handle->dev = NULL;
• return 0;

+}

2.37.2

Thanks /Ilias

On 10/3/22 12:18, Ilias Apalodimas wrote:

Hi Heinrich,

On Mon, Oct 03, 2022 at 11:44:59AM +0200, Heinrich Schuchardt wrote:

...

If creating the block device fails,

• delete all created objects and references
• close the protocol interface on the controller

Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

include/efi_driver.h | 2 +- lib/efi_driver/efi_block_device.c | 60 +++++++++++++++++-------------- lib/efi_driver/efi_uclass.c | 23 ++++++------ 3 files changed, 46 insertions(+), 39 deletions(-)

diff --git a/include/efi_driver.h b/include/efi_driver.h index 2b62219c5b..dc0c1c7ac0 100644 --- a/include/efi_driver.h +++ b/include/efi_driver.h @@ -25,7 +25,7 @@ struct efi_driver_ops { const efi_guid_t *protocol; const efi_guid_t *child_protocol;

• int (*bind)(efi_handle_t handle, void *interface);

• efi_status_t (*bind)(efi_handle_t handle, void *interface); };

  /*

diff --git a/lib/efi_driver/efi_block_device.c b/lib/efi_driver/efi_block_device.c index 3177ab67b8..9ccc148590 100644 --- a/lib/efi_driver/efi_block_device.c +++ b/lib/efi_driver/efi_block_device.c @@ -112,12 +112,13 @@ static ulong efi_bl_write(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt,

• @handle: handle
• @interface: block io protocol

• • Return: 0 = success

• • Return: status code
  */

-static int efi_bl_bind(efi_handle_t handle, void *interface) +static efi_status_t efi_bl_bind(efi_handle_t handle, void *interface) {

• struct udevice *bdev, *parent = dm_root();
• int ret, devnum;

• struct udevice *bdev = NULL, *parent = dm_root();
• efi_status_t ret;
• int devnum; char *name; struct efi_object *obj = efi_search_obj(handle); struct efi_block_io *io = interface;

@@ -125,28 +126,28 @@ static int efi_bl_bind(efi_handle_t handle, void *interface)

EFI_PRINT("%s: handle %p, interface %p\n", __func__, handle, io);

• if (!obj)

• return -ENOENT;
  

• if (!obj || !interface)

• return EFI_INVALID_PARAMETER;
  
  

  devnum = blk_find_max_devnum(UCLASS_EFI_LOADER); if (devnum == -ENODEV) devnum = 0; else if (devnum < 0)

• return devnum;
  

• return EFI_OUT_OF_RESOURCES;
  

Is EFI_NOT_FOUND a better option here?

We reach this point if either uclass UCLASS_EFI_LOADER does not exist or a device for the uclass has a negative value of devnum > -ENODEV.

The caller did not reference a UEFI object involved that we could not find. And EFI_NOT_FOUND is not a return value foreseen by the UEFI specification.

EFI_OUT_OF_RESOURCES reflects that there is a problem that the caller cannot fix.

...

name = calloc(1, 18); /* strlen("efiblk#2147483648") + 1 */ if (!name)

• return -ENOMEM;
  

• return EFI_OUT_OF_RESOURCES;
  

  sprintf(name, "efiblk#%d", devnum);

  /* Create driver model udevice for the EFI block io device */

• ret = blk_create_device(parent, "efi_blk", name, UCLASS_EFI_LOADER,

• 		devnum, io->media->block_size,
  

• 		(lbaint_t)io->media->last_block, &bdev);
  

• if (ret)

• return ret;
  

• if (!bdev)

• return -ENOENT;
  

• if (blk_create_device(parent, "efi_blk", name, UCLASS_EFI_LOADER,

• 	      devnum, io->media->block_size,
  

• 	      (lbaint_t)io->media->last_block, &bdev)) {
  

• ret = EFI_OUT_OF_RESOURCES;
  

• free(name);
  

• goto err;
  

• } /* Set the DM_FLAG_NAME_ALLOCED flag to avoid a memory leak */ device_set_name_alloced(bdev);

@@ -154,20 +155,25 @@ static int efi_bl_bind(efi_handle_t handle, void *interface) plat->handle = handle; plat->io = interface;

• /*

• * FIXME: necessary because we won't do almost nothing in
  

• * efi_disk_create() when called from device_probe().
  

• */
  

• if (efi_link_dev(handle, bdev))

• /* FIXME: cleanup for bdev */
  

• return ret;
  

• ret = device_probe(bdev);
• if (ret)

• return ret;
  

• if (efi_link_dev(handle, bdev)) {

• ret = EFI_OUT_OF_RESOURCES;
  

• goto err;
  

• }
• if (device_probe(bdev)) {

• ret = EFI_DEVICE_ERROR;
  

• goto err;
  

• } EFI_PRINT("%s: block device '%s' created\n", __func__, bdev->name);

• return 0;

• return EFI_SUCCESS;

+err:

• efi_unlink_dev(handle);
• if (bdev)

• device_unbind(bdev);
  

• return ret;

The efi_unlink_dev() is definitely needed. Would it also make sense to replace the open coded 'dev_tag_del(dev, DM_TAG_EFI);' instances with it? (there are 2 in efi_disk.c)

Yes, they should be moved to efi_delete_handle(). But that is beyond the scope of this patch.

Best regards

Heinrich

...

}

/* Block device driver operators */ diff --git a/lib/efi_driver/efi_uclass.c b/lib/efi_driver/efi_uclass.c index 74dd003243..5a285aad89 100644 --- a/lib/efi_driver/efi_uclass.c +++ b/lib/efi_driver/efi_uclass.c @@ -11,7 +11,7 @@

• The uclass provides the bind, start, and stop entry points for the driver
• binding protocol.

• • In bind() and stop() it checks if the controller implements the protocol

• • In supported() and bind() it checks if the controller implements the protocol
  • supported by the EFI driver. In the start() function it calls the bind()
  • function of the EFI driver. In the stop() function it destroys the child
  • controllers.

@@ -144,18 +144,19 @@ static efi_status_t EFIAPI efi_uc_start( goto out; } ret = check_node_type(controller_handle);

• if (ret != EFI_SUCCESS) {

• r = EFI_CALL(systab.boottime->close_protocol(
  

• 		controller_handle, bp->ops->protocol,
  

• 		this->driver_binding_handle,
  

• 		controller_handle));
  

• if (r != EFI_SUCCESS)
  

• 	EFI_PRINT("Failure to close handle\n");
  

• if (ret != EFI_SUCCESS)

• goto err;
  

• ret = bp->ops->bind(controller_handle, interface);
• if (ret == EFI_SUCCESS) goto out;

• }

• /* TODO: driver specific stuff */

• bp->ops->bind(controller_handle, interface);

+err:

• r = EFI_CALL(systab.boottime->close_protocol(

• 	controller_handle, bp->ops->protocol,
  

• 	this->driver_binding_handle,
  

• 	controller_handle));
  

• if (r != EFI_SUCCESS)

• EFI_PRINT("Failure to close handle\n");
  
  

  out: return EFI_EXIT(ret);

-- 2.37.2

Thanks /Ilias