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

[Herve Codina]

On Tue, 8 Sep 2026 16:41:06 +1000
David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx> 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_()

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.

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.

Best regards,
Hervé