On Wed, 14 May 2014 16:49:05 +0200, Alexander Holler [off-list ref] wrote:

Am 14.05.2014 16:05, schrieb Grant Likely:

quoted

On Mon, 12 May 2014 18:47:53 +0200, Alexander Holler [off-list ref] wrote:

quoted

Use the properties named 'dependencies' in binary device tree blobs to build
a dependency based initialization order for platform devices and drivers.

This is done by building a directed acyclic graph using an adjacency list
and doing a topological sort to retrieve the order in which devices/drivers
should be created/initialized.

Signed-off-by: Alexander Holler <redacted>

Hi Alexander,

Thanks for looking at this. It is a difficult problem. I've made
comments below, but first I've got some general comments...

First, I'm going to be very cautious about this. It is a complicated
piece of code making the device initialization process a lot more
complicated than it already is. I'm the first one to admit that deferred
probe handles the problem in quite a naive manner, but it is simple,
correct (when drivers support deferred probe) and easy to audit. This
series digs deep into the registration order of *both* devices and
drivers which gives me the heebee jeebees.

Sure, but the approach I present is deterministic. The deferred stuff, 
while it's simple and works, is imho just a workaround. Besides that 
this series isn't about pro or cons of the deferred probe, the deferred 
probes I have seen where just the reason why I had a look at what 
happens. To conclude, I like the deferred probe because it fixes 
problems, but trying to do things right is much better. And there are 
already many workarounds trying fix the initialization order (e.g. 
drivers which classify themself as a subsys), so starting do it right 
makes imho sense.

So, I'm sorry if you see this feature as an attack on the deferred probe 
stuff, it isn't meant as such, no offense here or there.

Hahaha, calm down. I'm not upset nor do I see it as an attack on
deferred probe.... Okay, maybe I do, but I've been asking people to
attack deferred probe for years! It works, but it is by no means optimized.

quoted

Personally, I think the parts of this patch that manipulate the device registration
order is entirely the wrong way to handle it. If anything, I would say
continue to register the devices, even if the dependencies are unmet.

That would just be a removal of 2 lines. I've no problem with that. ;)

quoted

Instead, I would focus on the driver core (drivers/base) to catch
device probe attempts when a known dependency is not met, remember it,
and perform the probe after the other driver shows up. That is also
going to be a complicated bit of code, but it works for every kind of
device, not just platform_devices, and has far less impact on the
platform setup code.

BTW, this has to be able to work at the level of struct device instead
of struct platform_device. There are far more kinds of devices than just
platform_device, and they all have the same problem.

Changing to care for devices instead of just drivers is easy to do.

quoted

Also, may I suggest that the more pieces that you can break this series
up into, the greater chance you'll have of getting a smaller subset
merged earlier if it can be proven to be useful on its own.

Hmm, I don't really care if that will be merged. I have no motivation to 
fight with Linux kernel maintainers and I don't know if I will spend the 
necessary time to do so.

The people you need to convince are Rob, Greg, and me, and you've got my
attention. If I'm convinced, then I can probably convince Greg also.
You've got an interesting approach, and I hope you won't give up on it.

quoted

quoted

+#ifdef CONFIG_OF_DEPENDENCIES
+		if (!of_init_build_order(NULL, NULL))
+			of_init_create_devices(NULL, NULL);
+		else
+			of_init_free_order();

What happens when of_init_build_order() fails? Does the whole system
fall over?

Yes. The only reason it can fail is when there is a cycle, and dtc 
checks (and fails) for that when building the blob (dtb).

quoted

quoted

+#else
  		of_platform_populate(NULL, of_default_bus_match_table,
  					NULL, NULL);
  #endif
+	}
+#endif
+
  	return 0;
  }
  arch_initcall(customize_machine);

@@ -914,7 +924,13 @@ void __init setup_arch(char **cmdline_p)
  		arm_pm_restart = mdesc->restart;

  	unflatten_device_tree();
-
+#ifdef CONFIG_OF_DEPENDENCIES
+	/*
+	 * No alloc used in of_init_build_order(), therefor it would work
+	 * already here too.
+	 */
+	/* of_init_build_order(NULL, NULL); */
+#endif

Stale hunk left in patch?

See here: https://lkml.org/lkml/2014/5/14/102

This are NOT patches meant for final merging!

Understood. No malice is intended. That's just the normal stuff that
comes up in review.

quoted

