[RFC] efi_driver: fix a parent issue in efi-created block devices
Heinrich Schuchardt
xypron.glpk at gmx.de
Wed Jul 19 04:12:50 CEST 2023
On 7/19/23 03:08, Simon Glass wrote:
> 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?
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
More information about the U-Boot
mailing list