Refactor device structures #23407

tbursztyka · 2020-03-11T18:01:13Z

Original struct device was only meant for actual devices. But its infrastructure has been then used also for non-hardware based boot-time critical piece of software. Though this was a simple solution, it brought the device concept to non-device systems.

So refactoring all of this in order to get rid of struct device from these systems, as well as clarifying the struct device as well.

This leads to some bytes saved from both ROM and RAM.

zephyrbot · 2020-03-12T09:35:11Z

Some checks failed. Please fix and resubmit.

checkpatch issues

-:1342: WARNING:LONG_LINE: line over 80 characters
#1342: FILE: drivers/counter/maxim_ds3231.c:1342:
+	Z_OOPS(Z_SYSCALL_SPECIFIC_DRIVER(dev, K_OBJ_DRIVER_COUNTER, &ds3231_api));

-:1351: WARNING:LONG_LINE: line over 80 characters
#1351: FILE: drivers/counter/maxim_ds3231.c:1359:
+	Z_OOPS(Z_SYSCALL_SPECIFIC_DRIVER(dev, K_OBJ_DRIVER_COUNTER, &ds3231_api));

-:10721: ERROR:MULTISTATEMENT_MACRO_USE_DO_WHILE: Macros with multiple statements should be enclosed in a do - while loop
#10721: FILE: include/device.h:98:
+#define DEVICE_AND_API_INIT(dev_name, drv_name, init_fn, data, cfg_info, \
+			    level, prio, api)				\
+	static Z_DECL_ALIGN(struct device)				\
+		_CONCAT(__device_, dev_name) __used			\
+	__attribute__((__section__(".device_" #level STRINGIFY(prio)))) = { \
+		.name = drv_name,					\
+		.config_info = (cfg_info),				\
+		.driver_api = (api),					\
+		.driver_data = (data),					\
+	};								\
+	Z_INIT_ENTRY_DEFINE(_CONCAT(__device_, dev_name), init_fn,	\
+			    (&_CONCAT(__device_, dev_name)), level, prio)

-:10725: WARNING:PREFER_SECTION: __section(.device_) is preferred over __attribute__((section(".device_")))
#10725: FILE: include/device.h:102:
+	__attribute__((__section__(".device_" #level STRINGIFY(prio)))) = { \

-:10781: WARNING:PREFER_SECTION: __section(.device_) is preferred over __attribute__((section(".device_")))
#10781: FILE: include/device.h:156:
+	__attribute__((__section__(".device_" #level STRINGIFY(prio)))) = { \

-:11137: WARNING:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#11137: FILE: include/init.h:104:
+#define Z_INIT_ENTRY_DEFINE(entry_name, init_fn, device, level, prio)	\
+	static const Z_DECL_ALIGN(struct init_entry)			\
+		_CONCAT(__init_, entry_name) __used			\
+	__attribute__((__section__(".init_" #level STRINGIFY(prio)))) = { \
+		.init = (init_fn),					\
+		.dev = (device),					\
+	}

-:11140: WARNING:PREFER_SECTION: __section(.init_) is preferred over __attribute__((section(".init_")))
#11140: FILE: include/init.h:107:
+	__attribute__((__section__(".init_" #level STRINGIFY(prio)))) = { \

-:11282: ERROR:MULTISTATEMENT_MACRO_USE_DO_WHILE: Macros with multiple statements should be enclosed in a do - while loop
#11282: FILE: include/linker/linker-defs.h:65:
+#define CREATE_OBJ_LEVEL(object, level)				\
+		__##object##_##level##_start = .;		\
+		KEEP(*(SORT(.##object##_##level[0-9])));	\
+		KEEP(*(SORT(.##object##_##level[1-9][0-9])));	\
 

-:11282: WARNING:TRAILING_SEMICOLON: macros should not use a trailing semicolon
#11282: FILE: include/linker/linker-defs.h:65:
+#define CREATE_OBJ_LEVEL(object, level)				\
+		__##object##_##level##_start = .;		\
+		KEEP(*(SORT(.##object##_##level[0-9])));	\
+		KEEP(*(SORT(.##object##_##level[1-9][0-9])));	\
 

-:11303: ERROR:MULTISTATEMENT_MACRO_USE_DO_WHILE: Macros with multiple statements should be enclosed in a do - while loop
#11303: FILE: include/linker/linker-defs.h:76:
+#define	INIT_SECTIONS()					\
+		__init_start = .;			\
+		CREATE_OBJ_LEVEL(init, PRE_KERNEL_1)	\
+		CREATE_OBJ_LEVEL(init, PRE_KERNEL_2)	\
+		CREATE_OBJ_LEVEL(init, POST_KERNEL)	\
+		CREATE_OBJ_LEVEL(init, APPLICATION)	\
+		CREATE_OBJ_LEVEL(init, SMP)		\
+		__init_end = .;				\
 

-:11303: WARNING:TRAILING_SEMICOLON: macros should not use a trailing semicolon
#11303: FILE: include/linker/linker-defs.h:76:
+#define	INIT_SECTIONS()					\
+		__init_start = .;			\
+		CREATE_OBJ_LEVEL(init, PRE_KERNEL_1)	\
+		CREATE_OBJ_LEVEL(init, PRE_KERNEL_2)	\
+		CREATE_OBJ_LEVEL(init, POST_KERNEL)	\
+		CREATE_OBJ_LEVEL(init, APPLICATION)	\
+		CREATE_OBJ_LEVEL(init, SMP)		\
+		__init_end = .;				\
 

-:11324: ERROR:MULTISTATEMENT_MACRO_USE_DO_WHILE: Macros with multiple statements should be enclosed in a do - while loop
#11324: FILE: include/linker/linker-defs.h:96:
+#define	DEVICE_SECTIONS()				\
+		__device_start = .;			\
+		CREATE_OBJ_LEVEL(device, PRE_KERNEL_1)	\
+		CREATE_OBJ_LEVEL(device, PRE_KERNEL_2)	\
+		CREATE_OBJ_LEVEL(device, POST_KERNEL)	\
+		CREATE_OBJ_LEVEL(device, APPLICATION)	\
+		CREATE_OBJ_LEVEL(device, SMP)		\
+		__device_end = .;			\
+		DEVICE_BUSY_BITFIELD()			\
+

- total: 4 errors, 8 warnings, 9952 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPDX_LICENSE_TAG SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

tbursztyka · 2020-03-16T11:56:25Z

@nashif no idea why these timeouts are happening (on msp2_an385 and msp2_an521). It work just fine on my machine. Any clue?

mbolivar-nordic

Thanks a lot for sharing this.

I have some questions about the way system initialization is changing. For now I've only thought through the device parts, but I will think a bit more about dependencies that software services have on devices and how that could fit here.

mbolivar-nordic · 2020-03-23T19:04:33Z

include/init.h

+ */
+struct init_entry {
+	int (*init)(struct device *dev);
+	struct device *dev;


Seems like this could be moved into device.h entirely, if it is really device specific. Any point to keep it here?

Because init_entry stuff is not device specific, actually. It's "generic". Note that, if required, in future we could have a union {} around the dev, and a marker about the type on the init. (Though I don't any requirement for that yet. But it's possible)

Then why not just have this be a void *arg and be done with it?

I don't want to do casting on such low level parts. First because it will not give you any gain from memory point of view and is less readable. Plus, next phase (with the device status reporting stuff) will be to "const-ify" all struct device everywhere, with all that implies.

I get the point about "less readable", but casting a struct device * to a struct notdevice * violates MISRA 11.3, which is a required constraint, compared to 11.5 which only advises against casting a void * to a struct device *. So maybe it really is better as:

struct device_init_entry { int (*init)(struct void *p); void *p; };

But I'm not sure if the future constifying makes that irrelevant.

I propose that this issue is resolved by the previous comment block and documentation that a null device pointer field/argument indicates that the init function does not take any arguments.
(Clarification: It takes a null argument, but never a non-null one referencing a non-device object.)

mbolivar-nordic · 2020-03-23T19:06:15Z

include/init.h

+ * Must be one of the following symbols, which are listed in the order
+ * they are performed by the kernel:
+ * \n
+ * \li PRE_KERNEL_1: Used for devices that have no dependencies, such as those


I am still not sure why PRE_KERNEL_1 and PRE_KERNEL_2 are meaningful. They used to be necessary, but now that we have proper dependency tracking in devicetree they do not seem needed. Further, their definitions here are about dependencies, but they don't really seem meaningful to me, since dependencies are a bit nuanced even with\in the SoC: e.g. peripheral devices can depend on clock devices, etc.

Other than keeping legacy code working (which is not an objective of this effort), is there a reason to keep these levels? They seem to me to do more harm than good, but I don't know all the use cases, of course.

This PR is not about changing the levels, so no harm here.

Anyway, note that each level have priority only between 0 to 99. That limits a bit the sorting possibility (devices ordering is made on level, then priority and finally their name). That was basically the reason of splitting the pre kernel level in 2, historically, if I remember well.

This PR is not about changing the levels, so no harm here.

Are you open to the discussion of removing those levels later then? I ask because this PR seems to be specifically about refactoring initialization, and that's what the levels are part of.

No it's refactoring device structures only. Not levels, not the initialization (it's exactly the same process, I did not change anything there).

I would not change anything about the level yet. For the good reason that it is not yet a major issue, compared to the rest. level/prio dts generation is imo of a lesser priority compared to dts device instance generation (because if that's made right, then level/prio generation will be super trivial to do)

Let's proceed step by step (and make sure people go along each of these changes)

Are you open to the discussion of removing those levels later then?

Why do you want to remove them? those are not exclusive to device initialization or setup. A basic feature those levels provide is giving the developer the possibility to run code at any step of the booting process, sometimes you have to do HW initialization that is not managed by a driver or DTS and you need that at early stage. Removing those level will break many application and a useful interface.

Driver initialization priorities need to be improved and should utilise DTS for that, however, this is orthogonal and unrelated IMO. The driver priority happens on top of the existing scheme, does not replace it.

Can you point to a definition for _1 vs. _2 that isn't in terms of device dependencies? Right now that's how they're defined. That's a hack. We have real dependencies from DT now.

This is just a bug in the documentation, nobody bothered to change this and was long there and before device tree. So the fact it talks about devices does not mean this is 1:1 match with devicetree. Right now this is used for drivers, services and hardware initialization of different sorts that is not managed by DT, I am sure some of this can be optimized to be devicetree "devices" but we should not limit this to device tree, there is a need for this in the tree currently and there was always a need, the reason it is there.

I suggest the following change...

--- a/include/device.h +++ b/include/device.h @@ -57,25 +57,31 @@ extern "C" { * Must be one of the following symbols, which are listed in the order * they are performed by the kernel: * \n - * \li PRE_KERNEL_1: Used for devices that have no dependencies, such as those - * that rely solely on hardware present in the processor/SOC. These devices + * \li PRE_KERNEL_1: Used for units that have no dependencies, such as those + * that rely solely on hardware present in the processor/SOC. These units * cannot use any kernel services during configuration, since they are not * yet available. * \n - * \li PRE_KERNEL_2: Used for devices that rely on the initialization of devices - * initialized as part of the PRE_KERNEL_1 level. These devices cannot use any - * kernel services during configuration, since they are not yet available. + * \li PRE_KERNEL_2: Used for units that rely on the initialization of other + * units initialized as part of the PRE_KERNEL_1 level. These units cannot + * use any kernel services during configuration, since they are not yet + * available. * \n - * \li POST_KERNEL: Used for devices that require kernel services during + * \li POST_KERNEL: Used for units that require kernel services during * configuration. * \n - * \li POST_KERNEL_SMP: Used for devices that require kernel services during + * \li POST_KERNEL_SMP: Used for units that require kernel services during * configuration after SMP initialization. * \n * \li APPLICATION: Used for application components (i.e. non-kernel components) - * that need automatic configuration. These devices can use all services + * that need automatic configuration. These units can use all services * provided by the kernel during configuration. * + * Units are the objects that Zephyr knows how to manage. They represent system + * resources that can be initialised and executed based on the level and + * priority defined for each unit. Units in some ways are similar to services + * or jobs in other init systems and additionally include device drivers. + * * @param prio The initialization priority of the device, relative to * other devices of the same initialization level. Specified as an integer * value in the range 0 to 99; lower values indicate earlier initialization.

We have system services via SYS_INIT() which get invoked from PRE_KERNEL phases, and I fail to see how removing this improves the kernel in a significant way.

@anas I did change the doc already, could you check? I did it before you proposal, basically I used "initialization objects" instead of "units".

tbursztyka · 2020-05-07T16:55:29Z

@carlescufi I guess that's ready to be merged any time you want. (I'll rebase if new commits makes it non-mergable"

pabigot · 2020-05-07T20:15:25Z

@tbursztyka @carlescufi The merge of #17631 requires an update to this.

https://github.com/pabigot/zephyr/commits/nordic/pr/23407 has the necessary fixup patch, plus a second DNM patch that has the coccinelle script that will fix other cases if more things get merged before this gets merged.

carlescufi · 2020-05-08T09:31:39Z

@tbursztyka @carlescufi The merge of #17631 requires an update to this.

https://github.com/pabigot/zephyr/commits/nordic/pr/23407 has the necessary fixup patch, plus a second DNM patch that has the coccinelle script that will fix other cases if more things get merged before this gets merged.

Thanks @pabigot. I believe @tbursztyka is off on holiday today, so would it be possible to get this PR merged and then merge the PR (I think you listed the wrong one since you point at this very one) that has the required fixes?

tbursztyka · 2020-05-08T10:48:39Z

@carlescufi Yes it's off here, but I had to login, so I can do it

pabigot · 2020-05-08T14:47:41Z

@tbursztyka Three more patches are necessary due to problems with Z_SYSCALL_SPECIFIC_DRIVER and subsequent changes in master. These are in https://github.com/pabigot/zephyr/commits/nordic/pr/23407

I can apply them and similar patches resulting from further delays if you authorize me to push to your branch---I don't do that without clear prior assent.

pabigot · 2020-05-08T17:21:03Z

include/syscall_handler.h

@@ -453,15 +452,15 @@ static inline int z_obj_validation_check(struct z_object *ko,
 *
 * @param _device Untrusted device pointer
 * @param _dtype Expected kernel object type for the provided device pointer
- * @param _init_fn Expected init function memory address
+ * @param _api Expected driver API structure memory address
 * @return 0 on success, nonzero on failure
 */
 #define Z_SYSCALL_SPECIFIC_DRIVER(_device, _dtype, _init_fn) \


Missed the rename of _init_fn to _api to match use in the macro body.

tbursztyka · 2020-05-08T18:09:52Z

@pabigot can you re-check?

pabigot · 2020-05-08T18:19:23Z

@tbursztyka Yes, this one seems to have everything: maxim test of syscall driver works, no additional changes snuck in.

The whole file is perfectly indented everywhere else. Signed-off-by: Tomasz Bursztyka <[email protected]>

When the device driver model got introduced, there were no concept of SYS_INIT() which can be seen as software service. These were introduced afterwards and reusing the device infrastructure for simplicity. However, it meant to allocate a bit too much for something that only required an initialization function to be called at right time. Thus refactoring the devices structures relevantly: - introducing struct init_entry which is a generic init end-point - struct deviceconfig is removed and struct device owns everything now. - SYS_INIT() generates only a struct init_entry via calling INIT_ENTRY_DEFINE() - DEVICE_AND_API_INIT() generates a struct device and calls INIT_ENTRY_DEFINE() - init objects sections are in ROM - device objects sections are in RAM (but will end up in ROM once they will be 'constified') It also generate a tiny memory gain on both ROM and RAM, which is nice. Perhaps kernel/device.c could be renamed to something more relevant. Signed-off-by: Tomasz Bursztyka <[email protected]>

Since struct devconfig was merged earlier into struct device, let's fix accessing config_info, name, ... attributes everywhere via: grep -rlZ 'dev->config->' | xargs -0 sed -i 's/dev->config->/dev->/g' Signed-off-by: Tomasz Bursztyka <[email protected]>

device_api attribute is not at offset 4 but 8 now as name and config_info has been directly imported into struct device. Signed-off-by: Tomasz Bursztyka <[email protected]>

…ring init_fn is not anymore part of struct device, so let's test instead the driver's API structure pointer which is also unique per device driver. Signed-off-by: Tomasz Bursztyka <[email protected]>

s/init.h/device.h Signed-off-by: Tomasz Bursztyka <[email protected]>

The '_' is not necessary, plus it makes the sys init object name aligning with all others. Signed-off-by: Tomasz Bursztyka <[email protected]>

tbursztyka force-pushed the device_shrink branch from 271be45 to ee00b5d Compare March 12, 2020 09:32

tbursztyka force-pushed the device_shrink branch 7 times, most recently from 855c639 to b7e4ea0 Compare March 16, 2020 08:02

tbursztyka force-pushed the device_shrink branch 6 times, most recently from e5deb1d to 55fe2a4 Compare March 23, 2020 08:20

tbursztyka mentioned this pull request Mar 23, 2020

Refining Zephyr's Device Driver Model - Take II #22941

Open

13 tasks

tbursztyka changed the title ~~[DNM][do not review]test on devices~~ [DNM][RFC] Refactor device structures Mar 23, 2020

mbolivar-nordic self-requested a review March 23, 2020 18:34

tbursztyka force-pushed the device_shrink branch 2 times, most recently from 16b177a to 2dd22aa Compare March 23, 2020 18:50

mbolivar-nordic reviewed Mar 23, 2020

View reviewed changes

tbursztyka force-pushed the device_shrink branch 7 times, most recently from ca41a6e to 9bab271 Compare March 25, 2020 13:18

tbursztyka changed the title ~~[DNM][RFC] Refactor device structures~~ [RFC] Refactor device structures Mar 25, 2020

tbursztyka force-pushed the device_shrink branch from 8872b92 to e25471e Compare May 7, 2020 06:08

tbursztyka requested review from jhedberg, joerchan and Vudentz as code owners May 7, 2020 06:08

tbursztyka force-pushed the device_shrink branch from e25471e to 8edc8a6 Compare May 7, 2020 12:53

tbursztyka force-pushed the device_shrink branch from 8edc8a6 to cc3286c Compare May 8, 2020 10:50

tbursztyka force-pushed the device_shrink branch 2 times, most recently from eacf14b to 891dc84 Compare May 8, 2020 16:53

pabigot requested changes May 8, 2020

View reviewed changes

tbursztyka force-pushed the device_shrink branch from 891dc84 to c62368e Compare May 8, 2020 18:06

pabigot approved these changes May 8, 2020

View reviewed changes

Tomasz Bursztyka added 8 commits May 8, 2020 21:03

device: Fix tiny indentation issue

9aade5b

The whole file is perfectly indented everywhere else. Signed-off-by: Tomasz Bursztyka <[email protected]>

init: Fix tiny indentation issue

402b146

The whole file is perfectly indented everywhere else. Signed-off-by: Tomasz Bursztyka <[email protected]>

scripts: Fix gen_kobject_list.py to fit with device refactoring

4817771

device_api attribute is not at offset 4 but 8 now as name and config_info has been directly imported into struct device. Signed-off-by: Tomasz Bursztyka <[email protected]>

usermode: Rework Z_SYSCALL_SPECIFIC_DRIVER to fit with device refacto…

2a2ffc6

…ring init_fn is not anymore part of struct device, so let's test instead the driver's API structure pointer which is also unique per device driver. Signed-off-by: Tomasz Bursztyka <[email protected]>

drivers/interrupt_controller: Fix loapic/ioapic header inclusion

bdfbab6

s/init.h/device.h Signed-off-by: Tomasz Bursztyka <[email protected]>

drivers/interrupt_controller: Rename ioapic init function

7798173

The '_' is not necessary, plus it makes the sys init object name aligning with all others. Signed-off-by: Tomasz Bursztyka <[email protected]>

tbursztyka force-pushed the device_shrink branch from c62368e to 7798173 Compare May 8, 2020 19:03

carlescufi merged commit c0ab0de into zephyrproject-rtos:master May 8, 2020

pabigot mentioned this pull request Jun 9, 2020

refactor struct device to reduce RAM demands #26072

Closed

tbursztyka deleted the device_shrink branch June 11, 2020 08:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor device structures #23407

Refactor device structures #23407

tbursztyka commented Mar 11, 2020 •

edited by pabigot

Loading

zephyrbot commented Mar 12, 2020 •

edited

Loading

tbursztyka commented Mar 16, 2020

mbolivar-nordic left a comment

mbolivar-nordic Mar 23, 2020

tbursztyka Mar 23, 2020

mbolivar-nordic Mar 23, 2020

tbursztyka Mar 23, 2020

pabigot Mar 26, 2020 •

edited

Loading

pabigot Apr 27, 2020 •

edited

Loading

mbolivar-nordic Mar 23, 2020 •

edited

Loading

tbursztyka Mar 23, 2020

mbolivar-nordic Mar 23, 2020

tbursztyka Mar 24, 2020

nashif Mar 25, 2020

mbolivar-nordic Mar 25, 2020

nashif Mar 26, 2020

andrewboie Mar 26, 2020

tbursztyka Mar 27, 2020

tbursztyka commented May 7, 2020

pabigot commented May 7, 2020

carlescufi commented May 8, 2020

tbursztyka commented May 8, 2020

pabigot commented May 8, 2020

pabigot May 8, 2020 •

edited

Loading

tbursztyka commented May 8, 2020

pabigot commented May 8, 2020

Refactor device structures #23407

Refactor device structures #23407

Conversation

tbursztyka commented Mar 11, 2020 • edited by pabigot Loading

zephyrbot commented Mar 12, 2020 • edited Loading

checkpatch issues

tbursztyka commented Mar 16, 2020

mbolivar-nordic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pabigot Mar 26, 2020 • edited Loading

Choose a reason for hiding this comment

pabigot Apr 27, 2020 • edited Loading

Choose a reason for hiding this comment

mbolivar-nordic Mar 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbursztyka commented May 7, 2020

pabigot commented May 7, 2020

carlescufi commented May 8, 2020

tbursztyka commented May 8, 2020

pabigot commented May 8, 2020

pabigot May 8, 2020 • edited Loading

Choose a reason for hiding this comment

tbursztyka commented May 8, 2020

pabigot commented May 8, 2020

tbursztyka commented Mar 11, 2020 •

edited by pabigot

Loading

zephyrbot commented Mar 12, 2020 •

edited

Loading

pabigot Mar 26, 2020 •

edited

Loading

pabigot Apr 27, 2020 •

edited

Loading

mbolivar-nordic Mar 23, 2020 •

edited

Loading

pabigot May 8, 2020 •

edited

Loading