[PATCH 0/3] make cyclic_(un)register idempotent

[PATCH 0/3] make cyclic_(un)register idempotent

[Rasmus Villemoes]

Three small patches I've been meaning to send for a while; I really
believe (as I now learnt Thomas Gleixner also does) this is the way
timer frameworks should behave.

Rasmus Villemoes (3):
  cyclic: make cyclic_unregister() idempotent
  cyclic: make cyclic_register safe to call on already-registered info
  cyclic: document new guarantees for cyclic_(un)register

 common/cyclic.c        | 17 +++++++++++++++++
 doc/develop/cyclic.rst | 13 +++++++++++++
 2 files changed, 30 insertions(+)

-- 
2.49.0

[PATCH 1/3] cyclic: make cyclic_unregister() idempotent

[Rasmus Villemoes]

Make cyclic_unregister() safe to call with an already unregistered, or
possibly never registered, struct cyclic_info. This is similar to how
the various timer APIs in the linux kernel work (they all allow
calling delete/cancel/... on an inactive timer object).

This means callers don't have to separately keep track of whether
their cyclic callback is registered or not, and avoids them trying to
peek into the struct cyclic_info for that information - which leads to
somewhat ugly code as it would have to be guarded by ifdef
CONFIG_CYCLIC.

Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
---
 common/cyclic.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/common/cyclic.c b/common/cyclic.c
index b695f092f52..75662d9f613 100644
--- a/common/cyclic.c
+++ b/common/cyclic.c
@@ -28,6 +28,18 @@ struct hlist_head *cyclic_get_list(void)
 	return (struct hlist_head *)&gd->cyclic_list;
 }
 
+static bool cyclic_is_registered(const struct cyclic_info *cyclic)
+{
+	const struct cyclic_info *c;
+
+	hlist_for_each_entry(c, cyclic_get_list(), list) {
+		if (c == cyclic)
+			return true;
+	}
+
+	return false;
+}
+
 void cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func,
 		     uint64_t delay_us, const char *name)
 {
@@ -43,6 +55,9 @@ void cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func,
 
 void cyclic_unregister(struct cyclic_info *cyclic)
 {
+	if (!cyclic_is_registered(cyclic))
+		return;
+
 	hlist_del(&cyclic->list);
 }
 
-- 
2.49.0

[PATCH 2/3] cyclic: make cyclic_register safe to call on already-registered info

[Rasmus Villemoes]

Now that cyclic_unregister() is safe to call on a not-registered
cyclic_info, we can make cyclic_register() behave like the mod_timer()
and hrtimer_start() APIs in linux, in that they don't distinguish
between whether the timer was already enabled or not; from the point
of the call it is, with whatever timeout/period is set in that most
recent call.

This avoids users of the cyclic API from separately keeping track of
whether their callback is already registered or not, and even if they
know it is, can be used for changing the period (and/or the callback
function) without first doing unregister().

See also this recent'ish message from kernel maintainer Thomas
Gleixner on that API design for timer frameworks:

  https://lore.kernel.org/lkml/87ikn6sibi.ffs@tglx/

  First of all the question is whether add() and mod() are really
  valuable distinctions. I'm not convinced at all. Back then, when we
  introduced hrtimers, we came to the conclusion that hrtimer_start()
  is sufficient.

Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
---
 common/cyclic.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/common/cyclic.c b/common/cyclic.c
index 75662d9f613..ec952a01ee1 100644
--- a/common/cyclic.c
+++ b/common/cyclic.c
@@ -43,6 +43,8 @@ static bool cyclic_is_registered(const struct cyclic_info *cyclic)
 void cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func,
 		     uint64_t delay_us, const char *name)
 {
+	cyclic_unregister(cyclic);
+
 	memset(cyclic, 0, sizeof(*cyclic));
 
 	/* Store values in struct */
-- 
2.49.0

[PATCH 3/3] cyclic: document new guarantees for cyclic_(un)register

[Rasmus Villemoes]

Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
---
 doc/develop/cyclic.rst | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/doc/develop/cyclic.rst b/doc/develop/cyclic.rst
index 6f1da6f0d9b..a99b17052f5 100644
--- a/doc/develop/cyclic.rst
+++ b/doc/develop/cyclic.rst
@@ -54,3 +54,16 @@ responsible for calling all registered cyclic functions, into the
 common schedule() function. This guarantees that cyclic_run() is
 executed very often, which is necessary for the cyclic functions to
 get scheduled and executed at their configured periods.
+
+Idempotence
+-----------
+
+Both the cyclic_register() and cyclic_unregister() functions are safe
+to call on any struct cyclic_info, regardless of whether that instance
+is already registered or not.
+
+More specifically, calling cyclic_unregister() with a cyclic_info
+which is not currently registered is a no-op, while calling
+cyclic_register() with a cyclic_info which is currently registered
+results in it being automatically unregistered, and then registered
+with the new callback function and timeout parameters.
-- 
2.49.0

Re: [PATCH 1/3] cyclic: make cyclic_unregister() idempotent

[Stefan Roese]

On 07.05.25 12:58, Rasmus Villemoes wrote:
> Make cyclic_unregister() safe to call with an already unregistered, or
> possibly never registered, struct cyclic_info. This is similar to how
> the various timer APIs in the linux kernel work (they all allow
> calling delete/cancel/... on an inactive timer object).
> 
> This means callers don't have to separately keep track of whether
> their cyclic callback is registered or not, and avoids them trying to
> peek into the struct cyclic_info for that information - which leads to
> somewhat ugly code as it would have to be guarded by ifdef
> CONFIG_CYCLIC.
> 
> Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   common/cyclic.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
> 
> diff --git a/common/cyclic.c b/common/cyclic.c
> index b695f092f52..75662d9f613 100644
> --- a/common/cyclic.c
> +++ b/common/cyclic.c
> @@ -28,6 +28,18 @@ struct hlist_head *cyclic_get_list(void)
>   	return (struct hlist_head *)&gd->cyclic_list;
>   }
>   
> +static bool cyclic_is_registered(const struct cyclic_info *cyclic)
> +{
> +	const struct cyclic_info *c;
> +
> +	hlist_for_each_entry(c, cyclic_get_list(), list) {
> +		if (c == cyclic)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
>   void cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func,
>   		     uint64_t delay_us, const char *name)
>   {
> @@ -43,6 +55,9 @@ void cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func,
>   
>   void cyclic_unregister(struct cyclic_info *cyclic)
>   {
> +	if (!cyclic_is_registered(cyclic))
> +		return;
> +
>   	hlist_del(&cyclic->list);
>   }
>   

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH 2/3] cyclic: make cyclic_register safe to call on already-registered info

[Stefan Roese]

On 07.05.25 12:58, Rasmus Villemoes wrote:
> Now that cyclic_unregister() is safe to call on a not-registered
> cyclic_info, we can make cyclic_register() behave like the mod_timer()
> and hrtimer_start() APIs in linux, in that they don't distinguish
> between whether the timer was already enabled or not; from the point
> of the call it is, with whatever timeout/period is set in that most
> recent call.
> 
> This avoids users of the cyclic API from separately keeping track of
> whether their callback is already registered or not, and even if they
> know it is, can be used for changing the period (and/or the callback
> function) without first doing unregister().
> 
> See also this recent'ish message from kernel maintainer Thomas
> Gleixner on that API design for timer frameworks:
> 
>    https://lore.kernel.org/lkml/87ikn6sibi.ffs@tglx/
> 
>    First of all the question is whether add() and mod() are really
>    valuable distinctions. I'm not convinced at all. Back then, when we
>    introduced hrtimers, we came to the conclusion that hrtimer_start()
>    is sufficient.
> 
> Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>

Many thanks for digging into this and improving this cyclic interface
over the time.

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   common/cyclic.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/common/cyclic.c b/common/cyclic.c
> index 75662d9f613..ec952a01ee1 100644
> --- a/common/cyclic.c
> +++ b/common/cyclic.c
> @@ -43,6 +43,8 @@ static bool cyclic_is_registered(const struct cyclic_info *cyclic)
>   void cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func,
>   		     uint64_t delay_us, const char *name)
>   {
> +	cyclic_unregister(cyclic);
> +
>   	memset(cyclic, 0, sizeof(*cyclic));
>   
>   	/* Store values in struct */

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH 3/3] cyclic: document new guarantees for cyclic_(un)register

