From d7de5ef629352fe12ad99b6539ba1480b923f31e Mon Sep 17 00:00:00 2001 From: Rasmus Villemoes Date: Fri, 28 Oct 2022 13:50:50 +0200 Subject: cyclic: use a flag in gd->flags for recursion protection 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 Reviewed-by: Stefan Roese Tested-by: Stefan Roese Tested-by: Tim Harvey # imx8mm-venice-* --- common/cyclic.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'common') diff --git a/common/cyclic.c b/common/cyclic.c index 7abb82c16a9..ff75c8cadb9 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) -- cgit v1.2.3 From 6b84b1db2d69609a783ab2fd6990c9e72903d367 Mon Sep 17 00:00:00 2001 From: Rasmus Villemoes Date: Fri, 28 Oct 2022 13:50:51 +0200 Subject: cyclic: drop redundant cyclic_ready flag 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 Reviewed-by: Stefan Roese Tested-by: Stefan Roese Tested-by: Tim Harvey # imx8mm-venice-* --- common/cyclic.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) (limited to 'common') diff --git a/common/cyclic.c b/common/cyclic.c index ff75c8cadb9..d6f11b002ed 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; } -- cgit v1.2.3 From 28968394839bec37dacf6ffc2ae880e38756e917 Mon Sep 17 00:00:00 2001 From: Rasmus Villemoes Date: Fri, 28 Oct 2022 13:50:53 +0200 Subject: cyclic: switch to using hlist instead of list 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 Reviewed-by: Stefan Roese Tested-by: Stefan Roese Tested-by: Tim Harvey # imx8mm-venice-* --- common/cyclic.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) (limited to 'common') diff --git a/common/cyclic.c b/common/cyclic.c index d6f11b002ed..32d9bf0d02b 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; } -- cgit v1.2.3 From 50128aeb0f8bb5a2d820e4c7a6ac0bb745809fc1 Mon Sep 17 00:00:00 2001 From: Rasmus Villemoes Date: Fri, 28 Oct 2022 13:50:54 +0200 Subject: cyclic: get rid of cyclic_init() 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 Reviewed-by: Stefan Roese Tested-by: Stefan Roese Tested-by: Tim Harvey # imx8mm-venice-* --- common/board_f.c | 11 ++++++++++- common/board_r.c | 1 - common/cyclic.c | 32 +++++++------------------------- 3 files changed, 17 insertions(+), 27 deletions(-) (limited to 'common') diff --git a/common/board_f.c b/common/board_f.c index faa096479b3..7c26e235fc0 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -839,7 +839,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, @@ -957,6 +956,16 @@ static const init_fnc_t init_sequence_f[] = { do_elf_reloc_fixups, #endif clear_bss, + /* + * 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. + */ + 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 828ad448668..db9cfb33ad2 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -617,7 +617,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 32d9bf0d02b..a49bfc88f5c 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; -} -- cgit v1.2.3