From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Gibson Date: Wed, 15 Jun 2016 20:19:35 +1000 Subject: [U-Boot] [PATCH v2 7/9] libfdt: Add overlay application function In-Reply-To: <7E8A7CBD-D682-45E5-AD2C-19F137E5ED38@konsulko.com> References: <1464340402-2249-1-git-send-email-maxime.ripard@free-electrons.com> <1464340402-2249-8-git-send-email-maxime.ripard@free-electrons.com> <34997AD3-B621-4823-920E-22E4A6F0E0D1@konsulko.com> <20160614002550.GA4882@voom.fritz.box> <20160615031402.GF4882@voom.fritz.box> <7E8A7CBD-D682-45E5-AD2C-19F137E5ED38@konsulko.com> Message-ID: <20160615101935.GA16563@voom.fritz.box> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Wed, Jun 15, 2016 at 12:34:00PM +0300, Pantelis Antoniou wrote: > Hi David, > > > On Jun 15, 2016, at 06:14 , David Gibson 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 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. !?! No, really, silent partial application is just horrible. > >> 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 at X? are errors), we have flexibility in introducing > nodes that contain configuration data for the loader. There's no significant difference between the approaches from this point of view. Metadata nodes are certainly possible (we already have __symbols__ and __fixups__) but calling them something other than fragment@ is no harder than leaving off the target property. In fact even if it was workable, calling new metadata nodes fragment@ would be stupidly confusing. > > 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 -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: