[PATCH 0/5] cyclic: get rid of (the need for) cyclic_init()

[PATCH 0/5] cyclic: get rid of (the need for) cyclic_init()

[Rasmus Villemoes]

I have only compile-tested each of these for sandbox_defconfig and
imx8mq_cm_defconfig. I couldn't even figure out how to run the cyclic
test inside sandbox by itself, and I don't have any hardware here at
home. So perhaps just consider these a POC of the overall idea, namely
to use a list abstraction which doesn't need initialization other than
what we already guarantee to do for all of struct global_data.

As positive side effects, the generated code will be a little smaller,
we reduce the use of the early malloc() pool, and the diffstat below
is also nice.

I don't know if we ever do anything in SPL that would require a call
to cyclic_unregister_all().

Rasmus Villemoes (5):
  cyclic: use a flag in gd->flags for recursion protection
  cyclic: drop redundant cyclic_ready flag
  list.h: synchronize hlist_for_each_entry* iterators with linux
  cyclic: switch to using hlist instead of list
  cyclic: get rid of cyclic_init()

 cmd/cyclic.c                      |  5 +--
 common/board_f.c                  |  2 +-
 common/board_r.c                  |  1 -
 common/cyclic.c                   | 50 ++++++++++-------------------
 include/asm-generic/global_data.h |  8 +++--
 include/cyclic.h                  | 35 +++-----------------
 include/linux/list.h              | 53 +++++++++++++++----------------
 test/test-main.c                  |  3 +-
 8 files changed, 57 insertions(+), 100 deletions(-)

-- 
2.37.2

[PATCH 1/5] cyclic: use a flag in gd->flags for recursion protection

[Rasmus Villemoes]

As a preparation for future patches, use a flag in gd->flags rather
than a separate member in (the singleton) struct cyclic_drv to keep
track of whether we're already inside cyclic_run().

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 common/cyclic.c                   | 6 +++---
 include/asm-generic/global_data.h | 4 ++++
 include/cyclic.h                  | 2 --
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/common/cyclic.c b/common/cyclic.c
index 7abb82c16a..ff75c8cadb 100644
--- a/common/cyclic.c
+++ b/common/cyclic.c
@@ -66,10 +66,10 @@ void cyclic_run(void)
 	uint64_t now, cpu_time;
 
 	/* Prevent recursion */
-	if (gd->cyclic->cyclic_running)
+	if (gd->flags & GD_FLG_CYCLIC_RUNNING)
 		return;
 
-	gd->cyclic->cyclic_running = true;
+	gd->flags |= GD_FLG_CYCLIC_RUNNING;
 	list_for_each_entry_safe(cyclic, tmp, &gd->cyclic->cyclic_list, list) {
 		/*
 		 * Check if this cyclic function needs to get called, e.g.
@@ -99,7 +99,7 @@ void cyclic_run(void)
 			}
 		}
 	}
-	gd->cyclic->cyclic_running = false;
+	gd->flags &= ~GD_FLG_CYCLIC_RUNNING;
 }
 
 void schedule(void)
diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
index 2d55fe2ac0..ca36907b20 100644
--- a/include/asm-generic/global_data.h
+++ b/include/asm-generic/global_data.h
@@ -650,6 +650,10 @@ enum gd_flags {
 	 * @GD_FLG_FDT_CHANGED: Device tree change has been detected by tests
 	 */
 	GD_FLG_FDT_CHANGED = 0x100000,
+	/**
+	 * GD_FLG_CYCLIC_RUNNING: cyclic_run is in progress
+	 */
+	GD_FLG_CYCLIC_RUNNING = 0x200000,
 };
 
 #endif /* __ASSEMBLY__ */
diff --git a/include/cyclic.h b/include/cyclic.h
index 9c5c4fcc54..50427baa3f 100644
--- a/include/cyclic.h
+++ b/include/cyclic.h
@@ -19,12 +19,10 @@
  *
  * @cyclic_list: Cylic list node
  * @cyclic_ready: Flag if cyclic infrastructure is ready
- * @cyclic_running: Flag if cyclic infrastructure is running
  */
 struct cyclic_drv {
 	struct list_head cyclic_list;
 	bool cyclic_ready;
-	bool cyclic_running;
 };
 
 /**
-- 
2.37.2

[PATCH 2/5] cyclic: drop redundant cyclic_ready flag

[Rasmus Villemoes]

We're already relying on gd->cyclic being NULL before cyclic_init() is
called - i.e., we're relying on all of gd being zeroed before entering
any C code. And when we do populate gd->cyclic, its ->cyclic_ready
member is automatically set to true. So we can actually just rely on
testing gd->cyclic itself.

The only wrinkle is that cyclic_uninit() actually did set
->cyclic_ready to false. However, since it doesn't free gd->cyclic,
the cyclic infrastructure is actually still ready (i.e., the list_head
is properly initialized as an empty list).

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 common/cyclic.c  | 6 ++----
 include/cyclic.h | 2 --
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/common/cyclic.c b/common/cyclic.c
index ff75c8cadb..d6f11b002e 100644
--- a/common/cyclic.c
+++ b/common/cyclic.c
@@ -30,7 +30,7 @@ struct cyclic_info *cyclic_register(cyclic_func_t func, uint64_t delay_us,
 {
 	struct cyclic_info *cyclic;
 
-	if (!gd->cyclic->cyclic_ready) {
+	if (!gd->cyclic) {
 		pr_debug("Cyclic IF not ready yet\n");
 		return NULL;
 	}
@@ -112,7 +112,7 @@ void schedule(void)
 	 * schedule() might get called very early before the cyclic IF is
 	 * ready. Make sure to only call cyclic_run() when it's initalized.
 	 */
-	if (gd && gd->cyclic && gd->cyclic->cyclic_ready)
+	if (gd && gd->cyclic)
 		cyclic_run();
 }
 
@@ -122,7 +122,6 @@ int cyclic_uninit(void)
 
 	list_for_each_entry_safe(cyclic, tmp, &gd->cyclic->cyclic_list, list)
 		cyclic_unregister(cyclic);
-	gd->cyclic->cyclic_ready = false;
 
 	return 0;
 }
@@ -137,7 +136,6 @@ int cyclic_init(void)
 
 	memset(gd->cyclic, '\0', size);
 	INIT_LIST_HEAD(&gd->cyclic->cyclic_list);
-	gd->cyclic->cyclic_ready = true;
 
 	return 0;
 }
diff --git a/include/cyclic.h b/include/cyclic.h
index 50427baa3f..263b74d89b 100644
--- a/include/cyclic.h
+++ b/include/cyclic.h
@@ -18,11 +18,9 @@
  * struct cyclic_drv - Cyclic driver internal data
  *
  * @cyclic_list: Cylic list node
- * @cyclic_ready: Flag if cyclic infrastructure is ready
  */
 struct cyclic_drv {
 	struct list_head cyclic_list;
-	bool cyclic_ready;
 };
 
 /**
-- 
2.37.2

[PATCH 3/5] list.h: synchronize hlist_for_each_entry* iterators with linux

[Rasmus Villemoes]

All the way back in 2013, the linux kernel updated the four
hlist_for_each_entry* iterators to require one less auxiliary
variable:

  commit b67bfe0d42cac56c512dd5da4b1b347a23f4b70a
  Author: Sasha Levin <sasha.levin@oracle.com>
  Date:   Wed Feb 27 17:06:00 2013 -0800

      hlist: drop the node parameter from iterators

Currently, there is only one "user" of any of these, namely in
fs/ubifs/super.c, but that actually uses the "new-style" form, and
is (obviously, or it wouldn't have built) inside #ifndef __UBOOT__.

Before adding actual users of these, import the version as of linux
v6.1-rc1, including the hlist_entry_safe() helper used by the new
versions.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 include/linux/list.h | 53 +++++++++++++++++++++-----------------------
 1 file changed, 25 insertions(+), 28 deletions(-)

diff --git a/include/linux/list.h b/include/linux/list.h
index 3eacf68e3a..6910721c00 100644
--- a/include/linux/list.h
+++ b/include/linux/list.h
@@ -646,54 +646,51 @@ static inline void hlist_add_after(struct hlist_node *n,
 	for (pos = (head)->first; pos && ({ n = pos->next; 1; }); \
 	     pos = n)
 
+#define hlist_entry_safe(ptr, type, member) \
+	({ typeof(ptr) ____ptr = (ptr); \
+	   ____ptr ? hlist_entry(____ptr, type, member) : NULL; \
+	})
+
 /**
  * hlist_for_each_entry	- iterate over list of given type
- * @tpos:	the type * to use as a loop cursor.
- * @pos:	the &struct hlist_node to use as a loop cursor.
+ * @pos:	the type * to use as a loop cursor.
  * @head:	the head for your list.
  * @member:	the name of the hlist_node within the struct.
  */
