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.
> While I can certainly use fdt_get_max_phandle() to achieve the same
> thing given the current state of the DTS files I'm working with and the
> current implementation of DTC (in fact, my first series of patches that
> use this did just that), it's pretty brittle because it is not
> guaranteed to work in the future.
>
> Thierry
>
