On Thu, Apr 04, 2019 at 10:10:40AM -0700, Frank Rowand wrote:
> 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)
Ah, so we're retaining B's adjusted phandles, even though A has been
removed. I guess we have to, if we're operating on the live tree.
> 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.
Well, yeah. But if we do that, we can also potentially record the
phandle offset for ever-applied overlays.
>
> 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.
> >>>
> >>
> >
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
Attachment:
signature.asc
Description: PGP signature
