[PATCH 3/3] cyclic: make clients embed a struct cyclic_info in their own data structure

Stefan Roese sr at denx.de
Thu May 16 08:43:27 CEST 2024


Hi Rasmus,

On 5/9/24 02:47, Rasmus Villemoes wrote:
> There are of course not a whole lot of examples in-tree yet, but
> before they appear, let's make this API change: Instead of separately
> allocating a 'struct cyclic_info', make the users embed such an
> instance in their own structure, and make the convention that the
> callback simply receives the 'struct cyclic_info *', from which the
> clients can get their own data using the container_of() macro.
> 
> This has a number of advantages.
> 
> First, it means cyclic_register() simply cannot fail, simplifying the
> code. The necessary storage will simply be allocated automatically
> when the client's own structure is allocated (often via
> uclass_priv_auto or similar).
> 
> Second, code for which CONFIG_CYCLIC is just an option can more easily
> be written without #ifdefs, if we just provide an empty struct
> cyclic_info {}. For example, the nested CONFIG_IS_ENABLED()s in
> https://lore.kernel.org/u-boot/20240316201416.211480-1-marek.vasut+renesas@mailbox.org/
> are mostly due to the existence of the 'struct cyclic_info *' member
> being guarded by #ifdef CONFIG_CYCLIC.
> 
> And we do probably want to avoid the extra memory overhead of that
> member when !CONFIG_CYCLIC. But that is automatic if, instead of a
> 'struct cyclic_info *', one simply embeds a 'struct cyclic_info',
> which will have size 0 when !CONFIG_CYCLIC. Also, the no-op
> cyclic_register() function can just unconditionally be called, and the
> compiler will see that (1) the callback is referenced, so not emit a
> warning for a maybe-unused function and (2) see that it can actually
> never be reached, so not emit any code for it.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes at prevas.dk>
> ---
>   board/Marvell/octeon_nic23/board.c |  9 +++++---
>   cmd/cyclic.c                       | 12 +++++------
>   common/cyclic.c                    | 22 +++++--------------
>   doc/develop/cyclic.rst             | 26 ++++++++++++++---------
>   drivers/watchdog/wdt-uclass.c      | 33 +++++++++++++----------------
>   include/cyclic.h                   | 34 +++++++++++++++---------------
>   6 files changed, 64 insertions(+), 72 deletions(-)

I like this approach. And would like to pull this in as well, perhaps
before leaving on a 2 week vacation mid of next week. Unfortunately
this patch 3/3 does not apply on TOT any more. Could you please rebase
and re-submit?

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

Thanks,
Stefan

