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

On Wed, Oct 28, 2026 at 09:41:10AM +0100, Herve Codina wrote:
> Hi David,
> 
> On Wed, 28 Oct 2026 13:41:14 +1100
> David Gibson <david@gibson.dropbear.id.au> 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?

Sorry, I wasn't very clear.  I was just suggesting temporary renaming
as the best way to make sure you don't have any leftover accesses to
the field in places you didn't expect.

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

Right, that's what I had in mind.

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

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