[PATCH 1/1] efi_loader: improve block device integration with DM

[PATCH 1/1] efi_loader: improve block device integration with DM

[Heinrich Schuchardt]

Up to now when devices became available after executing the UEFI
sub-system initialization where not available for EFI applications.

With the patch block devices are added to the UEFI object list whenever
they are probed.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 drivers/core/device.c               |  7 +++
 include/efi_loader.h                |  6 +++
 lib/efi_driver/Makefile             |  1 +
 lib/efi_driver/efi_dm_integration.c | 36 +++++++++++++++
 lib/efi_loader/efi_disk.c           | 72 +++++++++++++++++------------
 5 files changed, 93 insertions(+), 29 deletions(-)
 create mode 100644 lib/efi_driver/efi_dm_integration.c

diff --git a/drivers/core/device.c b/drivers/core/device.c
index cb960f8ec4..7355a5c2a9 100644
--- a/drivers/core/device.c
+++ b/drivers/core/device.c
@@ -14,6 +14,7 @@
 #include <asm/global_data.h>
 #include <asm/io.h>
 #include <clk.h>
+#include <efi_loader.h>
 #include <fdtdec.h>
 #include <fdt_support.h>
 #include <malloc.h>
@@ -579,6 +580,12 @@ int device_probe(struct udevice *dev)
 	if (dev->parent && device_get_uclass_id(dev) == UCLASS_PINCTRL)
 		pinctrl_select_state(dev, "default");

+	if (CONFIG_IS_ENABLED(EFI_LOADER)) {
+		ret = efi_post_probe_device(dev);
+		if (ret)
+			goto fail_uclass;
+	}
+
 	return 0;
 fail_uclass:
 	if (device_remove(dev, DM_REMOVE_NORMAL)) {
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 0a9c82a257..78dd687913 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -17,6 +17,7 @@
 #include <pe.h>

 struct blk_desc;
+struct udevice;

 static inline int guidcmp(const void *g1, const void *g2)
 {
@@ -28,6 +29,9 @@ static inline void *guidcpy(void *dst, const void *src)
 	return memcpy(dst, src, sizeof(efi_guid_t));
 }

+/* Called by device_probe() */
+int efi_post_probe_device(struct udevice *dev);
+
 /* No need for efi loader support in SPL */
 #if CONFIG_IS_ENABLED(EFI_LOADER)

@@ -420,6 +424,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 */
+efi_status_t efi_block_device_register(struct udevice *dev);
 /* 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_driver/Makefile b/lib/efi_driver/Makefile
index 83baa1c9a4..f0d5fa5074 100644
--- a/lib/efi_driver/Makefile
+++ b/lib/efi_driver/Makefile
@@ -5,6 +5,7 @@
 # This file only gets included with CONFIG_EFI_LOADER set, so all
 # object inclusion implicitly depends on it

+obj-y += efi_dm_integration.o
 obj-y += efi_uclass.o
 ifeq ($(CONFIG_BLK)$(CONFIG_PARTITIONS),yy)
 obj-y += efi_block_device.o
diff --git a/lib/efi_driver/efi_dm_integration.c b/lib/efi_driver/efi_dm_integration.c
new file mode 100644
index 0000000000..9c6c339473
--- /dev/null
+++ b/lib/efi_driver/efi_dm_integration.c
@@ -0,0 +1,36 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2021, Heinrich Schuchardt <xypron.glpk@gmx.de>
+ */
+
+#define LOG_CATEGORY LOGC_EFI
+
+#include <common.h>
+#include <dm.h>
+#include <efi_loader.h>
+#include <log.h>
+
+/**
+ * efi_post_probe_device() - set up handle for probed device
+ *
+ * This function is called by device_probe(). After the UEFI sub-system is
+ * initialized this function adds handles for new devices.
+ *
+ * @dev:	probed device
+ * Return:	0 on success
+ */
+int efi_post_probe_device(struct udevice *dev)
+{
+	if (!dev || !dev->uclass)
+		return -EINVAL;
+
+	switch (dev->uclass->uc_drv->id) {
+	case UCLASS_BLK:
+		if (efi_block_device_register(dev) != EFI_SUCCESS)
+			log_err("Failed to register %s\n", dev->name);
+	default:
+		break;
+	}
+
+	return 0;
+}
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
index 988907ecb9..b798cab345 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>
@@ -535,6 +536,41 @@ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc,
 	return disks;
 }

