[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 at 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 at denx.de>
Tested-by: Stefan Roese <sr at denx.de>

Thanks,
Stefan