Re: [PATCH 08/19] dm: add event notification

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

On 2/2/22 02:08, AKASHI Takahiro wrote:
> From: Simon Glass <sjg@chromium.org>
>
> This is a draft implementation of event notification mechanism from Simon.
> Under this scheme, any U-Boot subsystem can register some kind of callback
> function to a particular event (more event types will be added later) and
> that function will be invoked once the event is fired.
>
> As a first user, UEFI subsystem makes use of PROBE and REMOVE events so
> that we will be able to automatically create/remove efi_disk objects
> relying on associated block devices (UCLASS_BLK).
>
> To run the test:
> ./u-boot -T -c "ut common test_event_probe"
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> [add REMOVE event; fix checkpatch warnings]
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>   common/Kconfig                    |  11 ++++
>   common/Makefile                   |   2 +
>   common/board_f.c                  |   2 +
>   common/event.c                    | 103 +++++++++++++++++++++++++++++
>   common/log.c                      |   1 +
>   drivers/core/device-remove.c      |   9 +++
>   drivers/core/device.c             |   9 +++
>   include/asm-generic/global_data.h |   3 +
>   include/dm/device-internal.h      |  10 +++
>   include/event.h                   | 105 ++++++++++++++++++++++++++++++
>   include/event_internal.h          |  34 ++++++++++
>   include/log.h                     |   2 +
>   test/common/Makefile              |   1 +
>   test/common/event.c               |  87 +++++++++++++++++++++++++
>   test/test-main.c                  |   5 ++
>   15 files changed, 384 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 82cd864baf93..d411e5bf7a50 100644
> --- a/common/Kconfig
> +++ b/common/Kconfig
> @@ -492,6 +492,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 3eff71960160..4e9f59cc07ad 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -89,6 +89,8 @@ obj-y += malloc_simple.o
>   endif
>   endif
>
> +obj-$(CONFIG_EVENT) += event.o
> +
>   obj-$(CONFIG_$(SPL_TPL_)HASH) += hash.o
>   obj-$(CONFIG_IO_TRACE) += iotrace.o
>   obj-y += memsize.o
> diff --git a/common/board_f.c b/common/board_f.c
> index a68760092ac1..e36bdbc988fa 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>
> @@ -828,6 +829,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 000000000000..428628da44d6
> --- /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 f7e0c0fbf556..7254aa70bfdf 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-remove.c b/drivers/core/device-remove.c
> index e6ec6ff42121..c3f8ed3e5721 100644
> --- a/drivers/core/device-remove.c
> +++ b/drivers/core/device-remove.c
> @@ -12,6 +12,7 @@
>
>   #include <common.h>
>   #include <errno.h>
> +#include <event.h>
>   #include <log.h>
>   #include <malloc.h>
>   #include <dm/device.h>
> @@ -207,6 +208,10 @@ int device_remove(struct udevice *dev, uint flags)
>   	if (!(dev_get_flags(dev) & DM_FLAG_ACTIVATED))
>   		return 0;
>
> +	ret = device_notify(dev, EVT_DM_PRE_REMOVE);
> +	if (ret)
> +		return ret;
> +
>   	/*
>   	 * If the child returns EKEYREJECTED, continue. It just means that it
>   	 * didn't match the flags.
> @@ -256,6 +261,10 @@ int device_remove(struct udevice *dev, uint flags)
>
>   	dev_bic_flags(dev, DM_FLAG_ACTIVATED);
>
> +	ret = device_notify(dev, EVT_DM_POST_REMOVE);
> +	if (ret)
> +		goto err_remove;
> +
>   	return 0;
>
>   err_remove:
> diff --git a/drivers/core/device.c b/drivers/core/device.c
> index 901c1e2f7db3..6f69b234a182 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>
> @@ -493,6 +494,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);
>
> @@ -597,6 +602,10 @@ int device_probe(struct udevice *dev)
>   				  dev->name, ret, errno_str(ret));
>   	}
>
> +	ret = device_notify(dev, EVT_DM_POST_PROBE);
> +	if (ret)
> +		goto fail;
> +
>   	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 c2f8fad1cb92..9f4598f09ee5 100644
> --- a/include/asm-generic/global_data.h
> +++ b/include/asm-generic/global_data.h
> @@ -467,6 +467,9 @@ struct global_data {
>   	 */
>   	char *smbios_version;
>   #endif
> +#if CONFIG_IS_ENABLED(EVENT)
> +	struct event_state *event_state;

This breaks 'make htmldocs'.

./include/asm-generic/global_data.h:474: warning: Function parameter or
member 'event_state' not described in 'global_data'

Best regards

Heinrich

> +#endif
>   };
>   #ifndef DO_DEPS_ONLY
>   static_assert(sizeof(struct global_data) == GD_SIZE);
> diff --git a/include/dm/device-internal.h b/include/dm/device-internal.h
> index 02002acb787c..63074e1de509 100644
> --- a/include/dm/device-internal.h
> +++ b/include/dm/device-internal.h
> @@ -10,6 +10,7 @@
>   #ifndef _DM_DEVICE_INTERNAL_H
>   #define _DM_DEVICE_INTERNAL_H
>
> +#include <event.h>
>   #include <linker_lists.h>
>   #include <dm/ofnode.h>
>
> @@ -426,4 +427,13 @@ static inline void devres_release_all(struct udevice *dev)
>   }
>
>   #endif /* ! CONFIG_DEVRES */
> +
> +static inline int device_notify(const struct udevice *dev, enum event_t type)
> +{
> +#ifdef CONFIG_EVENT
> +	return event_notify(type, &dev, sizeof(dev));
> +#else
> +	return 0;
> +#endif
> +}
>   #endif
> diff --git a/include/event.h b/include/event.h
> new file mode 100644
> index 000000000000..e2b74e6e62f0
> --- /dev/null
> +++ b/include/event.h
> @@ -0,0 +1,105 @@
> +/* 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_DM_PRE_REMOVE,
> +	EVT_DM_POST_REMOVE,
> +
> +	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 inline 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 000000000000..19308453f7b2
> --- /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 ce48d51446f5..8f35c10abb5e 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 24c9145dccc8..9087788ba6a8 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 000000000000..ddce7400f269
> --- /dev/null
> +++ b/test/common/event.c
> @@ -0,0 +1,87 @@
> +// 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 8fcb02ecea5c..fd89ff558a0e 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;
>   }