+/**
+ * efi_block_device_register() - register a block device in the UEFI sub-system
+ *
+ * @dev:	block device
+ * Return:	status code
+ */
+efi_status_t efi_block_device_register(struct udevice *dev)
+{
+	struct blk_desc *desc = dev_get_uclass_plat(dev);
+	const char *if_typename = blk_get_if_type_name(desc->if_type);
+	struct efi_disk_obj *disk;
+	efi_status_t ret;
+
+	/* Add block device for the full device */
+	ret = device_probe(dev);
+	if (ret)
+		return EFI_NOT_FOUND;
+	log_info("Scanning disk %s...\n", dev->name);
+	ret = efi_disk_add_dev(NULL, NULL, if_typename,
+				desc, desc->devnum, NULL, 0, &disk);
+	if (ret == EFI_NOT_READY) {
+		log_notice("Disk %s not ready\n", dev->name);
+		return ret;
+	} else if (ret != EFI_SUCCESS) {
+		log_err("ERROR: failure to add disk device %s, r = %lu\n",
+			dev->name, ret & ~EFI_ERROR_MASK);
+		return ret;
+	}
+
+	/* Partitions show up as block devices in EFI */
+	efi_disk_create_partitions(&disk->header, desc, if_typename,
+				   desc->devnum, dev->name);
+	return ret;
+}
+
 /**
  * efi_disk_register() - register block devices
  *
@@ -552,38 +588,16 @@ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc,
  */
 efi_status_t efi_disk_register(void)
 {
-	struct efi_disk_obj *disk;
-	int disks = 0;
-	efi_status_t ret;
 #ifdef CONFIG_BLK
 	struct udevice *dev;
-
+	/* Probe all block devices */
 	for (uclass_first_device_check(UCLASS_BLK, &dev); dev;
-	     uclass_next_device_check(&dev)) {
-		struct blk_desc *desc = dev_get_uclass_plat(dev);
-		const char *if_typename = blk_get_if_type_name(desc->if_type);
-
-		/* Add block device for the full device */
-		log_info("Scanning disk %s...\n", dev->name);
-		ret = efi_disk_add_dev(NULL, NULL, if_typename,
-					desc, desc->devnum, NULL, 0, &disk);
-		if (ret == EFI_NOT_READY) {
-			log_notice("Disk %s not ready\n", dev->name);
-			continue;
-		}
-		if (ret) {
-			log_err("ERROR: failure to add disk device %s, r = %lu\n",
-				dev->name, ret & ~EFI_ERROR_MASK);
-			return ret;
-		}
-		disks++;
-
-		/* Partitions show up as block devices in EFI */
-		disks += efi_disk_create_partitions(
-					&disk->header, desc, if_typename,
-					desc->devnum, dev->name);
-	}
+	     uclass_next_device_check(&dev))
+		;
 #else
+	struct efi_disk_obj *disk;
+	int disks = 0;
+	efi_status_t ret;
 	int i, if_type;

 	/* Search for all available disk devices */
@@ -630,8 +644,8 @@ efi_status_t efi_disk_register(void)
 						 if_typename, i, devname);
 		}
 	}
-#endif
 	log_info("Found %d disks\n", disks);
+#endif

 	return EFI_SUCCESS;
 }
--
2.30.2

Re: [PATCH 1/1] efi_loader: improve block device integration with DM

[Matwey V. Kornilov]

Hi Heinrich,


This patch fixes my issue with am335x: EFI Grub works as expected now.


чт, 17 июн. 2021 г. в 18:15, Heinrich Schuchardt <xypron.glpk@gmx.de>:
>
> Up to now when devices became available after executing the UEFI
> sub-system initialization where not available for EFI applications.
>
> With the patch block devices are added to the UEFI object list whenever
> they are probed.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

Tested-by: Matwey V. Kornilov <matwey.kornilov@gmail.com>


