Re: [PATCH v2 1/2] overlays: auto allocate phandles for nodes in base fdt

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 6873 bytes --]

On Tue, Jan 16, 2018 at 09:24:44AM -0600, Kyle Evans wrote:
> On Tue, Jan 16, 2018 at 8:05 AM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Fri, Jan 05, 2018 at 06:47:46PM -0600, Kyle Evans wrote:
> >> On Fri, Jan 5, 2018 at 3:22 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >> > On 01/05/18 13:04, Kyle Evans wrote:
> >> >> On Fri, Jan 5, 2018 at 2:40 PM, Frank Rowand <frowand.list-Re5JQEeQqe8@public.gmane.orgm> wrote:
> >> >>> On 01/04/18 21:47, Kyle Evans wrote:
> >> >>>> On Thu, Jan 4, 2018 at 11:02 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> > [snip]
> >> > Yes, so not a base overlay that has not been compiled by the dtc (in
> >> > this project) with the "-@" flag specified.  Either the __symbols__
> >> > node was hand coded, or a compiler other than dtc was used, which
> >> > chooses to not create a phandle property for a node whose label
> >> > is not de-referenced in the same source file.
> >>
> >> My understanding was that libfdt was not necessarily supposed to be so
> >> tightly coupled to this dtc that it would make such assumptions when
> >> it's otherwise not a malformed blob. Ditto for the comment below about
> >> the use case being described in the patch- I had goofed and not
> >> checked that this particular dtc implementation does it differently,
> >> but I also was of the belief that libfdt was only supposed to be
> >> loosely coupled to the dtc implementation in this same repository.
> >>
> >> Of course, I can't recall/find what I had looked at that gave me this
> >> impression. =)
> >>
> >> > This use case should have been clearly described in the patch
> >> > description.  And David can then choose to accept or reject the
> >> > patch as he sees fit.  I clearly think it is a bad idea, but
> >> > David will either agree or disagree with my opinion.
> >
> > Sorry I've taken so long to reply to all these, I've been super busy.
> >
> >> David: If I may, I would appreciate if you would re-re-consider this
> >> patch set on the following basis:
> >>
> >> 1.) For base blobs compiled by your dtc, this is a no-op. The latest
> >> version does entail one extra tree walk at the beginning of applying
> >> /__fixups__ even if all phandles are assigned, but I've since realized
> >> this could be moved further down to a tree walk the first time it has
> >> to assign a phandle so that it really is a no-op for blobs compiled by
> >> your compiler.
> >>
> >> 2.) For other base blobs, you have compatibility. As of right now, how
> >> phandles are assigned in a base blob is an implementation detail,
> >> given that there is no clear spec on this. You gain nothing from
> >> remaining strict on this, and you lose compatibility with blobs that
> >> aren't blatantly invalid that libfdt can't control as well as
> >> potential (other) users of your library that may value this. Of
> >> course, you wouldn't lose us as users, we'd adapt.
> >>
> >> I consider this one to be fairly important. We had to implement
> >> reading of older formats (see nathanw@'s recent patch to this effect)
> >> because we can't control what we're given in all cases. I fully expect
> >> that even if I'm asked to drop this and we adapt our dtc, we'll
> >> eventually run into a case where this is needed because of
> >> vendor-provided DTS via EFI.
> >>
> >> 3.) There's nothing inherently wrong with what I've done in this
> >> patch, I've just supplemented the arbitrary assignment of phandles by
> >> dtc with arbitrary assignment of phandles by libfdt. There is no
> >> change to how the referenced node is looked up or generated, and this
> >> has no bearing on how anything is actually written.
> >
> > Note that the comments I've made elsewhere in the thread about not
> > having seem strong arguments for your position were written before I
> > got to reading this mail.
> >
> > You've got a reasonably compelling case above - let me mull on it a
> > while.
> >
> >> As an aside, IMO, phandles assigned to nodes are pointless once you
> >> have a symbol table to reference; just a nice thing to have so you
> >> don't *have* to take the performance hit at runtime to assign them.
> >
> > I'm really not sure what you're getting at here.  phandles are a
> > fundamental structural element of device trees going back to the year
> > dot, symbols really can't make them go away.  In particular you can't
> > easily determine if the phandle on a given node is necessary - doing
> > so requires full understanding of *every* binding used anywhere in the
> > tree so that you can interpret every other property to see if they
> > reference the phandle.
> 
> Sorry, my wording is quite inconsistent here: the above should read
> "explicit phandles" as I say in some other e-mails.
> 
> From my perspective, this isn't a particularly hard problem to solve-
> dtc already has to read the entire DTS into its internal
> representation before writing it to another format. It already has
> some understanding of every binding used everywhere in the tree by the
> time it's done. Deferring explicit phandle assignment until after it's
> made a full pass over the DTS and knows what's been referenced
> elsewhere isn't too bad.

That doesn't follow.  dtc only operates more-or-less at the structural
level - it knows what the nodes are and what the properties are at
byte strings, but not how those byte strings are interpreted.  That
means it *doesn't* know about bindings, which tell you how to
interpret each property - whether it consists of addresses, strings,
phandles or whatever else.

dtc gets a *hint* about phandles, if you use the reference syntax, but
it's just a hint - and it's lost after the tree has passed through the
compiler once.

> 
> >> The labels you would otherwise be losing are now stored in the blob's
> >> symbol table, so references are not ambiguous as they would be in a
> >> normal blob compiled without -@.
> >
> > Not sure what you're getting at here.  References aren't _ambiguous_
> > without -@; they simply make no sense - what are you referencing.
> 
> Sorry, I've been dealing with a lot of "the labels match the unit
> name" nodes lately. You're right. =) The point here is that you can
> resolve these references in a DTS compiled with -@, with or without
> phandles being explicitly assigned. They don't offer that much value
> in this case, other than keeping the resolution of the references into
> the compiler.

I don't see what you mean by "keeping the resolution of the references
into the compiler".

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]