* [PATCH 0/2] efi_driver: fix error handling
@ 2022-10-03 9:44 Heinrich Schuchardt
2022-10-03 9:44 ` [PATCH 1/2] efi_loader: function to unlink udevice and handle Heinrich Schuchardt
2022-10-03 9:44 ` [PATCH 2/2] efi_driver: fix error handling Heinrich Schuchardt
0 siblings, 2 replies; 7+ messages in thread
From: Heinrich Schuchardt @ 2022-10-03 9:44 UTC (permalink / raw)
To: Ilias Apalodimas; +Cc: u-boot, AKASHI Takahiro, Heinrich Schuchardt
Fix the error handling in the start() method of the EFI block device
driver.
Heinrich Schuchardt (2):
efi_loader: function to unlink udevice and handle
efi_driver: fix error handling
include/efi_driver.h | 2 +-
include/efi_loader.h | 1 +
lib/efi_driver/efi_block_device.c | 60 +++++++++++++++++--------------
lib/efi_driver/efi_uclass.c | 23 ++++++------
lib/efi_loader/efi_helper.c | 19 ++++++++++
5 files changed, 66 insertions(+), 39 deletions(-)
--
2.37.2
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] efi_loader: function to unlink udevice and handle
2022-10-03 9:44 [PATCH 0/2] efi_driver: fix error handling Heinrich Schuchardt
@ 2022-10-03 9:44 ` Heinrich Schuchardt
2022-10-03 10:20 ` Ilias Apalodimas
2022-10-03 9:44 ` [PATCH 2/2] efi_driver: fix error handling Heinrich Schuchardt
1 sibling, 1 reply; 7+ messages in thread
From: Heinrich Schuchardt @ 2022-10-03 9:44 UTC (permalink / raw)
To: Ilias Apalodimas; +Cc: u-boot, AKASHI Takahiro, Heinrich Schuchardt
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;
+ handle->dev = NULL;
+
+ return 0;
+}
--
2.37.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] efi_driver: fix error handling
2022-10-03 9:44 [PATCH 0/2] efi_driver: fix error handling Heinrich Schuchardt
2022-10-03 9:44 ` [PATCH 1/2] efi_loader: function to unlink udevice and handle Heinrich Schuchardt
@ 2022-10-03 9:44 ` Heinrich Schuchardt
2022-10-03 10:18 ` Ilias Apalodimas
1 sibling, 1 reply; 7+ messages in thread
From: Heinrich Schuchardt @ 2022-10-03 9:44 UTC (permalink / raw)
To: Ilias Apalodimas; +Cc: u-boot, AKASHI Takahiro, Heinrich Schuchardt
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;
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;
}
/* 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] efi_driver: fix error handling
2022-10-03 9:44 ` [PATCH 2/2] efi_driver: fix error handling Heinrich Schuchardt
@ 2022-10-03 10:18 ` Ilias Apalodimas
2022-10-03 11:01 ` Heinrich Schuchardt
0 siblings, 1 reply; 7+ messages in thread
From: Ilias Apalodimas @ 2022-10-03 10:18 UTC (permalink / raw)
To: Heinrich Schuchardt; +Cc: u-boot, AKASHI Takahiro
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?
>
> 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)
> }
>
> /* 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] efi_loader: function to unlink udevice and handle
2022-10-03 9:44 ` [PATCH 1/2] efi_loader: function to unlink udevice and handle Heinrich Schuchardt
@ 2022-10-03 10:20 ` Ilias Apalodimas
2022-10-03 10:43 ` Heinrich Schuchardt
0 siblings, 1 reply; 7+ messages in thread
From: Ilias Apalodimas @ 2022-10-03 10:20 UTC (permalink / raw)
To: Heinrich Schuchardt; +Cc: u-boot, AKASHI Takahiro
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] efi_loader: function to unlink udevice and handle
2022-10-03 10:20 ` Ilias Apalodimas
@ 2022-10-03 10:43 ` Heinrich Schuchardt
0 siblings, 0 replies; 7+ messages in thread
From: Heinrich Schuchardt @ 2022-10-03 10:43 UTC (permalink / raw)
To: Ilias Apalodimas; +Cc: u-boot, AKASHI Takahiro
On 10/3/22 12:20, Ilias Apalodimas wrote:
> 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?
dev_tag_del() returns -EINVAL if !dev. We should not check twice.
> Would it make sense to add an if here and use this function everywhere
> instead of sprinkling dev_tag_del() around?
We should move the calls in lib/efi_loader/efi_disk.c to
efi_delete_handle() to make sure that we clean up.
Best regards
Heinrich
>
>> + handle->dev = NULL;
>> +
>> + return 0;
>> +}
>> --
>> 2.37.2
>>
>
> Thanks
> /Ilias
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] efi_driver: fix error handling
2022-10-03 10:18 ` Ilias Apalodimas
@ 2022-10-03 11:01 ` Heinrich Schuchardt
0 siblings, 0 replies; 7+ messages in thread
From: Heinrich Schuchardt @ 2022-10-03 11:01 UTC (permalink / raw)
To: Ilias Apalodimas; +Cc: u-boot, AKASHI Takahiro
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-10-03 11:01 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-03 9:44 [PATCH 0/2] efi_driver: fix error handling Heinrich Schuchardt
2022-10-03 9:44 ` [PATCH 1/2] efi_loader: function to unlink udevice and handle Heinrich Schuchardt
2022-10-03 10:20 ` Ilias Apalodimas
2022-10-03 10:43 ` Heinrich Schuchardt
2022-10-03 9:44 ` [PATCH 2/2] efi_driver: fix error handling Heinrich Schuchardt
2022-10-03 10:18 ` Ilias Apalodimas
2022-10-03 11:01 ` Heinrich Schuchardt
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.