[U-Boot] [PATCH v3 2/3] efi_loader: open_info in OpenProtocol, CloseProtocol

[Alexander Graf]

On 13.08.17 13:09, Heinrich Schuchardt wrote:
> On 08/12/2017 03:37 PM, Alexander Graf wrote:
>>
>>
>> On 05.08.17 22:32, Heinrich Schuchardt wrote:
>>> efi_open_protocol and close_protocol have to keep track of
>>> opened protocols.
>>>
>>> So we add an array open_info to each protocol of each handle.
>>>
>>> Cc: Rob Clark <robdclark at gmail.com>
>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>>> ---
>>> v3:
>>>      use EFI_CALL to avoid wrapper for efi_disconnect_controller
>>>      use list_for_each_entry
>>>      move variable declarations out of loops
>>> v2:
>>>      new patch
>>> ---
>>>    include/efi_loader.h          |   1 +
>>>    lib/efi_loader/efi_boottime.c | 164
>>> +++++++++++++++++++++++++++++++++++++++---
>>>    2 files changed, 155 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>> index 553c615f11..222b251a38 100644
>>> --- a/include/efi_loader.h
>>> +++ b/include/efi_loader.h
>>> @@ -88,6 +88,7 @@ extern unsigned int __efi_runtime_rel_start,
>>> __efi_runtime_rel_stop;
>>>    struct efi_handler {
>>>        const efi_guid_t *guid;
>>>        void *protocol_interface;
>>> +    struct efi_open_protocol_info_entry open_info[4];
>>>    };
>>>      /*
>>> diff --git a/lib/efi_loader/efi_boottime.c
>>> b/lib/efi_loader/efi_boottime.c
>>> index ebb557abb2..e969814476 100644
>>> --- a/lib/efi_loader/efi_boottime.c
>>> +++ b/lib/efi_loader/efi_boottime.c
>>> @@ -898,23 +898,78 @@ static efi_status_t EFIAPI efi_connect_controller(
>>>        return EFI_EXIT(EFI_NOT_FOUND);
>>>    }
>>>    -static efi_status_t EFIAPI efi_disconnect_controller(void
>>> *controller_handle,
>>> -                             void *driver_image_handle,
>>> -                             void *child_handle)
>>> +static efi_status_t EFIAPI efi_disconnect_controller(
>>> +                        void *controller_handle,
>>> +                        void *driver_image_handle,
>>> +                        void *child_handle)
>>>    {
>>>        EFI_ENTRY("%p, %p, %p", controller_handle, driver_image_handle,
>>>              child_handle);
>>>        return EFI_EXIT(EFI_INVALID_PARAMETER);
>>>    }
>>>    +static efi_status_t efi_close_protocol_int(struct efi_handler
>>> *protocol,
>>
>> Please either wrap _ext or use EFI_CALL :).
> 
> Why?
> 
> Function efi_close_protocol_int is only used to avoid lines over 80
> characters in efi_disconnect_controller. It is not called from anywhere
> else.
> 
> Should I add some comment in the code or in the commit message?

Ah, now I see. No, I think the function name is misleading, so that 
needs change :). How about efi_close_one_protocol()?

> 
>>
>>> +                       void *agent_handle,
>>> +                       void *controller_handle)
>>> +{
>>> +    size_t i;
>>> +    struct efi_open_protocol_info_entry *open_info;
>>> +
>>> +    for (i = 0; i < ARRAY_SIZE(protocol->open_info); ++i) {
>>> +        open_info = &protocol->open_info[i];
>>> +
>>> +        if (!open_info->open_count)
>>> +            continue;
>>> +
>>> +        if (open_info->agent_handle == agent_handle &&
>>> +            open_info->controller_handle ==
>>> +            controller_handle) {
>>> +            open_info->open_count--;
>>> +            return EFI_SUCCESS;
>>> +        }
>>> +    }
>>> +    return EFI_NOT_FOUND;
>>> +}
>>> +
>>>    static efi_status_t EFIAPI efi_close_protocol(void *handle,
>>>                              efi_guid_t *protocol,
>>>                              void *agent_handle,
>>>                              void *controller_handle)
>>>    {
>>> +    struct efi_object *efiobj;
>>> +    size_t i;
>>> +    efi_status_t r = EFI_NOT_FOUND;
>>> +
>>>        EFI_ENTRY("%p, %p, %p, %p", handle, protocol, agent_handle,
>>>              controller_handle);
>>> -    return EFI_EXIT(EFI_NOT_FOUND);
>>> +
>>> +    if (!handle || !protocol || !agent_handle) {
>>> +        r = EFI_INVALID_PARAMETER;
>>> +        goto out;
>>> +    }
>>> +
>>> +    EFI_PRINT_GUID("protocol:", protocol);
>>> +
>>> +    list_for_each_entry(efiobj, &efi_obj_list, link) {
>>> +        if (efiobj->handle != handle)
>>> +            continue;
>>> +
>>> +        for (i = 0; i < ARRAY_SIZE(efiobj->protocols); i++) {
>>> +            struct efi_handler *handler = &efiobj->protocols[i];
>>> +            const efi_guid_t *hprotocol = handler->guid;
>>> +            if (!hprotocol)
>>> +                continue;
>>> +            if (!guidcmp(hprotocol, protocol)) {
>>> +                r = efi_close_protocol_int(handler,
>>> +                               agent_handle,
>>> +                               controller_handle);
>>> +                goto out;
>>> +            }
>>> +        }
>>> +        goto out;
>>> +    }
>>> +out:
>>> +    return EFI_EXIT(r);
>>>    }
>>>      static efi_status_t EFIAPI
>>> efi_open_protocol_information(efi_handle_t handle,
>>> @@ -1119,6 +1174,96 @@ static void EFIAPI efi_set_mem(void *buffer,
>>> unsigned long size, uint8_t value)
>>>        memset(buffer, value, size);
>>>    }
>>>    +static efi_status_t efi_open_protocol_int(
>>
>> Same here.
>>
>>
>> Alex
> 
> See above.
> 
> Was the rest of the patch ok for you?

I didn't spot anything obviously bad, but that doesn't usually mean 
much. My review foo isn't quite as good as others'. When applying I 
would push the patches through some more detailed testing though.


Alex