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