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