On 1/30/19 6:48 AM, AKASHI Takahiro wrote:
> On Tue, Jan 29, 2019 at 11:33:31PM +0100, Heinrich Schuchardt wrote:
>> On 1/29/19 3:59 AM, AKASHI Takahiro wrote:
>>> efi_disk_create() will initialize efi_disk attributes for each device,
>>> either UCLASS_BLK or UCLASS_PARTITION.
>>>
>>> Currently (temporarily), efi_disk_obj structure is embedded into
>>> blk_desc to hold efi-specific attributes.
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.aka...@linaro.org>
>>> ---
>>> drivers/block/blk-uclass.c | 9 ++
>>> drivers/core/device.c | 3 +
>>> include/blk.h | 24 +++++
>>> include/dm/device.h | 4 +
>>> lib/efi_loader/efi_disk.c | 192 ++++++++++++++++++++++++++-----------
>>> 5 files changed, 174 insertions(+), 58 deletions(-)
>>>
>>> diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
>>> index d4ca30f23fc1..28c45d724113 100644
>>> --- a/drivers/block/blk-uclass.c
>>> +++ b/drivers/block/blk-uclass.c
>>> @@ -657,6 +657,9 @@ UCLASS_DRIVER(blk) = {
>>> .per_device_platdata_auto_alloc_size = sizeof(struct blk_desc),
>>> };
>>>
>>> +/* FIXME */
>>> +extern int efi_disk_create(struct udevice *dev);
>>> +
>>> U_BOOT_DRIVER(blk_partition) = {
>>> .name = "blk_partition",
>>> .id = UCLASS_PARTITION,
>>> @@ -695,6 +698,12 @@ int blk_create_partitions(struct udevice *parent)
>>> part_data->partnum = part;
>>> part_data->gpt_part_info = info;
>>>
>>> + ret = efi_disk_create(dev);
>>> + if (ret) {
>>> + device_unbind(dev);
>>> + return ret;
>>> + }
>>> +
>>> disks++;
>>> }
>>>
>>> diff --git a/drivers/core/device.c b/drivers/core/device.c
>>> index 0d15e5062b66..8625fccb0dcb 100644
>>> --- a/drivers/core/device.c
>>> +++ b/drivers/core/device.c
>>> @@ -67,6 +67,9 @@ static int device_bind_common(struct udevice *parent,
>>> const struct driver *drv,
>>> dev->parent = parent;
>>> dev->driver = drv;
>>> dev->uclass = uc;
>>> +#ifdef CONFIG_EFI_LOADER
>>> + INIT_LIST_HEAD(&dev->efi_obj.protocols);
>>
>> This looks like an incomplete initialization of efi_obj. Why don't we
>> use efi_create_handle.
>
> I think that it is more or less a matter of implementation.
> I will address this issue later if necessary.
>
>> Why should a device be aware of struct efi_obj? We only need a handle to
>> install protocols.
>>
>>> +#endif
>>>
>>> dev->seq = -1;
>>> dev->req_seq = -1;
>>> diff --git a/include/blk.h b/include/blk.h
>>> index d0c033aece0f..405f6f790d66 100644
>>> --- a/include/blk.h
>>> +++ b/include/blk.h
>>> @@ -8,6 +8,7 @@
>>> #define BLK_H
>>>
>>> #include <efi.h>
>>> +#include <efi_api.h>
>>>
>>> #ifdef CONFIG_SYS_64BIT_LBA
>>> typedef uint64_t lbaint_t;
>>> @@ -53,6 +54,26 @@ enum sig_type {
>>> SIG_TYPE_COUNT /* Number of signature types */
>>> };
>>>
>>> +/* FIXME */
>>
>> Fix what?
>
> For simplicity, this data structure was copied from efi_disk.c
> in this initial release.
> As the implementation goes *sophisticated*, some members may go away
> or move somewhere, say in blk_desc itself.
>
>>> +/**
>>> + * struct efi_disk_obj - EFI disk object
>>> + *
>>> + * @ops: EFI disk I/O protocol interface
>>> + * @media: block I/O media information
>>> + * @dp: device path to the block device
>>> + * @part: partition
>>> + * @volume: simple file system protocol of the partition
>>> + * @offset: offset into disk for simple partition
>>> + */
>>> +struct efi_disk_obj {
>>> + struct efi_block_io ops;
>>> + struct efi_block_io_media media;
>>> + struct efi_device_path *dp;
>>> + unsigned int part;
>>> + struct efi_simple_file_system_protocol *volume;
>>> + lbaint_t offset;
>>> +};
>>> +
>>> /*
>>> * With driver model (CONFIG_BLK) this is uclass platform data, accessible
>>> * with dev_get_uclass_platdata(dev)
>>> @@ -92,6 +113,9 @@ struct blk_desc {
>>> * device. Once these functions are removed we can drop this field.
>>> */
>>> struct udevice *bdev;
>>> +#ifdef CONFIG_EFI_LOADER
>>> + struct efi_disk_obj efi_disk;
>>
>> This must be a handle (i.e. a pointer). Otherwise when the last protocol
>> is removed we try to free memory that was never malloc'ed.
>
> Who actually frees?
see UEFI spec for UninstallProtocolInterface():
"If the last protocol interface is removed from a handle, the handle is
freed and is no longer valid."
Best regards
Heinrich
>
>>> +#endif
>>> #else
>>> unsigned long (*block_read)(struct blk_desc *block_dev,
>>> lbaint_t start,
>>> diff --git a/include/dm/device.h b/include/dm/device.h
>>> index 27a6d7b9fdb0..121bfb46b1a0 100644
>>> --- a/include/dm/device.h
>>> +++ b/include/dm/device.h
>>> @@ -12,6 +12,7 @@
>>>
>>> #include <dm/ofnode.h>
>>> #include <dm/uclass-id.h>
>>> +#include <efi_loader.h>
>>> #include <fdtdec.h>
>>> #include <linker_lists.h>
>>> #include <linux/compat.h>
>>> @@ -146,6 +147,9 @@ struct udevice {
>>> #ifdef CONFIG_DEVRES
>>> struct list_head devres_head;
>>> #endif
>>> +#ifdef CONFIG_EFI_LOADER
>>> + struct efi_object efi_obj;
>>> +#endif
>>> };
>>>
>>> /* Maximum sequence number supported */
>>> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
>>> index c037526ad2d0..84fa0ddfba14 100644
>>> --- a/lib/efi_loader/efi_disk.c
>>> +++ b/lib/efi_loader/efi_disk.c
>>> @@ -14,33 +14,6 @@
>>>
>>> const efi_guid_t efi_block_io_guid = BLOCK_IO_GUID;
>>>
>>> -/**
>>> - * struct efi_disk_obj - EFI disk object
>>> - *
>>> - * @header: EFI object header
>>> - * @ops: EFI disk I/O protocol interface
>>> - * @ifname: interface name for block device
>>> - * @dev_index: device index of block device
>>> - * @media: block I/O media information
>>> - * @dp: device path to the block device
>>> - * @part: partition
>>> - * @volume: simple file system protocol of the partition
>>> - * @offset: offset into disk for simple partition
>>> - * @desc: internal block device descriptor
>>> - */
>>> -struct efi_disk_obj {
>>> - struct efi_object header;
>>> - struct efi_block_io ops;
>>> - const char *ifname;
>>> - int dev_index;
>>> - struct efi_block_io_media media;
>>> - struct efi_device_path *dp;
>>> - unsigned int part;
>>> - struct efi_simple_file_system_protocol *volume;
>>> - lbaint_t offset;
>>> - struct blk_desc *desc;
>>> -};
>>> -
>>> static efi_status_t EFIAPI efi_disk_reset(struct efi_block_io *this,
>>> char extended_verification)
>>> {
>>> @@ -64,7 +37,7 @@ static efi_status_t efi_disk_rw_blocks(struct
>>> efi_block_io *this,
>>> unsigned long n;
>>>
>>> diskobj = container_of(this, struct efi_disk_obj, ops);
>>> - desc = (struct blk_desc *) diskobj->desc;
>>> + desc = container_of(diskobj, struct blk_desc, efi_disk);
>>> blksz = desc->blksz;
>>> blocks = buffer_size / blksz;
>>> lba += diskobj->offset;
>>> @@ -217,6 +190,7 @@ efi_fs_from_path(struct efi_device_path *full_path)
>>> return handler->protocol_interface;
>>> }
>>>
>>> +#ifndef CONFIG_BLK
>>> /*
>>> * Create a handle for a partition or disk
>>> *
>>> @@ -343,6 +317,136 @@ int efi_disk_create_partitions(efi_handle_t parent,
>>> struct blk_desc *desc,
>>>
>>> return disks;
>>> }
>>> +#else
>>> +static int efi_disk_create_common(efi_handle_t handle,
>>> + struct efi_disk_obj *disk,
>>> + struct blk_desc *desc)
>>> +{
>>> + efi_status_t ret;
>>> +
>>> + /* Hook up to the device list */
>>> + efi_add_handle(handle);
>>> +
>>> + /* Fill in EFI IO Media info (for read/write callbacks) */
>>> + disk->media.removable_media = desc->removable;
>>> + disk->media.media_present = 1;
>>> + disk->media.block_size = desc->blksz;
>>> + disk->media.io_align = desc->blksz;
>>> + disk->media.last_block = desc->lba - disk->offset;
>>> + disk->ops.media = &disk->media;
>>> +
>>> + /* add protocols */
>>> + disk->ops = block_io_disk_template;
>>> + ret = efi_add_protocol(handle, &efi_block_io_guid, &disk->ops);
>>> + if (ret != EFI_SUCCESS)
>>> + goto err;
>>> +
>>> + ret = efi_add_protocol(handle, &efi_guid_device_path, disk->dp);
>>> + if (ret != EFI_SUCCESS)
>>> + goto err;
>>> +
>>> + return 0;
>>> +
>>> +err:
>>> + /* FIXME: undo adding protocols */
>>> + return -1;
>>> +}
>>> +
>>> +/*
>>> + * Create a handle for a raw disk
>>> + *
>>> + * @dev uclass device
>>> + * @return 0 on success, -1 otherwise
>>> + */
>>> +int efi_disk_create_raw(struct udevice *dev)
>>> +{
>>> + struct blk_desc *desc = dev_get_uclass_platdata(dev);
>>> + struct efi_disk_obj *disk = &desc->efi_disk;
>>> +
>>> + /* Don't add empty devices */
>>> + if (!desc->lba)
>>> + return -1;
>>> +
>>> + /* raw block device */
>>> + disk->offset = 0;
>>> + disk->part = 0;
>>> + disk->dp = efi_dp_from_part(desc, 0);
>>> +
>>> + /* efi IO media */
>>> + disk->media.logical_partition = 0;
>>> +
>>> + return efi_disk_create_common(&dev->efi_obj, disk, desc);
>>> +}
>>> +
>>> +/*
>>> + * Create a handle for a partition
>>> + *
>>> + * @dev uclass device
>>> + * @return 0 on success, -1 otherwise
>>> + */
>>> +int efi_disk_create_part(struct udevice *dev)
>>> +{
>>> + struct udevice *parent = dev->parent;
>>> + struct blk_desc *desc = dev_get_uclass_platdata(parent);
>>> + struct blk_desc *this;
>>> + struct disk_part *pdata = dev_get_uclass_platdata(dev);
>>> + struct efi_disk_obj *disk;
>>> + struct efi_device_path *node;
>>> +
>>> + int ret;
>>> +
>>> + /* dummy block device */
>>> + this = malloc(sizeof(*this));
>>> + if (!this)
>>> + return -1;
>>> + disk = &this->efi_disk;
>>> +
>>> + /* logical disk partition */
>>> + disk->offset = pdata->gpt_part_info.start;
>>> + disk->part = pdata->partnum;
>>> +
>>> + node = efi_dp_part_node(desc, disk->part);
>>> + disk->dp = efi_dp_append_node(desc->efi_disk.dp, node);
>>> + efi_free_pool(node);
>>> +
>>> + /* efi IO media */
>>> + disk->media.logical_partition = 1;
>>> +
>>> + ret = efi_disk_create_common(&dev->efi_obj, disk, desc);
>>> + if (ret)
>>> + goto err;
>>> +
>>> + /* partition may support file system access */
>>> + disk->volume = efi_simple_file_system(desc, disk->part, disk->dp);
>>> + ret = efi_add_protocol(&dev->efi_obj,
>>> + &efi_simple_file_system_protocol_guid,
>>> + disk->volume);
>>> + if (ret != EFI_SUCCESS)
>>> + goto err;
>>> +
>>> + return 0;
>>> +
>>> +err:
>>> + free(this);
>>> + /* FIXME: undo create */
>>> +
>>> + return -1;
>>> +}
>>> +
>>> +int efi_disk_create(struct udevice *dev)
>>> +{
>>> + enum uclass_id id;
>>> +
>>> + id = device_get_uclass_id(dev);
>>> +
>>> + if (id == UCLASS_BLK)
>>> + return efi_disk_create_raw(dev);
>>> + else if (id == UCLASS_PARTITION)
>>> + return efi_disk_create_part(dev);
>>> + else
>>> + return -1;
>>> +}
>>> +#endif /* CONFIG_BLK */
>>>
>>> /*
>>> * U-Boot doesn't have a list of all online disk devices. So when running
>>> our
>>> @@ -357,38 +461,10 @@ int efi_disk_create_partitions(efi_handle_t parent,
>>> struct blk_desc *desc,
>>> */
>>> efi_status_t efi_disk_register(void)
>>> {
>>> +#ifndef CONFIG_BLK
>>> struct efi_disk_obj *disk;
>>> int disks = 0;
>>> efi_status_t ret;
>>> -#ifdef CONFIG_BLK
>>> - struct udevice *dev;
>>> -
>>> - for (uclass_first_device_check(UCLASS_BLK, &dev); dev;
>>> - uclass_next_device_check(&dev)) {
>>> - struct blk_desc *desc = dev_get_uclass_platdata(dev);
>>> - const char *if_typename = blk_get_if_type_name(desc->if_type);
>>> -
>>> - /* Add block device for the full device */
>>> - printf("Scanning disk %s...\n", dev->name);
>>
>> Who cares for this output? If really needed make it debug().
>
> Please note that this is a line to be deleted.
>
>>> - ret = efi_disk_add_dev(NULL, NULL, if_typename,
>>> - desc, desc->devnum, 0, 0, &disk);
>>> - if (ret == EFI_NOT_READY) {
>>> - printf("Disk %s not ready\n", dev->name);
>>
>> Who minds if it is a CD-ROM drive with no disk inserted? Debug() might
>> be adequate here.
>
> Ditto
>
>>> - continue;
>>> - }
>>> - if (ret) {
>>> - printf("ERROR: failure to add disk device %s, r =
>>> %lu\n",
>>> - dev->name, ret & ~EFI_ERROR_MASK);
>>> - return ret;
>>> - }
>>> - disks++;
>>> -
>>> - /* Partitions show up as block devices in EFI */
>>> - disks += efi_disk_create_partitions(
>>> - &disk->header, desc, if_typename,
>>> - desc->devnum, dev->name);
>>> - }
>>> -#else
>>> int i, if_type;
>>>
>>> /* Search for all available disk devices */
>>> @@ -435,8 +511,8 @@ efi_status_t efi_disk_register(void)
>>> if_typename, i, devname);
>>> }
>>> }
>>> -#endif
>>> printf("Found %d disks\n", disks);
>>
>> I would prefer this to be debug() or eliminated.
>
> I didn't change anything on this line and the function, efi_disk_register(),
> will eventually go away.
>
> Thanks,
> -Takahiro Akashi
>
>
>> Best regards
>>
>> Heinrich
>>
>>> +#endif
>>>
>>> return EFI_SUCCESS;
>>> }
>>>
>>
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
