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

AKASHI Takahiro
2021-10-11T06:52:46Z

On Sun, Oct 10, 2021 at 08:14:21AM -0600, Simon Glass wrote:
> On Thu, 30 Sept 2021 at 23:04, AKASHI Takahiro
> <takahiro.akashi@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@linaro.org>
> > ---
> > include/efi_loader.h | 2 +
> > lib/efi_loader/efi_disk.c | 92 +++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 94 insertions(+)
> >
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> But some nits below.
>
> Don't worry about !CONFIG_BLK - that code should be removed.

Yes. I added a tentative patch to remove !CONFIG_BLK code in efi_disk
in patch#13.

> > 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.

OK.

> > /* 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?

(Will fix: UCLASS_BLK)

> > + * @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

Yeah, but I don't want to assume EFI_SUCCESS is *zero*.

> > + 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
>
> ??

(UCLASS_PARTITION)

> > + * @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)

I will replace all of "dev->parent" with dev_get_parent()
if I will not forget to do that in the next version :)

Thanks,
-Takahiro Akashi

> > + 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