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