On Sat, Jan 25 2025, Marek Vasut <marek.va...@mailbox.org> wrote:
> On 1/20/25 10:17 AM, Rasmus Villemoes wrote:
>> On Sat, Jan 18 2025, Marek Vasut <marek.vasut+rene...@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.
>
> 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.
>
Please. I tried to preempt those kinds of objections by pointing out
that we already traverse the list thousands of times per second, and I
highly doubt we'll ever have more than, say, 10 elements registered, so
we're adding at most that many extra traversals, of a ridiculously short
linked list.
>> 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.
I disagree.
> Better introduce a dedicated function for that kind of period adjustment.
>
>> Also, that avoids making the
>> interfaces fallible.
And I believe that _this_ is an important property to get/preserve.
>> 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.
Huh? Of course it will. I'm not looking at that next_call member at all
(that's not a pointer, that's just some future time stamp), I'm
merely comparing the passed-in pointer to the elements already on the
list.
Yeah, my pseudo-code should probably have used hlist_for_each_entry()
rather than hlist_for_each(), but that should be obvious from the type I
used for the iterator variable.
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 */
> + }
> +
Please no. Just compare the pointers; if they're the same, it's
literally the same struct cyclic_info; if not, it's not. Why would you
dereference the maybe-to-be-registered struct cyclic_info when you don't
expect that to be in any kind of properly initialized state?
> + /* 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 ?
>
This is what I tried to point out here:
>> 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).
So, let's look at the mmc example. In mmc_deinit(), we do
unregister. OK. Later, we might do mmc_init(). Now mmc_init() tries to
figure out if its cyclic_info instance is already registered or
not. There, mmc_init cannot possibly just zero-init the cyclic_info
before the register, because it might be registered, and it can't not do
the zeroing, because then the new registration might spuriously fail.
So for that to work, the code that calls unregister and thus knows that
it has been unregistered would have to immediately do the zero'ing. At
which point it would much better if cyclic_unregister() did that
zero'ing, instead of all callers having to remember that memset().
Rasmus
