Re: [U-Boot] [PATCH 12/13] efi_loader: manage protocols in a linked list

Wed, 08 Nov 2017 10:52:24 -0800 

On 11/08/2017 04:43 PM, Alexander Graf wrote:

On 11/01/2017 09:31 AM, Heinrich Schuchardt wrote:

Signed-off-by: Heinrich Schuchardt <xypron.g...@gmx.de>
---
  include/efi_loader.h          |   6 ++-
  lib/efi_loader/efi_boottime.c | 107 ++++++++++++++++++++---------------------- 
  lib/efi_loader/efi_disk.c     |   1 +
  lib/efi_loader/efi_gop.c      |   1 +
  lib/efi_loader/efi_net.c      |   1 +
  5 files changed, 58 insertions(+), 58 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index e1f0af3496..a73bbc1269 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -101,6 +101,8 @@ extern unsigned int __efi_runtime_rel_start, __efi_runtime_rel_stop;    * interface (usually a struct with callback functions), this struct maps the 
   * protocol GUID to the respective protocol interface */
  struct efi_handler {
+    /* Link to the list of protocols of a handle */
+    struct list_head link;
      const efi_guid_t *guid;
      void *protocol_interface;
  };
@@ -115,8 +117,8 @@ struct efi_handler {
  struct efi_object {
      /* Every UEFI object is part of a global object list */
      struct list_head link;
-    /* We support up to 16 "protocols" an object can be accessed through */ 
-    struct efi_handler protocols[16];
+    /* The list of protocols */
+    struct list_head protocols;
      /* The object spawner can either use this for data or as identifier */ 
      void *handle;
  };
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c 
index 2b3db162a1..cee0cb1390 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -339,6 +339,7 @@ efi_status_t efi_create_handle(void **handle)
          return r;
      memset(obj, 0, sizeof(struct efi_object));
      obj->handle = obj;
+    INIT_LIST_HEAD(&obj->protocols);
      list_add_tail(&obj->link, &efi_obj_list);
      *handle = obj;
      return r;
@@ -715,18 +716,17 @@ efi_status_t efi_search_protocol(const void *handle, 
                   struct efi_handler **handler)
  {
      struct efi_object *efiobj;
-    size_t i;
-    struct efi_handler *protocol;
+    struct list_head *lhandle;
      if (!handle || !protocol_guid)
          return EFI_INVALID_PARAMETER;
      efiobj = efi_search_obj(handle);
      if (!efiobj)
          return EFI_INVALID_PARAMETER;
-    for (i = 0; i < ARRAY_SIZE(efiobj->protocols); i++) {
-        protocol = &efiobj->protocols[i];
-        if (!protocol->guid)
-            continue;
+    list_for_each(lhandle, &efiobj->protocols) {
+        struct efi_handler *protocol;
+
+        protocol = list_entry(lhandle, struct efi_handler, link);
          if (!guidcmp(protocol->guid, protocol_guid)) {
              if (handler)
                  *handler = protocol;
@@ -750,7 +750,6 @@ efi_status_t efi_add_protocol(const void *handle, const efi_guid_t *protocol, 
      struct efi_object *efiobj;
      struct efi_handler *handler;
      efi_status_t ret;
-    size_t i;
      efiobj = efi_search_obj(handle);
      if (!efiobj)
@@ -761,16 +760,10 @@ efi_status_t efi_add_protocol(const void *handle, const efi_guid_t *protocol, 
      handler = calloc(1, sizeof(struct efi_handler));
      if (!handler)
          return EFI_OUT_OF_RESOURCES;
-    /* Install protocol in first empty slot. */
-    for (i = 0; i < ARRAY_SIZE(efiobj->protocols); i++) {
-        handler = &efiobj->protocols[i];
-        if (handler->guid)
-            continue;
-        handler->guid = protocol;
-        handler->protocol_interface = protocol_interface;
-        return EFI_SUCCESS;
-    }
-    return EFI_OUT_OF_RESOURCES;
+    handler->guid = protocol;
+    handler->protocol_interface = protocol_interface;
+    list_add_tail(&handler->link, &efiobj->protocols);
+    return EFI_SUCCESS;
  }
  /*
@@ -790,10 +783,10 @@ efi_status_t efi_remove_protocol(const void *handle, const efi_guid_t *protocol, 
      ret = efi_search_protocol(handle, protocol, &handler);
      if (ret != EFI_SUCCESS)
          return ret;
-    if (handler->protocol_interface != protocol_interface)
-        return EFI_NOT_FOUND;
-    handler->guid = NULL;
-    handler->protocol_interface = NULL;
+    if (guidcmp(handler->guid, protocol))
+        return EFI_INVALID_PARAMETER;
+    list_del(&handler->link);
+    free(handler);
      return EFI_SUCCESS;
  }
@@ -806,17 +799,22 @@ efi_status_t efi_remove_protocol(const void *handle, const efi_guid_t *protocol, 
  efi_status_t efi_remove_all_protocols(const void *handle)
  {
      struct efi_object *efiobj;
-    struct efi_handler *handler;
-    size_t i;
+    struct list_head *lhandle;
+    struct list_head *pos;
      efiobj = efi_search_obj(handle);
      if (!efiobj)
          return EFI_INVALID_PARAMETER;
+    list_for_each_safe(lhandle, pos, &efiobj->protocols) {
+        struct efi_handler *protocol;
+        efi_status_t ret;
-    for (i = 0; i < ARRAY_SIZE(efiobj->protocols); i++) {
-        handler = &efiobj->protocols[i];
-        handler->guid = NULL;
-        handler->protocol_interface = NULL;
+        protocol = list_entry(lhandle, struct efi_handler, link);
+
+        ret = efi_remove_protocol(handle, protocol->guid,
+                      protocol->protocol_interface);
+        if (ret != EFI_SUCCESS)
+            return ret;
      }
      return EFI_SUCCESS;
  }
@@ -1171,6 +1169,7 @@ void efi_setup_loaded_image(struct efi_loaded_image *info, struct efi_object *ob 
      if (device_path)
          info->device_handle = efi_dp_find_obj(device_path, NULL);
+    INIT_LIST_HEAD(&obj->protocols);
      list_add_tail(&obj->link, &efi_obj_list);
      /*
       * When asking for the device path interface, return
@@ -1648,8 +1647,7 @@ static efi_status_t EFIAPI efi_protocols_per_handle(void *handle, 
  {
      unsigned long buffer_size;
      struct efi_object *efiobj;
-    unsigned long i, j;
-    struct list_head *lhandle;
+    struct list_head *protocol_handle;
      efi_status_t r;
      EFI_ENTRY("%p, %p, %p", handle, protocol_buffer,
@@ -1660,36 +1658,33 @@ static efi_status_t EFIAPI efi_protocols_per_handle(void *handle, 
      *protocol_buffer = NULL;
      *protocol_buffer_count = 0;
-    list_for_each(lhandle, &efi_obj_list) {
-        efiobj = list_entry(lhandle, struct efi_object, link);
-        if (efiobj->handle != handle)
-            continue;
+    efiobj = efi_search_obj(handle);
+    if (!efiobj)
+        return EFI_EXIT(EFI_INVALID_PARAMETER);
-        /* Count protocols */
-        for (i = 0; i < ARRAY_SIZE(efiobj->protocols); i++) {
-            if (efiobj->protocols[i].guid)
-                ++*protocol_buffer_count;
-        }
-        /* Copy guids */
-        if (*protocol_buffer_count) {
-            buffer_size = sizeof(efi_guid_t *) *
-                    *protocol_buffer_count;
-            r = efi_allocate_pool(EFI_ALLOCATE_ANY_PAGES,
-                          buffer_size,
-                          (void **)protocol_buffer);
-            if (r != EFI_SUCCESS)
-                return EFI_EXIT(r);
-            j = 0;
-            for (i = 0; i < ARRAY_SIZE(efiobj->protocols); ++i) {
-                if (efiobj->protocols[i].guid) {
-                    (*protocol_buffer)[j] = (void *)
-                        efiobj->protocols[i].guid;
-                    ++j;
-                }
-            }
+    /* Count protocols */
+    list_for_each(protocol_handle, &efiobj->protocols) {
+        ++*protocol_buffer_count;
+    }
+
+    /* Copy guids */
+    if (*protocol_buffer_count) {
+        size_t j = 0;
+
+        buffer_size = sizeof(efi_guid_t *) * *protocol_buffer_count;
+        r = efi_allocate_pool(EFI_ALLOCATE_ANY_PAGES, buffer_size,
+                      (void **)protocol_buffer);
+        if (r != EFI_SUCCESS)
+            return EFI_EXIT(r);
+        list_for_each(protocol_handle, &efiobj->protocols) {
+            struct efi_handler *protocol;
+
+            protocol = list_entry(protocol_handle,
+                          struct efi_handler, link);
+            (*protocol_buffer)[j] = (void *)protocol->guid;
+            ++j;
          }
-        break;
      }
      return EFI_EXIT(EFI_SUCCESS);
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
index 1d6cf3122f..af8db40e19 100644
--- a/lib/efi_loader/efi_disk.c
+++ b/lib/efi_loader/efi_disk.c
@@ -224,6 +224,7 @@ static void efi_disk_add_dev(const char *name,
          goto out_of_memory;
      /* Hook up to the device list */
+    INIT_LIST_HEAD(&diskobj->parent.protocols);
      list_add_tail(&diskobj->parent.link, &efi_obj_list);
This seems to have become quite a common theme now. Maybe it's about time we add a helper function to manually add a precreated object to the object list (and clear the protocol list along the way)? 

Currently we have quite different ways to initialize parent->handle:

lib/efi_loader/efi_net.c:303:   netobj->parent.handle = &netobj->net;
lib/efi_loader/efi_gop.c:187:   gopobj->parent.handle = &gopobj->ops;
lib/efi_loader/efi_disk.c:233:  diskobj->parent.handle = diskobj;
lib/efi_loader/efi_boottime.c:1166 obj->handle = info;
lib/efi_loader/efi_boottime.c:341: obj->handle = obj;
I could not find any place where we actually dereference a handle. So shouldn't such a helper function also set obj.handle = &obj?
I would prefer to put the introduction of the helper function into a separate patch.
So could you, please, merge patches 2-13 of the series. And I will follow up with: 

New version of patch 1 which is really standalone.
Fix ups for this patch and the previous patch series.

Regards

Heinrich
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Reply via email to