[Stefan Roese]

On 07.05.25 12:58, Rasmus Villemoes wrote:
> Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   doc/develop/cyclic.rst | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/doc/develop/cyclic.rst b/doc/develop/cyclic.rst
> index 6f1da6f0d9b..a99b17052f5 100644
> --- a/doc/develop/cyclic.rst
> +++ b/doc/develop/cyclic.rst
> @@ -54,3 +54,16 @@ responsible for calling all registered cyclic functions, into the
>   common schedule() function. This guarantees that cyclic_run() is
>   executed very often, which is necessary for the cyclic functions to
>   get scheduled and executed at their configured periods.
> +
> +Idempotence
> +-----------
> +
> +Both the cyclic_register() and cyclic_unregister() functions are safe
> +to call on any struct cyclic_info, regardless of whether that instance
> +is already registered or not.
> +
> +More specifically, calling cyclic_unregister() with a cyclic_info
> +which is not currently registered is a no-op, while calling
> +cyclic_register() with a cyclic_info which is currently registered
> +results in it being automatically unregistered, and then registered
> +with the new callback function and timeout parameters.

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH 0/3] make cyclic_(un)register idempotent

[Stefan Roese]

On 07.05.25 12:58, Rasmus Villemoes wrote:
> Three small patches I've been meaning to send for a while; I really
> believe (as I now learnt Thomas Gleixner also does) this is the way
> timer frameworks should behave.
> 
> Rasmus Villemoes (3):
>    cyclic: make cyclic_unregister() idempotent
>    cyclic: make cyclic_register safe to call on already-registered info
>    cyclic: document new guarantees for cyclic_(un)register
> 
>   common/cyclic.c        | 17 +++++++++++++++++
>   doc/develop/cyclic.rst | 13 +++++++++++++
>   2 files changed, 30 insertions(+)
> 

Applied to u-boot-watchdog/master

Thanks,
Stefan