Re: [PATCH 1/1] efi_driver: carve out function to create block device

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

Hi Heinrich, 

On Tue, Oct 04, 2022 at 04:29:25PM +0200, Heinrich Schuchardt wrote:
> * Carve out function efi_bl_create_block_device() from efi_bl_bind().
> * Add a check for U-Boot devices to efi_bl_bind().

What the code does is obvious here,  can we slightly change the commit
message and explain why this is needed?  Readability?

> 
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  lib/efi_driver/efi_block_device.c | 32 ++++++++++++++++++++++++-------
>  lib/efi_driver/efi_uclass.c       |  9 ---------
>  2 files changed, 25 insertions(+), 16 deletions(-)
> 
> diff --git a/lib/efi_driver/efi_block_device.c b/lib/efi_driver/efi_block_device.c
> index 9ccc148590..3103040870 100644
> --- a/lib/efi_driver/efi_block_device.c
> +++ b/lib/efi_driver/efi_block_device.c
> @@ -114,21 +114,16 @@ static ulong efi_bl_write(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt,
>   * @interface:	block io protocol
>   * Return:	status code
>   */
> -static efi_status_t efi_bl_bind(efi_handle_t handle, void *interface)
> +static efi_status_t
> +efi_bl_create_block_device(efi_handle_t handle, void *interface)
>  {
>  	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;
>  	struct efi_blk_plat *plat;
>  
> -	EFI_PRINT("%s: handle %p, interface %p\n", __func__, handle, io);
> -
> -	if (!obj || !interface)
> -		return EFI_INVALID_PARAMETER;
> -
>  	devnum = blk_find_max_devnum(UCLASS_EFI_LOADER);
>  	if (devnum == -ENODEV)
>  		devnum = 0;
> @@ -176,6 +171,29 @@ err:
>  	return ret;
>  }
>  
> +/**
> + * efi_bl_bind() - bind to a block io protocol
> + *
> + * @handle:	handle
> + * @interface:	block io protocol
> + * Return:	status code
> + */
> +static efi_status_t efi_bl_bind(efi_handle_t handle, void *interface)
> +{
> +	efi_status_t ret;
> +	struct efi_object *obj = efi_search_obj(handle);
> +
> +	EFI_PRINT("%s: handle %p, interface %p\n", __func__, handle, interface);
> +
> +	if (!obj || !interface)
> +		return EFI_INVALID_PARAMETER;
> +
> +	if (!handle->dev)
> +		ret = efi_bl_create_block_device(handle, interface);

Keeping in mind devices should usually be bound once (unless we reinit the
subsystem) should we print something here?
Also ret need to be initialized on the declaration.

> +
> +	return ret;
> +}
> +
>  /* Block device driver operators */
>  static const struct blk_ops efi_blk_ops = {
>  	.read	= efi_bl_read,
> -- 
> 2.37.2
> 

Thanks
/Ilias