Re: [RFC PATCH v2 08/74] livetree: Improve get_node_by_phandle()

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 8664 bytes --]

On Fri, Oct 23, 2026 at 06:15:33PM +0200, Herve Codina wrote:
> Hi David,
> 
> On Fri, 23 Oct 2026 14:25:29 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Wed, Aug 26, 2026 at 11:48:37AM +0200, Herve Codina wrote:
> > > get_node_by_phandle() allows to get a node based on its phandle value.
> > > It checks the phandle value against value available in internal node
> > > structure.
> > > 
> > > This internal phandle value is updated during process_check() and so,
> > > get_node_by_phandle() cannot give correct results before the
> > > process_check() call.
> > > 
> > > Improve get_node_by_phandle() to look at node phandle properties when
> > > the internal phandle value is not valid.
> > > 
> > > This allows to return a correct matching node even if process_check()
> > > was not called yet.  
> > 
> > The idea is sound - it's always been kind of clunky that we need the
> > process_checks() pass to fill the phandles before we can use them.
> > But, there's at least one significant catch:
> > 
> > [Aside: you might notice the way it's done violates the principle I
> >  stated elsewhere of avoiding structures where you must know where you
> >  are in the flow to know which parts are populated.  My type
> >  discipline has improved since I first wrote this code :)]
> > 
> > We need to gracefully handle the case of malformed phandle /
> > linux,phandle properties; that's actually the reason for the weird
> > placement of this in checks.c.  By treating it as a "check" we handle
> > this case by failing the check.  Other checks that rely on phandles
> > then have the 'explicit_phandles' check as a prereq so they don't need
> > to verify this again.  We can still report errors for other checks
> > that don't rely on that (which is the whole point of the checks
> > mechanism).
> > 
> > That's not the only way to do it, of course, but we need something to
> > check for that error case.  Further notes on the code.
> > 
> > > With the recently introduced FDT_PROPDATA_HANDLE dtb tag, this will be
> > > needed to update internal phandle references before the call to
> > > process_check(). Indeed, this tag allows to identify phandles and
> > > internal references need to be updated based on the phandle value before
> > > the process_check() call.
> > > 
> > > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > > ---
> > >  livetree.c | 21 ++++++++++++++++++++-
> > >  1 file changed, 20 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/livetree.c b/livetree.c
> > > index abe73de1..a9f8c5d2 100644
> > > --- a/livetree.c
> > > +++ b/livetree.c
> > > @@ -628,16 +628,35 @@ struct node *get_node_by_label(struct node *tree, const char *label)
> > >  	return NULL;
> > >  }
> > >  
> > > +static cell_t get_node_phandle_existing(struct node *node)
> > > +{
> > > +	struct property *prop;
> > > +
> > > +	if (phandle_is_valid(node->phandle))
> > > +		return node->phandle;
> > > +
> > > +	prop = get_property(node, "phandle");
> > > +	if (!prop) {
> > > +		prop = get_property(node, "linux,phandle");
> > > +		if (!prop)
> > > +			return 0;
> > > +	}
> > > +
> > > +	return propval_cell(prop);  
> > 
> > In the case of a malformed property, this could SEGV.
> 
> Ok, will update to
> --- 8< ---
> 	if (prop->val.len != sizeof(cell_t))
> 		return 0;
> 
> 	return propval_cell(prop)
> --- 8< ---
> 
> > 
> > > +}
> > > +
> > >  struct node *get_node_by_phandle(struct node *tree, cell_t phandle)
> > >  {
> > >  	struct node *child, *node;
> > > +	cell_t tree_phandle;
> > >  
> > >  	if (!phandle_is_valid(phandle)) {
> > >  		assert(generate_fixups);
> > >  		return NULL;
> > >  	}
> > >  
> > > -	if (tree->phandle == phandle) {
> > > +	tree_phandle = get_node_phandle_existing(tree);  
> > 
> > This means that every time we search for a phandle, for every node
> > scanned before the destination with no phandle, we do a full property
> > scan to check that no, it still doesn't have a phandle.
> > 
> > I don't care that much about dtc performance, but that's still a bit
> > much to swallow.  I think we need to cache negative lookups to avoid
> > that.  Happily, since there are two invalid phandle values this
> > shouldn't be too hard.  We could, say, use tree->phandle == 0 to mean
> > we don't know if there's a phandle, and tree->phandle == -1 to mean
> > we've scanned the properties and we know there isn't one.
> > 
> > In fact, caching property lookups might be the better way to do this.
> > Rather than having an explicit pass where tree->phandle is populated,
> > we could have get_node_by_phandle() cache its property scan (success
> > or failure) in tree->phandle.
> > 
> > I think that's a more elegant approach than the current one, but we do
> > still need to handle the malformed property case.
> 
> Well, get_node_phandle_existing() can be improved:
> ---- 8< ----
> static cell_t get_node_phandle_existing(struct node *node)
> {
> 	cell_t phandle1, phandle2;
> 	struct property *prop1, *prop2;
> 
> 	if (phandle_is_valid(node->phandle))
> 		return node->phandle;
> 	if (node->phandle == ~0UL) {
> 		/*
> 		 * We've already looked at existing properties and no valid
> 		 * phandle value were present. Avoid looking at them again
> 		 * and just return the invalid phandle value.
> 		 */
> 		return 0;
> 	}
> 
> 	/* Try to get it from the 'phandle' property */
> 	phandle1 = 0; /* Assume invalid */
> 	prop1 = get_property(node, "phandle");
> 	if (prop1) {
> 		if (prop1->val.len == sizeof(cell_t))
> 			phandle1 = propval_cell(prop1); /* Well formed -> Get it */
> 	}
> 
> 	/* Try to get it from the 'linux,phandle' property */
> 	phandle2 = 0; /* Assume invalid */
> 	prop2 = get_property(node, "linux,phandle");
> 	if (prop2) {
> 		if (prop2->val.len == sizeof(cell_t))
> 			phandle2 = propval_cell(prop2); /* Well formed -> Get it */
> 	}
> 
> 	if (prop1 && prop2) {
> 		/*
> 		 * Both property are present. They values must be the same
> 		 * and valid.
> 		 */
> 		if (phandle1 == phandle2) && phandle_is_valid(phandle1)) {
> 			/* Ok, Cache this correct phandle value and return it */
> 			node->phandle = phandle1;
> 			return phandle1;
> 		}
> 	} else if (prop1) {
> 		/*
> 		 * Only the 'phandle' property is present.
> 		 * Check its value validity
> 		 */
> 		if (phandle_is_valid(phandle1)) {
> 			/* Ok, Cache this correct phandle value and return it */
> 			node->phandle = phandle1;
> 			return phandle1;
> 		}
> 	} else if (prop2) {
> 		/*
> 		 * Only the 'linux,phandle' property is present.
> 		 * Check its value validity
> 		 */
> 		if (phandle_is_valid(phandle2)) {
> 			/* Ok, Cache this correct phandle value and return it */
> 			node->phandle = phandle2;
> 			return phandle2;
> 		}
> 	}
> 
> 	/*
> 	 * No valid phandle value were found. Cache this information and return
> 	 * the invalid phandle value.
> 	 */
> 	node->phandle == ~0UL;
> 	return 0;
> }
> --- 8< ---

That looks good to me.  But the "existing" in the name no longer says
anything useful - this can be the standard way to get a node's
phandle.  In fact it might be worth changing the name of the
node->phandle field to make sure we don't have any stale users rather
than going through the get_phandle() function.

> In get_node_by_phandle(), no change compared to my proposition and so the
> full function is:
> --- 8< ---
> struct node *get_node_by_phandle(struct node *tree, cell_t phandle)
> {
>         struct node *child, *node;
>         cell_t tree_phandle;
> 
>         if (!phandle_is_valid(phandle)) {
>                 assert(generate_fixups);
>                 return NULL;
>         }
> 
>         tree_phandle = get_node_phandle_existing(tree);
>         if (phandle_is_valid(tree_phandle) && tree_phandle == phandle) {
>                 if (tree->deleted)
>                         return NULL;
>                 return tree;
>         }
> 
>         for_each_child(tree, child) {
>                 node = get_node_by_phandle(child, phandle);
>                 if (node)
>                         return node;
>         }
> 
>         return NULL;
> }
> --- 8< ---
> 
> Caching should work correctly without any other modification.
> 
> I let you double-check on your side. Let me know about your opinion.

Yes, I think that should work.

-- 
David Gibson (he or they)	| 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]