Hi Heinrich, On Wed, 13 Dec 2023 at 23:22, Heinrich Schuchardt <xypron.g...@gmx.de> wrote: > > On 13.12.23 09:57, 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. > > > > efi_link_dev() call is moved inside of efi_disk_add_dev() function > > to simplify the cleanup process in case of error. > > I agree that using struct efi_disk_obj as a container for protocols of a > block IO device is a bit messy. > > But we should keep looking up the handle easy. Currently we use > > diskobj = container_of(this, struct efi_disk_obj, ops); > > Instead we could append a private field with the handle to the > EFI_BLOCK_IO_PROTOCOL structure. > > > > > 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; > > - struct efi_block_io ops; > > - int dev_index; > > - struct efi_block_io_media media; > > - struct efi_device_path *dp; > > - unsigned int part; > > - struct efi_simple_file_system_protocol *volume; > > -}; > > +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; > > + } > > Depending on the number of handles and pointers this will take a > considerable time. A private field for the handle appended to struct > efi_block_io would allow a fast lookup. > > EDK II does the same. See the definition of RAM_DISK_PRIVATE_FROM_BLKIO > which uses macro BASE_CR() to find the private fields.
This patch tries to address this issue[0]. If I understand correctly, two options are suggested here. 1) a private field for the handle appended to struct efi_block_io 2) keep the private struct something like current struct efi_disk_obj, same as EDK II does struct efi_block_io is defined in UEFI spec, so probably option#2 is preferable and it is almost the same as the current implementation. I think we can proceed with the minor cleanup of struct efi_disk_obj as Akashi-san suggested. [0] https://source.denx.de/u-boot/custodians/u-boot-efi/-/issues/9 Thanks, Masahisa Kojima > > Best regards > > Heinrich > > > + > > + 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); > > >