Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] device: allow deferred initialization #64869

Closed
wants to merge 9 commits into from

Conversation

gmarull
Copy link
Member

@gmarull gmarull commented Nov 6, 2023

Introduction

This RFC is an attempt to solve deferred device initialization as described in #39896, by adopting the Option 4.

How it works

  • Any device associated with devicetree node that has status = "disabled" and the zephyr,deferred-init will still be instantiated, but not initialized automatically.
  • Device drivers need to be changed to use the DEVICE_DT_INST_FOREACH_ENABLED instead of DT_INST_FOREACH_STATUS_OKAY if they want to support this new mode. This new macro will expand if the device has status okay or if status is disabled + zephyr,deferred-init. Drivers that are not updated will not support deferred initialization, but they will still work.
  • Applications must call device_init to manually initialize a device later in time (see bf9825d for a functional demo).

Example:

/* application overlay */
&lsm6dso {
	status = "disabled";
	zephyr,deferred-init;
};
/* application code */
const struct device *const dev = DEVICE_DT_GET(DT_NODELABEL(lsm6dso));

ret = device_init(dev);
if (ret < 0) {
	printk("Could not initialize device %s\n", dev->name);
	return 0;
}

Note that because #61986 is not ready, the device init call is duplicated into struct device (we cannot access struct init entry from struct device) so that it can be called later in time. This is all optional via CONFIG_DEVICE_ALLOW_DEFERRED, so applications can save such extra pointer if the feature is not used.

Open points/questions

This change has many subtle implications that need to be discussed and/or considered, some I've found important:

  • Dependencies are not considered in this RFC. An easy solution would be to mandate zephyr,deferred-init to all children nodes so that their initialization is never attempted.
  • okay status is deeply embedded in many places. For example, many drivers use DT_ANY_INST_ON_BUS_STATUS_OKAY to define certain struct fields, or DEVICE_DT_GET_ONE relies on status being okay. This all needs to change, devices should use the concept "enabled" or "instantiated", but not "okay".
  • Applications require changes to support device deferred initialization, that is, they need to call device_init. Maybe we should instead switch to a reference counted model where we have device_get/device_put everywhere, solving at the same time the de-init problem.
  • Some subsystems may fail if devices are not ready, now, they should likely support deferred initialization as well.
  • Analyze driver instantiation, some drivers don't use DT_INST_FOREACH_STATUS_OKAY and do many ugly things (e.g. Nordic)

Add a new property that allows tagging device nodes so that they are
initialized later in Zephyr, i.e., not part of the automatic init
process.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
A compatible is considered as enabled if:

- status = "okay"
- status = "disabled" and "zephyr,deferred-init" property is enabled.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
Add new macros that allow iterating over each disabled node instance.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
Add a new macro that allows to iterate over each disabled node.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
So that even disabled nodes are considered. This is a temporary hack to
make some drivers work.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
This patch allows to initialize devices later (ie, not at boot time) by
setting status = "disabled" and zephyr,deferred-init property in
devicetree. Devices can be initialized later using device_init().

