Re: [PATCH v3 02/15] libfdt: Don't assume the root node is available at offset 0

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

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

On Tue, Sep 08, 2026 at 10:08:23AM +0200, Herve Codina wrote:
> On Tue, 8 Sep 2026 16:41:06 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Mon, Sep 07, 2026 at 06:46:41PM +0200, Herve Codina wrote:
> > > Hi David,
> > > 
> > > On Wed, 2 Sep 2026 17:06:03 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > 
> > > ...  
> > > > > 
> > > > > Ok, I will update fdt_check_node_offset_() to have it updating its offset
> > > > > parameter to the real offset of the root node when its value is 0.
> > > > > 
> > > > > Based on this update, will see where it goes. I mean, impacts on callers, if
> > > > > it simplifies things or not, if the offset update needs also to be propagate
> > > > > to caller's parameter or any other similar point that we can see during the
> > > > > implementation.
> > > > > 
> > > > > Having something implemented and available in a patch will be the best to
> > > > > compare changes and impacts related to fdt_check_node_offset_() update.
> > > > > 
> > > > > Here we have a version of handling offset 0 vs real root node without any
> > > > > offset update done in fdt_check_node_offset_(). In the next iteration we will
> > > > > have the version with update done in fdt_check_node_offset_().
> > > > > 
> > > > > I think the golden rules to follow on this point is "keep it as simple as
> > > > > possible".    
> > > > 
> > > > Agreed.  Feel free to repost just the NOP before root patches on their
> > > > own.  At this time, frequent small series is easier for me to tackle
> > > > than occasional large series.
> > > >   
> > > 
> > > I've moved forward on the fdt_check_node_offset_() update.
> > > 
> > > The new fdt_check_node_offset_() looks like this:
> > > --- 8< ---
> > > int fdt_check_node_offset_(const void *fdt, int *offset)
> > > {
> > > 	int nextoffset;
> > > 
> > > 	if (!can_assume(VALID_INPUT)
> > > 	    && ((*offset < 0) || (*offset % FDT_TAGSIZE)))
> > > 		return -FDT_ERR_BADOFFSET;
> > > 
> > > 	if (*offset == 0) {
> > > 		*offset = fdt_root_offset(fdt);
> > > 		if (*offset < 0)
> > > 			return *offset;
> > > 	}
> > > 
> > > 	if (fdt_next_tag(fdt, *offset, &nextoffset) != FDT_BEGIN_NODE)
> > > 		return -FDT_ERR_BADOFFSET;
> > > 
> > > 	return nextoffset;
> > > }
> > > --- 8< ---  
> > 
> > That looks fine.  If it was an exposed function I'd want to look for a
> > better interface, but as an internal function it's fine.
> > 
> > > If the given offset is 0, fdt_check_node_offset_() considers we want to check
> > > the root node and so update offset to the real root node offset.
> > > 
> > > Ok, I still need some fdt_root_offset() calls from some other parts but that's
> > > not my main issue.
> > > 
> > > My main issue comes with orphan nodes in addons. fdt_check_node_offset_() is
> > > called with offset pointing to an orphan node and this offset can be 0.
> > > 
> > > The offset 0 seen by fdt_check_node_offset_() can be the "fake" offset of a root
> > > node and in that case fdt_check_node_offset_() should update offset to the real
> > > root node offset but it can also be the offset of the orphan node we want to check
> > > and in that case the offset should not be updated.
> > > 
> > > fdt_check_node_offset_() cannot determine whether or not the offset should be
> > > updated.  
> > 
> > Hrm.  But if offset 0 has an orphan node, we can see that with the
> > FDT_BEGIN_NODE or FDT_BEGIN_NODE_REF there, right?
> > 
> > > With orphan nodes in the loop, fdt_check_node_offset_() becomes:
> > > --- 8< ---
> > > int fdt_check_node_offset_(const void *fdt, int *offset)
> > > {
> > >         int nextoffset;
> > >         uint32_t tag;
> > > 
> > >         if (!can_assume(VALID_INPUT)
> > >             && ((*offset < 0) || (*offset % FDT_TAGSIZE)))
> > >                 return -FDT_ERR_BADOFFSET;
> > > 
> > >         if (*offset == 0) {
> > > #pragma message "We have a problem!"
> > >                 /*
> > >                  * An orphan node can be present at offset 0.
> > >                  * In that case, looking for the root node may or may not be
> > >                  * correct.
> > >                  * Indeed is offset = 0 requested because we want the root
> > >                  * node and sadly an orphan node is available at offset 0 or
> > >                  * is it requested because we want to really check the orphan
> > >                  * node available at offset 0. How to determine the correct
> > >                  * case?
> > >                  */
> > >                 tag = fdt_next_tag(fdt, *offset, &nextoffset);
> > >                 if (tag == FDT_BEGIN_NODE || tag == FDT_BEGIN_NODE_REF)
> > >                         return nextoffset;
> > > 
> > >                 *offset = fdt_root_offset(fdt);
> > >                 if (*offset < 0)
> > >                         return *offset;
> > >         }
> > > 
> > >         tag = fdt_next_tag(fdt, *offset, &nextoffset);
> > >         if (tag != FDT_BEGIN_NODE && tag != FDT_BEGIN_NODE_REF)
> > >                 return -FDT_ERR_BADOFFSET;
> > > 
> > >         return nextoffset;
> > > }
> > > --- 8< ---  
> > 
> > Right.. like that.  Except simpler would be to have fdt_root_offset()
> > stop when it sees a FDT_BEGIN_NODE_REF as well as a FDT_BEGIN_NODE.
> > Or maybe that should be, say, fdt_root_offset_(), and
> > fdt_root_offset() will wrap it to return an error on an addon tree
> > with no root.
> 
> No, fdt_root_offset() must get the root node offset. An orphan node identified
> with FDT_BEGIN_NODE_REF cannot be a root node.
> 
> An addon can have, a root node only, orphan nodes only or both a root node and
> orphan nodes.
> 
> fdt_root_offset() looks for the root node (i.e. the first FDT_BEGIN_NODE at
> top level). If not found, -ERRNOTFOUND (addon) or -ERRBADSTRUCTURE (not addon).
> 
> Having a fdt_root_offset() stop at either FDT_BEGIN_NODE_REF or FDT_BEGIN_NODE
> is "fdt_first_node_offset()".

