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

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(-)

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; };
/**

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

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; };
/**

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

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

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

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

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

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);

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

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

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

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

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

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
participants (3)
-
Rasmus Villemoes
-
Stefan Roese
-
Tim Harvey