Signed-off-by: Gerard Marull-Paretas <[email protected]>
Drivers should not be compiled if this is true.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
So that device is also instantiated if it is disabled but flagged as
zephyr,deferred-init.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
Show how deferred initialization works.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
@@ -601,12 +601,20 @@ def dt_has_compat(kconf, _, compat):
def dt_compat_enabled(kconf, _, compat):
"""
This function takes a 'compat' and returns "y" if we find a status "okay"
compatible node in the EDT otherwise we return "n"
compatible node, or a status "disabled" + "zephyr,deferred-init" compatible
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure overloading enabled like this (in which disabled is part of the meaning) is helpful. Maybe we should explore another word for this? At #39896 available is also mentioned. I think it's more meaningful.
So we'd have dt_compat_available, DT_INST_FOREACH_STATUS_AVAILABLE, DT_HAS_XXX_AVAILABLE, etc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the "available" term, I think it's more descriptive. However, I'm hesitant to introduce it directly into the DT API because devicetree doesn't have such a concept. It's really a Zephyr thing (it will be strictly tied to the special zephyr,deferred-init property). I don't even like the zephyr,deferred-init property, it exists because Zephyr doesn't have a better compile-time configuration language (Kconfig does not play well with device instances...).

Let's discuss it today because the new terminology will propagate to many places (see 2nd point in open points).

@@ -897,6 +947,8 @@ static inline bool z_impl_device_is_ready(const struct device *dev)
.data = (data_), \
IF_ENABLED(CONFIG_DEVICE_DEPS, (.deps = (deps_),)) /**/ \
IF_ENABLED(CONFIG_PM_DEVICE, (.pm = (pm_),)) /**/ \
IF_ENABLED(CONFIG_DEVICE_ALLOW_DEFERRED, \
(.init_fn = (init_fn_),)) /**/ \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is probably ugly, but you could save this pointer if device_init loops through the device initialisers and runs it when there's a match on the device pointer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's definitely another option, O(N) though, while now it's O(1) at the cost of an extra pointer in every device. In any case, it should end up O(1) if we manage to succeed with #61986

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in favor of avoiding the extra pointer. The number of devices that an app will "defer init" will typically be small, and the performance hit happens once.

@fabiobaltieri
Copy link
Member

Why not keeping the device as okay and just skip init if it's flagged with zephyr,deferred-init so that you could flag any device and not have to modify the individual drivers to support the feature?

@gmarull
Copy link
Member Author

gmarull commented Nov 7, 2023

Why not keeping the device as okay and just skip init if it's flagged with zephyr,deferred-init so that you could flag any device and not have to modify the individual drivers to support the feature?

That's one of the options, probably easier and worth reconsidering. Option 4 was found to stick closer to what devicetree spec says about "okay" and "disabled", but it now has many implications because "okay" is spread everywhere.

@keith-zephyr
Copy link
Contributor

Why not keeping the device as okay and just skip init if it's flagged with zephyr,deferred-init so that you could flag any device and not have to modify the individual drivers to support the feature?

That's one of the options, probably easier and worth reconsidering. Option 4 was found to stick closer to what devicetree spec says about "okay" and "disabled", but it now has many implications because "okay" is spread everywhere.

I agree it's probably the easiest to implement and okay+deferred was offered as Option 1. But this was rejected by a few folks because it further deviates from the devicetree spec.

@carlescufi
Copy link
Member

carlescufi commented Nov 7, 2023

Architecture WG:

  • Option 4 (zephyr, deferred-init property) has several issues, mainly clashing with the semantics of the status property values in the Devicetree spec
  • @gmarull goes over the issues he has found, described in the PR.
  • @henrikbrixandersen asks whether we are OK with going back to Option 1 given that there was controversy last time when we discussed this. @galak would prefer us to bite the bullet and select an option that maps more cleanly to the spec.
  • @galak suggests having the node in the tree, regardless of the status, includes instantiation. Removing a node from instantiation would mean deleteing it from the tree.
  • @keith-zephyr suggest contacting the spec maintainers to introduce a new value for the status property that reflects the use we want to make of it in Zephyr.
  • @MaureenHelm says that the concept of deleting nodes is going to get very messy quickly. These instantiations today rely on the node being there and bein marked as status=okay instead of status=disabled.
  • @henrikbrixandersen mentions that status=okay is also used in Linux. @galak agrees that perhaps Option 1 is the better approach to fixing this
  • General consensus towards Option 1 starts to form

@nashif
Copy link
Member

nashif commented Nov 7, 2023

I agree it's probably the easiest to implement and okay+deferred was offered as Option 1. But this was rejected by a few folks because it further deviates from the devicetree spec.

I think the "operational" part of the spec was confusing, now it makes more sense IMO given what @henrikbrixandersen mentioned above re Linux.

@cfriedt
Copy link
Member

cfriedt commented Nov 29, 2023

Likely a lot of this has been considered already, but..

I wonder if it would be possible to have two separate iterable sections for device init; one for immediate (and deterministic) init, and the other for deferred init.

Deferred init could introduce some nondeterminism (e.g.

  • does init depend on some hotplug event by a human?
  • are all of the dependencies of X already initialized?
  • has this removable flash device burned through its write cycles?
  • is a wire loose / connector worn?

Rather than require all child nodes of a device to support deferred init in one global iterable section, just fail at build time if any device depends on a deferred init in the deterministic section.

It's still possible for devices in the deterministic section to fail init for some similar reasons.

Do we have some Kconfig option such that init fails if all devices were not initialized successfully? I would imagine that the desired behaviour is quite application dependent.

Applications require changes..

I think that is unavoidable for deferred init. And +1 to the refcount suggestion / link to de-init.

@gmarull
Copy link
Member Author

gmarull commented Jan 11, 2024

Closing in favor of #67335

@gmarull gmarull closed this Jan 11, 2024
@gmarull gmarull deleted the deferred-init-rfc branch January 11, 2024 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants