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

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@m... 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@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@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