From c4be70f6ff5379bcaeb7f0d2f17b140c70fc12cf Mon Sep 17 00:00:00 2001 From: Yu-Ping Wu Date: Fri, 9 Jan 2026 16:58:14 +0800 Subject: [PATCH] commonlib/list: Support circular list In some use cases, we want to add items to the linked list and then iterate over them with the insertion order. With the current API, the call site needs to either use the inefficient list_append() function to append items to the end of the list, or manually maintain a "tail" node pointer. To support that use case and make the change backward compatible, add a helper list_init() function to initialize the list as a circular linked list. list_init() is automatically called within list_insert_after() and list_append(). In list_insert_before(), an assertion is added to avoid an insertion before the head node (which should be invalid). The implementation ensures that the list is initialized as a circular one whenever the first element is added. That also allows all call sites to be auto-upgraded to the "circular list" implementation without any modification. Modify list_for_each() to support circular lists, and improve list_append() efficiency by inserting the new node before the placeholder head node. Also add a few assertions in the implementation. Add a new test case to test iterating over an empty list. Note that '(uintptr_t)ptr + (uintptr_t)offsetof(typeof(*(ptr)), member)' was used instead of the simpler '&((ptr)->member)' because GCC9+ assumes that the address can never be NULL. See commit 88991caf00d3 ("include/list.h: Add support for GCC9+) for details. Now, with the new list_for_each() implementation, that pointer value can never be NULL. Change-Id: I8451f711d4e522e239c241b3943e00070896dec9 Signed-off-by: Yu-Ping Wu Reviewed-on: https://review.coreboot.org/c/coreboot/+/90799 Reviewed-by: Julius Werner Tested-by: build bot (Jenkins) --- src/commonlib/include/commonlib/list.h | 21 ++++++-- src/commonlib/list.c | 24 +++++++-- tests/commonlib/list-test.c | 70 ++++++++++++++++++-------- 3 files changed, 84 insertions(+), 31 deletions(-) diff --git a/src/commonlib/include/commonlib/list.h b/src/commonlib/include/commonlib/list.h index 970d68ea49..7cd3460f8a 100644 --- a/src/commonlib/include/commonlib/list.h +++ b/src/commonlib/include/commonlib/list.h @@ -17,14 +17,25 @@ void list_remove(struct list_node *node); // Insert list_node node after list_node after in a doubly linked list. void list_insert_after(struct list_node *node, struct list_node *after); // Insert list_node node before list_node before in a doubly linked list. +// `before` must not be the placeholder head node. void list_insert_before(struct list_node *node, struct list_node *before); -// Appends the node to the end of the list. +// Append the node to the end of the list. void list_append(struct list_node *node, struct list_node *head); -#define list_for_each(ptr, head, member) \ - for ((ptr) = container_of((head).next, typeof(*(ptr)), member); \ - (uintptr_t)ptr + (uintptr_t)offsetof(typeof(*(ptr)), member); \ - (ptr) = container_of((ptr)->member.next, \ +/* + * Explanation of `ptr` initialization: + * 1. head.next != NULL: This means the list isn't empty. As the implementation ensures that + * list_init() is called when the very first element is added, we can safely assume that + * the list is circular, and hence set `ptr` to the 1st element. + * 2. head.next == NULL: This means the list is empty, and list_init() hasn't been called. + * As the `head` arg might be const, we cannot simply call list_init() here. Instead, we set + * `ptr` to a special value such that `&(ptr->member) == &head`, causing the loop to + * terminate immediately. + */ +#define list_for_each(ptr, head, member) \ + for ((ptr) = container_of((head).next ?: &(head), typeof(*(ptr)), member); \ + (&((ptr)->member) != &(head)); \ + (ptr) = container_of((ptr)->member.next, \ typeof(*(ptr)), member)) #endif /* __COMMONLIB_LIST_H__ */ diff --git a/src/commonlib/list.c b/src/commonlib/list.c index b1030c8263..9cbb554b41 100644 --- a/src/commonlib/list.c +++ b/src/commonlib/list.c @@ -1,10 +1,22 @@ /* Taken from depthcharge: src/base/list.c */ /* SPDX-License-Identifier: GPL-2.0-or-later */ +#include #include +// Initialize a circular list, with `head` being a placeholder head node. +static void list_init(struct list_node *head) +{ + if (!head->next) { + assert(!head->prev); + head->next = head->prev = head; + } +} + void list_remove(struct list_node *node) { + /* Cannot remove the head node. */ + assert(node->prev && node->next); if (node->prev) node->prev->next = node->next; if (node->next) @@ -13,6 +25,9 @@ void list_remove(struct list_node *node) void list_insert_after(struct list_node *node, struct list_node *after) { + /* Check uninitialized head node. */ + if (after->prev == NULL) + list_init(after); node->next = after->next; node->prev = after; after->next = node; @@ -22,6 +37,8 @@ void list_insert_after(struct list_node *node, struct list_node *after) void list_insert_before(struct list_node *node, struct list_node *before) { + /* `before` cannot be an uninitialized head node. */ + assert(before->prev) node->prev = before->prev; node->next = before; before->prev = node; @@ -31,8 +48,7 @@ void list_insert_before(struct list_node *node, struct list_node *before) void list_append(struct list_node *node, struct list_node *head) { - while (head->next) - head = head->next; - - list_insert_after(node, head); + list_init(head); + /* With a circular list, we just need to insert before the head. */ + list_insert_before(node, head); } diff --git a/tests/commonlib/list-test.c b/tests/commonlib/list-test.c index 4ca48a468f..eeeeded059 100644 --- a/tests/commonlib/list-test.c +++ b/tests/commonlib/list-test.c @@ -1,9 +1,10 @@ /* SPDX-License-Identifier: GPL-2.0-only */ -#include +#include #include #include -#include +#include +#include struct test_container { int value; @@ -11,10 +12,20 @@ struct test_container { struct list_node list_node; }; -void test_list_insert_after(void **state) +static void test_list_empty(void **state) +{ + struct list_node head = {}; + struct test_container *node; + + list_for_each(node, head, list_node) { + assert_true(false); + } +} + +static void test_list_insert_after(void **state) { int i = 0; - struct list_node root = { .prev = NULL, .next = NULL }; + struct list_node head = {}; struct test_container *c1 = (struct test_container *)malloc(sizeof(*c1)); struct test_container *c2 = (struct test_container *)malloc(sizeof(*c2)); struct test_container *c3 = (struct test_container *)malloc(sizeof(*c2)); @@ -29,11 +40,11 @@ void test_list_insert_after(void **state) c2->value = values[1]; c3->value = values[2]; - list_insert_after(&c1->list_node, &root); + list_insert_after(&c1->list_node, &head); list_insert_after(&c2->list_node, &c1->list_node); list_insert_after(&c3->list_node, &c2->list_node); - list_for_each(ptr, root, list_node) { + list_for_each(ptr, head, list_node) { assert_int_equal(values[i], ptr->value); i++; } @@ -45,10 +56,10 @@ void test_list_insert_after(void **state) free(c1); } -void test_list_insert_before(void **state) +static void test_list_insert_before(void **state) { int i = 0; - struct list_node root = { .prev = NULL, .next = NULL }; + struct list_node head = {}; struct test_container *c1 = (struct test_container *)malloc(sizeof(*c1)); struct test_container *c2 = (struct test_container *)malloc(sizeof(*c2)); struct test_container *c3 = (struct test_container *)malloc(sizeof(*c2)); @@ -63,12 +74,11 @@ void test_list_insert_before(void **state) c2->value = values[1]; c3->value = values[2]; - list_insert_after(&c3->list_node, &root); + list_insert_after(&c3->list_node, &head); list_insert_before(&c2->list_node, &c3->list_node); list_insert_before(&c1->list_node, &c2->list_node); - - list_for_each(ptr, root, list_node) { + list_for_each(ptr, head, list_node) { assert_int_equal(values[i], ptr->value); i++; } @@ -80,19 +90,27 @@ void test_list_insert_before(void **state) free(c1); } -void test_list_remove(void **state) +static void test_list_insert_before_head(void **state) { - struct list_node root = { .prev = NULL, .next = NULL }; + struct list_node head = {}; + struct test_container c = {}; + + expect_assert_failure(list_insert_before(&c.list_node, &head)); +} + +static void test_list_remove(void **state) +{ + struct list_node head = {}; struct test_container *c1 = (struct test_container *)malloc(sizeof(*c1)); struct test_container *c2 = (struct test_container *)malloc(sizeof(*c2)); struct test_container *ptr; int len; - list_insert_after(&c1->list_node, &root); + list_insert_after(&c1->list_node, &head); list_insert_after(&c2->list_node, &c1->list_node); len = 0; - list_for_each(ptr, root, list_node) { + list_for_each(ptr, head, list_node) { len++; } assert_int_equal(2, len); @@ -100,14 +118,14 @@ void test_list_remove(void **state) list_remove(&c1->list_node); len = 0; - list_for_each(ptr, root, list_node) { + list_for_each(ptr, head, list_node) { len++; } assert_int_equal(1, len); list_remove(&c2->list_node); len = 0; - list_for_each(ptr, root, list_node) { + list_for_each(ptr, head, list_node) { len++; } assert_int_equal(0, len); @@ -116,20 +134,26 @@ void test_list_remove(void **state) free(c1); } -void test_list_append(void **state) +static void test_list_remove_head(void **state) +{ + struct list_node head = {}; + expect_assert_failure(list_remove(&head)); +} + +static void test_list_append(void **state) { size_t idx; struct test_container *node; - struct list_node root = {}; + struct list_node head = {}; struct test_container nodes[] = { {1}, {2}, {3} }; for (idx = 0; idx < ARRAY_SIZE(nodes); ++idx) - list_append(&nodes[idx].list_node, &root); + list_append(&nodes[idx].list_node, &head); idx = 0; - list_for_each(node, root, list_node) { + list_for_each(node, head, list_node) { assert_ptr_equal(node, &nodes[idx]); idx++; } @@ -138,12 +162,14 @@ void test_list_append(void **state) int main(void) { const struct CMUnitTest tests[] = { + cmocka_unit_test(test_list_empty), cmocka_unit_test(test_list_insert_after), cmocka_unit_test(test_list_insert_before), + cmocka_unit_test(test_list_insert_before_head), cmocka_unit_test(test_list_remove), + cmocka_unit_test(test_list_remove_head), cmocka_unit_test(test_list_append), }; - return cb_run_group_tests(tests, NULL, NULL); }