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.
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
Attachment:
signature.asc
Description: PGP signature
