Re: [PATCH v3 12/15] libfdt: Introduce fdt_getprop_offset_namelen()

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4517 bytes --]

On Fri, Oct 23, 2026 at 09:25:59AM +0200, Herve Codina wrote:
> Hi David,
> 
> On Tue, 22 Sep 2026 18:25:47 +0200
> Herve Codina <herve.codina@bootlin.com> wrote:
> 
> > On Mon, 21 Sep 2026 16:07:21 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> > 
> > > On Wed, Aug 26, 2026 at 10:31:43AM +0200, Herve Codina wrote:  
> > > > Future operations, such as handling unknown tags on dtb modifications,
> > > > need to get an offset to a property based on the node offset and the
> > > > property name.
> > > > 
> > > > Several function exists to get a property based on the node offset
> > > > and the property name but none of them allows to get the property
> > > > offset.
> > > > 
> > > > Fill the lack and introduce fdt_getprop_offset_namelen() and its
> > > > simple variant fdt_getprop_offset().
> > > > 
> > > > Signed-off-by: Herve Codina <herve.codina@bootlin.com>    
> > > 
> > > This seems redundant with fdt_ptr_offset_(), which can trivially get
> > > this information when combined with fdt_get_property().  
> > 
> > Indeed, It should be ok using both functions you pointed out.
> > 
> > I will remove fdt_getprop_offset_namelen() introduced here and use directly 
> > functions mentionned in the next iteration without wrapping those calls in
> > a new function. 
> 
> I've started to replace fdt_getprop_offset_namelen() by the combination
> of fdt_ptr_offset_() and fdt_get_property_namelen().
> 
> As a reminder fdt_getprop_offset_namelen() I proposed was the following:
> --- 8< ---
> > +int fdt_getprop_offset_namelen(const void *fdt, int nodeoffset,
> > +			       const char *name, int namelen)
> > +{
> > +	const struct fdt_property *prop;
> > +	int lenp, offset;
> > +
> > +	prop = fdt_get_property_namelen_(fdt, nodeoffset, name, namelen,
> > +					 &lenp, &offset);
> > +	if (!prop)
> > +		return lenp;
> > +	return offset;
> > +}
> --- 8< ---
> 
> fdt_get_property_namelen_() was called.
> 
> Switching to fdt_get_property_namelen() introduces a regression.
> 
> fdt_get_property_namelen() calls fdt_get_property_namelen() and this function
> (without the '_' suffix) performs a check on fdt_version() returning an error
> on version < 16.
> 
> Using it to replace my fdt_getprop_offset_namelen() function will break
> write-in-place setprop() for dtb version < 16. Indeed, it is used in [1]
> 
> [1]: https://lore.kernel.org/devicetree-compiler/20260826083146.304291-15-herve.codina@bootlin.com/#Z31libfdt:fdt_wip.c
> 
> Now, several options:
> 
> a) No problem if we break dtb version < 16
>    Ok, I will use directly fdt_get_property_namelen() + fdt_ptr_offset_() to
>    get an offset and remove my fdt_getprop_offset_namelen() wrapper.

So, practically speaking this is probably true.  Version before 16 are
truly ancient, and versions 1..3 were all before dtbs really caught
on.

> b) Keep my fdt_getprop_offset_namelen() function without any modification
>    We support version < 16, problem solved.

Still don't like it.

> c) Keep my fdt_getprop_offset_namelen() but static inline in libfdt_internal.h
>    Export fdt_get_property_namelen_() in libfdt_internal.h and use it in the
>    static inline fdt_getprop_offset_namelen().
>    This implies that fdt_get_property_namelen_() will be no more static and
>    usable internally out of fdt_ro.c but only the fdt_getprop_offset_namelen()
>    wrapper should be used.

Eh.

> d) Use directly fdt_get_property_namelen_()
>    Export fdt_get_property_namelen_() in libfdt_internal.h and use it
>    directly to get the offset.
>    I don't really like this solution. I would prefer to have a wrapper to
>    explicitly return the offset instead of exposing and using directly
>    fdt_get_property_namelen_().

Hm, I think this is my preferred option.  libfdt_internal.h exists
precisely to "expose" things across modules internally, but without
exposing them to external users.  Maybe I'm missing something, I don't
really see why that's objectionable.

> On my side, I would prefer b) as it support version < 16 without the need
> to expose fdt_get_property_namelen_() out of fdt_ro.c.
> 
> I would like to have your opinion on this point.
> Is also option b) ok on your side ?
> 
> 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 --]