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

[Heinrich Schuchardt]

On 07/24/2017 11:48 AM, 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)?
> 
> BR,
> -R
> 


Hello Rob,

a big gap we currently have in the EFI implementation is the missing
handling of lock entries in OpenProtocol which have to be stored per
protocol as a list of EFI_OPEN_PROTOCOL_INFORMATION_ENTRYs.

Could you, please, elaborate how OpenProtocolInformation,
ConnectController, and DisconnectController shall work together with
your suggested open() approach.

We need be able to iterate efficiently over the
EFI_OPEN_PROTOCOL_INFORMATION_ENTRYs in these functions.

I fear your proposal will make the code overly complicated here.

Best regards

Heinrich