[PATCH v4 3/3] cyclic: make clients embed a struct cyclic_info in their own data structure
Rasmus Villemoes
rasmus.villemoes at prevas.dk
Tue May 21 10:46:52 CEST 2024
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.
Reviewed-by: Stefan Roese <sr at denx.de>
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 | 35 +++++++++++++++---------------
test/common/cyclic.c | 19 ++++++++++------
7 files changed, 77 insertions(+), 79 deletions(-)
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 40e966de9aa..339dd4a7bce 100644
--- a/cmd/cyclic.c
+++ b/cmd/cyclic.c
@@ -15,14 +15,16 @@
#include <time.h>
#include <vsprintf.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);
@@ -32,7 +34,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)
@@ -48,10 +49,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..893c269099a 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 12850016c93..e2e7f9ab84b 100644
--- a/drivers/watchdog/wdt-uclass.c
+++ b/drivers/watchdog/wdt-uclass.c
@@ -17,12 +17,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;
/*
@@ -40,18 +43,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;
@@ -124,20 +126,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;
@@ -162,7 +158,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;
}
@@ -262,6 +258,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..2c3d383c5ef 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
@@ -27,10 +26,12 @@
* @next_call: Next time in us, when the function shall be executed again
* @list: List node
* @already_warned: Flag that we've warned about exceeding CPU time usage
+ *
+ * When !CONFIG_CYCLIC, this struct is empty.
*/
struct cyclic_info {
- void (*func)(void *ctx);
- void *ctx;
+#if defined(CONFIG_CYCLIC)
+ void (*func)(struct cyclic_info *c);
const char *name;
uint64_t delay_us;
uint64_t start_time_us;
@@ -39,31 +40,34 @@ struct cyclic_info {
uint64_t next_call;
struct hlist_node list;
bool already_warned;
+#endif
};
/** 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 +101,14 @@ 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)
+
+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)
diff --git a/test/common/cyclic.c b/test/common/cyclic.c
index 461f8cf91f4..51a07c576b6 100644
--- a/test/common/cyclic.c
+++ b/test/common/cyclic.c
@@ -12,22 +12,27 @@
#include <linux/delay.h>
/* Test that cyclic function is called */
-static bool cyclic_active = false;
+static struct cyclic_test {
+ struct cyclic_info cyclic;
+ bool called;
+} cyclic_test;
-static void cyclic_test(void *ctx)
+static void test_cb(struct cyclic_info *c)
{
- cyclic_active = true;
+ struct cyclic_test *t = container_of(c, struct cyclic_test, cyclic);
+ t->called = true;
}
static int dm_test_cyclic_running(struct unit_test_state *uts)
{
- cyclic_active = false;
- ut_assertnonnull(cyclic_register(cyclic_test, 10 * 1000, "cyclic_demo",
- NULL));
+ cyclic_test.called = false;
+ cyclic_register(&cyclic_test.cyclic, test_cb, 10 * 1000, "cyclic_test");
/* Execute all registered cyclic functions */
schedule();
- ut_asserteq(true, cyclic_active);
+ ut_asserteq(true, cyclic_test.called);
+
+ cyclic_unregister(&cyclic_test.cyclic);
return 0;
}
--
2.40.1.1.g1c60b9335d
More information about the U-Boot
mailing list