I would suggest splitting the core graph support into a separate patch
to keep things smaller and to keep the behaviour changes separate from
the support function additions.

Could be done.

quoted

quoted

+
+
+/* Copied from drivers/of/base.c (because it's lockless). */

Copying isn't a good idea. The function will need to be made accessible
to other files in the drivers/of directory.

See above.

quoted

quoted

+int __init of_init_build_order(struct device_node *root,
+			const struct of_device_id *matches)
+{
+	struct device_node *child;
+	int rc = 0;
+
+	root = root ? of_node_get(root) : of_find_node_by_path("/");
+	if (unlikely(!root))
+		return -EINVAL;
+
+	calc_max_phandle();
+	order.old_max_phandle = order.max_phandle;
+
+	for_each_child_of_node(root, child) {
+		rc = add_deps(root, child, matches);
+		if (unlikely(rc))
+			break;
+	}
+
+	of_node_put(root);
+	topological_sort();

Can the sort be performed incrementally? The DT is a dynamic structure
on some platforms. Search for OF_RECONFIG_. There is work in progress to
add overlay support to the device tree so that batches of new nodes can
be added to the tree after userspace has started. The dependency code
will need to handle that situation gracefully.

The stuff I present is only about what happens before userspace starts 
and is all gone away when userspace start. I know about the overlay 
support (e.g. for bbb-capes), but I don't care. So there is no need to 
think about what happens if such happens.

ok. As long as there is no impact, then I don't have an issue.

quoted

I don't like that of_init_create_devices() has a completely different
calling convention from of_platform_populate(). of_platform_populate()
is passed a match table for devices that are to act as buses (which
means register the children also). This function is passed a blacklist
instead which is a completely different semantic.

Acting on buses is a workaround.

Can you elaborate on what you mean? The of_platform_populate() semantics
are quite specific because they are the filter for which devices are
going to be platform_devices.

quoted

That means it cannot be used by device drivers that register their own
children and it has to make a lot of assumptions about what should and
should not be registered as platform_devices.

How does the dependency code decide which devices can be
platform_devices? It's not clear to me from what I've read so far.

Dependencies currently are only considered on stuff which has a 
"compatibility" property, thus drivers. I wanted to get the drivers 
loaded in order, not really caring for devices. Let me quote from 
(outdated) ldd3:

"For the most part, the Linux device model code takes care of all these 
considerations without imposing itself upon driver authors. It sits 
mostly in the background; direct interaction with the device model is 
generally handled by bus-level logic and various other kernel 
subsystems. As a result, many driver authors can ignore the device model 
entirely, and trust it to take care of itself."

So do I. ;)

of_platform_populate() currently makes the decision for
platform_devices. Other busses do the same for their own child devices.
In this case after calculating the dependency information we would want
to make it available to all bus types so that they to can take advantage
of dependency information.

quoted

quoted

+
+	for (i = 0; i < order.count; ++i) {
+		struct device_node *node = order.order[i];
+		uint32_t parent_ph = order.parent_by_phandle[node->phandle];
+
+		if (unlikely(blacklist &&
+				of_match_node(blacklist, node))) {
+			of_node_put(node);
+			continue;
+		}
+		if (unlikely(parent_ph &&
+			!order.device_by_phandle[parent_ph])) {
+			/* init of parent failed */
+			of_node_put(node);
+			continue;
+		}
+		dev = of_dependencies_device_create(node, lookup,
+			order.device_by_phandle[parent_ph]);
+		if (dev)
+			order.device_by_phandle[node->phandle] = &dev->dev;
+		of_node_put(node);
+	}
+	/* remove_new_phandles(); */
+}

I could use some help understanding what is being done here. It looks
like it is going through and only registering devices that have a
dependency parent already created, or don't have a parent at all. Am I
correct?

Yes, that part assumes that if a parent is present, the parent is needed 
and it doesn't make sense to create a device if the parent already 
failed. That are those two lines I mentioned above.

Ah, so it is just the normal node's parent, not a dependency link. okay.

quoted

It looks like this patch alone will break the kernel because it depends
also on the functionality in patch 5. The patches would need to be
reordered to handle that situation.

I currently don't care if this feature breaks something. Therefor it is 
marked in big letters as experimental. But I already see you don't want 
it and you see it all as an offense.

I'm sorry you feel that way. I actually have quite the opposite opinion.
I'm asking the questions and pushing back to a) make sure I understand
what you're doing, and b) figure out wether it can be brought into a
state where it can be merged.

g.