On Tue, Sep 08, 2026 at 10:08:23AM +0200, Herve Codina wrote: > On Tue, 8 Sep 2026 16:41:06 +1000 > David Gibson 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 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