> ---
>  drivers/core/device.c               |  7 +++
>  include/efi_loader.h                |  6 +++
>  lib/efi_driver/Makefile             |  1 +
>  lib/efi_driver/efi_dm_integration.c | 36 +++++++++++++++
>  lib/efi_loader/efi_disk.c           | 72 +++++++++++++++++------------
>  5 files changed, 93 insertions(+), 29 deletions(-)
>  create mode 100644 lib/efi_driver/efi_dm_integration.c
>
> diff --git a/drivers/core/device.c b/drivers/core/device.c
> index cb960f8ec4..7355a5c2a9 100644
> --- a/drivers/core/device.c
> +++ b/drivers/core/device.c
> @@ -14,6 +14,7 @@
>  #include <asm/global_data.h>
>  #include <asm/io.h>
>  #include <clk.h>
> +#include <efi_loader.h>
>  #include <fdtdec.h>
>  #include <fdt_support.h>
>  #include <malloc.h>
> @@ -579,6 +580,12 @@ int device_probe(struct udevice *dev)
>         if (dev->parent && device_get_uclass_id(dev) == UCLASS_PINCTRL)
>                 pinctrl_select_state(dev, "default");
>
> +       if (CONFIG_IS_ENABLED(EFI_LOADER)) {
> +               ret = efi_post_probe_device(dev);
> +               if (ret)
> +                       goto fail_uclass;
> +       }
> +
>         return 0;
>  fail_uclass:
>         if (device_remove(dev, DM_REMOVE_NORMAL)) {
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 0a9c82a257..78dd687913 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -17,6 +17,7 @@
>  #include <pe.h>
>
>  struct blk_desc;
> +struct udevice;
>
>  static inline int guidcmp(const void *g1, const void *g2)
>  {
> @@ -28,6 +29,9 @@ static inline void *guidcpy(void *dst, const void *src)
>         return memcpy(dst, src, sizeof(efi_guid_t));
>  }
>
> +/* Called by device_probe() */
> +int efi_post_probe_device(struct udevice *dev);
> +
>  /* No need for efi loader support in SPL */
>  #if CONFIG_IS_ENABLED(EFI_LOADER)
>
> @@ -420,6 +424,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 */
> +efi_status_t efi_block_device_register(struct udevice *dev);
>  /* 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_driver/Makefile b/lib/efi_driver/Makefile
> index 83baa1c9a4..f0d5fa5074 100644
> --- a/lib/efi_driver/Makefile
> +++ b/lib/efi_driver/Makefile
> @@ -5,6 +5,7 @@
>  # This file only gets included with CONFIG_EFI_LOADER set, so all
>  # object inclusion implicitly depends on it
>
> +obj-y += efi_dm_integration.o
>  obj-y += efi_uclass.o
>  ifeq ($(CONFIG_BLK)$(CONFIG_PARTITIONS),yy)
>  obj-y += efi_block_device.o
> diff --git a/lib/efi_driver/efi_dm_integration.c b/lib/efi_driver/efi_dm_integration.c
> new file mode 100644
> index 0000000000..9c6c339473
> --- /dev/null
> +++ b/lib/efi_driver/efi_dm_integration.c
> @@ -0,0 +1,36 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2021, Heinrich Schuchardt <xypron.glpk@gmx.de>
> + */
> +
> +#define LOG_CATEGORY LOGC_EFI
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <efi_loader.h>
> +#include <log.h>
> +
> +/**
> + * efi_post_probe_device() - set up handle for probed device
> + *
> + * This function is called by device_probe(). After the UEFI sub-system is
> + * initialized this function adds handles for new devices.
> + *
> + * @dev:       probed device
> + * Return:     0 on success
> + */
> +int efi_post_probe_device(struct udevice *dev)
> +{
> +       if (!dev || !dev->uclass)
> +               return -EINVAL;
> +
> +       switch (dev->uclass->uc_drv->id) {
> +       case UCLASS_BLK:
> +               if (efi_block_device_register(dev) != EFI_SUCCESS)
> +                       log_err("Failed to register %s\n", dev->name);
> +       default:
> +               break;
> +       }
> +
> +       return 0;
> +}
> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> index 988907ecb9..b798cab345 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>
> @@ -535,6 +536,41 @@ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc,
>         return disks;
>  }
>
> +/**
> + * efi_block_device_register() - register a block device in the UEFI sub-system
> + *
> + * @dev:       block device
> + * Return:     status code
> + */
> +efi_status_t efi_block_device_register(struct udevice *dev)
> +{
> +       struct blk_desc *desc = dev_get_uclass_plat(dev);
> +       const char *if_typename = blk_get_if_type_name(desc->if_type);
> +       struct efi_disk_obj *disk;
> +       efi_status_t ret;
> +
> +       /* Add block device for the full device */
> +       ret = device_probe(dev);
> +       if (ret)
> +               return EFI_NOT_FOUND;
> +       log_info("Scanning disk %s...\n", dev->name);
> +       ret = efi_disk_add_dev(NULL, NULL, if_typename,
> +                               desc, desc->devnum, NULL, 0, &disk);
> +       if (ret == EFI_NOT_READY) {
> +               log_notice("Disk %s not ready\n", dev->name);
> +               return ret;
> +       } else if (ret != EFI_SUCCESS) {
> +               log_err("ERROR: failure to add disk device %s, r = %lu\n",
> +                       dev->name, ret & ~EFI_ERROR_MASK);
> +               return ret;
> +       }
> +
> +       /* Partitions show up as block devices in EFI */
> +       efi_disk_create_partitions(&disk->header, desc, if_typename,
> +                                  desc->devnum, dev->name);
> +       return ret;
> +}
> +
>  /**
>   * efi_disk_register() - register block devices
>   *
> @@ -552,38 +588,16 @@ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc,
>   */
>  efi_status_t efi_disk_register(void)
>  {
> -       struct efi_disk_obj *disk;
> -       int disks = 0;
> -       efi_status_t ret;
>  #ifdef CONFIG_BLK
>         struct udevice *dev;
> -
> +       /* Probe all block devices */
>         for (uclass_first_device_check(UCLASS_BLK, &dev); dev;
> -            uclass_next_device_check(&dev)) {
> -               struct blk_desc *desc = dev_get_uclass_plat(dev);
> -               const char *if_typename = blk_get_if_type_name(desc->if_type);
> -
> -               /* Add block device for the full device */
> -               log_info("Scanning disk %s...\n", dev->name);
> -               ret = efi_disk_add_dev(NULL, NULL, if_typename,
> -                                       desc, desc->devnum, NULL, 0, &disk);
> -               if (ret == EFI_NOT_READY) {
> -                       log_notice("Disk %s not ready\n", dev->name);
> -                       continue;
> -               }
> -               if (ret) {
> -                       log_err("ERROR: failure to add disk device %s, r = %lu\n",
> -                               dev->name, ret & ~EFI_ERROR_MASK);
> -                       return ret;
> -               }
> -               disks++;
> -
> -               /* Partitions show up as block devices in EFI */
> -               disks += efi_disk_create_partitions(
> -                                       &disk->header, desc, if_typename,
> -                                       desc->devnum, dev->name);
> -       }
> +            uclass_next_device_check(&dev))
> +               ;
>  #else
> +       struct efi_disk_obj *disk;
> +       int disks = 0;
> +       efi_status_t ret;
>         int i, if_type;
>
>         /* Search for all available disk devices */
> @@ -630,8 +644,8 @@ efi_status_t efi_disk_register(void)
>                                                  if_typename, i, devname);
>                 }
>         }
> -#endif
>         log_info("Found %d disks\n", disks);
> +#endif
>
>         return EFI_SUCCESS;
>  }
> --
> 2.30.2
>


