Re: [PATCH v2 3/3] cyclic: make clients embed a struct cyclic_info in their own data structure
On 5/21/24 10:38, Rasmus Villemoes wrote: On 21/05/2024 08.57, Stefan Roese wrote: On 5/19/24 21:44, Rasmus Villemoes wrote: On 18/05/2024 09.34, Stefan Roese wrote: This introduces some problems when compiling e.g. sandbox: In file included from test/common/cyclic.c:10: test/common/cyclic.c: In function ‘dm_test_cyclic_running’: test/common/cyclic.c:25:42: warning: passing argument 1 of ‘cyclic_register’ from incompatible pointer type [-Wincompatible-pointer-types] 25 | ut_assertnonnull(cyclic_register(cyclic_test, 10 * 1000, "cyclic_demo", | ^~~ | | | void (*)(void *) include/test/ut.h:298:29: note: in definition of macro ‘ut_assertnonnull’ [...] Could you please also change the test file accordingly? I'll then try to get this upstream shortly. Whoops, I don't know how I managed to miss that when grepping for users. Sorry about that. Updated version coming shortly. Thanks. Still the new version also fails in the CI build. I'm using MS Azure for this, here the link to the failing build: https://dev.azure.com/sr0718/u-boot/_build/results?buildId=355=results Apparently I'm blind, the full definition of 'struct cyclic_info' was not guarded by CONFIG_CYCLIC. Yes, I already "played" a bit here but still got other problems. Could you please make sure that CI fully builds? Is there a way I can trigger that from my side without sending patches? You need to have an azure account and push a branch with your patches into your u-boot repo to trigger the CI build. Gitlab also is possible AFAIK. BTW: Not sure if I still can pull this (updated version) in, since I'm leaving for a 2 week vacation tomorrow morning. NP, I should have been more careful. I'll send an updated version in a moment anyway. Thanks, Stefan
Re: [PATCH v2 3/3] cyclic: make clients embed a struct cyclic_info in their own data structure
On 21/05/2024 08.57, Stefan Roese wrote: > On 5/19/24 21:44, Rasmus Villemoes wrote: >> On 18/05/2024 09.34, Stefan Roese wrote: >> >>> This introduces some problems when compiling e.g. sandbox: >>> >>> In file included from test/common/cyclic.c:10: >>> test/common/cyclic.c: In function ‘dm_test_cyclic_running’: >>> test/common/cyclic.c:25:42: warning: passing argument 1 of >>> ‘cyclic_register’ from incompatible pointer type >>> [-Wincompatible-pointer-types] >>> 25 | ut_assertnonnull(cyclic_register(cyclic_test, 10 * >>> 1000, >>> "cyclic_demo", >>> | ^~~ >>> | | >>> | void (*)(void *) >>> include/test/ut.h:298:29: note: in definition of macro >>> ‘ut_assertnonnull’ >> [...] >>> >>> Could you please also change the test file accordingly? I'll then >>> try to get this upstream shortly. >> >> Whoops, I don't know how I managed to miss that when grepping for users. >> Sorry about that. Updated version coming shortly. > > Thanks. Still the new version also fails in the CI build. I'm using > MS Azure for this, here the link to the failing build: > > https://dev.azure.com/sr0718/u-boot/_build/results?buildId=355=results Apparently I'm blind, the full definition of 'struct cyclic_info' was not guarded by CONFIG_CYCLIC. > Could you please make sure that CI fully builds? Is there a way I can trigger that from my side without sending patches? > BTW: Not sure if I still can pull this (updated version) in, since I'm > leaving for a 2 week vacation tomorrow morning. NP, I should have been more careful. I'll send an updated version in a moment anyway. Rasmus
Re: [PATCH v2 3/3] cyclic: make clients embed a struct cyclic_info in their own data structure
On 5/19/24 21:44, Rasmus Villemoes wrote: On 18/05/2024 09.34, Stefan Roese wrote: This introduces some problems when compiling e.g. sandbox: In file included from test/common/cyclic.c:10: test/common/cyclic.c: In function ‘dm_test_cyclic_running’: test/common/cyclic.c:25:42: warning: passing argument 1 of ‘cyclic_register’ from incompatible pointer type [-Wincompatible-pointer-types] 25 | ut_assertnonnull(cyclic_register(cyclic_test, 10 * 1000, "cyclic_demo", | ^~~ | | | void (*)(void *) include/test/ut.h:298:29: note: in definition of macro ‘ut_assertnonnull’ [...] Could you please also change the test file accordingly? I'll then try to get this upstream shortly. Whoops, I don't know how I managed to miss that when grepping for users. Sorry about that. Updated version coming shortly. Thanks. Still the new version also fails in the CI build. I'm using MS Azure for this, here the link to the failing build: https://dev.azure.com/sr0718/u-boot/_build/results?buildId=355=results Could you please make sure that CI fully builds? BTW: Not sure if I still can pull this (updated version) in, since I'm leaving for a 2 week vacation tomorrow morning. Thanks, Stefan
Re: [PATCH v2 3/3] cyclic: make clients embed a struct cyclic_info in their own data structure
On 18/05/2024 09.34, Stefan Roese wrote: > This introduces some problems when compiling e.g. sandbox: > > In file included from test/common/cyclic.c:10: > test/common/cyclic.c: In function ‘dm_test_cyclic_running’: > test/common/cyclic.c:25:42: warning: passing argument 1 of > ‘cyclic_register’ from incompatible pointer type > [-Wincompatible-pointer-types] > 25 | ut_assertnonnull(cyclic_register(cyclic_test, 10 * 1000, > "cyclic_demo", > | ^~~ > | | > | void (*)(void *) > include/test/ut.h:298:29: note: in definition of macro ‘ut_assertnonnull’ [...] > > Could you please also change the test file accordingly? I'll then > try to get this upstream shortly. Whoops, I don't know how I managed to miss that when grepping for users. Sorry about that. Updated version coming shortly. Rasmus
Re: [PATCH v2 3/3] cyclic: make clients embed a struct cyclic_info in their own data structure
Hi Rasmus,
On 5/16/24 09:53, 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.
Reviewed-by: Stefan Roese
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(-)
This introduces some problems when compiling e.g. sandbox:
In file included from test/common/cyclic.c:10:
test/common/cyclic.c: In function ‘dm_test_cyclic_running’:
test/common/cyclic.c:25:42: warning: passing argument 1 of
‘cyclic_register’ from incompatible pointer type
[-Wincompatible-pointer-types]
25 | ut_assertnonnull(cyclic_register(cyclic_test, 10 *
1000, "cyclic_demo",
| ^~~
| |
| void (*)(void *)
include/test/ut.h:298:29: note: in definition of macro ‘ut_assertnonnull’
298 | const void *_val = (expr);
\
| ^~~~
In file included from test/common/cyclic.c:6:
include/cyclic.h:58:42: note: expected ‘struct cyclic_info *’ but
argument is of type ‘void (*)(void *)’
58 | void cyclic_register(struct cyclic_info *cyclic, cyclic_func_t
func,
| ^~
test/common/cyclic.c:25:55: warning: passing argument 2 of
‘cyclic_register’ makes pointer from integer without a cast
[-Wint-conversion]
25 | ut_assertnonnull(cyclic_register(cyclic_test, 10 *
1000, "cyclic_demo",
| ^
| |
| int
Could you please also change the test file accordingly? I'll then
try to get this upstream shortly.
Many thanks in advance,
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 40e966de9aa..339dd4a7bce 100644
--- a/cmd/cyclic.c
+++ b/cmd/cyclic.c
@@ -15,14 +15,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
