[RFC] efi_driver: fix a parent issue in efi-created block devices
Simon Glass
sjg at chromium.org
Wed Jul 19 03:08:45 CEST 2023
Hi AKASHI,
On Tue, 18 Jul 2023 at 18:22, AKASHI Takahiro
<takahiro.akashi at 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.akashi at 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?
> + 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
More information about the U-Boot
mailing list