From aeb493e263329c67d79f0ba226d78149aa0d11ce Mon Sep 17 00:00:00 2001 From: Peter Bigot Date: Tue, 2 Feb 2021 09:19:58 -0600 Subject: [PATCH 1/5] device: add common structure for dynamic device state While devices have driver-specific dynamic state accessed through the data field, there is also dynamic state that is common to most if not all devices. Add a structure to hold that data. Signed-off-by: Peter Bigot --- include/device.h | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/include/device.h b/include/device.h index 8274f974cb53..a98482cfbfd5 100644 --- a/include/device.h +++ b/include/device.h @@ -285,7 +285,18 @@ struct device_pm { }; /** - * @brief Runtime device structure (in memory) per driver instance + * @brief Runtime device dynamic structure (in RAM) per driver instance + * + * Fields in this are expected to be default-initialized to zero. The + * kernel driver infrastructure and driver access functions are + * responsible for ensuring that any non-zero initialization is done + * before they are accessed. + */ +struct device_state { +}; + +/** + * @brief Runtime device structure (in ROM) per driver instance */ struct device { /** Name of the device instance */ @@ -294,6 +305,8 @@ struct device { const void *config; /** Address of the API structure exposed by the device instance */ const void *api; + /** Address of the common device state */ + struct device_state * const state; /** Address of the device instance private data */ void * const data; #ifdef CONFIG_PM_DEVICE @@ -697,9 +710,15 @@ static inline int device_pm_put_sync(const struct device *dev) { return -ENOTSUP */ #define Z_DEVICE_DT_DEV_NAME(node_id) _CONCAT(dts_ord_, DT_DEP_ORD(node_id)) +/* Synthesize a unique name for the device state associated with + * dev_name. + */ +#define Z_DEVICE_STATE_NAME(dev_name) _CONCAT(__devstate_, dev_name) + #define Z_DEVICE_DEFINE(node_id, dev_name, drv_name, init_fn, pm_control_fn, \ data_ptr, cfg_ptr, level, prio, api_ptr) \ Z_DEVICE_DEFINE_PM(dev_name) \ + static struct device_state Z_DEVICE_STATE_NAME(dev_name); \ COND_CODE_1(DT_NODE_EXISTS(node_id), (), (static)) \ const Z_DECL_ALIGN(struct device) \ DEVICE_NAME_GET(dev_name) __used \ @@ -707,6 +726,7 @@ static inline int device_pm_put_sync(const struct device *dev) { return -ENOTSUP .name = drv_name, \ .config = (cfg_ptr), \ .api = (api_ptr), \ + .state = &Z_DEVICE_STATE_NAME(dev_name), \ .data = (data_ptr), \ Z_DEVICE_DEFINE_PM_INIT(dev_name, pm_control_fn) \ }; \ From bd270b7dcdcbb2e1e7626ed0519449cdb446ae23 Mon Sep 17 00:00:00 2001 From: Peter Bigot Date: Tue, 2 Feb 2021 10:07:18 -0600 Subject: [PATCH 2/5] device: perform dynamic device initialization during system startup Initialize all device objects in a batch before invoking any code that might try to reference data in them. This eliminates a race condition enabled by the ability to resolve a device structure at build time, and reference it from one device's initialization routine before the device itself has been initialized. While the device is pulled from the sys_init records rather than static devices, all in-tree init_entry records that are associated with devices are produced via Z_DEVICE_DEFINE(), so there should be no static devices that would be missed by instead iterating over the device records. Signed-off-by: Peter Bigot --- kernel/device.c | 20 ++++++++++++++++---- kernel/include/kernel_internal.h | 2 ++ kernel/init.c | 3 +++ 3 files changed, 21 insertions(+), 4 deletions(-) diff --git a/kernel/device.c b/kernel/device.c index a30439ae599a..b95feaafb4ec 100644 --- a/kernel/device.c +++ b/kernel/device.c @@ -31,6 +31,22 @@ extern uint32_t __device_busy_end[]; #define DEVICE_BUSY_SIZE (__device_busy_end - __device_busy_start) #endif +/** + * @brief Initialize state for all static devices. + * + * The state object is always zero-initialized, but this may not be + * sufficient. + */ +void z_device_state_init(void) +{ + const struct device *dev = __device_start; + + while (dev < __device_end) { + z_object_init(dev); + ++dev; + } +} + /** * @brief Execute all the init entry initialization functions at a given level * @@ -60,10 +76,6 @@ void z_sys_init_run_level(int32_t level) for (entry = levels[level]; entry < levels[level+1]; entry++) { const struct device *dev = entry->dev; - if (dev != NULL) { - z_object_init(dev); - } - if ((entry->init(dev) != 0) && (dev != NULL)) { /* Initialization failed. * Set the init status bit so device is not declared ready. diff --git a/kernel/include/kernel_internal.h b/kernel/include/kernel_internal.h index 702d5c512cd2..8ae14ffbb071 100644 --- a/kernel/include/kernel_internal.h +++ b/kernel/include/kernel_internal.h @@ -37,6 +37,8 @@ static inline void z_data_copy(void) #endif FUNC_NORETURN void z_cstart(void); +void z_device_state_init(void); + extern FUNC_NORETURN void z_thread_entry(k_thread_entry_t entry, void *p1, void *p2, void *p3); diff --git a/kernel/init.c b/kernel/init.c index fe08c415fb28..66493dd39202 100644 --- a/kernel/init.c +++ b/kernel/init.c @@ -386,6 +386,9 @@ FUNC_NORETURN void z_cstart(void) #if defined(CONFIG_MMU) && defined(CONFIG_USERSPACE) z_kernel_map_fixup(); #endif + /* do any necessary initialization of static devices */ + z_device_state_init(); + /* perform basic hardware initialization */ z_sys_init_run_level(_SYS_INIT_LEVEL_PRE_KERNEL_1); z_sys_init_run_level(_SYS_INIT_LEVEL_PRE_KERNEL_2); From 3b08aa22b56cf2d4f9b7b0f4b26fae39883c555b Mon Sep 17 00:00:00 2001 From: Peter Bigot Date: Tue, 2 Feb 2021 10:09:14 -0600 Subject: [PATCH 3/5] device: move device power management state into common dynamic state This avoids the need for distinct object that uses flash to store its initializer. Instead the state is initialized when the kernel is starting up, before anything can reference it. In future refactoring the PM state could be accessed directly without storing an extra pointer in the static device state. Signed-off-by: Peter Bigot --- include/device.h | 20 +++++--------------- kernel/device.c | 16 ++++++++++++++++ 2 files changed, 21 insertions(+), 15 deletions(-) diff --git a/include/device.h b/include/device.h index a98482cfbfd5..c65c1a320638 100644 --- a/include/device.h +++ b/include/device.h @@ -293,6 +293,10 @@ struct device_pm { * before they are accessed. */ struct device_state { +#ifdef CONFIG_PM_DEVICE + /* Power management data */ + struct device_pm pm; +#endif /* CONFIG_PM_DEVICE */ }; /** @@ -717,7 +721,6 @@ static inline int device_pm_put_sync(const struct device *dev) { return -ENOTSUP #define Z_DEVICE_DEFINE(node_id, dev_name, drv_name, init_fn, pm_control_fn, \ data_ptr, cfg_ptr, level, prio, api_ptr) \ - Z_DEVICE_DEFINE_PM(dev_name) \ static struct device_state Z_DEVICE_STATE_NAME(dev_name); \ COND_CODE_1(DT_NODE_EXISTS(node_id), (), (static)) \ const Z_DECL_ALIGN(struct device) \ @@ -736,23 +739,10 @@ static inline int device_pm_put_sync(const struct device *dev) { return -ENOTSUP (&DEVICE_NAME_GET(dev_name)), level, prio) #ifdef CONFIG_PM_DEVICE -#define Z_DEVICE_DEFINE_PM(dev_name) \ - static struct device_pm _CONCAT(__pm_, dev_name) __used = { \ - .usage = ATOMIC_INIT(0), \ - .lock = Z_SEM_INITIALIZER( \ - _CONCAT(__pm_, dev_name).lock, 1, 1), \ - .signal = K_POLL_SIGNAL_INITIALIZER( \ - _CONCAT(__pm_, dev_name).signal), \ - .event = K_POLL_EVENT_INITIALIZER( \ - K_POLL_TYPE_SIGNAL, \ - K_POLL_MODE_NOTIFY_ONLY, \ - &_CONCAT(__pm_, dev_name).signal), \ - }; #define Z_DEVICE_DEFINE_PM_INIT(dev_name, pm_control_fn) \ .device_pm_control = (pm_control_fn), \ - .pm = &_CONCAT(__pm_, dev_name), + .pm = &Z_DEVICE_STATE_NAME(dev_name).pm, #else -#define Z_DEVICE_DEFINE_PM(dev_name) #define Z_DEVICE_DEFINE_PM_INIT(dev_name, pm_control_fn) #endif diff --git a/kernel/device.c b/kernel/device.c index b95feaafb4ec..ae08ddaa9340 100644 --- a/kernel/device.c +++ b/kernel/device.c @@ -31,6 +31,21 @@ extern uint32_t __device_busy_end[]; #define DEVICE_BUSY_SIZE (__device_busy_end - __device_busy_start) #endif +static inline void device_pm_state_init(const struct device *dev) +{ +#ifdef CONFIG_PM_DEVICE + *dev->pm = (struct device_pm){ + .usage = ATOMIC_INIT(0), + .lock = Z_SEM_INITIALIZER(dev->pm->lock, 1, 1), + .signal = K_POLL_SIGNAL_INITIALIZER(dev->pm->signal), + .event = K_POLL_EVENT_INITIALIZER( + K_POLL_TYPE_SIGNAL, + K_POLL_MODE_NOTIFY_ONLY, + &dev->pm->signal), + }; +#endif /* CONFIG_PM_DEVICE */ +} + /** * @brief Initialize state for all static devices. * @@ -42,6 +57,7 @@ void z_device_state_init(void) const struct device *dev = __device_start; while (dev < __device_end) { + device_pm_state_init(dev); z_object_init(dev); ++dev; } From 6bf4cf60832c04dcfd9e759d183a3be2ec89f22d Mon Sep 17 00:00:00 2001 From: Peter Bigot Date: Tue, 2 Feb 2021 10:23:55 -0600 Subject: [PATCH 4/5] device: store initialization status in the state structure Separate the state indicator of whether the initialization function has been invoked from the success or failure of the initialization. This allows precise confirmation that the device is ready (i.e. it has been initialized, and that initialization succeeded). Signed-off-by: Peter Bigot --- include/device.h | 14 ++++++++++++++ include/linker/common-ram.ld | 7 ------- kernel/device.c | 24 +++++++++++++++--------- 3 files changed, 29 insertions(+), 16 deletions(-) diff --git a/include/device.h b/include/device.h index c65c1a320638..51ee8b3dcb62 100644 --- a/include/device.h +++ b/include/device.h @@ -293,6 +293,20 @@ struct device_pm { * before they are accessed. */ struct device_state { + /** Non-negative result of initializing the device. + * + * The absolute value returned when the device initialization + * function was invoked, or `UINT8_MAX` if the value exceeds + * an 8-bit integer. If initialized is also set, a zero value + * indicates initialization succeeded. + */ + unsigned int init_res : 8; + + /** Indicates the device initialization function has been + * invoked. + */ + bool initialized : 1; + #ifdef CONFIG_PM_DEVICE /* Power management data */ struct device_pm pm; diff --git a/include/linker/common-ram.ld b/include/linker/common-ram.ld index e1c002ff79e1..c6838af3d81a 100644 --- a/include/linker/common-ram.ld +++ b/include/linker/common-ram.ld @@ -24,12 +24,6 @@ ((__device_end - __device_start) / _DEVICE_STRUCT_SIZEOF) #define DEVICE_BITFIELD_SIZE (((DEVICE_COUNT + 31) / 32) * 4) -#define DEVICE_INIT_STATUS_BITFIELD() \ - FILL(0x00); \ - __device_init_status_start = .; \ - . = . + DEVICE_BITFIELD_SIZE; \ - __device_init_status_end = .; - #ifdef CONFIG_PM_DEVICE #define DEVICE_BUSY_BITFIELD() \ FILL(0x00); \ @@ -53,7 +47,6 @@ CREATE_OBJ_LEVEL(device, APPLICATION) CREATE_OBJ_LEVEL(device, SMP) __device_end = .; - DEVICE_INIT_STATUS_BITFIELD() DEVICE_BUSY_BITFIELD() } GROUP_DATA_LINK_IN(RAMABLE_REGION, ROMABLE_REGION) diff --git a/kernel/device.c b/kernel/device.c index ae08ddaa9340..5e6556020b26 100644 --- a/kernel/device.c +++ b/kernel/device.c @@ -91,14 +91,22 @@ void z_sys_init_run_level(int32_t level) for (entry = levels[level]; entry < levels[level+1]; entry++) { const struct device *dev = entry->dev; + int rc = entry->init(dev); - if ((entry->init(dev) != 0) && (dev != NULL)) { - /* Initialization failed. - * Set the init status bit so device is not declared ready. + if (dev != NULL) { + /* Mark device initialized. If initialization + * failed, record the error condition. */ - sys_bitfield_set_bit( - (mem_addr_t) __device_init_status_start, - (dev - __device_start)); + if (rc != 0) { + if (rc < 0) { + rc = -rc; + } + if (rc > UINT8_MAX) { + rc = UINT8_MAX; + } + dev->state->init_res = rc; + } + dev->state->initialized = true; } } } @@ -157,9 +165,7 @@ size_t z_device_get_all_static(struct device const **devices) bool z_device_ready(const struct device *dev) { - /* Set bit indicates device failed initialization */ - return !(sys_bitfield_test_bit((mem_addr_t)__device_init_status_start, - (dev - __device_start))); + return dev->state->initialized && (dev->state->init_res == 0); } #ifdef CONFIG_PM_DEVICE From 714114c8a7368192f679429f64cc3eeed8892f4c Mon Sep 17 00:00:00 2001 From: Peter Bigot Date: Tue, 2 Feb 2021 10:37:30 -0600 Subject: [PATCH 5/5] device: store device pm busy status in the state structure Move the busy status from a global atomic bit sequence to atomic flags in the device PM state. While this temporarily adds 4 bytes to each PM structure the whole device PM infrastructure will be refactored and it's likely the extra memory can be recovered. Signed-off-by: Peter Bigot --- include/device.h | 10 +++++++++- include/linker/common-ram.ld | 22 --------------------- kernel/device.c | 37 +++++++++++++++++------------------- 3 files changed, 26 insertions(+), 43 deletions(-) diff --git a/include/device.h b/include/device.h index 51ee8b3dcb62..3eba62d4c706 100644 --- a/include/device.h +++ b/include/device.h @@ -270,8 +270,11 @@ struct device_pm { const struct device *dev; /** Lock to synchronize the get/put operations */ struct k_sem lock; + /* Following are packed fields protected by #lock. */ /** Device pm enable flag */ - bool enable; + bool enable : 1; + /* Following are packed fields accessed with atomic bit operations. */ + atomic_t atomic_flags; /** Device usage count */ atomic_t usage; /** Device idle internal power state */ @@ -284,6 +287,11 @@ struct device_pm { struct k_poll_signal signal; }; +/** Bit position in device_pm::atomic_flags that records whether the + * device is busy. + */ +#define DEVICE_PM_ATOMIC_FLAGS_BUSY_BIT 0 + /** * @brief Runtime device dynamic structure (in RAM) per driver instance * diff --git a/include/linker/common-ram.ld b/include/linker/common-ram.ld index c6838af3d81a..64285bd3b759 100644 --- a/include/linker/common-ram.ld +++ b/include/linker/common-ram.ld @@ -13,27 +13,6 @@ } GROUP_DATA_LINK_IN(RAMABLE_REGION, ROMABLE_REGION) #endif -/* - * Space for storing per device init status and busy bitmap in case PM is - * enabled. Since we do not know beforehand the number of devices, - * we go through the below mechanism to allocate the required space. - * Both are made of 1 bit per-device instance, so we compute the size of - * of an entire bitfield, aligned on 32bits. - */ -#define DEVICE_COUNT \ - ((__device_end - __device_start) / _DEVICE_STRUCT_SIZEOF) -#define DEVICE_BITFIELD_SIZE (((DEVICE_COUNT + 31) / 32) * 4) - -#ifdef CONFIG_PM_DEVICE -#define DEVICE_BUSY_BITFIELD() \ - FILL(0x00); \ - __device_busy_start = .; \ - . = . + DEVICE_BITFIELD_SIZE; \ - __device_busy_end = .; -#else -#define DEVICE_BUSY_BITFIELD() -#endif - SECTION_DATA_PROLOGUE(devices,,) { /* link in devices objects, which are tied to the init ones; @@ -47,7 +26,6 @@ CREATE_OBJ_LEVEL(device, APPLICATION) CREATE_OBJ_LEVEL(device, SMP) __device_end = .; - DEVICE_BUSY_BITFIELD() } GROUP_DATA_LINK_IN(RAMABLE_REGION, ROMABLE_REGION) SECTION_DATA_PROLOGUE(initshell,,) diff --git a/kernel/device.c b/kernel/device.c index 5e6556020b26..b561b9745c18 100644 --- a/kernel/device.c +++ b/kernel/device.c @@ -25,12 +25,6 @@ extern const struct device __device_end[]; extern uint32_t __device_init_status_start[]; -#ifdef CONFIG_PM_DEVICE -extern uint32_t __device_busy_start[]; -extern uint32_t __device_busy_end[]; -#define DEVICE_BUSY_SIZE (__device_busy_end - __device_busy_start) -#endif - static inline void device_pm_state_init(const struct device *dev) { #ifdef CONFIG_PM_DEVICE @@ -180,20 +174,23 @@ int device_pm_control_nop(const struct device *unused_device, int device_any_busy_check(void) { - int i = 0; + const struct device *dev = __device_start; - for (i = 0; i < DEVICE_BUSY_SIZE; i++) { - if (__device_busy_start[i] != 0U) { + while (dev < __device_end) { + if (atomic_test_bit(&dev->pm->atomic_flags, + DEVICE_PM_ATOMIC_FLAGS_BUSY_BIT)) { return -EBUSY; } + ++dev; } + return 0; } -int device_busy_check(const struct device *chk_dev) +int device_busy_check(const struct device *dev) { - if (atomic_test_bit((const atomic_t *)__device_busy_start, - (chk_dev - __device_start))) { + if (atomic_test_bit(&dev->pm->atomic_flags, + DEVICE_PM_ATOMIC_FLAGS_BUSY_BIT)) { return -EBUSY; } return 0; @@ -201,22 +198,22 @@ int device_busy_check(const struct device *chk_dev) #endif -void device_busy_set(const struct device *busy_dev) +void device_busy_set(const struct device *dev) { #ifdef CONFIG_PM_DEVICE - atomic_set_bit((atomic_t *) __device_busy_start, - (busy_dev - __device_start)); + atomic_set_bit(&dev->pm->atomic_flags, + DEVICE_PM_ATOMIC_FLAGS_BUSY_BIT); #else - ARG_UNUSED(busy_dev); + ARG_UNUSED(dev); #endif } -void device_busy_clear(const struct device *busy_dev) +void device_busy_clear(const struct device *dev) { #ifdef CONFIG_PM_DEVICE - atomic_clear_bit((atomic_t *) __device_busy_start, - (busy_dev - __device_start)); + atomic_clear_bit(&dev->pm->atomic_flags, + DEVICE_PM_ATOMIC_FLAGS_BUSY_BIT); #else - ARG_UNUSED(busy_dev); + ARG_UNUSED(dev); #endif }