-#define hlist_for_each_entry(tpos, pos, head, member)			 \
-	for (pos = (head)->first;					 \
-	     pos && ({ prefetch(pos->next); 1;}) &&			 \
-		({ tpos = hlist_entry(pos, typeof(*tpos), member); 1;}); \
-	     pos = pos->next)
+#define hlist_for_each_entry(pos, head, member)				\
+	for (pos = hlist_entry_safe((head)->first, typeof(*(pos)), member);\
+	     pos;							\
+	     pos = hlist_entry_safe((pos)->member.next, typeof(*(pos)), member))
 
 /**
  * hlist_for_each_entry_continue - iterate over a hlist continuing after current point
- * @tpos:	the type * to use as a loop cursor.
- * @pos:	the &struct hlist_node to use as a loop cursor.
+ * @pos:	the type * to use as a loop cursor.
  * @member:	the name of the hlist_node within the struct.
  */
-#define hlist_for_each_entry_continue(tpos, pos, member)		 \
-	for (pos = (pos)->next;						 \
-	     pos && ({ prefetch(pos->next); 1;}) &&			 \
-		({ tpos = hlist_entry(pos, typeof(*tpos), member); 1;}); \
-	     pos = pos->next)
+#define hlist_for_each_entry_continue(pos, member)			\
+	for (pos = hlist_entry_safe((pos)->member.next, typeof(*(pos)), member);\
+	     pos;							\
+	     pos = hlist_entry_safe((pos)->member.next, typeof(*(pos)), member))
 
 /**
  * hlist_for_each_entry_from - iterate over a hlist continuing from current point
- * @tpos:	the type * to use as a loop cursor.
- * @pos:	the &struct hlist_node to use as a loop cursor.
+ * @pos:	the type * to use as a loop cursor.
  * @member:	the name of the hlist_node within the struct.
  */
-#define hlist_for_each_entry_from(tpos, pos, member)			 \
-	for (; pos && ({ prefetch(pos->next); 1;}) &&			 \
-		({ tpos = hlist_entry(pos, typeof(*tpos), member); 1;}); \
-	     pos = pos->next)
+#define hlist_for_each_entry_from(pos, member)				\
+	for (; pos;							\
+	     pos = hlist_entry_safe((pos)->member.next, typeof(*(pos)), member))
 
 /**
  * hlist_for_each_entry_safe - iterate over list of given type safe against removal of list entry
- * @tpos:	the type * to use as a loop cursor.
- * @pos:	the &struct hlist_node to use as a loop cursor.
- * @n:		another &struct hlist_node to use as temporary storage
+ * @pos:	the type * to use as a loop cursor.
+ * @n:		a &struct hlist_node to use as temporary storage
  * @head:	the head for your list.
  * @member:	the name of the hlist_node within the struct.
  */
-#define hlist_for_each_entry_safe(tpos, pos, n, head, member)		 \
-	for (pos = (head)->first;					 \
-	     pos && ({ n = pos->next; 1; }) &&				 \
-		({ tpos = hlist_entry(pos, typeof(*tpos), member); 1;}); \
-	     pos = n)
+#define hlist_for_each_entry_safe(pos, n, head, member) 		\
+	for (pos = hlist_entry_safe((head)->first, typeof(*pos), member);\
+	     pos && ({ n = pos->member.next; 1; });			\
+	     pos = hlist_entry_safe(n, typeof(*pos), member))
 
 #endif
-- 
2.37.2

[PATCH 4/5] cyclic: switch to using hlist instead of list

[Rasmus Villemoes]

A hlist is headed by just a single pointer, so can only be traversed
forwards, and insertions can only happen at the head (or before/after
an existing list member). But each list node still consists of two
pointers, so arbitrary elements can still be removed in O(1).

This is precisely what we need for the cyclic_list - we never need to
traverse it backwards, and the order the callbacks appear in the list
should really not matter.

One advantage, and the main reason for doing this switch, is that an
empty list is represented by a NULL head pointer, so unlike a
list_head, it does not need separate C code to initialize - a
memset(,0,) of the containing structure is sufficient.

This is mostly mechanical:

- The iterators are updated with an h prefix, and the type of the
  temporary variable changed to struct hlist_node*.

- Adding/removing is now just hlist_add_head (and not tail) and
  hlist_del().

- struct members and function return values updated.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 cmd/cyclic.c     |  5 +++--
 common/cyclic.c  | 18 ++++++++++--------
 include/cyclic.h |  6 +++---
 3 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/cmd/cyclic.c b/cmd/cyclic.c
