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