Hi David, > On Jun 15, 2016, at 06:14 , David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote: > > On Tue, Jun 14, 2016 at 12:22:23PM +0300, Pantelis Antoniou wrote: >> Hi David, >>> On Jun 14, 2016, at 03:25 , David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote: >>> On Fri, Jun 10, 2016 at 05:28:11PM +0300, Pantelis Antoniou wrote: > [snip] >>>>> +static int fdt_overlay_merge(void *dt, void *dto) >>>>> +{ >>>>> + int root, fragment; >>>>> + >>>>> + root = fdt_path_offset(dto, "/"); >>>>> + if (root < 0) >>>>> + return root; >>>>> + >>>>> + fdt_for_each_subnode(dto, fragment, root) { >>>>> + const char *name = fdt_get_name(dto, fragment, NULL); >>>>> + uint32_t target; >>>>> + int overlay; >>>>> + int ret; >>>>> + >>>>> + if (strncmp(name, "fragment", 8)) >>>>> + continue; >>>>> + >>>> >>>> This is incorrect. The use of “fragment” is a convention only. >>>> The real test whether the node is an overlay fragment is that >>>> it contains a target property. >>> >>> Hmm.. I dislike that approach. First, it means that if new target >>> types are introduced in future, older code is likely to silently >>> ignore such fragments instead of realizing that it doesn't know how to >>> apply themm. Second, it raises weird issues if some node down within >>> a fragment also happens to have a property called "target”. >> >> Not really. If new targets are introduced then the fragment will be ignored. > > Yes.. and that's bad. > That’s arguable. >> We can have an argument about what is better to do (report an error or >> ignore a fragment) but what it comes down to is that that applicator >> does not know how to handle the new target method. > > Sure, let's have the argument. The overlay is constructed on the > assumption that all the pieces will be applied, or none of them. A > silent, partial application is an awful, awful failure mode. We > absolutely should report an error, and in order to do so we need to > know what are applicable fragments, whether or not we understand the > target designation (or any other meta-data the fragment has). > This way also allows having nodes being something other than fragments. So instead of being painted into a corner (all subnodes that are not named ‘fragment@X’ are errors), we have flexibility in introducing nodes that contain configuration data for the loader. > Given what's established so far, checking the name seems the obvious > way to do that. > Again, it’s arguable. Stricter checking against future-proofing. >> There is no issues with any target properties inside a fragment because >> the check is not made recursively. > > Ok, so the real test you're proposing is "at the top level AND has a > target property”. Yes > > -- > David Gibson | 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 Regards — Pantelis -- To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html