init: remove the need for a dummy device pointer in SYS_INIT functions #51217

gmarull · 2022-10-12T13:40:20Z

NOTE: This is a breaking change! All SYS_INIT functions will need
to be converted to the new signature. See the script offered in c5c637c.

The init infrastructure, found in init.h, is currently used by:

SYS_INIT: to call functions before main
DEVICE_*: to initialize devices

They are all sorted according to an initialization level + a priority.
SYS_INIT calls are really orthogonal to devices, however, the required
function signature requires a const struct device *dev as a first
argument. The only reason for that is because the same init machinery is
used by devices, so we have something like:

struct init_entry {
	int (*init)(const struct device *dev);
	/* only set by DEVICE_*, otherwise NULL */
	const struct device *dev;
}

As a result, we end up with such weird/ugly pattern:

static int my_init(const struct device *dev)
{
	/* always NULL! add ARG_UNUSED to avoid compiler warning */
	ARG_UNUSED(dev);
	...
}

This is really a result of poor internals isolation. This patch proposes
a to make init entries more flexible so that they can accept sytem
initialization calls like this:

static int my_init(void)
{
	...
}

This is achieved using a union:

union init_function {
	/* for SYS_INIT, used when init_entry.dev == NULL */
	int (*sys)(void);
	/* for DEVICE*, used when init_entry.dev != NULL */
	int (*dev)(const struct device *dev);
};

struct init_entry {
	/* stores init function (either for SYS_INIT or DEVICE*) */
	union init_function init_fn;
	/* stores device pointer for DEVICE*, NULL for SYS_INIT. Allows
	 * to know which union entry to call.
	 */
	const struct device *dev;
}

This solution does not increase ROM usage, and allows to offer clean
public APIs for both SYS_INIT and DEVICE*. Note that however, init
machinery keeps a coupling with devices.

NOTE: This is a breaking change! All SYS_INIT functions will need
to be converted to the new signature. See the script offered in c5c637c.

fabiobaltieri

Drop RFC from the PR title?

marc-hb · 2023-04-12T19:44:43Z

Since this commit SOF compilation with -Werror fails as below.

For exact reproduction steps stay tuned.

In file included from /zep_workspace/sof/zephyr/include/rtos/init.h:9, 
448 from /zep_workspace/sof/src/init/init.c:13: 
449/zep_workspace/zephyr/include/zephyr/init.h:156:44: error: initialization of
'int (*)(void)' from incompatible pointer type 'int (*)(const struct device *)' [-Werror=incompatible-pointer-types] 
450 156 | .init_fn = {.sys = (init_fn_)}, \ 
451 | ^ 
452/zep_workspace/zephyr/include/zephyr/init.h:137:9: note: in expansion of macro 'SYS_INIT_NAMED' 
453 137 | SYS_INIT_NAMED(init_fn, init_fn, level, prio) 
454 | ^~~~~~~~~~~~~~ 
455/zep_workspace/sof/src/init/init.c:350:1: note: in expansion of macro 'SYS_INIT' 
456 350 | SYS_INIT(sof_init, POST_KERNEL, 99); 
457 | ^~~~~~~~ 
458/zep_workspace/zephyr/include/zephyr/init.h:156:44: note: (near initialization for '__init_sof_init.init_fn.sys') 
459 156 | .init_fn = {.sys = (init_fn_)}, \ 
460 | ^ 
461/zep_workspace/zephyr/include/zephyr/init.h:137:9: note: in expansion of macro 'SYS_INIT_NAMED' 
462 137 | SYS_INIT_NAMED(init_fn, init_fn, level, prio) 
463 | ^~~~~~~~~~~~~~ 
464/zep_workspace/sof/src/init/init.c:350:1: note: in expansion of macro 'SYS_INIT' 
465 350 | SYS_INIT(sof_init, POST_KERNEL, 99); 
466 | ^~~~~~~~ 
467cc1: all warnings being treated as errors

marc-hb · 2023-04-12T19:51:55Z

For exact reproduction steps stay tuned.

OK that was really easy, nothing special required:

west build -b intel_adsp_cavs25 ../modules/audio/sof/app -- -DEXTRA_CFLAGS=-Werror

