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: 13696 bytes --]

On Mon, Oct 26, 2026 at 12:56:15PM +0100, Herve Codina wrote:
> Hi David,
> 
> On Mon, 26 Oct 2026 12:33:51 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > 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.
> 
> Ok, I will rename node->phandle to node->phandle_ and add a comment saying
> that phandle_ field must not be accessed directly.

You can even switch it back after you've checked there are no stray
accesses to it.

> Related to existing users, I can see check_explicit_phandles() available
> in the checks.c file.
> 
> check_explicit_phandles() could use the new get_node_phandle_exising() with
> the caching feature. The only problematic part will be
> --- 8< ---
> 	if (linux_phandle && phandle && (phandle != linux_phandle))
> 		FAIL(c, dti, node, "mismatching 'phandle' and 'linux,phandle'"
> 		     " properties");
> --- 8< ---
>
> Indeed, this check is done in get_node_phandle_existing() and the function
> returns 0 (invalid phandle) when both properties are set with a different
> value. A value other than 0 should be returned by get_node_phandle_existing()
> to identify that mismatching properties values.
> 
> This should not be a big deal if we introduced a new invalid phandle value:
>   - 0:
>     Invalid value. Can be returned by get_node_phandle_existing().
> 
>   - ~0:
>     Unresolved value (phandle value in properties) or value cached in 
>     phandle->node_ for "mismatch 'phandle' and 'linux,phandle' properties
>     values". Can be returned by get_node_phandle_existing().
> 
>   - (~0)-1:
>     Invalid value in properties and or value cached in phandle->node_ for Invalid value.
>     Will not be returned by get_node_phandle_existing().
>     When this cached value is found, 0 will be returned.
>
> In phandle_is_valid(), those 3 values will be checked to determine if the phandle
> value is valid or not.

Alas, we can't do that.  Which phandle values are valid is long
established from the IEEE1275 days, and we can't really change it.  I
don't _know_ if there are existing trees out there using 0xfffffffe as
a valid phandle, but there could be.

*thinks*

In some ways it would be elegant for get_node_phandle_existing() to
report the various different error conditions separately, but I think
the awkwardness of passing that information back to the checks is
going to outweigh it.

So, I think we're actually better off separating lookup of phandles
from validation.  So get_node_phandle_existing() (or whatever we call
it) will return 0 if there's no valid phandle, whether that's simply
because it's missing, or there's something malformed.  Separately
check_explicit_phandles() will report specific types of malformed
phandle properties.

It does result in a little redudant logic, but I think it's the best
overall approach for now.

> Now last point, renaming get_node_phandle_existing().
> 
> get_node_phandle() already exist. It get a phandle but also if the node doesn't
> have a phandle, it allocate a new phandle value and set node "phandle" or
> "linux, phandle" property to this allocated value.

Ah, sorry, I misunderstood what the "existing" was referring to.

> IHMO, get_node_phandle() should be renamed to set_node_phandle() and
> get_node_phandle_existing() should be renamed to get_node_phandle().

Right, the existing name is bad.  But let's go with
get_or_assign_phandle() for the existing one and get_phandle() for the
new one.  (The "node" added no useful information and this way they're
two new names to reduce confusion).

> Does all of these make sense?
> 
> I so, I will try to implement that and maybe I will see other issue I might
> have missed.
> 
> Worth noting that if you're ok to rename get_node_phandle() to
> set_node_phandle(), I will do it in a dedicated patch. Indeed, this will
> ensure that get_node_phandle() has been renamed everywhere (compilation
> without failure) and so I can safely rename get_node_phandle_existing() to
> get_node_phandle() in a next patch.
> 
> > 
> > > 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.
> > 
> 
> Best regards,
> Hervé
> 

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