On 4/3/19 8:16 PM, David Gibson wrote:
> On Wed, Apr 03, 2019 at 06:42:52PM -0700, 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.
>
> Right, and if you're doing that, all phandle fixups will be reapplied
> later, so the problem doesn't arise.
I wasn't very detailed in my scenario.
For simplicity of example, I will make up some numbers.
After boot, maximum phandle == 50
Overlay A contains 10 local (new) phandles
Overlay B contains 20 local (new) phandles
Assume that when the Linux kernel applies an overlay, it uses
a range starting at maximum current phandle + 1. (It actually
leaves a gap of 1, but that is implementation detail, not
architecture.)
1) apply overlay A
max phandle == 60
2) apply overlay B
max phandle == 80
3) remove overlay A
max phandle == 80
(the range of 51 - 60 is now unused)
4) apply overlay A
max phandle == 90
(the overlay is applied freshly from the fdto,
no history from the first time overlay A was
applied is retained)
The problem could occur __if__ the Linux kernel rules for removing
overlays became more liberal, allowing removal in an order different
than last applied first removed.
This is a bit off topic in terms of the patch. I was just agreeing
with Thierry that a large phandle value that is hand coded could
be problematic. I also agree with him that the patch's current
method of allocating new phandles with values above the current
maximum is a reasonable method instead of trying to find holes
in the allocated phandle range.
>>> 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.
>>>
>>
>
