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.
--
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:
signature.asc
Description: PGP signature