index c1bc556aad..97324d8240 100644
--- a/cmd/cyclic.c
+++ b/cmd/cyclic.c
@@ -61,10 +61,11 @@ static int do_cyclic_demo(struct cmd_tbl *cmdtp, int flag, int argc,
 static int do_cyclic_list(struct cmd_tbl *cmdtp, int flag, int argc,
 			  char *const argv[])
 {
-	struct cyclic_info *cyclic, *tmp;
+	struct cyclic_info *cyclic;
+	struct hlist_node *tmp;
 	u64 cnt, freq;
 
-	list_for_each_entry_safe(cyclic, tmp, cyclic_get_list(), list) {
+	hlist_for_each_entry_safe(cyclic, tmp, cyclic_get_list(), list) {
 		cnt = cyclic->run_cnt * 1000000ULL * 100ULL;
 		freq = lldiv(cnt, timer_get_us() - cyclic->start_time_us);
 		printf("function: %s, cpu-time: %lld us, frequency: %lld.%02d times/s\n",
diff --git a/common/cyclic.c b/common/cyclic.c
index d6f11b002e..32d9bf0d02 100644
--- a/common/cyclic.c
+++ b/common/cyclic.c
@@ -20,7 +20,7 @@ DECLARE_GLOBAL_DATA_PTR;
 
 void hw_watchdog_reset(void);
 
-struct list_head *cyclic_get_list(void)
+struct hlist_head *cyclic_get_list(void)
 {
 	return &gd->cyclic->cyclic_list;
 }
@@ -47,14 +47,14 @@ struct cyclic_info *cyclic_register(cyclic_func_t func, uint64_t delay_us,
 	cyclic->name = strdup(name);
 	cyclic->delay_us = delay_us;
 	cyclic->start_time_us = timer_get_us();
-	list_add_tail(&cyclic->list, &gd->cyclic->cyclic_list);
+	hlist_add_head(&cyclic->list, &gd->cyclic->cyclic_list);
 
 	return cyclic;
 }
 
 int cyclic_unregister(struct cyclic_info *cyclic)
 {
-	list_del(&cyclic->list);
+	hlist_del(&cyclic->list);
 	free(cyclic);
 
 	return 0;
@@ -62,7 +62,8 @@ int cyclic_unregister(struct cyclic_info *cyclic)
 
 void cyclic_run(void)
 {
-	struct cyclic_info *cyclic, *tmp;
+	struct cyclic_info *cyclic;
+	struct hlist_node *tmp;
 	uint64_t now, cpu_time;
 
 	/* Prevent recursion */
@@ -70,7 +71,7 @@ void cyclic_run(void)
 		return;
 
 	gd->flags |= GD_FLG_CYCLIC_RUNNING;
-	list_for_each_entry_safe(cyclic, tmp, &gd->cyclic->cyclic_list, list) {
+	hlist_for_each_entry_safe(cyclic, tmp, &gd->cyclic->cyclic_list, list) {
 		/*
 		 * Check if this cyclic function needs to get called, e.g.
 		 * do not call the cyclic func too often
@@ -118,9 +119,10 @@ void schedule(void)
 
 int cyclic_uninit(void)
 {
-	struct cyclic_info *cyclic, *tmp;
+	struct cyclic_info *cyclic;
+	struct hlist_node *tmp;
 
-	list_for_each_entry_safe(cyclic, tmp, &gd->cyclic->cyclic_list, list)
+	hlist_for_each_entry_safe(cyclic, tmp, &gd->cyclic->cyclic_list, list)
 		cyclic_unregister(cyclic);
 
 	return 0;
@@ -135,7 +137,7 @@ int cyclic_init(void)
 		return -ENOMEM;
 
 	memset(gd->cyclic, '\0', size);
-	INIT_LIST_HEAD(&gd->cyclic->cyclic_list);
+	INIT_HLIST_HEAD(&gd->cyclic->cyclic_list);
 
 	return 0;
 }
diff --git a/include/cyclic.h b/include/cyclic.h
index 263b74d89b..4a11f9b105 100644
--- a/include/cyclic.h
+++ b/include/cyclic.h
@@ -20,7 +20,7 @@
  * @cyclic_list: Cylic list node
  */
 struct cyclic_drv {
-	struct list_head cyclic_list;
+	struct hlist_head cyclic_list;
 };
 
 /**
@@ -46,7 +46,7 @@ struct cyclic_info {
 	uint64_t cpu_time_us;
 	uint64_t run_cnt;
 	uint64_t next_call;
-	struct list_head list;
+	struct hlist_node list;
 	bool already_warned;
 };
 
@@ -95,7 +95,7 @@ int cyclic_uninit(void);
  *
  * @return: pointer to cyclic_list
  */
-struct list_head *cyclic_get_list(void);
+struct hlist_head *cyclic_get_list(void);
 
 /**
  * cyclic_run() - Interate over all registered cyclic functions
-- 
2.37.2

[PATCH 5/5] cyclic: get rid of cyclic_init()

[Rasmus Villemoes]

Currently, we must call cyclic_init() at some point before
cyclic_register() becomes possible. That turns out to be somewhat
awkward, especially with SPL, and has resulted in a watchdog callback
not being registered, thus causing the board to prematurely reset.

We already rely on gd->cyclic reliably being set to NULL by the asm
code that clears all of gd. Now that the cyclic list is a hlist, and
thus an empty list is represented by a NULL head pointer, and struct
cyclic_drv has no other members, we can just as well drop a level of
indirection and put the hlist_head directly in struct
global_data. This doesn't increase the size of struct global_data,
gets rid of an early malloc(), and generates slightly smaller code.

But primarily, this avoids having to call cyclic_init() early; the cyclic
infrastructure is simply ready to register callbacks as soon as we
enter C code.

We can still end up with schedule() being called from asm very early,
so we still need to check that gd itself has been properly initialized
[*], but once it has, gd->cyclic_list is perfectly fine to access, and
will just be an empty list.

As for cyclic_uninit(), it was never really the opposite of
cyclic_init() since it didn't free the struct cyclic_drv nor set
gd->cyclic to NULL. Rename it to cyclic_unregister_all() and use that
in test/, and also insert a call at the end of the board_init_f
sequence so that gd->cyclic_list is a fresh empty list before we enter
board_init_r().

A small piece of ugliness is that I had to add a cast in
cyclic_get_list() to silence a "discards 'volatile' qualifier"
warning, but that is completely equivalent to the existing handling of
the uclass_root_s list_head member.

[*] I'm not really sure where we guarantee that the register used for
gd contains 0 until it gets explicitly initialized, but that must be
the case, otherwise testing gd for being NULL would not make much sense.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 common/board_f.c                  |  2 +-
 common/board_r.c                  |  1 -
 common/cyclic.c                   | 32 +++++++------------------------
 include/asm-generic/global_data.h |  4 ++--
 include/cyclic.h                  | 27 +++-----------------------
 test/test-main.c                  |  3 +--
 6 files changed, 14 insertions(+), 55 deletions(-)

diff --git a/common/board_f.c b/common/board_f.c
index 4355d1c82d..1f9140a065 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -844,7 +844,6 @@ static const init_fnc_t init_sequence_f[] = {
 	initf_malloc,
 	log_init,
 	initf_bootstage,	/* uses its own timer, so does not need DM */
-	cyclic_init,
 	event_init,
 #ifdef CONFIG_BLOBLIST
 	bloblist_init,
@@ -962,6 +961,7 @@ static const init_fnc_t init_sequence_f[] = {
 	do_elf_reloc_fixups,
 #endif
 	clear_bss,
+	cyclic_unregister_all,
 #if !defined(CONFIG_ARM) && !defined(CONFIG_SANDBOX) && \
 		!CONFIG_IS_ENABLED(X86_64)
 	jump_to_copy,
diff --git a/common/board_r.c b/common/board_r.c
index 92ca2066ee..0e2e63a21b 100644
--- a/common/board_r.c
+++ b/common/board_r.c
@@ -614,7 +614,6 @@ static init_fnc_t init_sequence_r[] = {
 #endif
 	initr_barrier,
 	initr_malloc,
-	cyclic_init,
 	log_init,
 	initr_bootstage,	/* Needs malloc() but has its own timer */
 #if defined(CONFIG_CONSOLE_RECORD)
diff --git a/common/cyclic.c b/common/cyclic.c
index 32d9bf0d02..a49bfc88f5 100644
--- a/common/cyclic.c
+++ b/common/cyclic.c
@@ -22,7 +22,8 @@ void hw_watchdog_reset(void);
 
 struct hlist_head *cyclic_get_list(void)
 {
-	return &gd->cyclic->cyclic_list;
+	/* Silence "discards 'volatile' qualifier" warning. */
+	return (struct hlist_head *)&gd->cyclic_list;
 }
 
 struct cyclic_info *cyclic_register(cyclic_func_t func, uint64_t delay_us,
@@ -30,11 +31,6 @@ struct cyclic_info *cyclic_register(cyclic_func_t func, uint64_t delay_us,
 {
 	struct cyclic_info *cyclic;
 
-	if (!gd->cyclic) {
-		pr_debug("Cyclic IF not ready yet\n");
-		return NULL;
-	}
-
 	cyclic = calloc(1, sizeof(struct cyclic_info));
 	if (!cyclic) {
 		pr_debug("Memory allocation error\n");
@@ -47,7 +43,7 @@ struct cyclic_info *cyclic_register(cyclic_func_t func, uint64_t delay_us,
 	cyclic->name = strdup(name);
 	cyclic->delay_us = delay_us;
 	cyclic->start_time_us = timer_get_us();
-	hlist_add_head(&cyclic->list, &gd->cyclic->cyclic_list);
+	hlist_add_head(&cyclic->list, cyclic_get_list());
 
 	return cyclic;
 }
@@ -71,7 +67,7 @@ void cyclic_run(void)
 		return;
 
 	gd->flags |= GD_FLG_CYCLIC_RUNNING;
-	hlist_for_each_entry_safe(cyclic, tmp, &gd->cyclic->cyclic_list, list) {
+	hlist_for_each_entry_safe(cyclic, tmp, cyclic_get_list(), list) {
 		/*
 		 * Check if this cyclic function needs to get called, e.g.
 		 * do not call the cyclic func too often
@@ -113,31 +109,17 @@ void schedule(void)
 	 * schedule() might get called very early before the cyclic IF is
 	 * ready. Make sure to only call cyclic_run() when it's initalized.
 	 */
-	if (gd && gd->cyclic)
+	if (gd)
 		cyclic_run();
 }
 
-int cyclic_uninit(void)
+int cyclic_unregister_all(void)
 {
 	struct cyclic_info *cyclic;
 	struct hlist_node *tmp;
 
-	hlist_for_each_entry_safe(cyclic, tmp, &gd->cyclic->cyclic_list, list)
+	hlist_for_each_entry_safe(cyclic, tmp, cyclic_get_list(), list)
 		cyclic_unregister(cyclic);
 
 	return 0;
 }
-
-int cyclic_init(void)
-{
-	int size = sizeof(struct cyclic_drv);
-
-	gd->cyclic = (struct cyclic_drv *)malloc(size);
-	if (!gd->cyclic)
-		return -ENOMEM;
-
-	memset(gd->cyclic, '\0', size);
-	INIT_HLIST_HEAD(&gd->cyclic->cyclic_list);
-
-	return 0;
-}
diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
index ca36907b20..1a2e55454d 100644
--- a/include/asm-generic/global_data.h
+++ b/include/asm-generic/global_data.h
@@ -481,9 +481,9 @@ struct global_data {
 #endif
 #ifdef CONFIG_CYCLIC
 	/**
-	 * @cyclic: cyclic driver data
+	 * @cyclic_list: list of registered cyclic functions
 	 */
-	struct cyclic_drv *cyclic;
+	struct hlist_head cyclic_list;
 #endif
 	/**
 	 * @dmtag_list: List of DM tags
diff --git a/include/cyclic.h b/include/cyclic.h
index 4a11f9b105..44ad3cb6b8 100644
--- a/include/cyclic.h
+++ b/include/cyclic.h
@@ -14,15 +14,6 @@
 #include <linux/list.h>
 #include <asm/types.h>
 
-/**
- * struct cyclic_drv - Cyclic driver internal data
- *
- * @cyclic_list: Cylic list node
- */
-struct cyclic_drv {
-	struct hlist_head cyclic_list;
-};
-
 /**
  * struct cyclic_info - Information about cyclic execution function
  *
@@ -75,18 +66,11 @@ struct cyclic_info *cyclic_register(cyclic_func_t func, uint64_t delay_us,
 int cyclic_unregister(struct cyclic_info *cyclic);
 
 /**
- * cyclic_init() - Set up cyclic functions
- *
- * Init a list of cyclic functions, so that these can be added as needed
- */
-int cyclic_init(void);
-
-/**
- * cyclic_uninit() - Clean up cyclic functions
+ * cyclic_unregister_all() - Clean up cyclic functions
  *
  * This removes all cyclic functions
  */
-int cyclic_uninit(void);
+int cyclic_unregister_all(void);
 
 /**
  * cyclic_get_list() - Get cyclic list pointer
@@ -134,12 +118,7 @@ static inline void schedule(void)
 {
 }
 
-static inline int cyclic_init(void)
-{
-	return 0;
-}
-
-static inline int cyclic_uninit(void)
+static inline int cyclic_unregister_all(void)
 {
 	return 0;
 }
diff --git a/test/test-main.c b/test/test-main.c
index a98a77d68f..554dd8d96b 100644
--- a/test/test-main.c
+++ b/test/test-main.c
@@ -287,7 +287,6 @@ static int dm_test_restore(struct device_node *of_root)
 static int test_pre_run(struct unit_test_state *uts, struct unit_test *test)
 {
 	ut_assertok(event_init());
-	ut_assertok(cyclic_init());
 
 	if (test->flags & UT_TESTF_DM)
 		ut_assertok(dm_test_pre_run(uts));
@@ -347,7 +346,7 @@ static int test_post_run(struct unit_test_state *uts, struct unit_test *test)
 	ut_unsilence_console(uts);
 	if (test->flags & UT_TESTF_DM)
 		ut_assertok(dm_test_post_run(uts));
-	ut_assertok(cyclic_uninit());
+	ut_assertok(cyclic_unregister_all());
 	ut_assertok(event_uninit());
 
 	free(uts->of_other);
-- 
2.37.2

Re: [PATCH 1/5] cyclic: use a flag in gd->flags for recursion protection

[Stefan Roese]

On 28.10.22 13:50, Rasmus Villemoes wrote:
> As a preparation for future patches, use a flag in gd->flags rather
> than a separate member in (the singleton) struct cyclic_drv to keep
> track of whether we're already inside cyclic_run().
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>   common/cyclic.c                   | 6 +++---
>   include/asm-generic/global_data.h | 4 ++++
>   include/cyclic.h                  | 2 --
>   3 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/common/cyclic.c b/common/cyclic.c
> index 7abb82c16a..ff75c8cadb 100644
> --- a/common/cyclic.c
> +++ b/common/cyclic.c
> @@ -66,10 +66,10 @@ void cyclic_run(void)
>   	uint64_t now, cpu_time;
>   
>   	/* Prevent recursion */
> -	if (gd->cyclic->cyclic_running)
> +	if (gd->flags & GD_FLG_CYCLIC_RUNNING)
>   		return;
>   
> -	gd->cyclic->cyclic_running = true;
> +	gd->flags |= GD_FLG_CYCLIC_RUNNING;
>   	list_for_each_entry_safe(cyclic, tmp, &gd->cyclic->cyclic_list, list) {
>   		/*
>   		 * Check if this cyclic function needs to get called, e.g.
> @@ -99,7 +99,7 @@ void cyclic_run(void)
>   			}
>   		}
>   	}
> -	gd->cyclic->cyclic_running = false;
> +	gd->flags &= ~GD_FLG_CYCLIC_RUNNING;
>   }
>   
>   void schedule(void)
> diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
> index 2d55fe2ac0..ca36907b20 100644
> --- a/include/asm-generic/global_data.h
> +++ b/include/asm-generic/global_data.h
> @@ -650,6 +650,10 @@ enum gd_flags {
>   	 * @GD_FLG_FDT_CHANGED: Device tree change has been detected by tests
>   	 */
>   	GD_FLG_FDT_CHANGED = 0x100000,
> +	/**
> +	 * GD_FLG_CYCLIC_RUNNING: cyclic_run is in progress
> +	 */
> +	GD_FLG_CYCLIC_RUNNING = 0x200000,
>   };
>   
>   #endif /* __ASSEMBLY__ */
> diff --git a/include/cyclic.h b/include/cyclic.h
> index 9c5c4fcc54..50427baa3f 100644
> --- a/include/cyclic.h
> +++ b/include/cyclic.h
> @@ -19,12 +19,10 @@
>    *
>    * @cyclic_list: Cylic list node
>    * @cyclic_ready: Flag if cyclic infrastructure is ready
> - * @cyclic_running: Flag if cyclic infrastructure is running
>    */
>   struct cyclic_drv {
>   	struct list_head cyclic_list;
>   	bool cyclic_ready;
> -	bool cyclic_running;
>   };
>   
>   /**

Reviewed-by: Stefan Roese <sr@denx.de>
Tested-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

Re: [PATCH 2/5] cyclic: drop redundant cyclic_ready flag

[Stefan Roese]

On 28.10.22 13:50, Rasmus Villemoes wrote:
> We're already relying on gd->cyclic being NULL before cyclic_init() is
> called - i.e., we're relying on all of gd being zeroed before entering
> any C code. And when we do populate gd->cyclic, its ->cyclic_ready
> member is automatically set to true. So we can actually just rely on
> testing gd->cyclic itself.
> 
> The only wrinkle is that cyclic_uninit() actually did set
> ->cyclic_ready to false. However, since it doesn't free gd->cyclic,
> the cyclic infrastructure is actually still ready (i.e., the list_head
> is properly initialized as an empty list).
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>   common/cyclic.c  | 6 ++----
>   include/cyclic.h | 2 --
>   2 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/common/cyclic.c b/common/cyclic.c
> index ff75c8cadb..d6f11b002e 100644
> --- a/common/cyclic.c
> +++ b/common/cyclic.c
> @@ -30,7 +30,7 @@ struct cyclic_info *cyclic_register(cyclic_func_t func, uint64_t delay_us,
>   {
>   	struct cyclic_info *cyclic;
>   
> -	if (!gd->cyclic->cyclic_ready) {
> +	if (!gd->cyclic) {
>   		pr_debug("Cyclic IF not ready yet\n");
>   		return NULL;
>   	}
> @@ -112,7 +112,7 @@ void schedule(void)
>   	 * schedule() might get called very early before the cyclic IF is
>   	 * ready. Make sure to only call cyclic_run() when it's initalized.
>   	 */
> -	if (gd && gd->cyclic && gd->cyclic->cyclic_ready)
> +	if (gd && gd->cyclic)
>   		cyclic_run();
>   }
>   
> @@ -122,7 +122,6 @@ int cyclic_uninit(void)
>   
>   	list_for_each_entry_safe(cyclic, tmp, &gd->cyclic->cyclic_list, list)
>   		cyclic_unregister(cyclic);
> -	gd->cyclic->cyclic_ready = false;
>   
>   	return 0;
>   }
> @@ -137,7 +136,6 @@ int cyclic_init(void)
>   
>   	memset(gd->cyclic, '\0', size);
>   	INIT_LIST_HEAD(&gd->cyclic->cyclic_list);
> -	gd->cyclic->cyclic_ready = true;
>   
>   	return 0;
>   }
> diff --git a/include/cyclic.h b/include/cyclic.h
> index 50427baa3f..263b74d89b 100644
> --- a/include/cyclic.h
> +++ b/include/cyclic.h
> @@ -18,11 +18,9 @@
>    * struct cyclic_drv - Cyclic driver internal data
>    *
>    * @cyclic_list: Cylic list node
> - * @cyclic_ready: Flag if cyclic infrastructure is ready
>    */
>   struct cyclic_drv {
>   	struct list_head cyclic_list;
> -	bool cyclic_ready;
>   };
>   
>   /**

Reviewed-by: Stefan Roese <sr@denx.de>
Tested-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

Re: [PATCH 3/5] list.h: synchronize hlist_for_each_entry* iterators with linux

[Stefan Roese]

On 28.10.22 13:50, Rasmus Villemoes wrote:
> All the way back in 2013, the linux kernel updated the four
> hlist_for_each_entry* iterators to require one less auxiliary
> variable:
> 
>    commit b67bfe0d42cac56c512dd5da4b1b347a23f4b70a
>    Author: Sasha Levin <sasha.levin@oracle.com>
>    Date:   Wed Feb 27 17:06:00 2013 -0800
> 
>        hlist: drop the node parameter from iterators
> 
> Currently, there is only one "user" of any of these, namely in
> fs/ubifs/super.c, but that actually uses the "new-style" form, and
> is (obviously, or it wouldn't have built) inside #ifndef __UBOOT__.
> 
> Before adding actual users of these, import the version as of linux
> v6.1-rc1, including the hlist_entry_safe() helper used by the new
> versions.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>   include/linux/list.h | 53 +++++++++++++++++++++-----------------------
>   1 file changed, 25 insertions(+), 28 deletions(-)
> 
> diff --git a/include/linux/list.h b/include/linux/list.h
> index 3eacf68e3a..6910721c00 100644
> --- a/include/linux/list.h
> +++ b/include/linux/list.h
> @@ -646,54 +646,51 @@ static inline void hlist_add_after(struct hlist_node *n,
>   	for (pos = (head)->first; pos && ({ n = pos->next; 1; }); \
>   	     pos = n)
>   
> +#define hlist_entry_safe(ptr, type, member) \
> +	({ typeof(ptr) ____ptr = (ptr); \
> +	   ____ptr ? hlist_entry(____ptr, type, member) : NULL; \
> +	})
> +
>   /**
>    * hlist_for_each_entry	- iterate over list of given type
> - * @tpos:	the type * to use as a loop cursor.
> - * @pos:	the &struct hlist_node to use as a loop cursor.
> + * @pos:	the type * to use as a loop cursor.
>    * @head:	the head for your list.
>    * @member:	the name of the hlist_node within the struct.
>    */
> -#define hlist_for_each_entry(tpos, pos, head, member)			 \
> -	for (pos = (head)->first;					 \
> -	     pos && ({ prefetch(pos->next); 1;}) &&			 \
> -		({ tpos = hlist_entry(pos, typeof(*tpos), member); 1;}); \
> -	     pos = pos->next)
> +#define hlist_for_each_entry(pos, head, member)				\
> +	for (pos = hlist_entry_safe((head)->first, typeof(*(pos)), member);\
> +	     pos;							\
> +	     pos = hlist_entry_safe((pos)->member.next, typeof(*(pos)), member))
>   
>   /**
>    * hlist_for_each_entry_continue - iterate over a hlist continuing after current point
> - * @tpos:	the type * to use as a loop cursor.
> - * @pos:	the &struct hlist_node to use as a loop cursor.
> + * @pos:	the type * to use as a loop cursor.
>    * @member:	the name of the hlist_node within the struct.
>    */
> -#define hlist_for_each_entry_continue(tpos, pos, member)		 \
> -	for (pos = (pos)->next;						 \
> -	     pos && ({ prefetch(pos->next); 1;}) &&			 \
> -		({ tpos = hlist_entry(pos, typeof(*tpos), member); 1;}); \
> -	     pos = pos->next)
> +#define hlist_for_each_entry_continue(pos, member)			\
> +	for (pos = hlist_entry_safe((pos)->member.next, typeof(*(pos)), member);\
> +	     pos;							\
> +	     pos = hlist_entry_safe((pos)->member.next, typeof(*(pos)), member))
>   
>   /**
>    * hlist_for_each_entry_from - iterate over a hlist continuing from current point
> - * @tpos:	the type * to use as a loop cursor.
> - * @pos:	the &struct hlist_node to use as a loop cursor.
> + * @pos:	the type * to use as a loop cursor.
>    * @member:	the name of the hlist_node within the struct.
>    */
> -#define hlist_for_each_entry_from(tpos, pos, member)			 \
> -	for (; pos && ({ prefetch(pos->next); 1;}) &&			 \
> -		({ tpos = hlist_entry(pos, typeof(*tpos), member); 1;}); \
> -	     pos = pos->next)
> +#define hlist_for_each_entry_from(pos, member)				\
> +	for (; pos;							\
> +	     pos = hlist_entry_safe((pos)->member.next, typeof(*(pos)), member))
>   
>   /**
>    * hlist_for_each_entry_safe - iterate over list of given type safe against removal of list entry
> - * @tpos:	the type * to use as a loop cursor.
> - * @pos:	the &struct hlist_node to use as a loop cursor.
> - * @n:		another &struct hlist_node to use as temporary storage
> + * @pos:	the type * to use as a loop cursor.
> + * @n:		a &struct hlist_node to use as temporary storage
>    * @head:	the head for your list.
>    * @member:	the name of the hlist_node within the struct.
>    */
> -#define hlist_for_each_entry_safe(tpos, pos, n, head, member)		 \
> -	for (pos = (head)->first;					 \
> -	     pos && ({ n = pos->next; 1; }) &&				 \
> -		({ tpos = hlist_entry(pos, typeof(*tpos), member); 1;}); \
> -	     pos = n)
> +#define hlist_for_each_entry_safe(pos, n, head, member) 		\
> +	for (pos = hlist_entry_safe((head)->first, typeof(*pos), member);\
> +	     pos && ({ n = pos->member.next; 1; });			\
> +	     pos = hlist_entry_safe(n, typeof(*pos), member))
>   
>   #endif

Reviewed-by: Stefan Roese <sr@denx.de>
Tested-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

Re: [PATCH 4/5] cyclic: switch to using hlist instead of list

[Stefan Roese]

On 28.10.22 13:50, Rasmus Villemoes wrote:
> A hlist is headed by just a single pointer, so can only be traversed
> forwards, and insertions can only happen at the head (or before/after
> an existing list member). But each list node still consists of two
> pointers, so arbitrary elements can still be removed in O(1).
> 
> This is precisely what we need for the cyclic_list - we never need to
> traverse it backwards, and the order the callbacks appear in the list
> should really not matter.
> 
> One advantage, and the main reason for doing this switch, is that an
> empty list is represented by a NULL head pointer, so unlike a
> list_head, it does not need separate C code to initialize - a
> memset(,0,) of the containing structure is sufficient.
> 
> This is mostly mechanical:
> 
> - The iterators are updated with an h prefix, and the type of the
>    temporary variable changed to struct hlist_node*.
> 
> - Adding/removing is now just hlist_add_head (and not tail) and
>    hlist_del().
> 
> - struct members and function return values updated.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>   cmd/cyclic.c     |  5 +++--
>   common/cyclic.c  | 18 ++++++++++--------
>   include/cyclic.h |  6 +++---
>   3 files changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/cmd/cyclic.c b/cmd/cyclic.c
> index c1bc556aad..97324d8240 100644
> --- a/cmd/cyclic.c
> +++ b/cmd/cyclic.c
> @@ -61,10 +61,11 @@ static int do_cyclic_demo(struct cmd_tbl *cmdtp, int flag, int argc,
>   static int do_cyclic_list(struct cmd_tbl *cmdtp, int flag, int argc,
>   			  char *const argv[])
>   {
> -	struct cyclic_info *cyclic, *tmp;
> +	struct cyclic_info *cyclic;
> +	struct hlist_node *tmp;
>   	u64 cnt, freq;
>   
> -	list_for_each_entry_safe(cyclic, tmp, cyclic_get_list(), list) {
> +	hlist_for_each_entry_safe(cyclic, tmp, cyclic_get_list(), list) {
>   		cnt = cyclic->run_cnt * 1000000ULL * 100ULL;
>   		freq = lldiv(cnt, timer_get_us() - cyclic->start_time_us);
>   		printf("function: %s, cpu-time: %lld us, frequency: %lld.%02d times/s\n",
> diff --git a/common/cyclic.c b/common/cyclic.c
> index d6f11b002e..32d9bf0d02 100644
> --- a/common/cyclic.c
> +++ b/common/cyclic.c
> @@ -20,7 +20,7 @@ DECLARE_GLOBAL_DATA_PTR;
>   
>   void hw_watchdog_reset(void);
>   
> -struct list_head *cyclic_get_list(void)
> +struct hlist_head *cyclic_get_list(void)
>   {
>   	return &gd->cyclic->cyclic_list;
>   }
> @@ -47,14 +47,14 @@ struct cyclic_info *cyclic_register(cyclic_func_t func, uint64_t delay_us,
>   	cyclic->name = strdup(name);
>   	cyclic->delay_us = delay_us;
>   	cyclic->start_time_us = timer_get_us();
> -	list_add_tail(&cyclic->list, &gd->cyclic->cyclic_list);
> +	hlist_add_head(&cyclic->list, &gd->cyclic->cyclic_list);
>   
>   	return cyclic;
>   }
>   
>   int cyclic_unregister(struct cyclic_info *cyclic)
>   {
> -	list_del(&cyclic->list);
> +	hlist_del(&cyclic->list);
>   	free(cyclic);
>   
>   	return 0;
> @@ -62,7 +62,8 @@ int cyclic_unregister(struct cyclic_info *cyclic)
>   
>   void cyclic_run(void)
>   {
> -	struct cyclic_info *cyclic, *tmp;
> +	struct cyclic_info *cyclic;
> +	struct hlist_node *tmp;
>   	uint64_t now, cpu_time;
>   
>   	/* Prevent recursion */
> @@ -70,7 +71,7 @@ void cyclic_run(void)
>   		return;
>   
>   	gd->flags |= GD_FLG_CYCLIC_RUNNING;
> -	list_for_each_entry_safe(cyclic, tmp, &gd->cyclic->cyclic_list, list) {
> +	hlist_for_each_entry_safe(cyclic, tmp, &gd->cyclic->cyclic_list, list) {
>   		/*
>   		 * Check if this cyclic function needs to get called, e.g.
>   		 * do not call the cyclic func too often
> @@ -118,9 +119,10 @@ void schedule(void)
>   
>   int cyclic_uninit(void)
>   {
> -	struct cyclic_info *cyclic, *tmp;
> +	struct cyclic_info *cyclic;
> +	struct hlist_node *tmp;
>   
> -	list_for_each_entry_safe(cyclic, tmp, &gd->cyclic->cyclic_list, list)
> +	hlist_for_each_entry_safe(cyclic, tmp, &gd->cyclic->cyclic_list, list)
>   		cyclic_unregister(cyclic);
>   
>   	return 0;
> @@ -135,7 +137,7 @@ int cyclic_init(void)
>   		return -ENOMEM;
>   
>   	memset(gd->cyclic, '\0', size);
> -	INIT_LIST_HEAD(&gd->cyclic->cyclic_list);
> +	INIT_HLIST_HEAD(&gd->cyclic->cyclic_list);
>   
>   	return 0;
>   }
> diff --git a/include/cyclic.h b/include/cyclic.h
> index 263b74d89b..4a11f9b105 100644
> --- a/include/cyclic.h
> +++ b/include/cyclic.h
> @@ -20,7 +20,7 @@
>    * @cyclic_list: Cylic list node
>    */
>   struct cyclic_drv {
> -	struct list_head cyclic_list;
> +	struct hlist_head cyclic_list;
>   };
>   
>   /**
> @@ -46,7 +46,7 @@ struct cyclic_info {
>   	uint64_t cpu_time_us;
>   	uint64_t run_cnt;
>   	uint64_t next_call;
> -	struct list_head list;
> +	struct hlist_node list;
>   	bool already_warned;
>   };
>   
> @@ -95,7 +95,7 @@ int cyclic_uninit(void);
>    *
>    * @return: pointer to cyclic_list
>    */
> -struct list_head *cyclic_get_list(void);
> +struct hlist_head *cyclic_get_list(void);
>   
>   /**
>    * cyclic_run() - Interate over all registered cyclic functions

Reviewed-by: Stefan Roese <sr@denx.de>
Tested-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

Re: [PATCH 5/5] cyclic: get rid of cyclic_init()

[Stefan Roese]

On 28.10.22 13:50, Rasmus Villemoes wrote:
> Currently, we must call cyclic_init() at some point before
> cyclic_register() becomes possible. That turns out to be somewhat
> awkward, especially with SPL, and has resulted in a watchdog callback
> not being registered, thus causing the board to prematurely reset.
> 
> We already rely on gd->cyclic reliably being set to NULL by the asm
> code that clears all of gd. Now that the cyclic list is a hlist, and
> thus an empty list is represented by a NULL head pointer, and struct
> cyclic_drv has no other members, we can just as well drop a level of
> indirection and put the hlist_head directly in struct
> global_data. This doesn't increase the size of struct global_data,
> gets rid of an early malloc(), and generates slightly smaller code.

Elegant, simple, good. Thanks.

> But primarily, this avoids having to call cyclic_init() early; the cyclic
> infrastructure is simply ready to register callbacks as soon as we
> enter C code.
> 
> We can still end up with schedule() being called from asm very early,
> so we still need to check that gd itself has been properly initialized
> [*], but once it has, gd->cyclic_list is perfectly fine to access, and
> will just be an empty list.
> 
> As for cyclic_uninit(), it was never really the opposite of
> cyclic_init() since it didn't free the struct cyclic_drv nor set
> gd->cyclic to NULL. Rename it to cyclic_unregister_all() and use that
> in test/, and also insert a call at the end of the board_init_f
> sequence so that gd->cyclic_list is a fresh empty list before we enter
> board_init_r().

While reviewing the code, this was the only thing I wanted to
ask about. Now with this explanation it makes perfect sense.
Perhaps a small comment with this reasoning in the code here in
board_init_r would be helpful.

> A small piece of ugliness is that I had to add a cast in
> cyclic_get_list() to silence a "discards 'volatile' qualifier"
> warning, but that is completely equivalent to the existing handling of
> the uclass_root_s list_head member.
> 
> [*] I'm not really sure where we guarantee that the register used for
> gd contains 0 until it gets explicitly initialized, but that must be
> the case, otherwise testing gd for being NULL would not make much sense.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>   common/board_f.c                  |  2 +-
>   common/board_r.c                  |  1 -
>   common/cyclic.c                   | 32 +++++++------------------------
>   include/asm-generic/global_data.h |  4 ++--
>   include/cyclic.h                  | 27 +++-----------------------
>   test/test-main.c                  |  3 +--
>   6 files changed, 14 insertions(+), 55 deletions(-)
> 
> diff --git a/common/board_f.c b/common/board_f.c
> index 4355d1c82d..1f9140a065 100644
> --- a/common/board_f.c
> +++ b/common/board_f.c
> @@ -844,7 +844,6 @@ static const init_fnc_t init_sequence_f[] = {
>   	initf_malloc,
>   	log_init,
>   	initf_bootstage,	/* uses its own timer, so does not need DM */
> -	cyclic_init,
>   	event_init,
>   #ifdef CONFIG_BLOBLIST
>   	bloblist_init,
> @@ -962,6 +961,7 @@ static const init_fnc_t init_sequence_f[] = {
>   	do_elf_reloc_fixups,
>   #endif
>   	clear_bss,
> +	cyclic_unregister_all,
>   #if !defined(CONFIG_ARM) && !defined(CONFIG_SANDBOX) && \
>   		!CONFIG_IS_ENABLED(X86_64)
>   	jump_to_copy,
> diff --git a/common/board_r.c b/common/board_r.c
> index 92ca2066ee..0e2e63a21b 100644
> --- a/common/board_r.c
> +++ b/common/board_r.c
> @@ -614,7 +614,6 @@ static init_fnc_t init_sequence_r[] = {
>   #endif
>   	initr_barrier,
>   	initr_malloc,
> -	cyclic_init,
>   	log_init,
>   	initr_bootstage,	/* Needs malloc() but has its own timer */
>   #if defined(CONFIG_CONSOLE_RECORD)
> diff --git a/common/cyclic.c b/common/cyclic.c
> index 32d9bf0d02..a49bfc88f5 100644
> --- a/common/cyclic.c
> +++ b/common/cyclic.c
> @@ -22,7 +22,8 @@ void hw_watchdog_reset(void);
>   
>   struct hlist_head *cyclic_get_list(void)
>   {
> -	return &gd->cyclic->cyclic_list;
> +	/* Silence "discards 'volatile' qualifier" warning. */
> +	return (struct hlist_head *)&gd->cyclic_list;
>   }
>   
>   struct cyclic_info *cyclic_register(cyclic_func_t func, uint64_t delay_us,
> @@ -30,11 +31,6 @@ struct cyclic_info *cyclic_register(cyclic_func_t func, uint64_t delay_us,
>   {
>   	struct cyclic_info *cyclic;
>   
> -	if (!gd->cyclic) {
> -		pr_debug("Cyclic IF not ready yet\n");
> -		return NULL;
> -	}
> -
>   	cyclic = calloc(1, sizeof(struct cyclic_info));
>   	if (!cyclic) {
>   		pr_debug("Memory allocation error\n");
> @@ -47,7 +43,7 @@ struct cyclic_info *cyclic_register(cyclic_func_t func, uint64_t delay_us,
>   	cyclic->name = strdup(name);
>   	cyclic->delay_us = delay_us;
>   	cyclic->start_time_us = timer_get_us();
> -	hlist_add_head(&cyclic->list, &gd->cyclic->cyclic_list);
> +	hlist_add_head(&cyclic->list, cyclic_get_list());
>   
>   	return cyclic;
>   }
> @@ -71,7 +67,7 @@ void cyclic_run(void)
>   		return;
>   
>   	gd->flags |= GD_FLG_CYCLIC_RUNNING;
> -	hlist_for_each_entry_safe(cyclic, tmp, &gd->cyclic->cyclic_list, list) {
> +	hlist_for_each_entry_safe(cyclic, tmp, cyclic_get_list(), list) {
>   		/*
>   		 * Check if this cyclic function needs to get called, e.g.
>   		 * do not call the cyclic func too often
> @@ -113,31 +109,17 @@ void schedule(void)
>   	 * schedule() might get called very early before the cyclic IF is
>   	 * ready. Make sure to only call cyclic_run() when it's initalized.
>   	 */
> -	if (gd && gd->cyclic)
> +	if (gd)
>   		cyclic_run();
>   }
>   
> -int cyclic_uninit(void)
> +int cyclic_unregister_all(void)
>   {
>   	struct cyclic_info *cyclic;
>   	struct hlist_node *tmp;
>   
> -	hlist_for_each_entry_safe(cyclic, tmp, &gd->cyclic->cyclic_list, list)
> +	hlist_for_each_entry_safe(cyclic, tmp, cyclic_get_list(), list)
>   		cyclic_unregister(cyclic);
>   
>   	return 0;
>   }
> -
> -int cyclic_init(void)
> -{
> -	int size = sizeof(struct cyclic_drv);
> -
> -	gd->cyclic = (struct cyclic_drv *)malloc(size);
> -	if (!gd->cyclic)
> -		return -ENOMEM;
> -
> -	memset(gd->cyclic, '\0', size);
> -	INIT_HLIST_HEAD(&gd->cyclic->cyclic_list);
> -
> -	return 0;
> -}
> diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
> index ca36907b20..1a2e55454d 100644
> --- a/include/asm-generic/global_data.h
> +++ b/include/asm-generic/global_data.h
> @@ -481,9 +481,9 @@ struct global_data {
>   #endif
>   #ifdef CONFIG_CYCLIC
>   	/**
> -	 * @cyclic: cyclic driver data
> +	 * @cyclic_list: list of registered cyclic functions
>   	 */
> -	struct cyclic_drv *cyclic;
> +	struct hlist_head cyclic_list;
>   #endif
>   	/**
>   	 * @dmtag_list: List of DM tags
> diff --git a/include/cyclic.h b/include/cyclic.h
> index 4a11f9b105..44ad3cb6b8 100644
> --- a/include/cyclic.h
> +++ b/include/cyclic.h
> @@ -14,15 +14,6 @@
>   #include <linux/list.h>
>   #include <asm/types.h>
>   
> -/**
> - * struct cyclic_drv - Cyclic driver internal data
> - *
> - * @cyclic_list: Cylic list node
> - */
> -struct cyclic_drv {
> -	struct hlist_head cyclic_list;
> -};
> -
>   /**
>    * struct cyclic_info - Information about cyclic execution function
>    *
> @@ -75,18 +66,11 @@ struct cyclic_info *cyclic_register(cyclic_func_t func, uint64_t delay_us,
>   int cyclic_unregister(struct cyclic_info *cyclic);
>   
>   /**
> - * cyclic_init() - Set up cyclic functions
> - *
> - * Init a list of cyclic functions, so that these can be added as needed
> - */
> -int cyclic_init(void);
> -
> -/**
> - * cyclic_uninit() - Clean up cyclic functions
> + * cyclic_unregister_all() - Clean up cyclic functions
>    *
>    * This removes all cyclic functions
>    */
> -int cyclic_uninit(void);
> +int cyclic_unregister_all(void);
>   
>   /**
>    * cyclic_get_list() - Get cyclic list pointer
> @@ -134,12 +118,7 @@ static inline void schedule(void)
>   {
>   }
>   
> -static inline int cyclic_init(void)
> -{
> -	return 0;
> -}
> -
> -static inline int cyclic_uninit(void)
> +static inline int cyclic_unregister_all(void)
>   {
>   	return 0;
>   }
> diff --git a/test/test-main.c b/test/test-main.c
> index a98a77d68f..554dd8d96b 100644
> --- a/test/test-main.c
> +++ b/test/test-main.c
> @@ -287,7 +287,6 @@ static int dm_test_restore(struct device_node *of_root)
>   static int test_pre_run(struct unit_test_state *uts, struct unit_test *test)
>   {
>   	ut_assertok(event_init());
> -	ut_assertok(cyclic_init());
>   
>   	if (test->flags & UT_TESTF_DM)
>   		ut_assertok(dm_test_pre_run(uts));
> @@ -347,7 +346,7 @@ static int test_post_run(struct unit_test_state *uts, struct unit_test *test)
>   	ut_unsilence_console(uts);
>   	if (test->flags & UT_TESTF_DM)
>   		ut_assertok(dm_test_post_run(uts));
> -	ut_assertok(cyclic_uninit());
> +	ut_assertok(cyclic_unregister_all());
>   	ut_assertok(event_uninit());
>   
>   	free(uts->of_other);

Reviewed-by: Stefan Roese <sr@denx.de>
Tested-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

Re: [PATCH 0/5] cyclic: get rid of (the need for) cyclic_init()

[Stefan Roese]

Hi Rasmus,

On 28.10.22 13:50, Rasmus Villemoes wrote:
> I have only compile-tested each of these for sandbox_defconfig and
> imx8mq_cm_defconfig. I couldn't even figure out how to run the cyclic
> test inside sandbox by itself, and I don't have any hardware here at
> home.

There is "cyclic demo" command which can be used to do some tests.
But as I just now noticed, this behaves a bit "abnormal" in sandbox.
This is with and w/o your patchset though. I need to look into this
a bit later.

> So perhaps just consider these a POC of the overall idea, namely
> to use a list abstraction which doesn't need initialization other than
> what we already guarantee to do for all of struct global_data.
> 
> As positive side effects, the generated code will be a little smaller,
> we reduce the use of the early malloc() pool, and the diffstat below
> is also nice.

Fully agreed. :)

> I don't know if we ever do anything in SPL that would require a call
> to cyclic_unregister_all().

I can't think of anything right now.

> Rasmus Villemoes (5):
>    cyclic: use a flag in gd->flags for recursion protection
>    cyclic: drop redundant cyclic_ready flag
>    list.h: synchronize hlist_for_each_entry* iterators with linux
>    cyclic: switch to using hlist instead of list
>    cyclic: get rid of cyclic_init()
> 
>   cmd/cyclic.c                      |  5 +--
>   common/board_f.c                  |  2 +-
>   common/board_r.c                  |  1 -
>   common/cyclic.c                   | 50 ++++++++++-------------------
>   include/asm-generic/global_data.h |  8 +++--
>   include/cyclic.h                  | 35 +++-----------------
>   include/linux/list.h              | 53 +++++++++++++++----------------
>   test/test-main.c                  |  3 +-
>   8 files changed, 57 insertions(+), 100 deletions(-)
> 

Yes, this diffstat is really nice. Thanks a lot for working on this and
improving and fixing this new cyclic infrastructure.

Thanks,
Stefan

Re: [PATCH 0/5] cyclic: get rid of (the need for) cyclic_init()

[Tim Harvey]

On Fri, Oct 28, 2022 at 4:51 AM Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> I have only compile-tested each of these for sandbox_defconfig and
> imx8mq_cm_defconfig. I couldn't even figure out how to run the cyclic
> test inside sandbox by itself, and I don't have any hardware here at
> home. So perhaps just consider these a POC of the overall idea, namely
> to use a list abstraction which doesn't need initialization other than
> what we already guarantee to do for all of struct global_data.
>
> As positive side effects, the generated code will be a little smaller,
> we reduce the use of the early malloc() pool, and the diffstat below
> is also nice.
>
> I don't know if we ever do anything in SPL that would require a call
> to cyclic_unregister_all().
>
> Rasmus Villemoes (5):
>   cyclic: use a flag in gd->flags for recursion protection
>   cyclic: drop redundant cyclic_ready flag
>   list.h: synchronize hlist_for_each_entry* iterators with linux
>   cyclic: switch to using hlist instead of list
>   cyclic: get rid of cyclic_init()
>
>  cmd/cyclic.c                      |  5 +--
>  common/board_f.c                  |  2 +-
>  common/board_r.c                  |  1 -
>  common/cyclic.c                   | 50 ++++++++++-------------------
>  include/asm-generic/global_data.h |  8 +++--
>  include/cyclic.h                  | 35 +++-----------------
>  include/linux/list.h              | 53 +++++++++++++++----------------
>  test/test-main.c                  |  3 +-
>  8 files changed, 57 insertions(+), 100 deletions(-)
>
> --
> 2.37.2

Rasmus,

Thanks, this does indeed resolve the SPL and U-Boot regressions
introduced. I verified the watchdig is initalized and cyclic wdt reset
is called in both SPL and U-Boot.

Tested-By: Tim Harvey <tharvey@gateworks.com> # imx8mm-venice-*

I'm not sure if a 'Fixes: 29caf9305b6f ("cyclic: Use schedule()
instead of WATCHDOG_RESET()")' is useful anywhere here.

Best Regards,

Tim

Re: [PATCH 5/5] cyclic: get rid of cyclic_init()

[Rasmus Villemoes]

On 28/10/2022 16.10, Stefan Roese wrote:
> On 28.10.22 13:50, Rasmus Villemoes wrote:

>> As for cyclic_uninit(), it was never really the opposite of
>> cyclic_init() since it didn't free the struct cyclic_drv nor set
>> gd->cyclic to NULL. Rename it to cyclic_unregister_all() and use that
>> in test/, and also insert a call at the end of the board_init_f
>> sequence so that gd->cyclic_list is a fresh empty list before we enter
>> board_init_r().
> 
> While reviewing the code, this was the only thing I wanted to
> ask about. Now with this explanation it makes perfect sense.
> Perhaps a small comment with this reasoning in the code here in
> board_init_r would be helpful.

Yeah, so I went back and forth on whether to put it early in
board_init_r or late in board_init_f, but went with the latter so that
whatever free() gets called goes with the same malloc() - i.e. to avoid
introducing any new ordering dependency against when we can initialize
the full malloc. Perhaps something like this above the
cyclic_unregister_all entry in board_init_f sequence:

/*
 * Deregister all cyclic functions before relocation, so that
gd->cyclic_list does not contain any references to pre-relocation
devices. Drivers will register their cyclic functions anew when the
devices are probed again.

This should happen as late as possible so that the window where a
watchdog device is not serviced is as small as possible.
*/

But I don't know if that's too verbose; many other important
initialization functions with implicit ordering dependencies do not have
anything similar. That's not necessarily an argument against starting to
add such comments.

> Reviewed-by: Stefan Roese <sr@denx.de>
> Tested-by: Stefan Roese <sr@denx.de>

Thanks,
Rasmus

Re: [PATCH 5/5] cyclic: get rid of cyclic_init()

[Stefan Roese]

On 29.10.22 00:38, Rasmus Villemoes wrote:
> On 28/10/2022 16.10, Stefan Roese wrote:
>> On 28.10.22 13:50, Rasmus Villemoes wrote:
> 
>>> As for cyclic_uninit(), it was never really the opposite of
>>> cyclic_init() since it didn't free the struct cyclic_drv nor set
>>> gd->cyclic to NULL. Rename it to cyclic_unregister_all() and use that
>>> in test/, and also insert a call at the end of the board_init_f
>>> sequence so that gd->cyclic_list is a fresh empty list before we enter
>>> board_init_r().
>>
>> While reviewing the code, this was the only thing I wanted to
>> ask about. Now with this explanation it makes perfect sense.
>> Perhaps a small comment with this reasoning in the code here in
>> board_init_r would be helpful.
> 
> Yeah, so I went back and forth on whether to put it early in
> board_init_r or late in board_init_f, but went with the latter so that
> whatever free() gets called goes with the same malloc() - i.e. to avoid
> introducing any new ordering dependency against when we can initialize
> the full malloc. Perhaps something like this above the
> cyclic_unregister_all entry in board_init_f sequence:
> 
> /*
>   * Deregister all cyclic functions before relocation, so that
> gd->cyclic_list does not contain any references to pre-relocation
> devices. Drivers will register their cyclic functions anew when the
> devices are probed again.
> 
> This should happen as late as possible so that the window where a
> watchdog device is not serviced is as small as possible.
> */
> 
> But I don't know if that's too verbose; many other important
> initialization functions with implicit ordering dependencies do not have
> anything similar. That's not necessarily an argument against starting to
> add such comments.

Thanks Rasmus. I've added this comment to the commit.

Thanks,
Stefan

Re: [PATCH 0/5] cyclic: get rid of (the need for) cyclic_init()

[Stefan Roese]

On 28.10.22 13:50, Rasmus Villemoes wrote:
> I have only compile-tested each of these for sandbox_defconfig and
> imx8mq_cm_defconfig. I couldn't even figure out how to run the cyclic
> test inside sandbox by itself, and I don't have any hardware here at
> home. So perhaps just consider these a POC of the overall idea, namely
> to use a list abstraction which doesn't need initialization other than
> what we already guarantee to do for all of struct global_data.
> 
> As positive side effects, the generated code will be a little smaller,
> we reduce the use of the early malloc() pool, and the diffstat below
> is also nice.
> 
> I don't know if we ever do anything in SPL that would require a call
> to cyclic_unregister_all().
> 
> Rasmus Villemoes (5):
>    cyclic: use a flag in gd->flags for recursion protection
>    cyclic: drop redundant cyclic_ready flag
>    list.h: synchronize hlist_for_each_entry* iterators with linux
>    cyclic: switch to using hlist instead of list
>    cyclic: get rid of cyclic_init()
> 
>   cmd/cyclic.c                      |  5 +--
>   common/board_f.c                  |  2 +-
>   common/board_r.c                  |  1 -
>   common/cyclic.c                   | 50 ++++++++++-------------------
>   include/asm-generic/global_data.h |  8 +++--
>   include/cyclic.h                  | 35 +++-----------------
>   include/linux/list.h              | 53 +++++++++++++++----------------
>   test/test-main.c                  |  3 +-
>   8 files changed, 57 insertions(+), 100 deletions(-)
> 

Applied to u-boot-watchdog/master

Thanks,
Stefan