Re: [PATCH 5/6] OF: Introduce Device Tree resolve support.

[Grant Likely]

On Tue, 19 Mar 2013 13:51:01 +0200, Pantelis Antoniou <panto@xxxxxxxxxxxxxxxxxxxxxxx> wrote:
> Hi Grant,
> 
> On Mar 16, 2013, at 11:24 AM, Grant Likely wrote:
> 
> > On Wed, 23 Jan 2013 12:58:02 +0200, Pantelis Antoniou <panto@xxxxxxxxxxxxxxxxxxxxxxx> wrote:
> >> Hi David,
> >> 
> >> On Jan 23, 2013, at 6:40 AM, David Gibson wrote:
> >>> Ok.  Nonetheless it's not hard to avoid a recursive approach here.
> >> 
> >> How can I find the maximum phandle value of a subtree without using recursion.
> >> Note that the whole function is just 6 lines long.
> > 
> > It's a failure in the existing kernel DT data structures. We need a hash
> > lookup for the phandles to eliminate the search entirely. Then you'd be
> > able to allocated new phandles on the fly easily and resolve phandles
> > without searching the whole tree (which has always been horrible).
> > 
> 
> Yes, it is pretty obvious that the in-kernel data structures are sub-optimal.
> But I was not after modifying them, since that's a different kind of problem.

Think about it this way; fixing up that aspect of the data structure
makes the job you're trying to do a lot easier. I don't feel bad about
asking you to add a radix tree for phandle lookups when it makes your
patches a whole lot better.  :-)

> Since we're having a 'sub-optimal' data structures, I'd like to point out that
> the usage of of_find_by_name(), mostly by drivers trying to find a child
> of their own node, works by a lucky accident of how the device nodes are instantiated
> by the flat tree loader. Most of the use cases should be replaced by a call
> to of_get_child_by_name() which does the right thing.

It is true. In fact, calling of_find_node_by_name() when using .dtb is
most likely a bug since using node name to determine behaviour is
strongly discouraged.

> Fair enough, but be warned that phandle resolution the overlay feature is mostly useless.
> 
> In actual practice the amount of driver nodes that can be overlaid without a single case
> of referencing phandles outside (or within) their own blob is close to zero.

That's not what I'm saying. I'm saying that (at least for now) we should
require the overlay to already know the phandles from the parent and to
refuse to load an overlay that defines phandles already in use in the
base. Overlays do become usable at that point. A mechanism for phandle
resolution so that conflicts can be found and resolved can be added
as a feature enhancement. By splitting it out you'll be able to get the
overlay feature merged even if we don't have agreement on the resolution
mechanism yet.

g.

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