Re: [RFC] efi_driver: fix a parent issue in efi-created block devices

[Heinrich Schuchardt]

On 7/19/23 03:08, Simon Glass wrote:

Hi AKASHI,

On Tue, 18 Jul 2023 at 18:22, AKASHI Takahiro
<takahiro.aka...@linaro.org> wrote:

An EFI application may create an EFI block device (EFI_BLOCK_IO_PROTOCOL) in
EFI world, which in turn generates a corresponding U-Boot block device based on
U-Boot's Driver Model.
The latter device, however, doesn't work as U-Boot proper block device
due to an issue in efi_driver's implementation. We saw discussions in the past,
most recently in [1].

   [1] https://lists.denx.de/pipermail/u-boot/2023-July/522565.html

This RFC patch tries to address (part of) the issue.
If it is considered acceptable, I will create a formal patch.

Withtout this patch,
===8<===
=> env set efi_selftest 'block device'
=> bootefi selftest
  ...

Summary: 0 failures

=> dm tree
  Class     Index  Probed  Driver                Name
-----------------------------------------------------------
  root          0  [ + ]   root_driver           root_driver
  ...
  bootmeth      7  [   ]   vbe_simple            |   `-- vbe_simple
  blk           0  [ + ]   efi_blk               `-- efiblk#0
  partition     0  [ + ]   blk_partition             `-- efiblk#0:1
=> ls efiloader 0:1
** Bad device specification efiloader 0 **
Couldn't find partition efiloader 0:1
===>8===

With this patch applied, efiblk#0(:1) now gets accessible.

===8<===
=> env set efi_selftest 'block device'
=> bootefi selftest
  ...

Summary: 0 failures

=> dm tree
  Class     Index  Probed  Driver                Name
-----------------------------------------------------------
  root          0  [ + ]   root_driver           root_driver
  ...
  bootmeth      7  [   ]   vbe_simple            |   `-- vbe_simple
  efi           0  [ + ]   EFI block driver      `-- 
/VenHw(dbca4c98-6cb0-694d-0872-819c650cb7b8)
  blk           0  [ + ]   efi_blk                   `-- efiblk#0
  partition     0  [ + ]   blk_partition                 `-- efiblk#0:1
=> ls efiloader 0:1
        13   hello.txt
         7   u-boot.txt

2 file(s), 0 dir(s)
===>8===

Signed-off-by: AKASHI Takahiro <takahiro.aka...@linaro.org>
---
  include/efi_driver.h                         |  2 +-
  lib/efi_driver/efi_block_device.c            | 17 ++++++++++++-----
  lib/efi_driver/efi_uclass.c                  |  8 +++++++-
  lib/efi_selftest/efi_selftest_block_device.c |  2 ++
  4 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/include/efi_driver.h b/include/efi_driver.h
index 63a95e4cf800..ed66796f3519 100644
--- a/include/efi_driver.h
+++ b/include/efi_driver.h
@@ -42,7 +42,7 @@ struct efi_driver_ops {
         const efi_guid_t *child_protocol;
         efi_status_t (*init)(struct efi_driver_binding_extended_protocol 
*this);
         efi_status_t (*bind)(struct efi_driver_binding_extended_protocol *this,
-                            efi_handle_t handle, void *interface);
+                            efi_handle_t handle, void *interface, char *name);
  };

  #endif /* _EFI_DRIVER_H */
diff --git a/lib/efi_driver/efi_block_device.c 
b/lib/efi_driver/efi_block_device.c
index add00eeebbea..43b7ed7c973c 100644
--- a/lib/efi_driver/efi_block_device.c
+++ b/lib/efi_driver/efi_block_device.c
@@ -115,9 +115,9 @@ static ulong efi_bl_write(struct udevice *dev, lbaint_t 
blknr, lbaint_t blkcnt,
   * Return:     status code
   */
  static efi_status_t
-efi_bl_create_block_device(efi_handle_t handle, void *interface)
+efi_bl_create_block_device(efi_handle_t handle, void *interface, struct 
udevice *parent)
  {
-       struct udevice *bdev = NULL, *parent = dm_root();
+       struct udevice *bdev = NULL;
         efi_status_t ret;
         int devnum;
         char *name;
@@ -181,7 +181,7 @@ err:
   */
  static efi_status_t efi_bl_bind(
                         struct efi_driver_binding_extended_protocol *this,
-                       efi_handle_t handle, void *interface)
+                       efi_handle_t handle, void *interface, char *name)
  {
         efi_status_t ret = EFI_SUCCESS;
         struct efi_object *obj = efi_search_obj(handle);
@@ -191,8 +191,15 @@ static efi_status_t efi_bl_bind(
         if (!obj || !interface)
                 return EFI_INVALID_PARAMETER;

-       if (!handle->dev)
-               ret = efi_bl_create_block_device(handle, interface);
+       if (!handle->dev) {
+               struct udevice *parent;
+
+               ret = device_bind_driver(dm_root(), "EFI block driver", name, 
&parent);

Can you use a non-root device as the parent?

The parent of an iSCSI drive handled by iPXE will be a network
interface. For a RAM drive there will be no physical parent at all.

The design bug is in blk_get_devnum_by_uclass_idname() which instead of
looking at blk_desc->uclass_id looks at the parent.

When will you fix this Simon?

Best regards

Heinrich

+               if (!ret)
+                       ret = efi_bl_create_block_device(handle, interface, 
parent);
+               else
+                       ret = EFI_DEVICE_ERROR;
+       }

         return ret;
  }
diff --git a/lib/efi_driver/efi_uclass.c b/lib/efi_driver/efi_uclass.c
index 45f935198874..bf669742783e 100644
--- a/lib/efi_driver/efi_uclass.c
+++ b/lib/efi_driver/efi_uclass.c
@@ -145,7 +145,13 @@ static efi_status_t EFIAPI efi_uc_start(
         ret = check_node_type(controller_handle);
         if (ret != EFI_SUCCESS)
                 goto err;
-       ret = bp->ops->bind(bp, controller_handle, interface);
+
+       struct efi_handler *handler;
+       char tmpname[256] = "AppName";
+       ret = efi_search_protocol(controller_handle, &efi_guid_device_path,
+                                 &handler);
+       snprintf(tmpname, 256, "%pD", handler->protocol_interface);
+       ret = bp->ops->bind(bp, controller_handle, interface, strdup(tmpname));
         if (ret == EFI_SUCCESS)
                 goto out;

diff --git a/lib/efi_selftest/efi_selftest_block_device.c 
b/lib/efi_selftest/efi_selftest_block_device.c
index a367e8b89d17..0ab8e4590dfe 100644
--- a/lib/efi_selftest/efi_selftest_block_device.c
+++ b/lib/efi_selftest/efi_selftest_block_device.c
@@ -248,6 +248,7 @@ static int teardown(void)
  {
         efi_status_t r = EFI_ST_SUCCESS;

+#if 0 /* Temporarily out for confirmation */
         if (disk_handle) {
                 r = boottime->uninstall_protocol_interface(disk_handle,
                                                            &guid_device_path,
@@ -273,6 +274,7 @@ static int teardown(void)
                         return EFI_ST_FAILURE;
                 }
         }
+#endif
         return r;
  }

--
2.41.0

Otherwise this looks good to me. We should have DM devices for all EFI
ones (in fact EFI ones should just be some extra data on top of DM
ones).

Regards,
Simon