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

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