Hi Marek, Hi Rasmus,
On 20.01.25 10:17, Rasmus Villemoes wrote:
On Sat, Jan 18 2025, Marek Vasut marek.vasut+renesas@mailbox.org wrote:
Make cyclic_register() return error code, 0 in case of success, -EALREADY in case the called attempts to re-register already registered struct cyclic_info. The re-registration would lead to corruption of gd->cyclic_list because the re-registration would memset() one of its nodes, prevent that. Unregister only initialized struct cyclic_info.
I had considered something like this, but I don't like it, because it relies on the cyclic structure (or more likely whatever structure it is embedded in) being initially zero-initialized. And if the caller doesn't know whether the cyclic_info is already registered or not, he can't do a memset() of it.
So my preference would be that we instead simply iterate the current list to see if the struct cyclic_info is already registered that way. Also, I think I'd prefer if double cyclic_register() is allowed and always succeeds; this could be used to change the period of an already registered instance, for example. Also, that avoids making the interfaces fallible.
And cyclic_unregister() could similarly just check whether the passed pointer is already on the list, and be a no-op in case it's not. Those extra list traversals are not expensive (we're traversing them thousands of times per second anyway in cyclic_run), and I doubt one would ever has more than about 10 items on the list.
IOW, I'd suggest adding an internal
bool cyclic_is_registered(struct cyclic_info *info) { struct cyclic_info *c; hlist_for_each(...) if (c == info) return true; return false; }
add
if (!cyclic_is_registered(c)) return;
to cyclic_unregister(), and have cyclic_register() unconditionally start by a
cyclic_unregister(c);
and then proceed to initialize it as it does currently. No other changes, apart from documentation, would be needed.
common/cyclic.c | 14 +++++++++++--- include/cyclic.h | 9 ++++++--- 2 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/common/cyclic.c b/common/cyclic.c index 196797fd61e..53156a704cc 100644 --- a/common/cyclic.c +++ b/common/cyclic.c @@ -27,9 +27,13 @@ struct hlist_head *cyclic_get_list(void) return (struct hlist_head *)&gd->cyclic_list; }
-void cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func,
uint64_t delay_us, const char *name)+int cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func,
uint64_t delay_us, const char *name){
/* Reassignment of function would corrupt cyclic list, exit */
if (cyclic->func)
return -EALREADY;memset(cyclic, 0, sizeof(*cyclic));
/* Store values in struct */
@@ -38,11 +42,15 @@ void cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func, cyclic->delay_us = delay_us; cyclic->start_time_us = timer_get_us(); hlist_add_head(&cyclic->list, cyclic_get_list());
return 0; }
void cyclic_unregister(struct cyclic_info *cyclic) {
- hlist_del(&cyclic->list);
- /* Unregister only initialized struct cyclic_info */
- if (cyclic->func)
hlist_del(&cyclic->list);So this already shows how error prone this approach is. You are not clearing cyclic->func, so if the caller subsequently tries to register that struct again, he would get -EALREADY, while a subsequent unregister could would lead to exactly the list corruption you want to avoid.
And unless the caller immediately himself clears ->func, other code in the client cannot rely on ->func being NULL or not as a proxy for whether the struct is already registered (and the caller shouldn't do either of those things, as the struct cyclic_info should be considered opaque).
I like the approach suggest by Rasmus. Marek, do you see any flaws using this version? If not, please send an updated version.
Thanks, Stefan