--
With best regards,
Matwey V. Kornilov

Re: [PATCH 1/1] efi_loader: improve block device integration with DM

[Simon Glass]

Hi Heinrich,

On Thu, 17 Jun 2021 at 09:20, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Up to now when devices became available after executing the UEFI
> sub-system initialization where not available for EFI applications.
>
> With the patch block devices are added to the UEFI object list whenever
> they are probed.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  drivers/core/device.c               |  7 +++
>  include/efi_loader.h                |  6 +++
>  lib/efi_driver/Makefile             |  1 +
>  lib/efi_driver/efi_dm_integration.c | 36 +++++++++++++++
>  lib/efi_loader/efi_disk.c           | 72 +++++++++++++++++------------
>  5 files changed, 93 insertions(+), 29 deletions(-)
>  create mode 100644 lib/efi_driver/efi_dm_integration.c
>
> diff --git a/drivers/core/device.c b/drivers/core/device.c
> index cb960f8ec4..7355a5c2a9 100644
> --- a/drivers/core/device.c
> +++ b/drivers/core/device.c
> @@ -14,6 +14,7 @@
>  #include <asm/global_data.h>
>  #include <asm/io.h>
>  #include <clk.h>
> +#include <efi_loader.h>
>  #include <fdtdec.h>
>  #include <fdt_support.h>
>  #include <malloc.h>
> @@ -579,6 +580,12 @@ int device_probe(struct udevice *dev)
>         if (dev->parent && device_get_uclass_id(dev) == UCLASS_PINCTRL)
>                 pinctrl_select_state(dev, "default");
>
> +       if (CONFIG_IS_ENABLED(EFI_LOADER)) {
> +               ret = efi_post_probe_device(dev);
> +               if (ret)
> +                       goto fail_uclass;
> +       }

