[PATCH 0/2] efi_driver: fix error handling

[PATCH 0/2] efi_driver: fix error handling

[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

[PATCH 1/2] efi_loader: function to unlink udevice and handle

[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

[PATCH 2/2] efi_driver: fix error handling

[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

Re: [PATCH 2/2] efi_driver: fix error handling

[Ilias Apalodimas]

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

Re: [PATCH 1/2] efi_loader: function to unlink udevice and handle

[Ilias Apalodimas]

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

Re: [PATCH 1/2] efi_loader: function to unlink udevice and handle

[Heinrich Schuchardt]

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

Re: [PATCH 2/2] efi_driver: fix error handling

[Heinrich Schuchardt]

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