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

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.

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

> +	if (phandle_is_valid(tree_phandle) && tree_phandle == phandle) {
>  		if (tree->deleted)
>  			return NULL;
>  		return tree;
> -- 
> 2.55.0
> 
> 

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