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

[Rob Clark]

On Mon, Jul 24, 2017 at 1:11 PM, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> 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.
>

I guess I'm not too sure what you see the issue would be, but I'm also
unfamiliar with what needs to be implemented.

The patch I sent just (on first OpenProtocol()) calls ->open() to
resolve the actual protocol interface.  Afaiu, ->OpenProtocol() always
have to be called first.  And ofc it is completely optional.  It
avoids having to up-front create the simple-file-system, and avoids
having to construct a device-path for each efi_file object (when it is
almost never accessed).

BR,
-R