> Let me introduce the internal fdt_first_node_offset_()
> - fdt_root_offset()
>   It calls fdt_first_node_offset_() and check that this node is a
>   FDT_BEGIN_NODE node.
> 
> - fdt_check_node_offset_()
>   if the offset == 0, it updates the value with the offset returned by
>   fdt_first_node_offset_()

Right, that's exactly what I'd suggest.

A possible tweak would be to have fdt_check_node_offset_() only apply
special case 0 handling if the _actual_ offset 0 doesn't look like a
valid node offset.  Not sure if that will make things messier or
cleaner.

> And so, a node offset 0 doesn't means the root node but the first node
> in the dtb (root or orphan). I am totally fine with this definition.

Right.  If you don't want that behaviour for addon dtbs, then I think
the way to go would be to explicitly avoid all special case handling
of 0 if the header flags an addon.  Handling addons requires new code,
so we don't have to maintain backwards compatibliity for things that
used 0 assuming it meant root node.

> At some point, maybe users of the API (when addon are involved) will have to
> take care of that and perform something like:
>   root = fdt_root_offset();
>   fdt_get_property(fdt, root, "prop", NULL);
> 
> Or
>   fdt_for_each_orphan(orphan, fdt) {
>      fdt_get_property(fdt, orphan, "prop", NULL);
>      ...
>   }
> 
> Here also, I am totally fine with that an I already use this kind of sequence
> in libfdt/fdt_addon.c to apply an addon on a base dtb.
> 
> I will introduce fdt_first_node_offset_() but let me know if you prefer
> having fdt_first_node_offset_() introduced right now in this "structure
> tags" series or later in the addon series.

Either is fine; do whichever results in less code churn.

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