[U-Boot] [PATCH v2 01/12] efi_loader: refactor efi_open_protocol

[Alexander Graf]

On 24.07.17 11:48, Rob Clark wrote:
> On Tue, Jul 11, 2017 at 4:06 PM, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>> efi_open_protocol was implemented to call a protocol specific open
>> function to retrieve the protocol interface.
>>
>> The UEFI specification does not know of such a function.
>>
>> It is not possible to implement InstallProtocolInterface with the
>> current design.
>>
>> With the patch the protocol interface itself is stored in the list
>> of installed protocols of an efi_object instead of an open function.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>> ---
>> v2
>>          Remove implementation of efi_return_handle.
>> ---
>>   cmd/bootefi.c                     | 14 +++-----------
>>   include/efi_loader.h              | 17 ++---------------
>>   lib/efi_loader/efi_boottime.c     | 18 ++++++++++++++----
>>   lib/efi_loader/efi_disk.c         | 29 +++--------------------------
>>   lib/efi_loader/efi_gop.c          |  2 +-
>>   lib/efi_loader/efi_image_loader.c |  8 --------
>>   lib/efi_loader/efi_net.c          | 30 +++---------------------------
>>   7 files changed, 26 insertions(+), 92 deletions(-)
>>
>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>> index 7ddeead671..7cf0129a18 100644
>> --- a/cmd/bootefi.c
>> +++ b/cmd/bootefi.c
>> @@ -54,14 +54,6 @@ static struct efi_device_path_file_path bootefi_device_path[] = {
>>          }
>>   };
>>
>> -static efi_status_t EFIAPI bootefi_open_dp(void *handle, efi_guid_t *protocol,
>> -                       void **protocol_interface, void *agent_handle,
>> -                       void *controller_handle, uint32_t attributes)
>> -{
>> -       *protocol_interface = bootefi_device_path;
>> -       return EFI_SUCCESS;
>> -}
>> -
>>   /* The EFI loaded_image interface for the image executed via "bootefi" */
>>   static struct efi_loaded_image loaded_image_info = {
>>          .device_handle = bootefi_device_path,
>> @@ -78,7 +70,7 @@ static struct efi_object loaded_image_info_obj = {
>>                           * return handle which points to loaded_image_info
>>                           */
>>                          .guid = &efi_guid_loaded_image,
>> -                       .open = &efi_return_handle,
>> +                       .protocol_interface = &loaded_image_info,
> 
> btw, I probably should have looked at this patchset earlier.. in
> general, I like it, since it removes a lot of boilerplate.  But there
> are some cases where ->open() allows deferred allocation.  For
> example, I'm working on efi_file and simple-file-system-protocol
> implementation.  A file object can have a device-path accessed by
> opening device-path protocol.  99% of the time this isn't used, so the
> ->open() approach lets me defer constructing a file's device-path.
> 
> How would you feel if I re-introduced ->open() as an optional thing
> (where the normal case if open is NULL would be to just return
> protocol_interface)?

Sounds very reasonable to me. I also think that first converting 
everything to data-driven is a good thing, as it makes sure we have 
everything that doesn't need to be deferred explicit.

So yes, please base an optional open method on top :).


Alex