> diff --git a/board/Marvell/octeon_nic23/board.c b/board/Marvell/octeon_nic23/board.c
> index bc9332cb74a..74b9c741b7b 100644
> --- a/board/Marvell/octeon_nic23/board.c
> +++ b/board/Marvell/octeon_nic23/board.c
> @@ -357,10 +357,13 @@ int board_late_init(void)
>   	board_configure_qlms();
>   
>   	/* Register cyclic function for PCIe FLR fixup */
> -	cyclic = cyclic_register(octeon_board_restore_pf, 100,
> -				 "pcie_flr_fix", NULL);
> -	if (!cyclic)
> +	cyclic = calloc(1, sizeof(*cyclic));
> +	if (cyclic) {
> +		cyclic_register(cyclic, octeon_board_restore_pf, 100,
> +				"pcie_flr_fix");
> +	} else {
>   		printf("Registering of cyclic function failed\n");
> +	}
>   
>   	return 0;
>   }
> diff --git a/cmd/cyclic.c b/cmd/cyclic.c
> index ad7fc3b975e..315546515f0 100644
> --- a/cmd/cyclic.c
> +++ b/cmd/cyclic.c
> @@ -14,14 +14,16 @@
>   #include <div64.h>
>   #include <malloc.h>
>   #include <linux/delay.h>
> +#include <linux/kernel.h>
>   
>   struct cyclic_demo_info {
> +	struct cyclic_info cyclic;
>   	uint delay_us;
>   };
>   
> -static void cyclic_demo(void *ctx)
> +static void cyclic_demo(struct cyclic_info *c)
>   {
> -	struct cyclic_demo_info *info = ctx;
> +	struct cyclic_demo_info *info = container_of(c, struct cyclic_demo_info, cyclic);
>   
>   	/* Just a small dummy delay here */
>   	udelay(info->delay_us);
> @@ -31,7 +33,6 @@ static int do_cyclic_demo(struct cmd_tbl *cmdtp, int flag, int argc,
>   			  char *const argv[])
>   {
>   	struct cyclic_demo_info *info;
> -	struct cyclic_info *cyclic;
>   	uint time_ms;
>   
>   	if (argc < 3)
> @@ -47,10 +48,7 @@ static int do_cyclic_demo(struct cmd_tbl *cmdtp, int flag, int argc,
>   	info->delay_us = simple_strtoul(argv[2], NULL, 0);
>   
>   	/* Register demo cyclic function */
> -	cyclic = cyclic_register(cyclic_demo, time_ms * 1000, "cyclic_demo",
> -				 info);
> -	if (!cyclic)
> -		printf("Registering of cyclic_demo failed\n");
> +	cyclic_register(&info->cyclic, cyclic_demo, time_ms * 1000, "cyclic_demo");
>   
>   	printf("Registered function \"%s\" to be executed all %dms\n",
>   	       "cyclic_demo", time_ms);
> diff --git a/common/cyclic.c b/common/cyclic.c
> index c62e7fa7d19..ec38fad6775 100644
> --- a/common/cyclic.c
> +++ b/common/cyclic.c
> @@ -26,34 +26,22 @@ struct hlist_head *cyclic_get_list(void)
>   	return (struct hlist_head *)&gd->cyclic_list;
>   }
>   
> -struct cyclic_info *cyclic_register(cyclic_func_t func, uint64_t delay_us,
> -				    const char *name, void *ctx)
> +void cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func,
> +		     uint64_t delay_us, const char *name)
>   {
> -	struct cyclic_info *cyclic;
> -
> -	cyclic = calloc(1, sizeof(struct cyclic_info));
> -	if (!cyclic) {
> -		pr_debug("Memory allocation error\n");
> -		return NULL;
> -	}
> +	memset(cyclic, 0, sizeof(*cyclic));
>   
>   	/* Store values in struct */
>   	cyclic->func = func;
> -	cyclic->ctx = ctx;
>   	cyclic->name = name;
>   	cyclic->delay_us = delay_us;
>   	cyclic->start_time_us = timer_get_us();
>   	hlist_add_head(&cyclic->list, cyclic_get_list());
> -
> -	return cyclic;
>   }
>   
> -int cyclic_unregister(struct cyclic_info *cyclic)
> +void cyclic_unregister(struct cyclic_info *cyclic)
>   {
>   	hlist_del(&cyclic->list);
> -	free(cyclic);
> -
> -	return 0;
>   }
>   
>   void cyclic_run(void)
> @@ -76,7 +64,7 @@ void cyclic_run(void)
>   		if (time_after_eq64(now, cyclic->next_call)) {
>   			/* Call cyclic function and account it's cpu-time */
>   			cyclic->next_call = now + cyclic->delay_us;
> -			cyclic->func(cyclic->ctx);
> +			cyclic->func(cyclic);
>   			cyclic->run_cnt++;
>   			cpu_time = timer_get_us() - now;
>   			cyclic->cpu_time_us += cpu_time;
> diff --git a/doc/develop/cyclic.rst b/doc/develop/cyclic.rst
> index 67831496a70..bcc1ca6ce6d 100644
> --- a/doc/develop/cyclic.rst
> +++ b/doc/develop/cyclic.rst
> @@ -19,20 +19,26 @@ Registering a cyclic function
>   
>   To register a cyclic function, use something like this::
>   
> -    static void cyclic_demo(void *ctx)
> +    struct donkey {
> +        struct cyclic_info cyclic;
> +	void (*say)(const char *s);
> +    };
> +
> +    static void cyclic_demo(struct cyclic_info *c)
>       {
> -        /* Just a small dummy delay here */
> -        udelay(10);
> +        struct donkey *donkey = container_of(c, struct donkey, cyclic);
> +
> +	donkey->say("Are we there yet?");
>       }
> -
> -    int board_init(void)
> +
> +    int donkey_init(void)
>       {
> -        struct cyclic_info *cyclic;
> -
> +        struct donkey *donkey;
> +
> +        /* Initialize donkey ... */
> +
>           /* Register demo cyclic function */
> -        cyclic = cyclic_register(cyclic_demo, 10 * 1000, "cyclic_demo", NULL);
> -        if (!cyclic)
> -        printf("Registering of cyclic_demo failed\n");
> +        cyclic_register(&donkey->cyclic, cyclic_demo, 10 * 1000, "cyclic_demo");
>           
>           return 0;
>       }
> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
> index caa1567e0c3..c0332f39dfb 100644
> --- a/drivers/watchdog/wdt-uclass.c
> +++ b/drivers/watchdog/wdt-uclass.c
> @@ -18,12 +18,15 @@
>   #include <asm/global_data.h>
>   #include <dm/device-internal.h>
>   #include <dm/lists.h>
> +#include <linux/kernel.h>
>   
>   DECLARE_GLOBAL_DATA_PTR;
>   
>   #define WATCHDOG_TIMEOUT_SECS	(CONFIG_WATCHDOG_TIMEOUT_MSECS / 1000)
>   
>   struct wdt_priv {
> +	/* The udevice owning this wdt_priv. */
> +	struct udevice *dev;
>   	/* Timeout, in seconds, to configure this device to. */
>   	u32 timeout;
>   	/*
> @@ -41,18 +44,17 @@ struct wdt_priv {
>   	/* autostart */
>   	bool autostart;
>   
> -	struct cyclic_info *cyclic;
> +	struct cyclic_info cyclic;
>   };
>   
> -static void wdt_cyclic(void *ctx)
> +static void wdt_cyclic(struct cyclic_info *c)
>   {
> -	struct udevice *dev = ctx;
> -	struct wdt_priv *priv;
> +	struct wdt_priv *priv = container_of(c, struct wdt_priv, cyclic);
> +	struct udevice *dev = priv->dev;
>   
>   	if (!device_active(dev))
>   		return;
>   
> -	priv = dev_get_uclass_priv(dev);
>   	if (!priv->running)
>   		return;
>   
> @@ -125,20 +127,14 @@ int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags)
>   		memset(str, 0, 16);
>   		if (IS_ENABLED(CONFIG_WATCHDOG)) {
>   			if (priv->running)
> -				cyclic_unregister(priv->cyclic);
> +				cyclic_unregister(&priv->cyclic);
>   
>   			/* Register the watchdog driver as a cyclic function */
> -			priv->cyclic = cyclic_register(wdt_cyclic,
> -						       priv->reset_period * 1000,
> -						       dev->name, dev);
> -			if (!priv->cyclic) {
> -				printf("cyclic_register for %s failed\n",
> -				       dev->name);
> -				return -ENODEV;
> -			} else {
> -				snprintf(str, 16, "every %ldms",
> -					 priv->reset_period);
> -			}
> +			cyclic_register(&priv->cyclic, wdt_cyclic,
> +					priv->reset_period * 1000,
> +					dev->name);
> +
> +			snprintf(str, 16, "every %ldms", priv->reset_period);
>   		}
>   
>   		priv->running = true;
> @@ -163,7 +159,7 @@ int wdt_stop(struct udevice *dev)
>   		struct wdt_priv *priv = dev_get_uclass_priv(dev);
>   
>   		if (IS_ENABLED(CONFIG_WATCHDOG) && priv->running)
> -			cyclic_unregister(priv->cyclic);
> +			cyclic_unregister(&priv->cyclic);
>   
>   		priv->running = false;
>   	}
> @@ -263,6 +259,7 @@ static int wdt_pre_probe(struct udevice *dev)
>   			autostart = true;
>   	}
>   	priv = dev_get_uclass_priv(dev);
> +	priv->dev = dev;
>   	priv->timeout = timeout;
>   	priv->reset_period = reset_period;
>   	priv->autostart = autostart;
> diff --git a/include/cyclic.h b/include/cyclic.h
> index 38946216fb8..dc0749ba03d 100644
> --- a/include/cyclic.h
> +++ b/include/cyclic.h
> @@ -18,7 +18,6 @@
>    * struct cyclic_info - Information about cyclic execution function
>    *
>    * @func: Function to call periodically
> - * @ctx: Context pointer to get passed to this function
>    * @name: Name of the cyclic function, e.g. shown in the commands
>    * @delay_ns: Delay is ns after which this function shall get executed
>    * @start_time_us: Start time in us, when this function started its execution
> @@ -29,8 +28,7 @@
>    * @already_warned: Flag that we've warned about exceeding CPU time usage
>    */
>   struct cyclic_info {
> -	void (*func)(void *ctx);
> -	void *ctx;
> +	void (*func)(struct cyclic_info *c);
>   	const char *name;
>   	uint64_t delay_us;
>   	uint64_t start_time_us;
> @@ -42,28 +40,30 @@ struct cyclic_info {
>   };
>   
>   /** Function type for cyclic functions */
> -typedef void (*cyclic_func_t)(void *ctx);
> +typedef void (*cyclic_func_t)(struct cyclic_info *c);
>   
>   #if defined(CONFIG_CYCLIC)
>   /**
>    * cyclic_register - Register a new cyclic function
>    *
> + * @cyclic: Cyclic info structure
>    * @func: Function to call periodically
>    * @delay_us: Delay is us after which this function shall get executed
>    * @name: Cyclic function name/id
> - * @ctx: Context to pass to the function
> - * @return: pointer to cyclic_struct if OK, NULL on error
> + *
> + * The function @func will be called with @cyclic as its
> + * argument. @cyclic will usually be embedded in some device-specific
> + * structure, which the callback can retrieve using container_of().
>    */
> -struct cyclic_info *cyclic_register(cyclic_func_t func, uint64_t delay_us,
> -				    const char *name, void *ctx);
> +void cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func,
> +		     uint64_t delay_us, const char *name);
>   
>   /**
>    * cyclic_unregister - Unregister a cyclic function
>    *
>    * @cyclic: Pointer to cyclic_struct of the function that shall be removed
> - * @return: 0 if OK, -ve on error
>    */
> -int cyclic_unregister(struct cyclic_info *cyclic);
> +void cyclic_unregister(struct cyclic_info *cyclic);
>   
>   /**
>    * cyclic_unregister_all() - Clean up cyclic functions
> @@ -97,17 +97,17 @@ void cyclic_run(void);
>    */
>   void schedule(void);
>   #else
> -static inline struct cyclic_info *cyclic_register(cyclic_func_t func,
> -						  uint64_t delay_us,
> -						  const char *name,
> -						  void *ctx)
> +
> +struct cyclic_info {
> +};
> +
> +static inline void cyclic_register(struct cyclic_info *cyclic, cyclic_func_t func,
> +				   uint64_t delay_us, const char *name)
>   {
> -	return NULL;
>   }
>   
> -static inline int cyclic_unregister(struct cyclic_info *cyclic)
> +static inline void cyclic_unregister(struct cyclic_info *cyclic)
>   {
> -	return 0;
>   }
>   
>   static inline void cyclic_run(void)

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 at denx.de


More information about the U-Boot mailing list