zephyr/include/zephyr/init.h:156:44: error: initialization of 'int (*)(void)' from incompatible pointer type 'int (*)(const struct device *)' [-Werror=incompatible-pointer-types]
  156 |                         .init_fn = {.sys = (init_fn_)},

Yet https://github.com/zephyrproject-rtos/zephyr/actions/runs/4678432008/jobs/8288559016 did not care.

Twister used to compile with -Werror, I know because a long time ago I added it to CMAKE_AFLAGS in 8a603da. I don't know when this was lost but it was a pretty big loss IMHO.

Twister also failed to catch this regression: #56701 (demoed again in #56791)

cc: @nashif, @galak

marc-hb · 2023-04-12T19:57:13Z

Just spotted this sorry "NOTE: This is a breaking change! All SYS_INIT functions will need..."

The 267 comments put me off.

Still, dropping -Werror is not the solution to "support" backwards incompatible changes in CI.

nashif · 2023-04-12T22:46:02Z

Since this commit SOF compilation with -Werror fails as below.

there is a PR submitted to SOF to deal with this.

nashif · 2023-04-12T22:48:14Z

Twister also failed to catch this regression: #56701 (demoed again in #56791)

catch it where? File a bug please instead of commenting in merged PRs

dbaluta · 2023-04-13T11:25:08Z

@gmarull any idea why SOF build are failing? I think this is related to your change.

https://github.com/thesofproject/sof/actions/runs/4686511690/jobs/8304685904?pr=7429

marc-hb · 2023-04-13T16:40:23Z

@dbaluta for SOF the API change was submitted a long time ago by @gmarull here:

zephyr: update SYS_INIT calls thesofproject/sof#6480

Next time a similar situation arises, corresponding module changes like this one will hopefully be present in the module branch used by Zephyr first (and required by Zephyr CI). Among others this should preserve bisectability on both sides (as long as west update is invoked at every step)

