[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 at 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