Hi Kojima-san,
On Wed, Dec 13, 2023 at 05:57:37PM +0900, Masahisa Kojima wrote:
> Current code uses struct efi_disk_obj to keep information
> about block devices and partitions. As the efi handle already
> has a field with the udevice, we should eliminate
> struct efi_disk_obj and use an pointer to struct efi_object
> for the handle.
I don't still understand what is an advantage of your approach.
> efi_link_dev() call is moved inside of efi_disk_add_dev() function
> to simplify the cleanup process in case of error.
>
> Signed-off-by: Masahisa Kojima <masahisa.koj...@linaro.org>
> ---
> lib/efi_loader/efi_disk.c | 209 +++++++++++++++++++++-----------------
> 1 file changed, 116 insertions(+), 93 deletions(-)
>
> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> index f0d76113b0..cfb7ace551 100644
> --- a/lib/efi_loader/efi_disk.c
> +++ b/lib/efi_loader/efi_disk.c
> @@ -27,27 +27,24 @@ struct efi_system_partition efi_system_partition = {
> const efi_guid_t efi_block_io_guid = EFI_BLOCK_IO_PROTOCOL_GUID;
> const efi_guid_t efi_system_partition_guid = PARTITION_SYSTEM_GUID;
>
> -/**
> - * struct efi_disk_obj - EFI disk object
> - *
> - * @header: EFI object header
> - * @ops: EFI disk I/O protocol interface
> - * @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
> - * @dev: associated DM device
> - */
> -struct efi_disk_obj {
> - struct efi_object header;
This is a handy way of making it ease to dereference from an internal
structure to an external reference as Heinrich mentioned.
> - struct efi_block_io ops;
Along with "header" and "media", this allows us to congregate
a couple of "malloc" calls, instead of your change, into one.
> - int dev_index;
> - struct efi_device_path *dp;
> - unsigned int part;
> - struct efi_simple_file_system_protocol *volume;
If we carefully look into the base code, we will no longer have to
have those fields in this structure because they are mostly used in
a single internal function (if I'm correct).
So we can simply replace them with local variables (with some extra
changes).
-Takahiro Akashi
> -};
> +static efi_handle_t efi_blkio_find_obj(struct efi_block_io *blkio)
> +{
> + efi_handle_t handle;
> +
> + list_for_each_entry(handle, &efi_obj_list, link) {
> + efi_status_t ret;
> + struct efi_handler *handler;
> +
> + ret = efi_search_protocol(handle, &efi_block_io_guid, &handler);
> + if (ret != EFI_SUCCESS)
> + continue;
> +
> + if (blkio == handler->protocol_interface)
> + return handle;
> + }
> +
> + return NULL;
> +}
>
> /**
> * efi_disk_reset() - reset block device
> @@ -107,13 +104,16 @@ static efi_status_t efi_disk_rw_blocks(struct
> efi_block_io *this,
> u32 media_id, u64 lba, unsigned long buffer_size,
> void *buffer, enum efi_disk_direction direction)
> {
> - struct efi_disk_obj *diskobj;
> + efi_handle_t handle;
> int blksz;
> int blocks;
> unsigned long n;
>
> - diskobj = container_of(this, struct efi_disk_obj, ops);
> - blksz = diskobj->media.block_size;
> + handle = efi_blkio_find_obj(this);
> + if (!handle)
> + return EFI_INVALID_PARAMETER;
> +
> + blksz = this->media->block_size;
> blocks = buffer_size / blksz;
>
> EFI_PRINT("blocks=%x lba=%llx blksz=%x dir=%d\n",
> @@ -124,18 +124,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->header.dev) == UCLASS_PARTITION) {
> + device_get_uclass_id(handle->dev) == UCLASS_PARTITION) {
> if (direction == EFI_DISK_READ)
> - n = disk_blk_read(diskobj->header.dev, lba, blocks,
> - buffer);
> + n = disk_blk_read(handle->dev, lba, blocks, buffer);
> else
> - n = disk_blk_write(diskobj->header.dev, lba, blocks,
> - buffer);
> + n = disk_blk_write(handle->dev, lba, blocks, buffer);
> } else {
> /* dev is a block device (UCLASS_BLK) */
> struct blk_desc *desc;
>
> - desc = dev_get_uclass_plat(diskobj->header.dev);
> + desc = dev_get_uclass_plat(handle->dev);
> if (direction == EFI_DISK_READ)
> n = blk_dread(desc, lba, blocks, buffer);
> else
> @@ -388,6 +386,7 @@ static int efi_fs_exists(struct blk_desc *desc, int part)
> * @part: partition
> * @disk: pointer to receive the created handle
> * @agent_handle: handle of the EFI block driver
> + * @dev: pointer to udevice
> * Return: disk object
> */
> static efi_status_t efi_disk_add_dev(
> @@ -397,24 +396,33 @@ static efi_status_t efi_disk_add_dev(
> int dev_index,
> struct disk_partition *part_info,
> unsigned int part,
> - struct efi_disk_obj **disk,
> - efi_handle_t agent_handle)
> + efi_handle_t *disk,
> + efi_handle_t agent_handle,
> + struct udevice *dev)
> {
> - struct efi_disk_obj *diskobj;
> - struct efi_object *handle;
> + efi_handle_t handle;
> const efi_guid_t *esp_guid = NULL;
> efi_status_t ret;
> + struct efi_block_io *blkio;
> + struct efi_block_io_media *media;
> + struct efi_device_path *dp = NULL;
> + struct efi_simple_file_system_protocol *volume = NULL;
>
> /* Don't add empty devices */
> if (!desc->lba)
> return EFI_NOT_READY;
>
> - diskobj = calloc(1, sizeof(*diskobj));
> - if (!diskobj)
> + ret = efi_create_handle(&handle);
> + if (ret != EFI_SUCCESS)
> + return ret;
> +
> + blkio = calloc(1, sizeof(*blkio));
> + media = calloc(1, sizeof(*media));
> + if (!blkio || !media)
> return EFI_OUT_OF_RESOURCES;
>
> - /* Hook up to the device list */
> - efi_add_handle(&diskobj->header);
> + *blkio = block_io_disk_template;
> + blkio->media = media;
>
> /* Fill in object data */
> if (part_info) {
> @@ -440,23 +448,22 @@ static efi_status_t efi_disk_add_dev(
> * (controller).
> */
> ret = efi_protocol_open(handler, &protocol_interface, NULL,
> - &diskobj->header,
> + handle,
> EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER);
> if (ret != EFI_SUCCESS) {
> log_debug("prot open failed\n");
> goto error;
> }
>
> - diskobj->dp = efi_dp_append_node(dp_parent, node);
> + dp = efi_dp_append_node(dp_parent, node);
> efi_free_pool(node);
> - diskobj->media.last_block = part_info->size - 1;
> + blkio->media->last_block = part_info->size - 1;
> if (part_info->bootable & PART_EFI_SYSTEM_PARTITION)
> esp_guid = &efi_system_partition_guid;
> } else {
> - diskobj->dp = efi_dp_from_part(desc, part);
> - diskobj->media.last_block = desc->lba - 1;
> + dp = efi_dp_from_part(desc, part);
> + blkio->media->last_block = desc->lba - 1;
> }
> - diskobj->part = part;
>
> /*
> * Install the device path and the block IO protocol.
> @@ -465,11 +472,10 @@ static efi_status_t efi_disk_add_dev(
> * already installed on an other handle and returns EFI_ALREADY_STARTED
> * in this case.
> */
> - handle = &diskobj->header;
> ret = efi_install_multiple_protocol_interfaces(
> &handle,
> - &efi_guid_device_path, diskobj->dp,
> - &efi_block_io_guid, &diskobj->ops,
> + &efi_guid_device_path, dp,
> + &efi_block_io_guid, blkio,
> /*
> * esp_guid must be last entry as it
> * can be NULL. Its interface is NULL.
> @@ -487,43 +493,39 @@ static efi_status_t efi_disk_add_dev(
> */
> if ((part || desc->part_type == PART_TYPE_UNKNOWN) &&
> efi_fs_exists(desc, part)) {
> - ret = efi_create_simple_file_system(desc, part, diskobj->dp,
> - &diskobj->volume);
> + ret = efi_create_simple_file_system(desc, part, dp, &volume);
> if (ret != EFI_SUCCESS)
> goto error;
>
> - ret = efi_add_protocol(&diskobj->header,
> + ret = efi_add_protocol(handle,
> &efi_simple_file_system_protocol_guid,
> - diskobj->volume);
> + volume);
> if (ret != EFI_SUCCESS)
> goto error;
> }
> - diskobj->ops = block_io_disk_template;
> - diskobj->dev_index = dev_index;
>
> /* Fill in EFI IO Media info (for read/write callbacks) */
> - diskobj->media.removable_media = desc->removable;
> - diskobj->media.media_present = 1;
> + blkio->media->removable_media = desc->removable;
> + blkio->media->media_present = 1;
> /*
> * MediaID is just an arbitrary counter.
> * We have to change it if the medium is removed or changed.
> */
> - diskobj->media.media_id = 1;
> - diskobj->media.block_size = desc->blksz;
> - diskobj->media.io_align = desc->blksz;
> + blkio->media->media_id = 1;
> + blkio->media->block_size = desc->blksz;
> + blkio->media->io_align = desc->blksz;
> if (part)
> - diskobj->media.logical_partition = 1;
> - diskobj->ops.media = &diskobj->media;
> + blkio->media->logical_partition = 1;
> if (disk)
> - *disk = diskobj;
> + *disk = handle;
>
> EFI_PRINT("BlockIO: part %u, present %d, logical %d, removable %d"
> ", last_block %llu\n",
> - diskobj->part,
> - diskobj->media.media_present,
> - diskobj->media.logical_partition,
> - diskobj->media.removable_media,
> - diskobj->media.last_block);
> + part,
> + blkio->media->media_present,
> + blkio->media->logical_partition,
> + blkio->media->removable_media,
> + blkio->media->last_block);
>
> /* Store first EFI system partition */
> if (part && efi_system_partition.uclass_id == UCLASS_INVALID) {
> @@ -536,11 +538,18 @@ static efi_status_t efi_disk_add_dev(
> desc->devnum, part);
> }
> }
> +
> + if (efi_link_dev(handle, dev))
> + goto error;
> +
> return EFI_SUCCESS;
> error:
> - efi_delete_handle(&diskobj->header);
> - free(diskobj->volume);
> - free(diskobj);
> + efi_delete_handle(handle);
> + efi_free_pool(dp);
> + free(blkio);
> + free(media);
> + free(volume);
> +
> return ret;
> }
>
> @@ -557,7 +566,7 @@ error:
> */
> static int efi_disk_create_raw(struct udevice *dev, efi_handle_t
> agent_handle)
> {
> - struct efi_disk_obj *disk;
> + efi_handle_t disk;
> struct blk_desc *desc;
> int diskid;
> efi_status_t ret;
> @@ -566,7 +575,7 @@ static int efi_disk_create_raw(struct udevice *dev,
> efi_handle_t agent_handle)
> diskid = desc->devnum;
>
> ret = efi_disk_add_dev(NULL, NULL, desc,
> - diskid, NULL, 0, &disk, agent_handle);
> + diskid, NULL, 0, &disk, agent_handle, dev);
> if (ret != EFI_SUCCESS) {
> if (ret == EFI_NOT_READY) {
> log_notice("Disk %s not ready\n", dev->name);
> @@ -578,12 +587,6 @@ static int efi_disk_create_raw(struct udevice *dev,
> efi_handle_t agent_handle)
>
> return ret;
> }
> - if (efi_link_dev(&disk->header, dev)) {
> - efi_free_pool(disk->dp);
> - efi_delete_handle(&disk->header);
> -
> - return -EINVAL;
> - }
>
> return 0;
> }
> @@ -609,7 +612,7 @@ static int efi_disk_create_part(struct udevice *dev,
> efi_handle_t agent_handle)
> int diskid;
> struct efi_handler *handler;
> struct efi_device_path *dp_parent;
> - struct efi_disk_obj *disk;
> + efi_handle_t disk;
> efi_status_t ret;
>
> if (dev_tag_get_ptr(dev_get_parent(dev), DM_TAG_EFI, (void **)&parent))
> @@ -628,17 +631,11 @@ static int efi_disk_create_part(struct udevice *dev,
> efi_handle_t agent_handle)
> dp_parent = (struct efi_device_path *)handler->protocol_interface;
>
> ret = efi_disk_add_dev(parent, dp_parent, desc, diskid,
> - info, part, &disk, agent_handle);
> + info, part, &disk, agent_handle, dev);
> if (ret != EFI_SUCCESS) {
> log_err("Adding partition for %s failed\n", dev->name);
> return -1;
> }
> - if (efi_link_dev(&disk->header, dev)) {
> - efi_free_pool(disk->dp);
> - efi_delete_handle(&disk->header);
> -
> - return -1;
> - }
>
> return 0;
> }
> @@ -693,13 +690,34 @@ int efi_disk_probe(void *ctx, struct event *event)
> return 0;
> }
>
> +static void get_disk_resources(efi_handle_t handle, struct efi_device_path
> **dp,
> + struct efi_block_io **blkio,
> + struct efi_simple_file_system_protocol **volume)
> +{
> + efi_status_t ret;
> + struct efi_handler *handler;
> +
> + ret = efi_search_protocol(handle, &efi_guid_device_path, &handler);
> + if (ret == EFI_SUCCESS)
> + *dp = handler->protocol_interface;
> +
> + ret = efi_search_protocol(handle, &efi_block_io_guid, &handler);
> + if (ret == EFI_SUCCESS)
> + *blkio = handler->protocol_interface;
> +
> + ret = efi_search_protocol(handle, &efi_simple_file_system_protocol_guid,
> + &handler);
> + if (ret == EFI_SUCCESS)
> + *volume = handler->protocol_interface;
> +}
> +
> /**
> - * efi_disk_remove - delete an efi_disk object for a block device or
> partition
> + * efi_disk_remove - delete an efi handle for a block device or partition
> *
> * @ctx: event context: driver binding protocol
> * @event: EV_PM_PRE_REMOVE event
> *
> - * Delete an efi_disk object which is associated with the UCLASS_BLK or
> + * Delete an efi handle which is associated with the UCLASS_BLK or
> * UCLASS_PARTITION device for which the EV_PM_PRE_REMOVE event is raised.
> *
> * Return: 0 on success, -1 otherwise
> @@ -710,8 +728,10 @@ int efi_disk_remove(void *ctx, struct event *event)
> struct udevice *dev = event->data.dm.dev;
> efi_handle_t handle;
> struct blk_desc *desc;
> - struct efi_disk_obj *diskobj = NULL;
> efi_status_t ret;
> + struct efi_device_path *dp = NULL;
> + struct efi_block_io *blkio = NULL;
> + struct efi_simple_file_system_protocol *volume = NULL;
>
> if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle))
> return 0;
> @@ -721,11 +741,11 @@ int efi_disk_remove(void *ctx, struct event *event)
> case UCLASS_BLK:
> desc = dev_get_uclass_plat(dev);
> if (desc && desc->uclass_id != UCLASS_EFI_LOADER)
> - diskobj = container_of(handle, struct efi_disk_obj,
> - header);
> + get_disk_resources(handle, &dp, &blkio, &volume);
> +
> break;
> case UCLASS_PARTITION:
> - diskobj = container_of(handle, struct efi_disk_obj, header);
> + get_disk_resources(handle, &dp, &blkio, &volume);
> break;
> default:
> return 0;
> @@ -736,8 +756,11 @@ int efi_disk_remove(void *ctx, struct event *event)
> if (ret != EFI_SUCCESS)
> return -1;
>
> - if (diskobj)
> - efi_free_pool(diskobj->dp);
> + efi_free_pool(dp);
> + if (blkio) {
> + free(blkio->media);
> + free(blkio);
> + }
>
> dev_tag_del(dev, DM_TAG_EFI);
>
> --
> 2.34.1
>