Huge flashing NAK on this.

Way back in the early days I mentioned that UEFI should not have its
own tables but should use driver model directly. It was discussed but
'do it later' was the answer.

To the extend that UEFI needs extra info to be tacked onto devices we
should figure out a consistent way to do it.

What I proposed 5-ish years ago is lots in the mists of time, but it
is something like:

- use driver model for *everything*
- use struct udevice everywhere
- avoid using parallel tables
- things on the UEFI side then become dynamic rather than static
- so there is no need for fixups
- figure out what extra info is needed and come up with a generic way
to attach it to devices

So this is definitely not the right approach.

Regards,
Simon

Re: [PATCH 1/1] efi_loader: improve block device integration with DM

[Matwey V. Kornilov]

Can we just (temporarily) revert

f3866909e350 ("distro_bootcmd: call EFI bootmgr even without having /EFI/boot")

until this issue is resolved properly? Because, currently it seems
like the commit breaking the things was introduced at some point and
cannot be fixed within a reasonable amount of time since it seems that
the issue is related to the EFI implementation design.


сб, 26 июн. 2021 г. в 21:31, Simon Glass <sjg@chromium.org>:
>
> Hi Heinrich,
>
> On Thu, 17 Jun 2021 at 09:20, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> > Up to now when devices became available after executing the UEFI
> > sub-system initialization where not available for EFI applications.
> >
> > With the patch block devices are added to the UEFI object list whenever
> > they are probed.
> >
> > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > ---
> >  drivers/core/device.c               |  7 +++
> >  include/efi_loader.h                |  6 +++
> >  lib/efi_driver/Makefile             |  1 +
> >  lib/efi_driver/efi_dm_integration.c | 36 +++++++++++++++
> >  lib/efi_loader/efi_disk.c           | 72 +++++++++++++++++------------
> >  5 files changed, 93 insertions(+), 29 deletions(-)
> >  create mode 100644 lib/efi_driver/efi_dm_integration.c
> >
> > diff --git a/drivers/core/device.c b/drivers/core/device.c
> > index cb960f8ec4..7355a5c2a9 100644
> > --- a/drivers/core/device.c
> > +++ b/drivers/core/device.c
> > @@ -14,6 +14,7 @@
> >  #include <asm/global_data.h>
> >  #include <asm/io.h>
> >  #include <clk.h>
> > +#include <efi_loader.h>
> >  #include <fdtdec.h>
> >  #include <fdt_support.h>
> >  #include <malloc.h>
> > @@ -579,6 +580,12 @@ int device_probe(struct udevice *dev)
> >         if (dev->parent && device_get_uclass_id(dev) == UCLASS_PINCTRL)
> >                 pinctrl_select_state(dev, "default");
> >
> > +       if (CONFIG_IS_ENABLED(EFI_LOADER)) {
> > +               ret = efi_post_probe_device(dev);
> > +               if (ret)
> > +                       goto fail_uclass;
> > +       }
>
> Huge flashing NAK on this.
>
> Way back in the early days I mentioned that UEFI should not have its
> own tables but should use driver model directly. It was discussed but
> 'do it later' was the answer.
>
> To the extend that UEFI needs extra info to be tacked onto devices we
> should figure out a consistent way to do it.
>
> What I proposed 5-ish years ago is lots in the mists of time, but it
> is something like:
>
> - use driver model for *everything*
> - use struct udevice everywhere
> - avoid using parallel tables
> - things on the UEFI side then become dynamic rather than static
> - so there is no need for fixups
> - figure out what extra info is needed and come up with a generic way
> to attach it to devices
>
> So this is definitely not the right approach.
>
> Regards,
> Simon



-- 
With best regards,
Matwey V. Kornilov

Re: [PATCH 1/1] efi_loader: improve block device integration with DM

[Michael Walle]

Am 2021-07-07 09:45, schrieb Matwey V. Kornilov:
> Can we just (temporarily) revert
> 
> f3866909e350 ("distro_bootcmd: call EFI bootmgr even without having 
> /EFI/boot")
> 
> until this issue is resolved properly? Because, currently it seems
> like the commit breaking the things was introduced at some point and
> cannot be fixed within a reasonable amount of time since it seems that
> the issue is related to the EFI implementation design.

Mh, that would then break booting boards with an EFI boot entry
and without an /EFI/boot/ binary.

Given that this is only a change in the environment, can't you
workaround your issue by changing your environment until there
is a proper fix?

-michael