[PR#51217](zephyrproject-rtos#51217) changed the `init_entry` structure, but `Z_INIT_ENTRY_NAME` (called by `SYS_INIT` or `SYS_INIT_NAMED`) does not initialize all of the members of the struct, leading to errors when building with `-Werror=missing-field-initializers`. Change the macro to initialize the `dev` member to `NULL` so that all members of the struct are initialized. Signed-off-by: Paul Fagerburg <[email protected]>

gmarull · 2023-04-13T16:48:38Z

@dbaluta for SOF the API change was submitted a long time ago by @gmarull here:

zephyr: update SYS_INIT calls thesofproject/sof#6480

Next time a similar situation arises, corresponding module changes like this one will hopefully be present in the module branch used by Zephyr first (and required by Zephyr CI). Among others this should preserve bisectability on both sides (as long as west update is invoked at every step)

As a breaking change, the update needs to happen in multiple steps here. First in Zephyr fork, then upstream, then sync Zephyr fork. I don't see any better way if we allow Zephyr glue code in modules that have an upstream counterpart and that are tested in Zephyr CI.

[PR#51217](#51217) changed the `init_entry` structure, but `Z_INIT_ENTRY_NAME` (called by `SYS_INIT` or `SYS_INIT_NAMED`) does not initialize all of the members of the struct, leading to errors when building with `-Werror=missing-field-initializers`. Change the macro to initialize the `dev` member to `NULL` so that all members of the struct are initialized. Signed-off-by: Paul Fagerburg <[email protected]>

The upstream change zephyrproject-rtos/zephyr#51217 modified the function signature for the SYS_INIT call, eliminating the unused device pointer. Run the Zephyr provided script ./zephyr/scripts/utils/migrate_sys_init.py to update all callers. BUG=b:278595739 BRANCH=none TEST=zmake build -a Cq-Depend: chromium:4422804 Change-Id: I881bc536cef43a7c3ac4bc5eb3ce1893237bbd1f Signed-off-by: Keith Short <[email protected]> Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/4437049 Reviewed-by: Tristan Honscheid <[email protected]>

[PR#51217](zephyrproject-rtos/zephyr#51217) changed the `init_entry` structure, but `Z_INIT_ENTRY_NAME` (called by `SYS_INIT` or `SYS_INIT_NAMED`) does not initialize all of the members of the struct, leading to errors when building with `-Werror=missing-field-initializers`. Change the macro to initialize the `dev` member to `NULL` so that all members of the struct are initialized. (cherry picked from commit 04611a5) Original-Signed-off-by: Paul Fagerburg <[email protected]> GitOrigin-RevId: 04611a5 Change-Id: Id6fb67679e514fc6ca7139a7eecf461fc344df3e Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/zephyr/+/4427441 Commit-Queue: Keith Short <[email protected]> Tested-by: CopyBot Service Account <[email protected]> Reviewed-by: Keith Short <[email protected]> Tested-by: Keith Short <[email protected]>

gmarull added RFC Request For Comments: want input from the community treewide 🧹 labels Oct 12, 2022

gmarull requested review from carlescufi, carlocaione, henrikbrixandersen, stephanosio and teburd October 12, 2022 13:40

gmarull requested review from anangl, andyross, dcpleung, jakub-uC, nashif, nordic-krch and tbursztyka as code owners October 12, 2022 13:40

zephyrbot added area: Kernel area: Logging area: Shell Shell subsystem platform: nRF Nordic nRFx area: Device Model area: Console area: Timer Timer labels Oct 12, 2022

zephyrbot requested review from KangJianX, aasthagr, ceolin, chen-png, peter-mitsis and pfalcon and removed request for anangl, dcpleung and jakub-uC October 12, 2022 13:41

gmarull requested a review from henrikbrixandersen April 12, 2023 12:46

nordicjm approved these changes Apr 12, 2023

View reviewed changes

henrikbrixandersen approved these changes Apr 12, 2023

View reviewed changes

fabiobaltieri approved these changes Apr 12, 2023

View reviewed changes

gmarull changed the title ~~[RFC] init: remove the need for a dummy device pointer in SYS_INIT functions~~ init: remove the need for a dummy device pointer in SYS_INIT functions Apr 12, 2023

carlocaione approved these changes Apr 12, 2023

View reviewed changes

fabiobaltieri merged commit c1a46e2 into zephyrproject-rtos:main Apr 12, 2023

gmarull deleted the sys-init-generic branch April 12, 2023 14:28

ElectricalPaul mentioned this pull request Apr 13, 2023

init: set init_entry.dev to NULL #56821

Merged

FRASTM mentioned this pull request Apr 19, 2023

tests/drivers/counter/counter_basic_api for rtc and timer counters #56466

Merged

mbolivar-nordic mentioned this pull request Apr 19, 2023

Upmerge 2023.04.17 nrfconnect/sdk-nrf#10876

Merged

20 tasks

ycsin mentioned this pull request Jul 20, 2023

RFC: include version.h in the build system #60651

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

init: remove the need for a dummy device pointer in SYS_INIT functions #51217

init: remove the need for a dummy device pointer in SYS_INIT functions #51217

Uh oh!

gmarull commented Oct 12, 2022 •

edited by marc-hb

Loading

Uh oh!

fabiobaltieri left a comment

Uh oh!

marc-hb commented Apr 12, 2023 •

edited

Loading

Uh oh!

marc-hb commented Apr 12, 2023 •

edited

Loading

Uh oh!

marc-hb commented Apr 12, 2023

Uh oh!

nashif commented Apr 12, 2023 •

edited

Loading

Uh oh!

nashif commented Apr 12, 2023

Uh oh!

dbaluta commented Apr 13, 2023

Uh oh!

marc-hb commented Apr 13, 2023

Uh oh!

gmarull commented Apr 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

33 participants

init: remove the need for a dummy device pointer in SYS_INIT functions #51217

init: remove the need for a dummy device pointer in SYS_INIT functions #51217

Uh oh!

Conversation

gmarull commented Oct 12, 2022 • edited by marc-hb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fabiobaltieri left a comment

Choose a reason for hiding this comment

Uh oh!

marc-hb commented Apr 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marc-hb commented Apr 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marc-hb commented Apr 12, 2023

Uh oh!

nashif commented Apr 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nashif commented Apr 12, 2023

Uh oh!

dbaluta commented Apr 13, 2023

Uh oh!

marc-hb commented Apr 13, 2023

Uh oh!

gmarull commented Apr 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

33 participants

gmarull commented Oct 12, 2022 •

edited by marc-hb

Loading

marc-hb commented Apr 12, 2023 •

edited

Loading

marc-hb commented Apr 12, 2023 •

edited

Loading

nashif commented Apr 12, 2023 •

edited

Loading