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

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

Simon,

On Mon, Oct 11, 2021 at 08:54:06AM -0600, Simon Glass wrote:
> Hi Takahiro,
> 
> On Mon, 11 Oct 2021 at 00:52, AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > 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*.
> 
> It is defined as 0 in 'Appendix D - Status Code' and cannot change, as
> I understand it. This is one of the things I don't like about the EFI
> code in U-Boot. Presumably the people who wrote the spec defined it as
> 0 to make use of C constructs.

Yeah, I confirmed that, but still want to keep the code
as "ret != EFI_SUCCESS" is used everywhere in UEFI code :)

-Takahiro Akashi


> [..]
> 
> Regards,
> Simon