[RFC 2/7] dm: implement uclass_get_dp_node()

[Simon Glass]

Hi Heinrich,

On Mon, 27 Mar 2023 at 06:27, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
>
> Provide a function to get the EFI device path node representing a
> device.
>
> If implemented, it invokes the uclass driver's get_dp_node() function.
> Otherwise a vendor hardware node is created.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
> ---
>  drivers/core/uclass.c | 26 ++++++++++++++++++++++++++
>  include/dm/uclass.h   |  8 ++++++++
>  include/efi_loader.h  |  7 +++++++
>  3 files changed, 41 insertions(+)

Can you add a new file, like drivers/core/efi.c for this stuff?

You should also pull out the header stuff you need and put it in
include/dm/efi.h since we don't want to include efi_loader.h from
driver/core.

>
> diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c
> index 1762a0796d..153c954ee3 100644
> --- a/drivers/core/uclass.c
> +++ b/drivers/core/uclass.c
> @@ -10,6 +10,7 @@
>
>  #include <common.h>
>  #include <dm.h>
> +#include <efi_loader.h>
>  #include <errno.h>
>  #include <log.h>
>  #include <malloc.h>
> @@ -802,6 +803,31 @@ int uclass_pre_remove_device(struct udevice *dev)
>  }
>  #endif
>
> +#if CONFIG_IS_ENABLED(EFI_LOADER)
> +struct efi_device_path *uclass_get_dp_node(struct udevice *dev)
> +{
> +       struct uclass *uc;
> +       struct efi_device_path_uboot *dp;
> +
> +       uc = dev->uclass;
> +       if (uc->uc_drv->get_dp_node)
> +               return uc->uc_drv->get_dp_node(dev);
> +
> +       dp = efi_alloc(sizeof(struct efi_device_path_uboot));

This should call malloc(), right?

How does this memory get freed? How do we avoid allocating it twice if
it is called twice?

> +       if (!dp)
> +               return NULL;

-ENOMEM ? I suggest changing the function return value to an int.

> +
> +       dp->dp.type = DEVICE_PATH_TYPE_HARDWARE_DEVICE;
> +       dp->dp.sub_type = DEVICE_PATH_SUB_TYPE_VENDOR;
> +       dp->dp.length = sizeof(struct efi_device_path_uboot);
> +       dp->guid = efi_u_boot_guid;
> +       dp->uclass_id = uc->uc_drv->id;
> +       dp->seq_ = dev->seq_;
> +
> +       return &dp->dp;
> +}
> +#endif
> +
>  int uclass_probe_all(enum uclass_id id)
>  {
>         struct udevice *dev;
> diff --git a/include/dm/uclass.h b/include/dm/uclass.h
> index e11637ce4d..f39dbac21d 100644
> --- a/include/dm/uclass.h
> +++ b/include/dm/uclass.h
> @@ -503,3 +503,11 @@ int uclass_id_count(enum uclass_id id);
>              uclass_next_device(&dev))
>
>  #endif
> +
> +/**
> + * uclass_get_dp_node() - get EFI device path node for device
> + *
> + * @dev:       device
> + * Return:     device path node or NULL if out of memory
> + */
> +struct efi_device_path *uclass_get_dp_node(struct udevice *dev);
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index cee04cbb9d..f111bc616d 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -22,6 +22,13 @@
>  struct blk_desc;
>  struct jmp_buf_data;
>
> +struct efi_device_path_uboot {
> +       struct efi_device_path dp;
> +       efi_guid_t guid;
> +       enum uclass_id uclass_id;
> +       int seq_;

What is this member for? Can you please comment this struct?

> +} __packed;
> +
>  static inline int guidcmp(const void *g1, const void *g2)
>  {
>         return memcmp(g1, g2, sizeof(efi_guid_t));
> --
> 2.39.2
>

Regards,
Simon