Re: [RFC PATCH 2/9] dt: deps: dependency based device creation

[Alexander Holler]

Am 14.05.2014 16:05, schrieb Grant Likely:
> On Mon, 12 May 2014 18:47:53 +0200, Alexander Holler <holler@xxxxxxxxxxxxx> wrote:
> > 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 <holler@xxxxxxxxxxxxx>
>
> 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.

> 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. ;)

> 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.

> 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.


> > +#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).

> > +#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!


> 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.

> > +
> > +
> > +/* 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.


> > +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.

> 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.

> 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. ;)

> > +
> > +	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.

> 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.

Regards,

Alexander Holler
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/