On Thu, Mar 28, 2019 at 10:06 PM David Gibson
<david@xxxxxxxxxxxxxxxxxxxxx> wrote:
>
> Ugh, sorry, I forgot about this for more than a year, as you can tell.
>
> 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@xxxxxxxxx> wrote:
> > > On 01/05/18 13:04, Kyle Evans wrote:
> > >> On Fri, Jan 5, 2018 at 2:40 PM, Frank Rowand <frowand.list@xxxxxxxxx> wrote:
> > >>> On 01/04/18 21:47, Kyle Evans wrote:
> > >>>> On Thu, Jan 4, 2018 at 11:02 PM, Frank Rowand <frowand.list@xxxxxxxxx> wrote:
> > >>>> Your implementation knows what my overlay means otherwise because I
> > >>>> reference a labelled node using &foo, dtc generated a /__fixup__ for
> > >>>> it, your implementation takes that /__fixup__ and does the lookup in
> > >>>> the symbol table. The symbol exists and points to a node, what's the
> > >>>> point of rejecting it there?>
> > >>>> It seems unreasonable to me, because you cannot always control the
> > >>>> source of your live tree. In many cases, sure, it's generated in-tree
> > >>>> or in U-Boot and passed to you, and you can reasonably expect you
> > >>>> won't encounter this. What if you have vendor-provided tree?
> > >>>>
> > >>>> I think the point I'm getting at is that it seems like this will have
> > >>>> to change at some point anyways simply because you can't control all
> > >>>> sources of your devicetree, and this isn't strictly wrong. Telling
> > >>>> someone "No, we can't apply that overlay because your vendor's
> > >>>> internal tool for generating dts and $other_format_used_internally
> > >>>> simultaneously didn't generate a phandle for that" is kind of hard,> especially when your reasoning ISN'T "their blob is malformed" or
> > >>>> "your overlay's reference is ambiguous" but rather, "we know what
> > >>>> that's pointing at, but we just don't generate handles."
> > >>>
> > >>> No, the blob _is_ malformed. I know there is no official binding
> > >>> document for overlays (we do need such things once we figure out
> > >>> what we are doing), so this comment is purely my opinion.
> > >>>
> > >>> The blob is malformed because it was not compiled with the "-@"
> > >>> flag. Mostly not because of anything in the source, although
> > >>> again the __symbols__ node should not be hand coded.
> > >>
> > >> I know this is only tangential to the point you're making above, but
> > >> the __symbols__ node in this specific example you're referencing was
> > >> hand coded by someone else, probably to avoid having to write
> > >> different invocations of DTC for the different test cases. I'm only
> > >> responsible for removing one line of this .dts, the phandle
> > >> assignment.
> > >>
> > >>> Here is an analogy: the overlay metadata is conceptually similar
> > >>> to the symbol table in an object file compiled from C source code.
> > >>> The linker will use the symbol table to link a second object file
> > >>> that contains references to the first object file, resulting in
> > >>> a program object file. It is not normal to hand code the symbol
> > >>> table in the object file. Similarly dtc creates the __symbols__
> > >>> node, which is essentially a symbol table. The Linux kernel
> > >>> (or a bootloader, or whatever) performs the linking role as
> > >>> part of applying on overlay devicetree to a base devicetree.
> > >>
> > >> There is no hand coding of /__symbols__ nodes anywhere, except for
> > >> this test case that was written by someone else. I think your analogy
> > >> is inaccurate for the actual situation, though.
> > >>
> > >> The linker has all of the metadata it needs to properly link, but
> > >> refuses to for dubious reasons. It can do the lookup in the symbol
> > >> table, and that symbol table points to a valid segment of code
> > >> ("properties and subnodes"), but is refusing to complete the link
> > >> based on a missing property that's arbitrarily assigned by someone
> > >> else anyways and cannot be relied on to have any meaning or special
> > >> value.
> > >>
> > >> To be perfectly clear here: we're talking about taking a blob compiled
> > >> from a sensible overlay, such as [1], and applying it to a fellow
> > >> sensible blob that has been generated with a proper /__symbols__ node
> > >
> > >> and phandles assigned only to nodes within its tree that have been
> > >> cross-referenced by other nodes within its tree.
> > >
> > > 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.
>
> You have a point.
>
> > 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.
> >
> > 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.
> >
> > 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.
> > 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 -@.
> >
> > 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.
>
> I am (belatedly) convinced by your arguments. If you care to rebase
> your patches (note that I've just merged a phandle generation function
> that would probably simplify your code a bit), making the improvements
> you describe above, I'd be happy to apply it.
>
Well, in the year that's passed I still haven't encountered a reak
case of firmware generating insufficient phandles to apply overlays
on, so I think it's safe to say that my concerns (while certainly
possible and not all that crazy) are not really an issue in practice
and I have no reason to revive this at this time. =) I do appreciate
the follow-up, though.
Thanks!
Kyle Evans
