On 7/30/24 13:11, Sughosh Ganu wrote:

Use the API function list_count_nodes() to count the number of EFI memory map entries.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org

lib/efi_loader/efi_memory.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 12cf23fa3f..bfadd6bd41 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -742,8 +742,8 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size, efi_uintn_t *descriptor_size, uint32_t *descriptor_version) {

• int map_entries;

This variable should be of type efi_uintn_t or size_t.

Best regards

Heinrich

efi_uintn_t map_size = 0;

• int map_entries = 0; struct list_head *lhandle; efi_uintn_t provided_map_size;

@@ -752,8 +752,7 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,

provided_map_size = *memory_map_size;

• list_for_each(lhandle, &efi_mem)

• map_entries++;
  

• map_entries = list_count_nodes(&efi_mem);

  map_size = map_entries * sizeof(struct efi_mem_desc);

On Tue, 30 Jul 2024 at 05:11, Sughosh Ganu sughosh.ganu@linaro.org wrote:

Populate the previous memory descriptor node pointer only after it's

its

parent struct has been initialised. The compiler fixes this logic to do the right thing, but it is better to have correct code in place.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org

lib/efi_loader/efi_memory.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Simon Glass sjg@chromium.org

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index bfadd6bd41..8d4f6e339d 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -138,7 +138,7 @@ static void efi_mem_sort(void) merge_again = false; list_for_each(lhandle, &efi_mem) { struct efi_mem_list *lmem;

•                   struct efi_mem_desc *prev = &prevmem->desc;
  

•                   struct efi_mem_desc *prev;
                    struct efi_mem_desc *cur;
                    uint64_t pages;
  
  

@@ -149,6 +149,7 @@ static void efi_mem_sort(void) }

                    cur = &lmem->desc;

•                   prev = &prevmem->desc;
  
                    if ((desc_get_end(cur) == prev->physical_start) &&
                        (prev->type == cur->type) &&
  

-- 2.34.1

On Wed, 31 Jul 2024 at 09:19, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

On 7/30/24 13:11, Sughosh Ganu wrote:

...

Populate the previous memory descriptor node pointer only after it's parent struct has been initialised. The compiler fixes this logic to do the right thing, but it is better to have correct code in place.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org

Fixes: 7b05667ce239 ("efi_loader: Merge memory map entries") Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de

...

---

lib/efi_loader/efi_memory.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index bfadd6bd41..8d4f6e339d 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -138,7 +138,7 @@ static void efi_mem_sort(void) merge_again = false; list_for_each(lhandle, &efi_mem) { struct efi_mem_list *lmem;

•                 struct efi_mem_desc *prev = &prevmem->desc;
  

•                 struct efi_mem_desc *prev;
                  struct efi_mem_desc *cur;
                  uint64_t pages;
  
  

@@ -149,6 +149,7 @@ static void efi_mem_sort(void) }

                  cur = &lmem->desc;

•                 prev = &prevmem->desc;
  
                  if ((desc_get_end(cur) == prev->physical_start) &&
                      (prev->type == cur->type) &&
  

Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

On 7/30/24 13:11, Sughosh Ganu wrote:

Use the list_for_each_entry() API to get the efi_mem_list node directly, instead of making an additional call to list_entry().

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org

Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de

---

lib/efi_loader/efi_memory.c | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 8d4f6e339d..6819c2ec90 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -127,7 +127,7 @@ static uint64_t desc_get_end(struct efi_mem_desc *desc) */ static void efi_mem_sort(void) {

• struct list_head *lhandle;

• struct efi_mem_list *lmem; struct efi_mem_list *prevmem = NULL; bool merge_again = true;

@@ -136,13 +136,11 @@ static void efi_mem_sort(void) /* Now merge entries that can be merged */ while (merge_again) { merge_again = false;

• list_for_each(lhandle, &efi_mem) {
  

• 	struct efi_mem_list *lmem;
  

• list_for_each_entry(lmem, &efi_mem, link) {
  struct efi_mem_desc *prev;
  struct efi_mem_desc *cur;
  uint64_t pages;
  
  

• 	lmem = list_entry(lhandle, struct efi_mem_list, link);
  if (!prevmem) {
  	prevmem = lmem;
  	continue;
  

@@ -269,7 +267,7 @@ static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, int memory_type, bool overlap_only_ram) {

• struct list_head *lhandle;

• struct efi_mem_list *lmem; struct efi_mem_list *newlist; bool carve_again; uint64_t carved_pages = 0;

@@ -309,11 +307,9 @@ static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, /* Add our new map */ do { carve_again = false;

• list_for_each(lhandle, &efi_mem) {
  

• 	struct efi_mem_list *lmem;
  

• list_for_each_entry(lmem, &efi_mem, link) {
  s64 r;
  
  

• 	lmem = list_entry(lhandle, struct efi_mem_list, link);
  r = efi_mem_carve_out(lmem, &newlist->desc,
  		      overlap_only_ram);
  switch (r) {
  

@@ -445,7 +441,7 @@ static efi_status_t efi_check_allocated(u64 addr, bool must_be_allocated) */ static uint64_t efi_find_free_memory(uint64_t len, uint64_t max_addr) {

• struct list_head *lhandle;

• struct efi_mem_list *lmem;

  /*

  • Prealign input max address, so we simplify our matching

@@ -453,9 +449,7 @@ static uint64_t efi_find_free_memory(uint64_t len, uint64_t max_addr) */ max_addr &= ~EFI_PAGE_MASK;

• list_for_each(lhandle, &efi_mem) {

• struct efi_mem_list *lmem = list_entry(lhandle,
  

• 	struct efi_mem_list, link);
  

• list_for_each_entry(lmem, &efi_mem, link) { struct efi_mem_desc *desc = &lmem->desc; uint64_t desc_len = desc->num_pages << EFI_PAGE_SHIFT; uint64_t desc_end = desc->physical_start + desc_len;

@@ -745,7 +739,7 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size, { int map_entries; efi_uintn_t map_size = 0;

• struct list_head *lhandle;

• struct efi_mem_list *lmem; efi_uintn_t provided_map_size;

  if (!memory_map_size)

@@ -774,10 +768,7 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size, /* Copy list into array */ /* Return the list in ascending order */ memory_map = &memory_map[map_entries - 1];

• list_for_each(lhandle, &efi_mem) {

• struct efi_mem_list *lmem;
  

• lmem = list_entry(lhandle, struct efi_mem_list, link);
  

• list_for_each_entry(lmem, &efi_mem, link) { *memory_map = lmem->desc; memory_map--; }

On 7/30/24 13:11, Sughosh Ganu wrote:

The linux kernel has the list_count_nodes() API functions which is used for counting nodes of a list. This has now been imported in U-Boot as part of an earlier commit. Use this function and drop the list_count_items().

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org

Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de

---

drivers/core/util.c | 14 ++------------ include/dm/util.h | 8 -------- test/dm/bus.c | 5 +++-- test/dm/core.c | 9 +++++---- test/dm/test-fdt.c | 5 +++-- 5 files changed, 13 insertions(+), 28 deletions(-)

diff --git a/drivers/core/util.c b/drivers/core/util.c index 108a3bc4da..fa893485a0 100644 --- a/drivers/core/util.c +++ b/drivers/core/util.c @@ -3,23 +3,13 @@

• Copyright (c) 2013 Google, Inc

*/

+#include <vsprintf.h> #include <dm/device.h> #include <dm/ofnode.h> #include <dm/read.h> #include <dm/util.h> #include <linux/libfdt.h> -#include <vsprintf.h>

-int list_count_items(struct list_head *head) -{

• struct list_head *node;
• int count = 0;
• list_for_each(node, head)

• count++;
  

• return count;

-} +#include <linux/list.h>

#if CONFIG_IS_ENABLED(OF_REAL) int pci_get_devfn(struct udevice *dev) diff --git a/include/dm/util.h b/include/dm/util.h index 95c3527a37..ec518c51d9 100644 --- a/include/dm/util.h +++ b/include/dm/util.h @@ -16,14 +16,6 @@ struct dm_stats;

struct list_head;

-/**

• • list_count_items() - Count number of items in a list
• • @param head: Head of list
• • Return: number of items, or 0 if empty
• */

-int list_count_items(struct list_head *head);

• /**
  • Dump out a tree of all devices starting @uclass

diff --git a/test/dm/bus.c b/test/dm/bus.c index a338c7f567..95326f23da 100644 --- a/test/dm/bus.c +++ b/test/dm/bus.c @@ -14,6 +14,7 @@ #include <dm/test.h> #include <dm/uclass-internal.h> #include <dm/util.h> +#include <linux/list.h> #include <test/test.h> #include <test/ut.h>

@@ -27,14 +28,14 @@ static int dm_test_bus_children(struct unit_test_state *uts) struct uclass *uc;

ut_assertok(uclass_get(UCLASS_TEST_FDT, &uc));

• ut_asserteq(num_devices, list_count_items(&uc->dev_head));

• ut_asserteq(num_devices, list_count_nodes(&uc->dev_head));

  /* Probe the bus, which should yield 3 more devices */ ut_assertok(uclass_get_device(UCLASS_TEST_BUS, 0, &bus)); num_devices += 3;

  ut_assertok(uclass_get(UCLASS_TEST_FDT, &uc));

• ut_asserteq(num_devices, list_count_items(&uc->dev_head));

• ut_asserteq(num_devices, list_count_nodes(&uc->dev_head));

  ut_assert(!dm_check_devices(uts, num_devices));

diff --git a/test/dm/core.c b/test/dm/core.c index dbad1b317d..5bc5e8e82e 100644 --- a/test/dm/core.c +++ b/test/dm/core.c @@ -16,6 +16,7 @@ #include <dm/util.h> #include <dm/test.h> #include <dm/uclass-internal.h> +#include <linux/list.h> #include <test/test.h> #include <test/ut.h>

@@ -123,15 +124,15 @@ static int dm_test_autobind(struct unit_test_state *uts) * device with no children. */ ut_assert(uts->root);

• ut_asserteq(1, list_count_items(gd->uclass_root));
• ut_asserteq(0, list_count_items(&gd->dm_root->child_head));

• ut_asserteq(1, list_count_nodes(gd->uclass_root));

• ut_asserteq(0, list_count_nodes(&gd->dm_root->child_head)); ut_asserteq(0, dm_testdrv_op_count[DM_TEST_OP_POST_BIND]);

  ut_assertok(dm_scan_plat(false));

  /* We should have our test class now at least, plus more children */

• ut_assert(1 < list_count_items(gd->uclass_root));
• ut_assert(0 < list_count_items(&gd->dm_root->child_head));

• ut_assert(1 < list_count_nodes(gd->uclass_root));

• ut_assert(0 < list_count_nodes(&gd->dm_root->child_head));

  /* Our 3 dm_test_infox children should be bound to the test uclass */ ut_asserteq(3, dm_testdrv_op_count[DM_TEST_OP_POST_BIND]);

diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c index 18c89eef43..31effff59c 100644 --- a/test/dm/test-fdt.c +++ b/test/dm/test-fdt.c @@ -19,6 +19,7 @@ #include <dm/util.h> #include <dm/of_access.h> #include <linux/ioport.h> +#include <linux/list.h> #include <test/test.h> #include <test/ut.h>

@@ -162,7 +163,7 @@ static int dm_test_fdt(struct unit_test_state *uts) ut_assert(!ret);

/* These are num_devices compatible root-level device tree nodes */

• ut_asserteq(num_devices, list_count_items(&uc->dev_head));

• ut_asserteq(num_devices, list_count_nodes(&uc->dev_head));

  /* Each should have platform data but no private data */ for (i = 0; i < num_devices; i++) {

@@ -217,7 +218,7 @@ static int dm_test_fdt_pre_reloc(struct unit_test_state *uts) * one with "bootph-all" property (a-test node), and the other * one whose driver marked with DM_FLAG_PRE_RELOC flag (h-test node). */

• ut_asserteq(2, list_count_items(&uc->dev_head));

• ut_asserteq(2, list_count_nodes(&uc->dev_head));

  return 0; }