[RFC 14/22] efi_loader: disk: a helper function to create efi_disk objects from udevice

[Simon Glass]

On Thu, 30 Sept 2021 at 23:04, AKASHI Takahiro
<takahiro.akashi at linaro.org> wrote:
>
> Add efi_disk_create() function.
>
> Any UEFI handle created by efi_disk_create() can be treated as a efi_disk
> object, the udevice is either a UCLASS_BLK (a whole raw disk) or
> UCLASS_PARTITION (a disk partition).
>
> So this function is expected to be called every time such an udevice
> is detected and activated through a device model's "probe" interface.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> ---
>  include/efi_loader.h      |  2 +
>  lib/efi_loader/efi_disk.c | 92 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 94 insertions(+)
>

Reviewed-by: Simon Glass <sjg at chromium.org>

But some nits below.

Don't worry about !CONFIG_BLK - that code should be removed.

> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index c440962fe522..751fde7fb153 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -517,6 +517,8 @@ efi_status_t EFIAPI efi_convert_pointer(efi_uintn_t debug_disposition,
>  void efi_carve_out_dt_rsv(void *fdt);
>  /* Called by bootefi to make console interface available */
>  efi_status_t efi_console_register(void);
> +/* Called when a block devices has been probed */
> +int efi_disk_create(struct udevice *dev);

Please buck the trend in this file and add a full function comment. In
this case it needs to cover @dev and the return value and also explain
what the function does.

>  /* Called by bootefi to make all disk storage accessible as EFI objects */
>  efi_status_t efi_disk_register(void);
>  /* Called by efi_init_obj_list() to install EFI_RNG_PROTOCOL */
> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> index cd5528046251..3fae40e034fb 100644
> --- a/lib/efi_loader/efi_disk.c
> +++ b/lib/efi_loader/efi_disk.c
> @@ -10,6 +10,7 @@
>  #include <common.h>
>  #include <blk.h>
>  #include <dm.h>
> +#include <dm/device-internal.h>
>  #include <efi_loader.h>
>  #include <fs.h>
>  #include <log.h>
> @@ -484,6 +485,7 @@ error:
>         return ret;
>  }
>
> +#ifndef CONFIG_BLK
>  /**
>   * efi_disk_create_partitions() - create handles and protocols for partitions
>   *
> @@ -531,6 +533,96 @@ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc,
>
>         return disks;
>  }
> +#endif /* CONFIG_BLK */
> +
> +/*
> + * Create a handle for a whole raw disk
> + *
> + * @dev                uclass device

?? what type of device?

> + * @return     0 on success, -1 otherwise
> + */
> +static int efi_disk_create_raw(struct udevice *dev)
> +{
> +       struct efi_disk_obj *disk;
> +       struct blk_desc *desc;
> +       const char *if_typename;
> +       int diskid;
> +       efi_status_t ret;
> +
> +       desc = dev_get_uclass_plat(dev);
> +       if_typename = blk_get_if_type_name(desc->if_type);
> +       diskid = desc->devnum;
> +
> +       ret = efi_disk_add_dev(NULL, NULL, if_typename, desc,
> +                              diskid, NULL, 0, &disk);
> +       if (ret != EFI_SUCCESS) {

if (ret)

is much shorter and easier to read

> +               log_err("Adding disk %s%d failed\n", if_typename, diskid);
> +               return -1;
> +       }
> +       disk->dev = dev;
> +       dev->efi_obj = &disk->header;
> +
> +       return 0;
> +}
> +
> +/*
> + * Create a handle for a disk partition
> + *
> + * @dev                uclass device

??

> + * @return     0 on success, -1 otherwise
> + */
> +static int efi_disk_create_part(struct udevice *dev)
> +{
> +       efi_handle_t parent;
> +       struct blk_desc *desc;
> +       const char *if_typename;
> +       struct disk_part *part_data;
> +       struct disk_partition *info;
> +       unsigned int part;
> +       int diskid;
> +       struct efi_device_path *dp = NULL;
> +       struct efi_disk_obj *disk;
> +       efi_status_t ret;
> +
> +       parent = dev->parent->efi_obj;

dev_get_parent(dev)

> +       desc = dev_get_uclass_plat(dev->parent);
> +       if_typename = blk_get_if_type_name(desc->if_type);
> +       diskid = desc->devnum;
> +
> +       part_data = dev_get_uclass_plat(dev);
> +       part = part_data->partnum;
> +       info = &part_data->gpt_part_info;
> +
> +       /* TODO: should not use desc? */
> +       dp = efi_dp_from_part(desc, 0);
> +
> +       ret = efi_disk_add_dev(parent, dp, if_typename, desc, diskid,
> +                              info, part, &disk);
> +       if (ret != EFI_SUCCESS) {
> +               log_err("Adding partition %s%d:%x failed\n",
> +                       if_typename, diskid, part);
> +               return -1;
> +       }
> +       disk->dev = dev;
> +       dev->efi_obj = &disk->header;
> +
> +       return 0;
> +}
> +
> +int efi_disk_create(struct udevice *dev)
> +{
> +       enum uclass_id id;
> +
> +       id = device_get_uclass_id(dev);
> +
> +       if (id == UCLASS_BLK)
> +               return efi_disk_create_raw(dev);
> +
> +       if (id == UCLASS_PARTITION)
> +               return efi_disk_create_part(dev);
> +
> +       return -1;
> +}
>
>  /**
>   * efi_disk_register() - register block devices
> --
> 2.33.0
>

Regards,
Simon