On 1/20/25 10:17 AM, Rasmus Villemoes wrote:
On Sat, Jan 18 2025, Marek Vasut <[email protected]> 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.
True
And if the caller doesn't
know whether the cyclic_info is already registered or not, he can't do a
memset() of it.
This is what can happen right now, which is dangerous and what this
series attempts to address.
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.
That I can do, but it will be a bit slower.
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.
This would be terribly overloaded interface, no, let's not do that.
Better introduce a dedicated function for that kind of period adjustment.
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;
I don't think this works, because that struct cyclic_info contains
.next_call member, which is updated over time, so this exact match would
not work as-is. I have something like this now:
diff --git a/common/cyclic.c b/common/cyclic.c
index 807a3d73f67..d721a21a575 100644
--- a/common/cyclic.c
+++ b/common/cyclic.c
@@ -27,11 +27,29 @@ struct hlist_head *cyclic_get_list(void)
return (struct hlist_head *)&gd->cyclic_list;
}
+static int cyclic_already_registered(struct cyclic_info *cyclic)
+{
+ struct cyclic_info *cycliclst;
+ struct hlist_node *tmp;
+
+ /* Reassignment of function would corrupt cyclic list, exit */
+ hlist_for_each_entry_safe(cycliclst, tmp, cyclic_get_list(), list) {
+ if (cycliclst->func == cyclic->func &&
+ cycliclst->name == cyclic->name && // or strcmp() ?
+ cycliclst->delay_us == cyclic->delay_us &&
+ cycliclst->start_time_us == cyclic->start_time_us)
+ return -EALREADY; /* Match found */
+ }
+
+ /* Match not found */
+ return 0;
+}
+
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)
+ if (!cyclic_already_registered(cyclic))
return -EALREADY;
memset(cyclic, 0, sizeof(*cyclic));
@@ -49,7 +67,7 @@ int cyclic_register(struct cyclic_info *cyclic,
cyclic_func_t func,
void cyclic_unregister(struct cyclic_info *cyclic)
{
/* Unregister only initialized struct cyclic_info */
- if (cyclic->func)
+ if (cyclic_already_registered(cyclic))
hlist_del(&cyclic->list);
}
[...]
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.
I would expect the caller should clear the structure before attempting
to register it again. Shall we actually memset() the structure in
cyclic_unregister() too ?
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).
