[RFC PATCH] RFC: Sketch of dm event notification
This is a patch for Takahiro to look at, just for illustration. Not ready for review.
To run the test:
./u-boot -T -c "ut common test_event_probe"
Signed-off-by: Simon Glass sjg@chromium.org ---
common/Kconfig | 11 ++++ common/Makefile | 2 + common/board_f.c | 2 + common/event.c | 103 ++++++++++++++++++++++++++++++ common/log.c | 1 + drivers/core/device.c | 14 ++++ include/asm-generic/global_data.h | 3 + include/event.h | 103 ++++++++++++++++++++++++++++++ include/event_internal.h | 34 ++++++++++ include/log.h | 2 + test/common/Makefile | 1 + test/common/event.c | 85 ++++++++++++++++++++++++ test/test-main.c | 5 ++ 13 files changed, 366 insertions(+) create mode 100644 common/event.c create mode 100644 include/event.h create mode 100644 include/event_internal.h create mode 100644 test/common/event.c
diff --git a/common/Kconfig b/common/Kconfig index d6f77ab7b9c..54d0adaa8d5 100644 --- a/common/Kconfig +++ b/common/Kconfig @@ -484,6 +484,17 @@ config DISPLAY_BOARDINFO_LATE
menu "Start-up hooks"
+config EVENT + bool "General-purpose event-handling mechanism" + default y if SANDBOX + help + This enables sending and processing of events, to allow interested + parties to be alerted when something happens. This is an attempt to + step the flow of weak functions, hooks, functions in board_f.c + and board_r.c and the Kconfig options below. + + See doc/develop/event.rst for more information. + config ARCH_EARLY_INIT_R bool "Call arch-specific init soon after relocation" help diff --git a/common/Makefile b/common/Makefile index e7839027b6c..1b4601ac13f 100644 --- a/common/Makefile +++ b/common/Makefile @@ -101,6 +101,8 @@ obj-y += malloc_simple.o endif endif
+obj-$(CONFIG_EVENT) += event.o + obj-y += image.o image-board.o obj-$(CONFIG_$(SPL_TPL_)HASH) += hash.o obj-$(CONFIG_ANDROID_AB) += android_ab.o diff --git a/common/board_f.c b/common/board_f.c index 3dc0eaa59c5..faf262d564c 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -19,6 +19,7 @@ #include <dm.h> #include <env.h> #include <env_internal.h> +#include <event.h> #include <fdtdec.h> #include <fs.h> #include <hang.h> @@ -820,6 +821,7 @@ static const init_fnc_t init_sequence_f[] = { initf_malloc, log_init, initf_bootstage, /* uses its own timer, so does not need DM */ + event_init, #ifdef CONFIG_BLOBLIST bloblist_init, #endif diff --git a/common/event.c b/common/event.c new file mode 100644 index 00000000000..428628da44d --- /dev/null +++ b/common/event.c @@ -0,0 +1,103 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Events provide a general-purpose way to react to / subscribe to changes + * within U-Boot + * + * Copyright 2021 Google LLC + * Written by Simon Glass sjg@chromium.org + */ + +#define LOG_CATEGORY LOGC_EVENT + +#include <common.h> +#include <event.h> +#include <event_internal.h> +#include <log.h> +#include <malloc.h> +#include <asm/global_data.h> +#include <linux/list.h> + +DECLARE_GLOBAL_DATA_PTR; + +static void spy_free(struct event_spy *spy) +{ + list_del(&spy->sibling_node); +} + +int event_register(const char *id, enum event_t type, event_handler_t func, void *ctx) +{ + struct event_state *state = gd->event_state; + struct event_spy *spy; + + spy = malloc(sizeof(*spy)); + if (!spy) + return log_msg_ret("alloc", -ENOMEM); + + spy->id = id; + spy->type = type; + spy->func = func; + spy->ctx = ctx; + list_add_tail(&spy->sibling_node, &state->spy_head); + + return 0; +} + +int event_notify(enum event_t type, void *data, int size) +{ + struct event_state *state = gd->event_state; + struct event_spy *spy, *next; + struct event event; + + event.type = type; + if (size > sizeof(event.data)) + return log_msg_ret("size", -E2BIG); + memcpy(&event.data, data, size); + list_for_each_entry_safe(spy, next, &state->spy_head, sibling_node) { + if (spy->type == type) { + int ret; + + log_debug("Sending event %x to spy '%s'\n", type, + spy->id); + ret = spy->func(spy->ctx, &event); + + /* + * TODO: Handle various return codes to + * + * - claim an event (no others will see it) + * - return an error from the event + */ + if (ret) + return log_msg_ret("spy", ret); + } + } + + return 0; +} + +int event_uninit(void) +{ + struct event_state *state = gd->event_state; + struct event_spy *spy, *next; + + if (!state) + return 0; + list_for_each_entry_safe(spy, next, &state->spy_head, sibling_node) + spy_free(spy); + + return 0; +} + +int event_init(void) +{ + struct event_state *state; + + state = malloc(sizeof(struct event_state)); + if (!state) + return log_msg_ret("alloc", -ENOMEM); + + INIT_LIST_HEAD(&state->spy_head); + + gd->event_state = state; + + return 0; +} diff --git a/common/log.c b/common/log.c index 1aaa6c1527b..aa2e780d2d7 100644 --- a/common/log.c +++ b/common/log.c @@ -28,6 +28,7 @@ static const char *const log_cat_name[] = { "devres", "acpi", "boot", + "event", };
_Static_assert(ARRAY_SIZE(log_cat_name) == LOGC_COUNT - LOGC_NONE, diff --git a/drivers/core/device.c b/drivers/core/device.c index efd07176e37..c52b47d5cc1 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -10,6 +10,7 @@
#include <common.h> #include <cpu_func.h> +#include <event.h> #include <log.h> #include <asm/global_data.h> #include <asm/io.h> @@ -482,6 +483,11 @@ static int device_get_dma_constraints(struct udevice *dev) return 0; }
+static int device_notify(const struct udevice *dev, enum event_t type) +{ + return event_notify(type, &dev, sizeof(dev)); +} + int device_probe(struct udevice *dev) { const struct driver *drv; @@ -493,6 +499,10 @@ int device_probe(struct udevice *dev) if (dev_get_flags(dev) & DM_FLAG_ACTIVATED) return 0;
+ ret = device_notify(dev, EVT_DM_PRE_PROBE); + if (ret) + return ret; + drv = dev->driver; assert(drv);
@@ -589,6 +599,10 @@ int device_probe(struct udevice *dev) if (dev->parent && device_get_uclass_id(dev) == UCLASS_PINCTRL) pinctrl_select_state(dev, "default");
+ ret = device_notify(dev, EVT_DM_POST_PROBE); + if (ret) + return ret; + return 0; fail_uclass: if (device_remove(dev, DM_REMOVE_NORMAL)) { diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h index 16fd305a65c..771cda57a58 100644 --- a/include/asm-generic/global_data.h +++ b/include/asm-generic/global_data.h @@ -459,6 +459,9 @@ struct global_data { */ char *smbios_version; #endif +#if CONFIG_IS_ENABLED(EVENT) + struct event_state *event_state; +#endif }; #ifndef DO_DEPS_ONLY static_assert(sizeof(struct global_data) == GD_SIZE); diff --git a/include/event.h b/include/event.h new file mode 100644 index 00000000000..d44fd53639a --- /dev/null +++ b/include/event.h @@ -0,0 +1,103 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/* + * Events provide a general-purpose way to react to / subscribe to changes + * within U-Boot + * + * Copyright 2021 Google LLC + * Written by Simon Glass sjg@chromium.org + */ + +#ifndef __event_h +#define __event_h + +/** + * enum event_t - Types of events supported by U-Boot + * + * @EVT_DM_PRE_PROBE: Device is about to be probed + */ +enum event_t { + EVT_NONE, + EVT_TEST, + + /* Events related to driver model */ + EVT_DM_PRE_PROBE, + EVT_DM_POST_PROBE, + + EVT_COUNT +}; + +union event_data { + /** + * struct event_data_test - test data + * + * @signal: A value to update the state with + */ + struct event_data_test { + int signal; + } test; + + /** + * struct event_dm - driver model event + * + * @dev: Device this event relates to + */ + struct event_dm { + struct udevice *dev; + } dm; +}; + +/** + * struct event - an event that can be sent and received + * + * @type: Event type + * @data: Data for this particular event + */ +struct event { + enum event_t type; + union event_data data; +}; + +/** Function type for event handlers */ +typedef int (*event_handler_t)(void *ctx, struct event *event); + +/** + * event_register - register a new spy + * + * @id: Spy ID + * @type: Event type to subscribe to + * @func: Function to call when the event is sent + * @ctx: Context to pass to the function + * @return 0 if OK, -ve on erropr + */ +int event_register(const char *id, enum event_t type, event_handler_t func, + void *ctx); + +/** + * event_notify() - notify spies about an event + * + * It is possible to pass in union event_data here but that may not be + * convenient if the data is elsewhere, or is one of the members of the union. + * So this uses a void * for @data, with a separate @size. + * + * @type: Event type + * @data: Event data to be sent (e.g. union_event_data) + * @size: Size of data in bytes + */ +int event_notify(enum event_t type, void *data, int size); + +#if CONFIG_IS_ENABLED(EVENT) +int event_uninit(void); +int event_init(void); +#else +static int event_uninit(void) +{ + return 0; +} + +static inline int event_init(void) +{ + return 0; +} +#endif + +#endif diff --git a/include/event_internal.h b/include/event_internal.h new file mode 100644 index 00000000000..19308453f7b --- /dev/null +++ b/include/event_internal.h @@ -0,0 +1,34 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/* + * Internal definitions for events + * + * Copyright 2021 Google LLC + * Written by Simon Glass sjg@chromium.org + */ + +#ifndef __event_internal_h +#define __event_internal_h + +#include <linux/list.h> + +/** + * struct event_spy - a spy that watches for an event of a particular type + * + * @id: Spy ID + * @type: Event type to subscribe to + * @func: Function to call when the event is sent + * @ctx: Context to pass to the function + */ +struct event_spy { + struct list_head sibling_node; + const char *id; + enum event_t type; + event_handler_t func; + void *ctx; +}; + +struct event_state { + struct list_head spy_head; +}; + +#endif diff --git a/include/log.h b/include/log.h index e0e12ce1944..7169fff7aa5 100644 --- a/include/log.h +++ b/include/log.h @@ -98,6 +98,8 @@ enum log_category_t { LOGC_ACPI, /** @LOGC_BOOT: Related to boot process / boot image processing */ LOGC_BOOT, + /** @LOGC_EVENT: Related to event and event handling */ + LOGC_EVENT, /** @LOGC_COUNT: Number of log categories */ LOGC_COUNT, /** @LOGC_END: Sentinel value for lists of log categories */ diff --git a/test/common/Makefile b/test/common/Makefile index 24c9145dccc..9087788ba6a 100644 --- a/test/common/Makefile +++ b/test/common/Makefile @@ -1,3 +1,4 @@ # SPDX-License-Identifier: GPL-2.0+ obj-y += cmd_ut_common.o obj-$(CONFIG_AUTOBOOT) += test_autoboot.o +obj-$(CONFIG_EVENT) += event.o diff --git a/test/common/event.c b/test/common/event.c new file mode 100644 index 00000000000..6f359e2a933 --- /dev/null +++ b/test/common/event.c @@ -0,0 +1,85 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/* + * Unit tests for event handling + * + * Copyright 2021 Google LLC + * Written by Simon Glass sjg@chromium.org + */ + +#include <common.h> +#include <dm.h> +#include <event.h> +#include <test/common.h> +#include <test/test.h> +#include <test/ut.h> + +struct test_state { + struct udevice *dev; + int val; +}; + +static int h_adder(void *ctx, struct event *event) +{ + struct event_data_test *data = &event->data.test; + struct test_state *test_state = ctx; + + test_state->val += data->signal; + + return 0; +} + +static int test_event_base(struct unit_test_state *uts) +{ + struct test_state state; + int signal; + + state.val = 12; + ut_assertok(event_register("wibble", EVT_TEST, h_adder, &state)); + + signal = 17; + + /* Check that the handler is called */ + ut_assertok(event_notify(EVT_TEST, &signal, sizeof(signal))); + ut_asserteq(12 + 17, state.val); + + return 0; +} +COMMON_TEST(test_event_base, 0); + +static int h_probe(void *ctx, struct event *event) +{ + struct test_state *test_state = ctx; + + test_state->dev = event->data.dm.dev; + switch (event->type) { + case EVT_DM_PRE_PROBE: + test_state->val |= 1; + break; + case EVT_DM_POST_PROBE: + test_state->val |= 2; + break; + default: + break; + } + + return 0; +} + +static int test_event_probe(struct unit_test_state *uts) +{ + struct test_state state; + struct udevice *dev; + + state.val = 0; + ut_assertok(event_register("pre", EVT_DM_PRE_PROBE, h_probe, &state)); + ut_assertok(event_register("post", EVT_DM_POST_PROBE, h_probe, &state)); + + /* Probe a device */ + ut_assertok(uclass_first_device_err(UCLASS_TEST_FDT, &dev)); + + /* Check that the handler is called */ + ut_asserteq(3, state.val); + + return 0; +} +COMMON_TEST(test_event_probe, UT_TESTF_DM | UT_TESTF_SCAN_FDT); diff --git a/test/test-main.c b/test/test-main.c index 3cdf6849c57..c3ccdf3fed8 100644 --- a/test/test-main.c +++ b/test/test-main.c @@ -7,6 +7,7 @@ #include <common.h> #include <console.h> #include <dm.h> +#include <event.h> #include <dm/root.h> #include <dm/test.h> #include <dm/uclass-internal.h> @@ -218,6 +219,9 @@ static int dm_test_restore(struct device_node *of_root) */ static int test_pre_run(struct unit_test_state *uts, struct unit_test *test) { + gd->event_state = NULL; + ut_assertok(event_init()); + if (test->flags & UT_TESTF_DM) ut_assertok(dm_test_pre_run(uts));
@@ -260,6 +264,7 @@ static int test_post_run(struct unit_test_state *uts, struct unit_test *test) ut_unsilence_console(uts); if (test->flags & UT_TESTF_DM) ut_assertok(dm_test_post_run(uts)); + ut_assertok(event_uninit());
return 0; }
Hello Simon,
On Sun, Oct 31, 2021 at 09:07:01PM -0600, Simon Glass wrote:
This is a patch for Takahiro to look at, just for illustration. Not ready for review.
Thank you for posting the draft. At a first glance, it looks good and I don't see any specific issue with your implementation. Since my code has already added DM_TAG support, I'm looking forward for getting your final patch.
The only remaining issue is *modeling* partitions :)
-Takahiro Akashi
To run the test:
./u-boot -T -c "ut common test_event_probe"
Signed-off-by: Simon Glass sjg@chromium.org
common/Kconfig | 11 ++++ common/Makefile | 2 + common/board_f.c | 2 + common/event.c | 103 ++++++++++++++++++++++++++++++ common/log.c | 1 + drivers/core/device.c | 14 ++++ include/asm-generic/global_data.h | 3 + include/event.h | 103 ++++++++++++++++++++++++++++++ include/event_internal.h | 34 ++++++++++ include/log.h | 2 + test/common/Makefile | 1 + test/common/event.c | 85 ++++++++++++++++++++++++ test/test-main.c | 5 ++ 13 files changed, 366 insertions(+) create mode 100644 common/event.c create mode 100644 include/event.h create mode 100644 include/event_internal.h create mode 100644 test/common/event.c
diff --git a/common/Kconfig b/common/Kconfig index d6f77ab7b9c..54d0adaa8d5 100644 --- a/common/Kconfig +++ b/common/Kconfig @@ -484,6 +484,17 @@ config DISPLAY_BOARDINFO_LATE
menu "Start-up hooks"
+config EVENT
- bool "General-purpose event-handling mechanism"
- default y if SANDBOX
- help
This enables sending and processing of events, to allow interestedparties to be alerted when something happens. This is an attempt tostep the flow of weak functions, hooks, functions in board_f.cand board_r.c and the Kconfig options below.See doc/develop/event.rst for more information.config ARCH_EARLY_INIT_R bool "Call arch-specific init soon after relocation" help diff --git a/common/Makefile b/common/Makefile index e7839027b6c..1b4601ac13f 100644 --- a/common/Makefile +++ b/common/Makefile @@ -101,6 +101,8 @@ obj-y += malloc_simple.o endif endif
+obj-$(CONFIG_EVENT) += event.o
obj-y += image.o image-board.o obj-$(CONFIG_$(SPL_TPL_)HASH) += hash.o obj-$(CONFIG_ANDROID_AB) += android_ab.o diff --git a/common/board_f.c b/common/board_f.c index 3dc0eaa59c5..faf262d564c 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -19,6 +19,7 @@ #include <dm.h> #include <env.h> #include <env_internal.h> +#include <event.h> #include <fdtdec.h> #include <fs.h> #include <hang.h> @@ -820,6 +821,7 @@ static const init_fnc_t init_sequence_f[] = { initf_malloc, log_init, initf_bootstage, /* uses its own timer, so does not need DM */
- event_init,
#ifdef CONFIG_BLOBLIST bloblist_init, #endif diff --git a/common/event.c b/common/event.c new file mode 100644 index 00000000000..428628da44d --- /dev/null +++ b/common/event.c @@ -0,0 +1,103 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- Events provide a general-purpose way to react to / subscribe to changes
- within U-Boot
- Copyright 2021 Google LLC
- Written by Simon Glass sjg@chromium.org
- */
+#define LOG_CATEGORY LOGC_EVENT
+#include <common.h> +#include <event.h> +#include <event_internal.h> +#include <log.h> +#include <malloc.h> +#include <asm/global_data.h> +#include <linux/list.h>
+DECLARE_GLOBAL_DATA_PTR;
+static void spy_free(struct event_spy *spy) +{
- list_del(&spy->sibling_node);
+}
+int event_register(const char *id, enum event_t type, event_handler_t func, void *ctx) +{
- struct event_state *state = gd->event_state;
- struct event_spy *spy;
- spy = malloc(sizeof(*spy));
- if (!spy)
return log_msg_ret("alloc", -ENOMEM);- spy->id = id;
- spy->type = type;
- spy->func = func;
- spy->ctx = ctx;
- list_add_tail(&spy->sibling_node, &state->spy_head);
- return 0;
+}
+int event_notify(enum event_t type, void *data, int size) +{
- struct event_state *state = gd->event_state;
- struct event_spy *spy, *next;
- struct event event;
- event.type = type;
- if (size > sizeof(event.data))
return log_msg_ret("size", -E2BIG);- memcpy(&event.data, data, size);
- list_for_each_entry_safe(spy, next, &state->spy_head, sibling_node) {
if (spy->type == type) {int ret;log_debug("Sending event %x to spy '%s'\n", type,spy->id);ret = spy->func(spy->ctx, &event);/** TODO: Handle various return codes to** - claim an event (no others will see it)* - return an error from the event*/if (ret)return log_msg_ret("spy", ret);}- }
- return 0;
+}
+int event_uninit(void) +{
- struct event_state *state = gd->event_state;
- struct event_spy *spy, *next;
- if (!state)
return 0;- list_for_each_entry_safe(spy, next, &state->spy_head, sibling_node)
spy_free(spy);- return 0;
+}
+int event_init(void) +{
- struct event_state *state;
- state = malloc(sizeof(struct event_state));
- if (!state)
return log_msg_ret("alloc", -ENOMEM);- INIT_LIST_HEAD(&state->spy_head);
- gd->event_state = state;
- return 0;
+} diff --git a/common/log.c b/common/log.c index 1aaa6c1527b..aa2e780d2d7 100644 --- a/common/log.c +++ b/common/log.c @@ -28,6 +28,7 @@ static const char *const log_cat_name[] = { "devres", "acpi", "boot",
- "event",
};
_Static_assert(ARRAY_SIZE(log_cat_name) == LOGC_COUNT - LOGC_NONE, diff --git a/drivers/core/device.c b/drivers/core/device.c index efd07176e37..c52b47d5cc1 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -10,6 +10,7 @@
#include <common.h> #include <cpu_func.h> +#include <event.h> #include <log.h> #include <asm/global_data.h> #include <asm/io.h> @@ -482,6 +483,11 @@ static int device_get_dma_constraints(struct udevice *dev) return 0; }
+static int device_notify(const struct udevice *dev, enum event_t type) +{
- return event_notify(type, &dev, sizeof(dev));
+}
int device_probe(struct udevice *dev) { const struct driver *drv; @@ -493,6 +499,10 @@ int device_probe(struct udevice *dev) if (dev_get_flags(dev) & DM_FLAG_ACTIVATED) return 0;
- ret = device_notify(dev, EVT_DM_PRE_PROBE);
- if (ret)
return ret;- drv = dev->driver; assert(drv);
@@ -589,6 +599,10 @@ int device_probe(struct udevice *dev) if (dev->parent && device_get_uclass_id(dev) == UCLASS_PINCTRL) pinctrl_select_state(dev, "default");
- ret = device_notify(dev, EVT_DM_POST_PROBE);
- if (ret)
return ret;- return 0;
fail_uclass: if (device_remove(dev, DM_REMOVE_NORMAL)) { diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h index 16fd305a65c..771cda57a58 100644 --- a/include/asm-generic/global_data.h +++ b/include/asm-generic/global_data.h @@ -459,6 +459,9 @@ struct global_data { */ char *smbios_version; #endif +#if CONFIG_IS_ENABLED(EVENT)
- struct event_state *event_state;
+#endif }; #ifndef DO_DEPS_ONLY static_assert(sizeof(struct global_data) == GD_SIZE); diff --git a/include/event.h b/include/event.h new file mode 100644 index 00000000000..d44fd53639a --- /dev/null +++ b/include/event.h @@ -0,0 +1,103 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*
- Events provide a general-purpose way to react to / subscribe to changes
- within U-Boot
- Copyright 2021 Google LLC
- Written by Simon Glass sjg@chromium.org
- */
+#ifndef __event_h +#define __event_h
+/**
- enum event_t - Types of events supported by U-Boot
- @EVT_DM_PRE_PROBE: Device is about to be probed
- */
+enum event_t {
- EVT_NONE,
- EVT_TEST,
- /* Events related to driver model */
- EVT_DM_PRE_PROBE,
- EVT_DM_POST_PROBE,
- EVT_COUNT
+};
+union event_data {
- /**
* struct event_data_test - test data** @signal: A value to update the state with*/- struct event_data_test {
int signal;- } test;
- /**
* struct event_dm - driver model event** @dev: Device this event relates to*/- struct event_dm {
struct udevice *dev;- } dm;
+};
+/**
- struct event - an event that can be sent and received
- @type: Event type
- @data: Data for this particular event
- */
+struct event {
- enum event_t type;
- union event_data data;
+};
+/** Function type for event handlers */ +typedef int (*event_handler_t)(void *ctx, struct event *event);
+/**
- event_register - register a new spy
- @id: Spy ID
- @type: Event type to subscribe to
- @func: Function to call when the event is sent
- @ctx: Context to pass to the function
- @return 0 if OK, -ve on erropr
- */
+int event_register(const char *id, enum event_t type, event_handler_t func,
void *ctx);+/**
- event_notify() - notify spies about an event
- It is possible to pass in union event_data here but that may not be
- convenient if the data is elsewhere, or is one of the members of the union.
- So this uses a void * for @data, with a separate @size.
- @type: Event type
- @data: Event data to be sent (e.g. union_event_data)
- @size: Size of data in bytes
- */
+int event_notify(enum event_t type, void *data, int size);
+#if CONFIG_IS_ENABLED(EVENT) +int event_uninit(void); +int event_init(void); +#else +static int event_uninit(void) +{
- return 0;
+}
+static inline int event_init(void) +{
- return 0;
+} +#endif
+#endif diff --git a/include/event_internal.h b/include/event_internal.h new file mode 100644 index 00000000000..19308453f7b --- /dev/null +++ b/include/event_internal.h @@ -0,0 +1,34 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*
- Internal definitions for events
- Copyright 2021 Google LLC
- Written by Simon Glass sjg@chromium.org
- */
+#ifndef __event_internal_h +#define __event_internal_h
+#include <linux/list.h>
+/**
- struct event_spy - a spy that watches for an event of a particular type
- @id: Spy ID
- @type: Event type to subscribe to
- @func: Function to call when the event is sent
- @ctx: Context to pass to the function
- */
+struct event_spy {
- struct list_head sibling_node;
- const char *id;
- enum event_t type;
- event_handler_t func;
- void *ctx;
+};
+struct event_state {
- struct list_head spy_head;
+};
+#endif diff --git a/include/log.h b/include/log.h index e0e12ce1944..7169fff7aa5 100644 --- a/include/log.h +++ b/include/log.h @@ -98,6 +98,8 @@ enum log_category_t { LOGC_ACPI, /** @LOGC_BOOT: Related to boot process / boot image processing */ LOGC_BOOT,
- /** @LOGC_EVENT: Related to event and event handling */
- LOGC_EVENT, /** @LOGC_COUNT: Number of log categories */ LOGC_COUNT, /** @LOGC_END: Sentinel value for lists of log categories */
diff --git a/test/common/Makefile b/test/common/Makefile index 24c9145dccc..9087788ba6a 100644 --- a/test/common/Makefile +++ b/test/common/Makefile @@ -1,3 +1,4 @@ # SPDX-License-Identifier: GPL-2.0+ obj-y += cmd_ut_common.o obj-$(CONFIG_AUTOBOOT) += test_autoboot.o +obj-$(CONFIG_EVENT) += event.o diff --git a/test/common/event.c b/test/common/event.c new file mode 100644 index 00000000000..6f359e2a933 --- /dev/null +++ b/test/common/event.c @@ -0,0 +1,85 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*
- Unit tests for event handling
- Copyright 2021 Google LLC
- Written by Simon Glass sjg@chromium.org
- */
+#include <common.h> +#include <dm.h> +#include <event.h> +#include <test/common.h> +#include <test/test.h> +#include <test/ut.h>
+struct test_state {
- struct udevice *dev;
- int val;
+};
+static int h_adder(void *ctx, struct event *event) +{
- struct event_data_test *data = &event->data.test;
- struct test_state *test_state = ctx;
- test_state->val += data->signal;
- return 0;
+}
+static int test_event_base(struct unit_test_state *uts) +{
- struct test_state state;
- int signal;
- state.val = 12;
- ut_assertok(event_register("wibble", EVT_TEST, h_adder, &state));
- signal = 17;
- /* Check that the handler is called */
- ut_assertok(event_notify(EVT_TEST, &signal, sizeof(signal)));
- ut_asserteq(12 + 17, state.val);
- return 0;
+} +COMMON_TEST(test_event_base, 0);
+static int h_probe(void *ctx, struct event *event) +{
- struct test_state *test_state = ctx;
- test_state->dev = event->data.dm.dev;
- switch (event->type) {
- case EVT_DM_PRE_PROBE:
test_state->val |= 1;break;- case EVT_DM_POST_PROBE:
test_state->val |= 2;break;- default:
break;- }
- return 0;
+}
+static int test_event_probe(struct unit_test_state *uts) +{
- struct test_state state;
- struct udevice *dev;
- state.val = 0;
- ut_assertok(event_register("pre", EVT_DM_PRE_PROBE, h_probe, &state));
- ut_assertok(event_register("post", EVT_DM_POST_PROBE, h_probe, &state));
- /* Probe a device */
- ut_assertok(uclass_first_device_err(UCLASS_TEST_FDT, &dev));
- /* Check that the handler is called */
- ut_asserteq(3, state.val);
- return 0;
+} +COMMON_TEST(test_event_probe, UT_TESTF_DM | UT_TESTF_SCAN_FDT); diff --git a/test/test-main.c b/test/test-main.c index 3cdf6849c57..c3ccdf3fed8 100644 --- a/test/test-main.c +++ b/test/test-main.c @@ -7,6 +7,7 @@ #include <common.h> #include <console.h> #include <dm.h> +#include <event.h> #include <dm/root.h> #include <dm/test.h> #include <dm/uclass-internal.h> @@ -218,6 +219,9 @@ static int dm_test_restore(struct device_node *of_root) */ static int test_pre_run(struct unit_test_state *uts, struct unit_test *test) {
- gd->event_state = NULL;
- ut_assertok(event_init());
- if (test->flags & UT_TESTF_DM) ut_assertok(dm_test_pre_run(uts));
@@ -260,6 +264,7 @@ static int test_post_run(struct unit_test_state *uts, struct unit_test *test) ut_unsilence_console(uts); if (test->flags & UT_TESTF_DM) ut_assertok(dm_test_post_run(uts));
ut_assertok(event_uninit());
return 0;
}
2.33.1.1089.g2158813163f-goog
Simon,
On Mon, Nov 01, 2021 at 03:41:50PM +0900, AKASHI Takahiro wrote:
Hello Simon,
On Sun, Oct 31, 2021 at 09:07:01PM -0600, Simon Glass wrote:
This is a patch for Takahiro to look at, just for illustration. Not ready for review.
Thank you for posting the draft. At a first glance, it looks good and I don't see any specific issue with your implementation.
I said OK and functionally it should work well, but I now have some concerns: 1) In my current implementation, I use post_probe/pre_remove hooks of BLK device to invoke efi callback functions. In this sense, event(POST_PROBE/ PRE_REMOVE) seems to be a duplicated feature in some way. 2) For the rest of uclass devices which don't utilise events yet, device_notify() is nothing but an overhead as it always tries to go through a list of event hooks.
Event notification can be more than just a dm feature, but ... What's your thought here?
-Takahiro Akashi
Since my code has already added DM_TAG support, I'm looking forward for getting your final patch.
The only remaining issue is *modeling* partitions :)
-Takahiro Akashi
To run the test:
./u-boot -T -c "ut common test_event_probe"
Signed-off-by: Simon Glass sjg@chromium.org
common/Kconfig | 11 ++++ common/Makefile | 2 + common/board_f.c | 2 + common/event.c | 103 ++++++++++++++++++++++++++++++ common/log.c | 1 + drivers/core/device.c | 14 ++++ include/asm-generic/global_data.h | 3 + include/event.h | 103 ++++++++++++++++++++++++++++++ include/event_internal.h | 34 ++++++++++ include/log.h | 2 + test/common/Makefile | 1 + test/common/event.c | 85 ++++++++++++++++++++++++ test/test-main.c | 5 ++ 13 files changed, 366 insertions(+) create mode 100644 common/event.c create mode 100644 include/event.h create mode 100644 include/event_internal.h create mode 100644 test/common/event.c
diff --git a/common/Kconfig b/common/Kconfig index d6f77ab7b9c..54d0adaa8d5 100644 --- a/common/Kconfig +++ b/common/Kconfig @@ -484,6 +484,17 @@ config DISPLAY_BOARDINFO_LATE
menu "Start-up hooks"
+config EVENT
- bool "General-purpose event-handling mechanism"
- default y if SANDBOX
- help
This enables sending and processing of events, to allow interestedparties to be alerted when something happens. This is an attempt tostep the flow of weak functions, hooks, functions in board_f.cand board_r.c and the Kconfig options below.See doc/develop/event.rst for more information.config ARCH_EARLY_INIT_R bool "Call arch-specific init soon after relocation" help diff --git a/common/Makefile b/common/Makefile index e7839027b6c..1b4601ac13f 100644 --- a/common/Makefile +++ b/common/Makefile @@ -101,6 +101,8 @@ obj-y += malloc_simple.o endif endif
+obj-$(CONFIG_EVENT) += event.o
obj-y += image.o image-board.o obj-$(CONFIG_$(SPL_TPL_)HASH) += hash.o obj-$(CONFIG_ANDROID_AB) += android_ab.o diff --git a/common/board_f.c b/common/board_f.c index 3dc0eaa59c5..faf262d564c 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -19,6 +19,7 @@ #include <dm.h> #include <env.h> #include <env_internal.h> +#include <event.h> #include <fdtdec.h> #include <fs.h> #include <hang.h> @@ -820,6 +821,7 @@ static const init_fnc_t init_sequence_f[] = { initf_malloc, log_init, initf_bootstage, /* uses its own timer, so does not need DM */
- event_init,
#ifdef CONFIG_BLOBLIST bloblist_init, #endif diff --git a/common/event.c b/common/event.c new file mode 100644 index 00000000000..428628da44d --- /dev/null +++ b/common/event.c @@ -0,0 +1,103 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- Events provide a general-purpose way to react to / subscribe to changes
- within U-Boot
- Copyright 2021 Google LLC
- Written by Simon Glass sjg@chromium.org
- */
+#define LOG_CATEGORY LOGC_EVENT
+#include <common.h> +#include <event.h> +#include <event_internal.h> +#include <log.h> +#include <malloc.h> +#include <asm/global_data.h> +#include <linux/list.h>
+DECLARE_GLOBAL_DATA_PTR;
+static void spy_free(struct event_spy *spy) +{
- list_del(&spy->sibling_node);
+}
+int event_register(const char *id, enum event_t type, event_handler_t func, void *ctx) +{
- struct event_state *state = gd->event_state;
- struct event_spy *spy;
- spy = malloc(sizeof(*spy));
- if (!spy)
return log_msg_ret("alloc", -ENOMEM);- spy->id = id;
- spy->type = type;
- spy->func = func;
- spy->ctx = ctx;
- list_add_tail(&spy->sibling_node, &state->spy_head);
- return 0;
+}
+int event_notify(enum event_t type, void *data, int size) +{
- struct event_state *state = gd->event_state;
- struct event_spy *spy, *next;
- struct event event;
- event.type = type;
- if (size > sizeof(event.data))
return log_msg_ret("size", -E2BIG);- memcpy(&event.data, data, size);
- list_for_each_entry_safe(spy, next, &state->spy_head, sibling_node) {
if (spy->type == type) {int ret;log_debug("Sending event %x to spy '%s'\n", type,spy->id);ret = spy->func(spy->ctx, &event);/** TODO: Handle various return codes to** - claim an event (no others will see it)* - return an error from the event*/if (ret)return log_msg_ret("spy", ret);}- }
- return 0;
+}
+int event_uninit(void) +{
- struct event_state *state = gd->event_state;
- struct event_spy *spy, *next;
- if (!state)
return 0;- list_for_each_entry_safe(spy, next, &state->spy_head, sibling_node)
spy_free(spy);- return 0;
+}
+int event_init(void) +{
- struct event_state *state;
- state = malloc(sizeof(struct event_state));
- if (!state)
return log_msg_ret("alloc", -ENOMEM);- INIT_LIST_HEAD(&state->spy_head);
- gd->event_state = state;
- return 0;
+} diff --git a/common/log.c b/common/log.c index 1aaa6c1527b..aa2e780d2d7 100644 --- a/common/log.c +++ b/common/log.c @@ -28,6 +28,7 @@ static const char *const log_cat_name[] = { "devres", "acpi", "boot",
- "event",
};
_Static_assert(ARRAY_SIZE(log_cat_name) == LOGC_COUNT - LOGC_NONE, diff --git a/drivers/core/device.c b/drivers/core/device.c index efd07176e37..c52b47d5cc1 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -10,6 +10,7 @@
#include <common.h> #include <cpu_func.h> +#include <event.h> #include <log.h> #include <asm/global_data.h> #include <asm/io.h> @@ -482,6 +483,11 @@ static int device_get_dma_constraints(struct udevice *dev) return 0; }
+static int device_notify(const struct udevice *dev, enum event_t type) +{
- return event_notify(type, &dev, sizeof(dev));
+}
int device_probe(struct udevice *dev) { const struct driver *drv; @@ -493,6 +499,10 @@ int device_probe(struct udevice *dev) if (dev_get_flags(dev) & DM_FLAG_ACTIVATED) return 0;
- ret = device_notify(dev, EVT_DM_PRE_PROBE);
- if (ret)
return ret;- drv = dev->driver; assert(drv);
@@ -589,6 +599,10 @@ int device_probe(struct udevice *dev) if (dev->parent && device_get_uclass_id(dev) == UCLASS_PINCTRL) pinctrl_select_state(dev, "default");
- ret = device_notify(dev, EVT_DM_POST_PROBE);
- if (ret)
return ret;- return 0;
fail_uclass: if (device_remove(dev, DM_REMOVE_NORMAL)) { diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h index 16fd305a65c..771cda57a58 100644 --- a/include/asm-generic/global_data.h +++ b/include/asm-generic/global_data.h @@ -459,6 +459,9 @@ struct global_data { */ char *smbios_version; #endif +#if CONFIG_IS_ENABLED(EVENT)
- struct event_state *event_state;
+#endif }; #ifndef DO_DEPS_ONLY static_assert(sizeof(struct global_data) == GD_SIZE); diff --git a/include/event.h b/include/event.h new file mode 100644 index 00000000000..d44fd53639a --- /dev/null +++ b/include/event.h @@ -0,0 +1,103 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*
- Events provide a general-purpose way to react to / subscribe to changes
- within U-Boot
- Copyright 2021 Google LLC
- Written by Simon Glass sjg@chromium.org
- */
+#ifndef __event_h +#define __event_h
+/**
- enum event_t - Types of events supported by U-Boot
- @EVT_DM_PRE_PROBE: Device is about to be probed
- */
+enum event_t {
- EVT_NONE,
- EVT_TEST,
- /* Events related to driver model */
- EVT_DM_PRE_PROBE,
- EVT_DM_POST_PROBE,
- EVT_COUNT
+};
+union event_data {
- /**
* struct event_data_test - test data** @signal: A value to update the state with*/- struct event_data_test {
int signal;- } test;
- /**
* struct event_dm - driver model event** @dev: Device this event relates to*/- struct event_dm {
struct udevice *dev;- } dm;
+};
+/**
- struct event - an event that can be sent and received
- @type: Event type
- @data: Data for this particular event
- */
+struct event {
- enum event_t type;
- union event_data data;
+};
+/** Function type for event handlers */ +typedef int (*event_handler_t)(void *ctx, struct event *event);
+/**
- event_register - register a new spy
- @id: Spy ID
- @type: Event type to subscribe to
- @func: Function to call when the event is sent
- @ctx: Context to pass to the function
- @return 0 if OK, -ve on erropr
- */
+int event_register(const char *id, enum event_t type, event_handler_t func,
void *ctx);+/**
- event_notify() - notify spies about an event
- It is possible to pass in union event_data here but that may not be
- convenient if the data is elsewhere, or is one of the members of the union.
- So this uses a void * for @data, with a separate @size.
- @type: Event type
- @data: Event data to be sent (e.g. union_event_data)
- @size: Size of data in bytes
- */
+int event_notify(enum event_t type, void *data, int size);
+#if CONFIG_IS_ENABLED(EVENT) +int event_uninit(void); +int event_init(void); +#else +static int event_uninit(void) +{
- return 0;
+}
+static inline int event_init(void) +{
- return 0;
+} +#endif
+#endif diff --git a/include/event_internal.h b/include/event_internal.h new file mode 100644 index 00000000000..19308453f7b --- /dev/null +++ b/include/event_internal.h @@ -0,0 +1,34 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*
- Internal definitions for events
- Copyright 2021 Google LLC
- Written by Simon Glass sjg@chromium.org
- */
+#ifndef __event_internal_h +#define __event_internal_h
+#include <linux/list.h>
+/**
- struct event_spy - a spy that watches for an event of a particular type
- @id: Spy ID
- @type: Event type to subscribe to
- @func: Function to call when the event is sent
- @ctx: Context to pass to the function
- */
+struct event_spy {
- struct list_head sibling_node;
- const char *id;
- enum event_t type;
- event_handler_t func;
- void *ctx;
+};
+struct event_state {
- struct list_head spy_head;
+};
+#endif diff --git a/include/log.h b/include/log.h index e0e12ce1944..7169fff7aa5 100644 --- a/include/log.h +++ b/include/log.h @@ -98,6 +98,8 @@ enum log_category_t { LOGC_ACPI, /** @LOGC_BOOT: Related to boot process / boot image processing */ LOGC_BOOT,
- /** @LOGC_EVENT: Related to event and event handling */
- LOGC_EVENT, /** @LOGC_COUNT: Number of log categories */ LOGC_COUNT, /** @LOGC_END: Sentinel value for lists of log categories */
diff --git a/test/common/Makefile b/test/common/Makefile index 24c9145dccc..9087788ba6a 100644 --- a/test/common/Makefile +++ b/test/common/Makefile @@ -1,3 +1,4 @@ # SPDX-License-Identifier: GPL-2.0+ obj-y += cmd_ut_common.o obj-$(CONFIG_AUTOBOOT) += test_autoboot.o +obj-$(CONFIG_EVENT) += event.o diff --git a/test/common/event.c b/test/common/event.c new file mode 100644 index 00000000000..6f359e2a933 --- /dev/null +++ b/test/common/event.c @@ -0,0 +1,85 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/*
- Unit tests for event handling
- Copyright 2021 Google LLC
- Written by Simon Glass sjg@chromium.org
- */
+#include <common.h> +#include <dm.h> +#include <event.h> +#include <test/common.h> +#include <test/test.h> +#include <test/ut.h>
+struct test_state {
- struct udevice *dev;
- int val;
+};
+static int h_adder(void *ctx, struct event *event) +{
- struct event_data_test *data = &event->data.test;
- struct test_state *test_state = ctx;
- test_state->val += data->signal;
- return 0;
+}
+static int test_event_base(struct unit_test_state *uts) +{
- struct test_state state;
- int signal;
- state.val = 12;
- ut_assertok(event_register("wibble", EVT_TEST, h_adder, &state));
- signal = 17;
- /* Check that the handler is called */
- ut_assertok(event_notify(EVT_TEST, &signal, sizeof(signal)));
- ut_asserteq(12 + 17, state.val);
- return 0;
+} +COMMON_TEST(test_event_base, 0);
+static int h_probe(void *ctx, struct event *event) +{
- struct test_state *test_state = ctx;
- test_state->dev = event->data.dm.dev;
- switch (event->type) {
- case EVT_DM_PRE_PROBE:
test_state->val |= 1;break;- case EVT_DM_POST_PROBE:
test_state->val |= 2;break;- default:
break;- }
- return 0;
+}
+static int test_event_probe(struct unit_test_state *uts) +{
- struct test_state state;
- struct udevice *dev;
- state.val = 0;
- ut_assertok(event_register("pre", EVT_DM_PRE_PROBE, h_probe, &state));
- ut_assertok(event_register("post", EVT_DM_POST_PROBE, h_probe, &state));
- /* Probe a device */
- ut_assertok(uclass_first_device_err(UCLASS_TEST_FDT, &dev));
- /* Check that the handler is called */
- ut_asserteq(3, state.val);
- return 0;
+} +COMMON_TEST(test_event_probe, UT_TESTF_DM | UT_TESTF_SCAN_FDT); diff --git a/test/test-main.c b/test/test-main.c index 3cdf6849c57..c3ccdf3fed8 100644 --- a/test/test-main.c +++ b/test/test-main.c @@ -7,6 +7,7 @@ #include <common.h> #include <console.h> #include <dm.h> +#include <event.h> #include <dm/root.h> #include <dm/test.h> #include <dm/uclass-internal.h> @@ -218,6 +219,9 @@ static int dm_test_restore(struct device_node *of_root) */ static int test_pre_run(struct unit_test_state *uts, struct unit_test *test) {
- gd->event_state = NULL;
- ut_assertok(event_init());
- if (test->flags & UT_TESTF_DM) ut_assertok(dm_test_pre_run(uts));
@@ -260,6 +264,7 @@ static int test_post_run(struct unit_test_state *uts, struct unit_test *test) ut_unsilence_console(uts); if (test->flags & UT_TESTF_DM) ut_assertok(dm_test_post_run(uts));
ut_assertok(event_uninit());
return 0;
}
2.33.1.1089.g2158813163f-goog
Hi Takahiro,
On Tue, 16 Nov 2021 at 19:31, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Simon,
On Mon, Nov 01, 2021 at 03:41:50PM +0900, AKASHI Takahiro wrote:
Hello Simon,
On Sun, Oct 31, 2021 at 09:07:01PM -0600, Simon Glass wrote:
This is a patch for Takahiro to look at, just for illustration. Not ready for review.
Thank you for posting the draft. At a first glance, it looks good and I don't see any specific issue with your implementation.
I said OK and functionally it should work well, but I now have some concerns:
- In my current implementation, I use post_probe/pre_remove hooks of BLK device to invoke efi callback functions. In this sense, event(POST_PROBE/ PRE_REMOVE) seems to be a duplicated feature in some way.
We should not be calling EFI functions from a BLK devince, unless it is an EFI block device, i.e. we should provide hooks for EFI or any other interested party to use.
- For the rest of uclass devices which don't utilise events yet, device_notify() is nothing but an overhead as it always tries to go through a list of event hooks.
Yes that's true, but it can be optimised to avoid useless searches/calls, since subscribers need to specify an even they are interested in.
Event notification can be more than just a dm feature, but ... What's your thought here?
Yes, more than just a DM feature. For example I think it could help with the initcalls we have in board_f and board_r.
Regards, Simon
-Takahiro Akashi
Since my code has already added DM_TAG support, I'm looking forward for getting your final patch.
The only remaining issue is *modeling* partitions :)
-Takahiro Akashi
To run the test:
./u-boot -T -c "ut common test_event_probe"
Signed-off-by: Simon Glass sjg@chromium.org
[..]
Regards, Simon
On Tue, Nov 16, 2021 at 07:48:59PM -0700, Simon Glass wrote:
Hi Takahiro,
On Tue, 16 Nov 2021 at 19:31, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Simon,
On Mon, Nov 01, 2021 at 03:41:50PM +0900, AKASHI Takahiro wrote:
Hello Simon,
On Sun, Oct 31, 2021 at 09:07:01PM -0600, Simon Glass wrote:
This is a patch for Takahiro to look at, just for illustration. Not ready for review.
Thank you for posting the draft. At a first glance, it looks good and I don't see any specific issue with your implementation.
I said OK and functionally it should work well, but I now have some concerns:
- In my current implementation, I use post_probe/pre_remove hooks of BLK device to invoke efi callback functions. In this sense, event(POST_PROBE/ PRE_REMOVE) seems to be a duplicated feature in some way.
We should not be calling EFI functions from a BLK devince, unless it is an EFI block device, i.e. we should provide hooks for EFI or any other interested party to use.
Even for now, part_init() is called in post_probe() (and blk_create_partitions(), which is to create another type of uclass device, in my patch as well). So I'm not sure what functions can and should be called and what not in those hooks.
Do you want to convert part_init() to an event subscriber along with my efi hooks?
-Takahiro Akashi
- For the rest of uclass devices which don't utilise events yet, device_notify() is nothing but an overhead as it always tries to go through a list of event hooks.
Yes that's true, but it can be optimised to avoid useless searches/calls, since subscribers need to specify an even they are interested in.
Event notification can be more than just a dm feature, but ... What's your thought here?
Yes, more than just a DM feature. For example I think it could help with the initcalls we have in board_f and board_r.
Regards, Simon
-Takahiro Akashi
Since my code has already added DM_TAG support, I'm looking forward for getting your final patch.
The only remaining issue is *modeling* partitions :)
-Takahiro Akashi
To run the test:
./u-boot -T -c "ut common test_event_probe"
Signed-off-by: Simon Glass sjg@chromium.org
[..]
Regards, Simon
Hi Takahiro,
On Tue, 16 Nov 2021 at 20:03, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Tue, Nov 16, 2021 at 07:48:59PM -0700, Simon Glass wrote:
Hi Takahiro,
On Tue, 16 Nov 2021 at 19:31, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Simon,
On Mon, Nov 01, 2021 at 03:41:50PM +0900, AKASHI Takahiro wrote:
Hello Simon,
On Sun, Oct 31, 2021 at 09:07:01PM -0600, Simon Glass wrote:
This is a patch for Takahiro to look at, just for illustration. Not ready for review.
Thank you for posting the draft. At a first glance, it looks good and I don't see any specific issue with your implementation.
I said OK and functionally it should work well, but I now have some concerns:
- In my current implementation, I use post_probe/pre_remove hooks of BLK device to invoke efi callback functions. In this sense, event(POST_PROBE/ PRE_REMOVE) seems to be a duplicated feature in some way.
We should not be calling EFI functions from a BLK devince, unless it is an EFI block device, i.e. we should provide hooks for EFI or any other interested party to use.
Even for now, part_init() is called in post_probe() (and blk_create_partitions(), which is to create another type of uclass device, in my patch as well). So I'm not sure what functions can and should be called and what not in those hooks.
Do you want to convert part_init() to an event subscriber along with my efi hooks?
You could do. From my reading of the code it makes sense to do that.
Regards, Simon
-Takahiro Akashi
- For the rest of uclass devices which don't utilise events yet, device_notify() is nothing but an overhead as it always tries to go through a list of event hooks.
Yes that's true, but it can be optimised to avoid useless searches/calls, since subscribers need to specify an even they are interested in.
Event notification can be more than just a dm feature, but ... What's your thought here?
Yes, more than just a DM feature. For example I think it could help with the initcalls we have in board_f and board_r.
Regards, Simon
-Takahiro Akashi
Since my code has already added DM_TAG support, I'm looking forward for getting your final patch.
The only remaining issue is *modeling* partitions :)
-Takahiro Akashi
To run the test:
./u-boot -T -c "ut common test_event_probe"
Signed-off-by: Simon Glass sjg@chromium.org
[..]
Regards, Simon
Hi Simon,
On Wed, Nov 24, 2021 at 05:12:45PM -0700, Simon Glass wrote:
Hi Takahiro,
On Tue, 16 Nov 2021 at 20:03, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Tue, Nov 16, 2021 at 07:48:59PM -0700, Simon Glass wrote:
Hi Takahiro,
On Tue, 16 Nov 2021 at 19:31, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Simon,
On Mon, Nov 01, 2021 at 03:41:50PM +0900, AKASHI Takahiro wrote:
Hello Simon,
On Sun, Oct 31, 2021 at 09:07:01PM -0600, Simon Glass wrote:
This is a patch for Takahiro to look at, just for illustration. Not ready for review.
Thank you for posting the draft. At a first glance, it looks good and I don't see any specific issue with your implementation.
I said OK and functionally it should work well, but I now have some concerns:
- In my current implementation, I use post_probe/pre_remove hooks of BLK device to invoke efi callback functions. In this sense, event(POST_PROBE/ PRE_REMOVE) seems to be a duplicated feature in some way.
We should not be calling EFI functions from a BLK devince, unless it is an EFI block device, i.e. we should provide hooks for EFI or any other interested party to use.
Even for now, part_init() is called in post_probe() (and blk_create_partitions(), which is to create another type of uclass device, in my patch as well). So I'm not sure what functions can and should be called and what not in those hooks.
Do you want to convert part_init() to an event subscriber along with my efi hooks?
You could do. From my reading of the code it makes sense to do that.
Ok, but I will put this change on hold for now.
One of problems with the proposed framework of event notification is that we cannot deterministically specify the order of callback functions when we want to add callbacks for a number of subsystems to a particular event. For instance, we have callbacks for efi_disk creations as well as partitions (part_init() & blk_create_partitions()) at POST_PROBE of BLK devices, they should (logically) be called in this order but currently no definite guarantee. If we utilise a callback to create efi_disk's for "partition" devices as well, this situation can get more complicated. # Why? If the order of invocations is the other way around, efi_disks # for partitions might be created before an efi_disk for a whole disk.
-Takahiro Akashi
Regards, Simon
-Takahiro Akashi
- For the rest of uclass devices which don't utilise events yet, device_notify() is nothing but an overhead as it always tries to go through a list of event hooks.
Yes that's true, but it can be optimised to avoid useless searches/calls, since subscribers need to specify an even they are interested in.
Event notification can be more than just a dm feature, but ... What's your thought here?
Yes, more than just a DM feature. For example I think it could help with the initcalls we have in board_f and board_r.
Regards, Simon
-Takahiro Akashi
Since my code has already added DM_TAG support, I'm looking forward for getting your final patch.
The only remaining issue is *modeling* partitions :)
-Takahiro Akashi
To run the test:
./u-boot -T -c "ut common test_event_probe"
Signed-off-by: Simon Glass sjg@chromium.org
[..]
Regards, Simon
Hi Tahahiro,
On Wed, 24 Nov 2021 at 19:57, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Hi Simon,
On Wed, Nov 24, 2021 at 05:12:45PM -0700, Simon Glass wrote:
Hi Takahiro,
On Tue, 16 Nov 2021 at 20:03, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Tue, Nov 16, 2021 at 07:48:59PM -0700, Simon Glass wrote:
Hi Takahiro,
On Tue, 16 Nov 2021 at 19:31, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Simon,
On Mon, Nov 01, 2021 at 03:41:50PM +0900, AKASHI Takahiro wrote:
Hello Simon,
On Sun, Oct 31, 2021 at 09:07:01PM -0600, Simon Glass wrote: > This is a patch for Takahiro to look at, just for illustration. Not ready > for review.
Thank you for posting the draft. At a first glance, it looks good and I don't see any specific issue with your implementation.
I said OK and functionally it should work well, but I now have some concerns:
- In my current implementation, I use post_probe/pre_remove hooks of BLK device to invoke efi callback functions. In this sense, event(POST_PROBE/ PRE_REMOVE) seems to be a duplicated feature in some way.
We should not be calling EFI functions from a BLK devince, unless it is an EFI block device, i.e. we should provide hooks for EFI or any other interested party to use.
Even for now, part_init() is called in post_probe() (and blk_create_partitions(), which is to create another type of uclass device, in my patch as well). So I'm not sure what functions can and should be called and what not in those hooks.
Do you want to convert part_init() to an event subscriber along with my efi hooks?
You could do. From my reading of the code it makes sense to do that.
Ok, but I will put this change on hold for now.
One of problems with the proposed framework of event notification is that we cannot deterministically specify the order of callback functions when we want to add callbacks for a number of subsystems to a particular event. For instance, we have callbacks for efi_disk creations as well as partitions (part_init() & blk_create_partitions()) at POST_PROBE of BLK devices, they should (logically) be called in this order but currently no definite guarantee. If we utilise a callback to create efi_disk's for "partition" devices as well, this situation can get more complicated. # Why? If the order of invocations is the other way around, efi_disks # for partitions might be created before an efi_disk for a whole disk.
Yes I was wondering if that would cause problems.
We also need to think about discoverability. It needs to be *very easy* to see a list of callbacks for an event a priori and we don't want debugging to be a pain.
I'll take a look at adding priority and some commands to list the callbacks.
Regards, Simon
Hi Takahiro,
On Tue, 7 Dec 2021 at 08:07, Simon Glass sjg@chromium.org wrote:
Hi Tahahiro,
On Wed, 24 Nov 2021 at 19:57, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Hi Simon,
On Wed, Nov 24, 2021 at 05:12:45PM -0700, Simon Glass wrote:
Hi Takahiro,
On Tue, 16 Nov 2021 at 20:03, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Tue, Nov 16, 2021 at 07:48:59PM -0700, Simon Glass wrote:
Hi Takahiro,
On Tue, 16 Nov 2021 at 19:31, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Simon,
On Mon, Nov 01, 2021 at 03:41:50PM +0900, AKASHI Takahiro wrote: > Hello Simon, > > On Sun, Oct 31, 2021 at 09:07:01PM -0600, Simon Glass wrote: > > This is a patch for Takahiro to look at, just for illustration. Not ready > > for review. > > Thank you for posting the draft. > At a first glance, it looks good and I don't see any specific issue > with your implementation.
I said OK and functionally it should work well, but I now have some concerns:
- In my current implementation, I use post_probe/pre_remove hooks of BLK device to invoke efi callback functions. In this sense, event(POST_PROBE/ PRE_REMOVE) seems to be a duplicated feature in some way.
We should not be calling EFI functions from a BLK devince, unless it is an EFI block device, i.e. we should provide hooks for EFI or any other interested party to use.
Even for now, part_init() is called in post_probe() (and blk_create_partitions(), which is to create another type of uclass device, in my patch as well). So I'm not sure what functions can and should be called and what not in those hooks.
Do you want to convert part_init() to an event subscriber along with my efi hooks?
You could do. From my reading of the code it makes sense to do that.
Ok, but I will put this change on hold for now.
One of problems with the proposed framework of event notification is that we cannot deterministically specify the order of callback functions when we want to add callbacks for a number of subsystems to a particular event. For instance, we have callbacks for efi_disk creations as well as partitions (part_init() & blk_create_partitions()) at POST_PROBE of BLK devices, they should (logically) be called in this order but currently no definite guarantee. If we utilise a callback to create efi_disk's for "partition" devices as well, this situation can get more complicated. # Why? If the order of invocations is the other way around, efi_disks # for partitions might be created before an efi_disk for a whole disk.
Yes I was wondering if that would cause problems.
We also need to think about discoverability. It needs to be *very easy* to see a list of callbacks for an event a priori and we don't want debugging to be a pain.
I'll take a look at adding priority and some commands to list the callbacks.
I added the commands but not the priority, yet. I will see if I get some comments first.
Regards, Simon
participants (2)
-
AKASHI Takahiro -
Simon Glass