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+rene...@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
---
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
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
#include
#include
+#include
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(>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 *)>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
[PATCH 3/3] cyclic: make clients embed a struct cyclic_info in their own data structure
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+rene...@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
---
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(-)
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
#include
#include
+#include
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(>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 *)>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,
