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 <[email protected]>
---
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
