[U-Boot] [PATCH v2 3/3] efi_loader: remove block device details from efi file

[Alexander Graf]

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.akashi at 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.


Alex