On 4/3/19 6:42 PM, Frank Rowand wrote:
> On 4/3/19 6:01 PM, David Gibson wrote:
>> On Wed, Apr 03, 2019 at 01:30:40PM -0700, Frank Rowand wrote:
>>> On 3/25/19 12:42 AM, Thierry Reding wrote:
>>>> On Mon, Mar 25, 2019 at 02:54:49PM +1100, David Gibson wrote:
>>>>> On Fri, Mar 22, 2019 at 04:47:34PM -0700, Frank Rowand wrote:
>>>>>> Hi David,
>>>>>>
>>>>>> On 3/20/19 5:38 PM, David Gibson wrote:
>>>>>>> On Wed, Mar 20, 2019 at 04:10:03PM +0100, Thierry Reding wrote:
>>>>>>>> From: Thierry Reding <treding@xxxxxxxxxx>
>>>>>>>>
>>>>>>>> The new fdt_generate_phandle() function can be used to generate a new,
>>>>>>>> unused phandle given a specific device tree blob. The implementation is
>>>>>>>> somewhat naive in that it simply walks the entire device tree to find
>>>>>>>> the highest phandle value and then returns a phandle value one higher
>>>>>>>> than that. A more clever implementation might try to find holes in the
>>>>>>>> current set of phandle values and fill them. But this implementation is
>>>>>>>> relatively simple and works reliably.
>>>>>>>>
>>>>>>>> Also add a test that validates that phandles generated by this new API
>>>>>>>> are indeed unique.
>>>>>>>>
>>>>>>>> Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
>>>>>>>
>>>>>>> Applied, thanks.
>>>>>>
>>>>>> I would like for you to think of possibly reverting this patch. Or doing
>>>>>> so in about two weeks. I have started discussing with Thierry whether
>>>>>> there is better way of handling the use case. But I am going to be off
>>>>>> grid for a week, so that conversation will be on hold.
>>>>>
>>>>> I have reverted it for now. I'm not really sure I'm convinced by your
>>>>> arguments in the thread (though I'm still reading).
>>>>>
>>>>> But, I'd forgotten that we already had an exposed
>>>>> fdt_get_max_phandle() function. I don't see that the
>>>>> fdt_generate_phandle() function really adds much to that.
>>>>
>>>> While the two are largely similar in functionality, there are two big
>>>> differences. One is that the signature of fdt_get_max_phandle(), while
>>>> it may perhaps be convenient in some cases, doesn't allow you to
>>>> propagate an error code, since it has to condense a multitude of error
>>>> conditions down into 0 or -1. It also returns -1 via an unsigned integer
>>>> which requires all callers to use an explicit cast while checking the
>>>> return value:
>>>>
>>>> phandle = fdt_get_max_phandle(fdt);
>>>> if (phandle == (uint32_t)-1) {
>>>> ...
>>>> }
>>>>
>>>> which is just really annoying. I think the lack of a proper error
>>>> message is much worse, though, because it makes this function
>>>> inconsistent with the rest of the API.
>>>>
>>>> Secondly, fdt_get_max_phandle() has a very narrow scope. It simply
>>>> determines the largest value currently used for a phandle. The use-case
>>>> for this seems to be to compute an offset that is applied to the
>>>> phandles in overlays. The problem is that fdt_get_max_phandle() doesn't
>>>> do any checks on the maximum phandle value (and it really can't because
>>>> it doesn't know what the value will be used for). So every user of the
>>>> function that uses the phandle to compute a new phandle value will have
>>>> to open-code the validity checks.
>>>>
>>>> While fdt_generate_phandle() is very similar in implementation, it is
>>>> also very explicit in what the phandle value will be used for, so it can
>>>> have all the validity checks built in. It can also later on be changed
>>>> to be more clever about how it choses the phandle value.
>>>
>>>
>>>> Consider the
>>>> case where you want to apply an overlay that contains 25 phandles to a
>>>> DTB that happens to have a maximum phandle value of (uint32_t)-10 (this
>>>> is arguably unlikely, but can happen if somebody sets an explicit
>>>> phandle value). Applying the overlay with fdt_get_max_phandle() will not
>>>> work, whereas with a better implementation of fdt_generate_phandle() it
>>>> could be made to work, by reusing phandle values from any holes that may
>>>> exist in the DTB.
>>>
>>> Yes, the issue you point out is real. The current solution from the
>>> Linux kernel perspective is to say "don't do that" (where "that" is
>>> hand-coding a large phandle value in a DTS). A related problem in
>>> the kernel is if we allowed an apply / remove sequence of the form:
>>>
>>> 1) apply overlay A
>>> 2) apply overlay B
>>> 3) remove overlay A
>>> 4) apply overlay A
>>> ...
>>>
>>> The problem is that the Linux kernel uses the same simplistic
>>> approach of allocating a new range of phandles for an applied
>>> overlay that is larger than the current largest phandle. If
>>> the above apply / remove sequence is repeated frequently then
>>> the maximum current phandle value continues to grow without
>>> limit (until it overflows).
>>>
>>> The Linux kernel currently avoids this problem by documenting
>>> that overlays must be removed in the order opposite to how
>>> they were applied.
>>
>> I think this is a bogus argument. Applying an overlay is a
>> destructive, non-reversible operation. Removing them can really only
>> work by keeping the original versions about and replaying the overlay
>> applications.
>
> The Linux kernel does keep information about the state of the live
> devicetree when applying an overlay. Then to remove an overlay
> it returns to the previous state.
Maybe I should also note that these changes are _not_ made to an FDT/DTB.
When the Linux kernel boots, it expands or unpacks the DTB into a live
data structure. When an overlay DTB (or dtbo) is applied, it is
unpacked into a live data structure and the kernel's live data
structure is then modified.
>> Changing that would require considerable changes to have the dtb
>> format works, in which case other things change as well and so the
>> argument's not really relevant any more.
>>
>
>
