On Fri, Jan 5, 2018 at 6:47 PM, Kyle Evans <kevans@xxxxxxxxxxx> 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.
>
> 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.
An afterthought to this: if you wouldn't consider it as a standard
behavior, I'd appreciate it if you would indicate possible acceptance
of another iteration that hides the allocation behind an #ifdef
FEATURE_ALLOCATE_PHANDLE or something of the sortr that neither you
nor the Linux folk will ever need to turn on. I don't foresee this
stuff changing enough in the future to actually require maintenance,
given how non-invasive the patch can be.
--
To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
