On Mon, Mar 25, 2019 at 08:42:51AM +0100, 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.
Hm, ok, those are fair points.
However given that these are similar functionality we should implement
one in terms of the other rather than having two parallel
implementations. Well, that's not quite possible, but we should be
able to implement them both in terms of a common (non exported)
helper. The helper should be as close as possible to the
fdt_generate_phandle() approach - basically implementing the old
crufty function in terms of the new nicer one.
At the very least that will make it clear that the two interfaces
exist because of history rather than mere carelessness.
> 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.
>
> 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
--
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
