On Thu, Jan 10, 2019 at 07:22:49AM +0100, Alexander Graf wrote:
>
>
> On 10.01.19 01:37, AKASHI Takahiro wrote:
> > Alex,
> >
> > On Wed, Jan 09, 2019 at 10:18:16AM +0100, Alexander Graf wrote:
> >>
> >>
> >> On 15.11.18 05:58, AKASHI Takahiro wrote:
> >>> Logically, details on u-boot block device used to implement efi file
> >>> protocol are mostly unnecessary, as well as being duplicated, in
> >>> efi_file structure.
> >>> Moreover, a newly introduced flag, _EFI_DISK_FLAG_INVALID, should be
> >>> honored in any file operations via efi file protocol.
> >>> These observation suggests that those internal details be set behind
> >>> efi_disk object.
> >>>
> >>> So rework in this patch addresses all those issues above although
> >>> there is little change in its functionality.
> >>>
> >>> Signed-off-by: AKASHI Takahiro <takahiro.aka...@linaro.org>
> >>> ---
> >>> include/efi_loader.h | 6 +++++-
> >>> lib/efi_loader/efi_disk.c | 38 ++++++++++++++++++++++++++++++++++++--
> >>> lib/efi_loader/efi_file.c | 21 ++++++++-------------
> >>> 3 files changed, 49 insertions(+), 16 deletions(-)
> >>>
> >>> diff --git a/include/efi_loader.h b/include/efi_loader.h
> >>> index 3bae1844befb..108ef3fe52ee 100644
> >>> --- a/include/efi_loader.h
> >>> +++ b/include/efi_loader.h
> >>> @@ -264,6 +264,10 @@ efi_status_t efi_disk_register(void);
> >>> bool efi_disk_is_valid(efi_handle_t handle);
> >>> /* Called by bootefi to find and update disk storage information */
> >>> efi_status_t efi_disk_update(void);
> >>> +/* Called by file protocol to set internal block io device */
> >>> +int efi_disk_set_blk_dev(efi_handle_t disk);
> >>> +/* Called by file protocol to get disk/partition information */
> >>> +int efi_disk_get_info(efi_handle_t disk, disk_partition_t *part);
> >>> /* Create handles and protocols for the partitions of a block device */
> >>> int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc
> >>> *desc,
> >>> const char *if_typename, int diskid,
> >>> @@ -355,7 +359,7 @@ void efi_signal_event(struct efi_event *event, bool
> >>> check_tpl);
> >>>
> >>> /* open file system: */
> >>> struct efi_simple_file_system_protocol *efi_simple_file_system(
> >>> - struct blk_desc *desc, int part, struct efi_device_path *dp);
> >>> + efi_handle_t disk);
> >>>
> >>> /* open file from device-path: */
> >>> struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp);
> >>> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> >>> index 0c4d79ee3fc9..180e8e10bb28 100644
> >>> --- a/lib/efi_loader/efi_disk.c
> >>> +++ b/lib/efi_loader/efi_disk.c
> >>> @@ -9,6 +9,7 @@
> >>> #include <blk.h>
> >>> #include <dm.h>
> >>> #include <efi_loader.h>
> >>> +#include <fs.h>
> >>> #include <part.h>
> >>> #include <malloc.h>
> >>>
> >>> @@ -254,6 +255,40 @@ efi_fs_from_path(struct efi_device_path *full_path)
> >>> return handler->protocol_interface;
> >>> }
> >>>
> >>> +/*
> >>> + * Set block device for later block io's from file system protocol
> >>> + *
> >>> + * @disk handle to uefi disk device
> >>> + * @return 0 for success, -1 for failure
> >>> + */
> >>> +int efi_disk_set_blk_dev(efi_handle_t disk)
> >>> +{
> >>> + struct efi_disk_obj *diskobj;
> >>> +
> >>> + diskobj = container_of(disk, struct efi_disk_obj, header);
> >>> +
> >>> + return fs_set_blk_dev_with_part(diskobj->desc, diskobj->part);
> >>> +}
> >>> +
> >>> +/*
> >>> + * Get disk/partition information
> >>> + *
> >>> + * @disk handle to uefi disk device
> >>> + * @part pointer to disk/partition information to be returned
> >>> + * @return 0 for success, -1 for failure
> >>> + */
> >>> +int efi_disk_get_info(efi_handle_t disk, disk_partition_t *part)
> >>> +{
> >>> + struct efi_disk_obj *diskobj;
> >>> +
> >>> + diskobj = container_of(disk, struct efi_disk_obj, header);
> >>> +
> >>> + if (diskobj->part >= 1)
> >>> + return part_get_info(diskobj->desc, diskobj->part, part);
> >>> + else
> >>> + return part_get_info_whole_disk(diskobj->desc, part);
> >>> +}
> >>> +
> >>> /*
> >>> * Create a handle for a partition or disk
> >>> *
> >>> @@ -308,8 +343,7 @@ static efi_status_t efi_disk_add_dev(
> >>> if (ret != EFI_SUCCESS)
> >>> return ret;
> >>> if (part >= 1) {
> >>> - diskobj->volume = efi_simple_file_system(desc, part,
> >>> - diskobj->dp);
> >>> + diskobj->volume = efi_simple_file_system(&diskobj->header);
> >>> ret = efi_add_protocol(&diskobj->header,
> >>> &efi_simple_file_system_protocol_guid,
> >>> diskobj->volume);
> >>> diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
> >>> index beb4fba917d8..944383224f30 100644
> >>> --- a/lib/efi_loader/efi_file.c
> >>> +++ b/lib/efi_loader/efi_file.c
> >>> @@ -17,9 +17,7 @@ const efi_guid_t efi_file_system_info_guid =
> >>> EFI_FILE_SYSTEM_INFO_GUID;
> >>>
> >>> struct file_system {
> >>> struct efi_simple_file_system_protocol base;
> >>> - struct efi_device_path *dp;
> >>> - struct blk_desc *desc;
> >>> - int part;
> >>> + efi_handle_t disk;
> >>
> >> Is there a particular reason we can't just make this a struct
> >> efi_disk_obj *?
> >
> > Just because efi_disk_obj is an internally-defined structure in efi_disk.c.
> >
> >> Inside our own code base, we don't need to abstract things away to
> >> handles, right? We also want to have the compiler catch the fact early
> >> if people pass in non-disk-objects in as parameter.
> >
> > If you don't mind efi_disk_obj definition being moved to, say, efi_loader.h,
> > I would follow your suggestion.
>
> I don't think we need to move the definition, just the hint that the
> name exists.
>
> If you add the following to efi_loader.h:
>
> struct efi_disk_obj;
> that should be enough to enable other subsystems (and APIs) to use
> pointers to that struct.
Ah, right. What we need to have in struct file_system is a *pointer*.
So such a declaration is good enough.
Thanks,
-Takahiro Akashi
>
>
> Alex
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
