[RESEND v9 1/9] efi_loader: move udevice pointer into struct efi_object

Heinrich Schuchardt xypron.glpk at gmx.de
Wed Jul 20 09:37:40 CEST 2022


On 7/20/22 07:23, Takahiro Akashi wrote:
> On Sun, Jul 17, 2022 at 01:23:41PM +0200, Heinrich Schuchardt wrote:
>> On 7/17/22 10:09, Heinrich Schuchardt wrote:
>>> On 7/15/22 16:47, Masahisa Kojima wrote:
>>>> This is a preparation patch to provide the unified method
>>>> to access udevice pointer associated with the block io device.
>>>> The EFI handles of both EFI block io driver implemented in
>>>> lib/efi_loader/efi_disk.c and EFI block io driver implemented
>>>> as EFI payload can posess the udevice pointer in the struct efi_object.
>>>>
>>>> We can use this udevice pointer to get the U-Boot friendly
>>>> block device name(e.g. mmc 0:1, nvme 0:1) through efi_handle_t.
>>>>
>>>> Signed-off-by: Masahisa Kojima <masahisa.kojima at linaro.org>
>>>> ---
>>>> Newly created in v9
>>>>
>>>>    include/efi_loader.h      |  8 ++++++++
>>>>    lib/efi_loader/efi_disk.c | 20 +++++++++++++-------
>>>>    2 files changed, 21 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>>> index 3a63a1f75f..bba5ffd482 100644
>>>> --- a/include/efi_loader.h
>>>> +++ b/include/efi_loader.h
>>>> @@ -226,6 +226,12 @@ const char *__efi_nesting_dec(void);
>>>>    #define EFI_CACHELINE_SIZE 128
>>>>    #endif
>>>>
>>>> +/**
>>>> + * efi_handle_to_udev - accessor to the DM device associated to the
>>>> EFI handle
>>>> + * @handle:    pointer to the EFI handle
>>>> + */
>>>> +#define efi_handle_to_udev(handle) (((struct efi_object *)handle)->dev)
>>>
>>> This conversion will hide errors if handle is not of type efi_handle_t.
>>> We should avoid the conversion and see build time errors instead.
>>> Please, remove the macro.
>>>
>>> For every handle of type efi_handle_t you can access the field
>>> handle->dev directly.
>>>
>>> For struct efi_disk_obj we can use disk->header.dev.
>>>
>>>> +
>>>>    /* Key identifying current memory map */
>>>>    extern efi_uintn_t efi_memory_map_key;
>>>>
>>>> @@ -375,6 +381,7 @@ enum efi_object_type {
>>>>     * @protocols:    linked list with the protocol interfaces installed
>>>> on this
>>>>     *        handle
>>>>     * @type:    image type if the handle relates to an image
>>>> + * @dev:    pointer to the DM device which is associated with this
>>>> EFI handle
>>>>     *
>>>>     * UEFI offers a flexible and expandable object model. The objects
>>>> in the UEFI
>>>>     * API are devices, drivers, and loaded images. struct efi_object is
>>>> our storage
>>>> @@ -392,6 +399,7 @@ struct efi_object {
>>>>        /* The list of protocols */
>>>>        struct list_head protocols;
>>>>        enum efi_object_type type;
>>>> +    struct udevice *dev;
>>>>    };
>>>>
>>>>    enum efi_image_auth_status {
>>>> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
>>>> index 1d700b2a6b..a8e8521e3e 100644
>>>> --- a/lib/efi_loader/efi_disk.c
>>>> +++ b/lib/efi_loader/efi_disk.c
>>>> @@ -46,7 +46,6 @@ struct efi_disk_obj {
>>>>        struct efi_device_path *dp;
>>>>        unsigned int part;
>>>>        struct efi_simple_file_system_protocol *volume;
>>>> -    struct udevice *dev; /* TODO: move it to efi_object */
>>>
>>> ok
>>>
>>>>    };
>>>>
>>>>    /**
>>>> @@ -124,16 +123,16 @@ static efi_status_t efi_disk_rw_blocks(struct
>>>> efi_block_io *this,
>>>>            return EFI_BAD_BUFFER_SIZE;
>>>>
>>>>        if (CONFIG_IS_ENABLED(PARTITIONS) &&
>>>> -        device_get_uclass_id(diskobj->dev) == UCLASS_PARTITION) {
>>>> +        device_get_uclass_id(efi_handle_to_udev(diskobj)) ==
>>>> UCLASS_PARTITION) {
>>>
>>> device_get_uclass_id(diskobj->header.hdev)) == UCLASS_PARTITION) {
>>>
>>>>            if (direction == EFI_DISK_READ)
>>>> -            n = dev_read(diskobj->dev, lba, blocks, buffer);
>>>> +            n = dev_read(efi_handle_to_udev(diskobj), lba, blocks,
>>>> buffer);
>>>
>>> dev_read(diskobj->header.hdev)
>>>
>>>>            else
>>>> -            n = dev_write(diskobj->dev, lba, blocks, buffer);
>>>> +            n = dev_write(efi_handle_to_udev(diskobj), lba, blocks,
>>>> buffer);
>>>
>>> dev_write(diskobj->header.hdev)
>>>
>>>>        } else {
>>>>            /* dev is a block device (UCLASS_BLK) */
>>>>            struct blk_desc *desc;
>>>>
>>>> -        desc = dev_get_uclass_plat(diskobj->dev);
>>>> +        desc = dev_get_uclass_plat(efi_handle_to_udev(diskobj));
>>>
>>> dev_get_uclass(diskobj->header.hdev)
>>>
>>>
>>>>            if (direction == EFI_DISK_READ)
>>>>                n = blk_dread(desc, lba, blocks, buffer);
>>>>            else
>>>> @@ -552,7 +551,7 @@ static int efi_disk_create_raw(struct udevice *dev)
>>>>
>>>>            return -1;
>>>>        }
>>>> -    disk->dev = dev;
>>>> +    efi_handle_to_udev(disk) = dev;
>>>>        if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) {
>>>>            efi_free_pool(disk->dp);
>>>>            efi_delete_handle(&disk->header);
>>>> @@ -609,7 +608,7 @@ static int efi_disk_create_part(struct udevice *dev)
>>>>            log_err("Adding partition for %s failed\n", dev->name);
>>>>            return -1;
>>>>        }
>>>> -    disk->dev = dev;
>>>> +    efi_handle_to_udev(disk) = dev;
>>>
>>> disk->header.dev = dev;
>>>
>>>>        if (dev_tag_set_ptr(dev, DM_TAG_EFI, &disk->header)) {
>>>>            efi_free_pool(disk->dp);
>>>>            efi_delete_handle(&disk->header);
>>>> @@ -656,6 +655,13 @@ static int efi_disk_probe(void *ctx, struct event
>>>> *event)
>>>>            ret = efi_disk_create_raw(dev);
>>>>            if (ret)
>>>>                return -1;
>>>> +    } else {
>>>> +        efi_handle_t handle;
>>>> +
>>>> +        if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle))
>>
>> Setting handle->dev can be done more easily in efi_bl_bind():
>
> I don't think so.
> "dev" field should be maintained in *one* place, i.e. efi_disk_probe(),
> which is uniquely called from probe event (even for efi_block_dev case).
>
> To make this clear, we'd better put the code in efi_disk_create_raw():
>    efi_disk_create_raw()
>        if (desc->if_type == IF_TYPE_EFI_LOADER) {
>            /* handle should exist now */
>            dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle));
>            efi_set_udev(handle, dev);
>            return 0;
>        }
>
>        /* normal block devices */
>        ...
>        efi_set_udev(&disk->header, dev);
>        ...

Linking devices and handles is not block device specific.

If we would properly link all devices and handles, we would be able to
generate device paths by walking the driver model device tree. This is
the direction into which our future development should go.

Can we have a wrapper around dev_tag_set_ptr(dev, DM_TAG_EFI, handle)
which takes care of setting handle->dev and the tag replacing all
current invocations:

int efi_link_dev(efi_handle_t handle, udevice dev) {
	handle->dev = dev;
	return dev_tag_set_ptr(dev, DM_TAG_EFI, handle);
}

Best regards

Heinrich

>
>
> -Takahiro Akashi
>
>> handle->dev = bdev;
>>
>> We can further remove the field handle from struct blk_create_device as
>> it is now available in handle->dev.
>>
>> Best regards
>>
>> Heinrich
>>
>>>> +            return -1;
>>>> +
>>>> +        efi_handle_to_udev(handle) = dev;
>>>
>>> handle->dev = dev;
>>>
>>> Best regards
>>>
>>> Heinrich
>>>
>>>>        }
>>>>
>>>>        device_foreach_child(child, dev) {
>>>
>>



More information about the U-Boot mailing list