Message-Id: <20140514200645.15984C4095B@trevor.secretlab.ca>
Date:	Wed, 14 May 2014 21:06:45 +0100
From:	Grant Likely <grant.likely@...aro.org>
To:	Alexander Holler <holler@...oftware.de>,
	linux-kernel@...r.kernel.org
Cc:	devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Russell King <linux@....linux.org.uk>,
	Jon Loeliger <jdl@....com>, Rob Herring <robh+dt@...nel.org>
Subject: Re: [RFC PATCH 2/9] dt: deps: dependency based device creation

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

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

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.

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

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

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

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

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

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

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

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

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

Powered by blists - more mailing lists