[RFC] efi_driver: fix a parent issue in efi-created block devices
Heinrich Schuchardt
xypron.glpk at gmx.de
Wed Jul 19 15:15:10 CEST 2023
On 19.07.23 15:04, Simon Glass wrote:
> Hi,
>
> On Tue, 18 Jul 2023 at 19:54, AKASHI Takahiro
> <takahiro.akashi at linaro.org> wrote:
>>
>> Hi Simon,
>>
>> On Tue, Jul 18, 2023 at 07:08:45PM -0600, 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?
>>
>> I have no idea what else can be the parent in this case.
>
> I suggest an EFI_MEDIA device.
>
>>
>> Please note:
>>>> => 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)
>>
>> This "efi" object is created by an EFI application (i.e. efi_selftest_block_device.c)
>> and don't have any practical parent.
>
> Block devices must have a media device as their parent. This seems to
> be a persistent area of confusion...probably when the uclass ID goes
> away from blk_desc it will be more obvious.
Dear Simon,
The only reason why you request to add an otherwise parent device is
that you use it to determine the device class name used in the CLI (mmc,
usb, nvme, ...).
That concept worked fine when all devices had physical parents from
which such an information could be derived.
This is not the case UCLASS_EFI block devices. We should not introduce
any DM devices which have no meaning in the EFI world.
If there is no other benefit, we should do the reasonable and keep a
field in blk_desc and use it to derive the CLI name of the block device.
Best regards
Heinrich
>
>>
>>>> blk 0 [ + ] efi_blk `-- efiblk#0
>>>> partition 0 [ + ] blk_partition `-- efiblk#0:1
>>
>>
>>>> + 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).
>>
>> Unfortunately, in this specific case (efi_block_device.c), UEFI object
>> (handle) is set to be created first, then U-Boot device (efiblk#xxx).
>> So "some extra data on top of DM ones" is not accurate (doesn't reflect
>> the current implementation).
>
> OK, so we should really sort that out :-)
>
>>
>> Please note again that efi_loader/efi_disk.c and efi_driver/efi_block_device.c
>> are totally different things.
>
> OK
>
> Regards,
> Simon
More information about the U-Boot
mailing list