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: 5127 bytes --]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >>> On 01/04/18 21:47, Kyle Evans wrote:
> >>>> On Thu, Jan 4, 2018 at 11:02 PM, Frank Rowand <frowand.list-Re5JQEeQqe8@public.gmane.orgm> 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.

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

> I have other thoughts about the argument of blobs being able to be
> applied to fdt but not a livetree, but I think they can all be summed
> up as: this is a non-issue if you're expecting all of your base blobs
> to be compiled by this dtc -@ anyways.

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