On Fri, Oct 23, 2026 at 01:29:49PM +0200, Herve Codina wrote: > Hi David, > > On Fri, 23 Oct 2026 12:23:58 +1100 > David Gibson wrote: > > ... > > > > > > > > -static struct data data_append_markers(struct data d, struct marker *m) > > > > > > > +struct data data_append_markers(struct data d, struct marker *m) > > > > > > > { > > > > > > > struct marker **mp = &d.markers; > > > > > > > > > > > > > > @@ -288,6 +288,7 @@ struct marker *alloc_marker(unsigned int offset, enum markertype type, > > > > > > > m->type = type; > > > > > > > m->ref = ref; > > > > > > > m->next = NULL; > > > > > > > + m->is_local = 0; > > > > > > > > > > > > Storing is_local in the marker doesn't make sense to me. I think the > > > > > > locality should be calculated only when we actually go to emit the dtb > > > > > > - I believe we should be able to deduce it from m->ref. > > > > > > > > > > When we parse a dtb, we do not have any reference for local phandle, we have > > > > > only the phandle value. > > > > > > > > Heh, right, I remarked on that further down. > > > > > > > > > Later in build_dt_info(), complete_references() is called. This new function > > > > > is introduced in patch 9 ("dtc: Complete REF_PHANDLE markers with ref strings > > > > > and is_local flags"). It completes the reference either filling the ref field > > > > > based on the phandle value for all local reference (parsing from a dtb) or > > > > > setting is_local based on the reference (parsing from a dts). > > > > > > > > Ah, right having that logic in a different patch makes it harder to > > > > follow. > > > > > > > > What concerns me here is that currently we have an invariant that a > > > > REF_PHANDLE marker always has a meaningful m->ref field - from as soon > > > > as it's added to the property data. > > > > > > > > This patch breaks that assumption, which means when looking at some > > > > random part of the code it's hard to know whether we're in a state > > > > where m->ref is valid or it might not me. > > > > > > > > One approach is to minimise the gap where m->ref is invalid. That's > > > > both in terms of code path - at minimum the m->ref fields should be > > > > valid by return from dt_from_*(). It's also in terms of not leaving > > > > intermediate patches where this is not fixed up. > > > > > > > > The other approach would be to create new marker types indicating > > > > these intermediate states. The markers are for dtc internal use, they > > > > don't need to have 1 to 1 correspondance with things in either dtb or > > > > source format. > > > > > > I don't want to introduce new markers for handling intermediate states. > > > > > > I really prefer avoiding those kind of "fake" marker which will have > > > a short lifetime and should not be seen by code (other than the code > > > moving this intermediate state to the final state). > > > > > > My strategy was to ensure that once build_dt_info() is called, all > > > markers are fully correct with all field (is_local and ref) correctly > > > set. > > > > > > build_dt_info() is called by all parser at the end of the parsing > > > operation to return a struct dt_info which is the dtc internal > > > internal representation used by the rest of code. > > > > > > Are you ok with this strategy? > > > > Having transitional states that don't last beyond build_dt_info() is > > acceptable in principle, however: > > > > 1) That leaves m->is_local and m->ref redudant after that point, > > which I don't like very much > > Ok I've looked deeper to remove the is_local field. It should be doable. > > complete_references(), probably renamed to fixup_references(), will still be > needed Right, I'd expect that. > and called from build_dt_info(). This I'm less certain about, since the function needs to fix up in opposite directions depending on the input mode. But, I'll look at the patch once it's posted. > Indeed, all refs need to point to a > valid string (path or label). > > The idea removing is_local is to have the transitional state where ref can be NULL. > (REF_PHANDLE created from FDT_PROPDATA_PHANDLE where no ref are available but just > the phandle value). > > Those NULL ref, identifying a local phandle, will be updated by fixup_references() > called from build_dt_info(). > > I plan also to remove the is_local field from export symbols structure. Indeed, the > exact same problematic exist with export symbol and related to this 'is_local' topic > REF_PHANDLE markers and export symbols have no reason to be handled in a different > way. Ok. I'll keep that in mind once I start looking at those patches. > I plan to introduce an new function: > bool ref_is_local(struct dt_info *dti, const char *ref) > in order to determine if a given reference is local or not. Sounds good. > As you already stated, this will be needed to emit the correct tag in the output dtb. > > This function will walk the tree, no way to avoid that with a 'is_local' flag. Ok. > > 2) As of this patch specifically, that's not the case. I don't want > > to have intermediate commits where this invariant is not > > preserved (it's bad for bisects). > > > > > If yes, patches order in this series will be changed. > > > > > > The current order is > > > - Patch 7: Add support for FDT_PROPDATA_PHANDLE dtb tag > > > - Patch 8: livetree: Improve get_node_by_phandle() > > > - Patch 9: dtc: Complete REF_PHANDLE markers with ref strings and is_local flags > > > > > > With that order we have the incomplete marker state present after patch 7 and > > > before patch 9. > > > > > > Moving 'Add support for FDT_PROPDATA_PHANDLE dtb tag' after introducing the marker > > > state completion will avoid the problem. > > > - Patch 7: livetree: Improve get_node_by_phandle() > > > - Patch 8: dtc: Complete REF_PHANDLE markers with ref strings and is_local flags > > > - Patch 9: Add support for FDT_PROPDATA_PHANDLE dtb tag > > > > > > Patch 8 and 9 could be squashed if you prefer to have the completion added > > > in the 'Add support for FDT_PROPDATA_PHANDLE dtb tag' patch. > > > > That sounds ok, though I think I'll have to look at the final result > > to be sure. I would prefer 8 & 9 be merged together for > > reviewability. That said, the libfdt parts of the changes could be > > split out into an earlier patch than the dtc parts to keep the sizes > > smaller. > > Ok, I will merge and extract libfdt related parts to a specific patch. > > > > > > > It seems to me there's an important difference between the -Idts and > > > > -Idtb cases: in the dts case, we have a valid reference, but the > > > > phandle needs to be filled in later; in the dtb case, we have a valid > > > > phandle, but the reference needs to be filled in later. Using > > > > different marker types for those cases doesn't seem unreasonable for > > > > me. In fact, for clarity it might even make sense to have a third > > > > variant for "both phandle and m->ref are valid" - the fixup code > > > > between input and output would convert either of the first two > > > > variants into the third, which is what the final output code would > > > > require. > > > > > > > > That's not necessarily the best way to do it, but I really prefer > > > > where what's valid in a structure can be determined from the type and > > > > contents alone - rather than having to know where you are in a complex > > > > code path to know what's valid. > > > > > > Performing marker completion in build_dt_info() avoid additional markers > > > and avoid also incomplete markers in the code path. > > > > > > Before build_dt_info() call, the internal devicetree representation doesn't > > > exits for dtc. We are in the process of parsing and so we are building this > > > internal representation. After this call, the internal representation exists > > > and all markers are fully set without the need to introduce (and handle) > > > additional markers. > > > > > > In other word, dti returned by all dt_from_*() functions returns a fully > > > complete dt_info. > > > --- 8< --- > > > if (streq(inform, "dts")) > > > dti = dt_from_source(arg); > > > else if (streq(inform, "fs")) > > > dti = dt_from_fs(arg); > > > else if(streq(inform, "dtb")) > > > dti = dt_from_blob(arg); > > > else > > > die("Unknown input format \"%s\"\n", inform); > > > --- 8< --- > > > > > > User of this dti will never see an incomplete marker. > > > > Ok, that works. > > Fine. > > ... > > > > > > > What do you thing about: > > > --- 8< --- > > > /** > > > * fdt_next_tag - get next tag in the device tree > > > ... > > > * returns: > > > * The tag type, either a known tag or an unknown structured tag > > > * if this unknown tag can be skipped (SAFE_SKIP bit set in this > > > * tag). > > > * In case of error, FDT_END is returned with @nextoffset set to > > > * a negative error code. > > > */ > > > --- 8< --- > > > > I don't think the extra detail really helps much - just "The tag > > type." would be fine, I think. > > Ok on my side, I will remove the detail. > > 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