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

[Herve Codina]

Hi David,

On Wed, 28 Oct 2026 13:41:14 +1100
David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:

...

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

If I switch it back, why talking about renaming?

Of course, I will do this renaming locally to be sure that I handle all
users but in the end no commit will rename this field.

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

Ok.

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

Right. Ok on my side to have this redundancy.

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

Ok, will use those names.

Best regards,
Hervé