Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753461AbaKQWzA (ORCPT ); Mon, 17 Nov 2014 17:55:00 -0500 Received: from mail-ie0-f172.google.com ([209.85.223.172]:61618 "EHLO mail-ie0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753116AbaKQWy5 (ORCPT ); Mon, 17 Nov 2014 17:54:57 -0500 MIME-Version: 1.0 In-Reply-To: <3DA4B86C-C294-400D-8D03-D850B940CF7B@konsulko.com> References: <1414528565-10907-1-git-send-email-pantelis.antoniou@konsulko.com> <1414528565-10907-2-git-send-email-pantelis.antoniou@konsulko.com> <20141113233618.1C378C40B52@trevor.secretlab.ca> <3DA4B86C-C294-400D-8D03-D850B940CF7B@konsulko.com> From: Grant Likely Date: Mon, 17 Nov 2014 22:54:36 +0000 X-Google-Sender-Auth: G4kl6ZLEGwz9XdQilyYM8lmRi6g Message-ID: Subject: Re: [PATCH v8 1/8] OF: Introduce DT overlay support. (v2) To: Pantelis Antoniou Cc: Rob Herring , Stephen Warren , Matt Porter , Koen Kooi , Greg Kroah-Hartman , Alison Chaiken , Dinh Nguyen , Jan Lubbe , Alexander Sverdlin , Michael Stickel , Guenter Roeck , Dirk Behme , Alan Tull , Sascha Hauer , Michael Bohan , Ionut Nicu , Michal Simek , Matt Ranostay , Joel Becker , "devicetree@vger.kernel.org" , Wolfram Sang , Linux I2C , Mark Brown , linux-spi , Linux Kernel Mailing List , Pete Popov , Dan Malek , Georgi Vlaev Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Nov 17, 2014 at 2:35 PM, Pantelis Antoniou wrote: > Hi Grant, > >> On Nov 14, 2014, at 01:36 , Grant Likely wrote: >> >> On Tue, 28 Oct 2014 22:35:58 +0200 >> , Pantelis Antoniou >> wrote: >>> Introduce DT overlay support. >>> >>> Makes it possible to dynamically overlay a part of the kernel's >>> tree with another tree that's been dynamically loaded. >>> Removal of nodes and properties is also possible. >>> >>> The hard part of applying and reverting the overlay is performed >>> using changesets. >>> >>> Documentation about internal and APIs is provided in >>> Documentation/devicetree/overlay-notes.txt >> >> Hi Pantelis, >> >> Comments below. I've picked up this patch, the selftest patch, and the >> platform bus patch into my working tree. You can supply me with fixup >> patches for the comments below. >> >> Hopefully I'll get my tree published to a test branch tomorrow . >> >>> >>> Changes since v1: >>> - Drop delete capability using '-' prefix. The '-' prefixed names >>> are valid properties and nodes and there is no need for it just yet. >>> - Do not update special properties - name & phandle ones. >>> - Change order of node attachment, so that the special property update >>> works. >>> >>> Signed-off-by: Pantelis Antoniou >>> --- >>> Documentation/devicetree/overlay-notes.txt | 137 ++++++ >>> drivers/of/Kconfig | 7 + >>> drivers/of/Makefile | 1 + >>> drivers/of/overlay.c | 656 +++++++++++++++++++++++++++++ >>> include/linux/of.h | 31 ++ >>> 5 files changed, 832 insertions(+) >>> create mode 100644 Documentation/devicetree/overlay-notes.txt >>> create mode 100644 drivers/of/overlay.c >>> >>> diff --git a/Documentation/devicetree/overlay-notes.txt b/Documentation/devicetree/overlay-notes.txt >>> new file mode 100644 >>> index 0000000..b060bd7 >>> --- /dev/null >>> +++ b/Documentation/devicetree/overlay-notes.txt >>> @@ -0,0 +1,137 @@ >>> +Device Tree Overlay Notes >>> +------------------------- >>> + >>> +This document describes the implementation of the in-kernel >>> +device tree overlay functionality residing in drivers/of/overlay.c and is a >>> +companion document to Documentation/devicetree/dt-object-internal.txt[1] & >>> +Documentation/devicetree/dynamic-resolution-notes.txt[2] >>> + >>> +How overlays work >>> +----------------- >>> + >>> +A Device Tree's overlay purpose is to modify the kernel's live tree, and >>> +have the modification affecting the state of the the kernel in a way that >>> +is reflecting the changes. >>> +Since the kernel mainly deals with devices, any new device node that result >>> +in an active device should have it created while if the device node is either >>> +disabled or removed all together, the affected device should be deregistered. >>> + >>> +Lets take an example where we have a foo board with the following base tree >>> +which is taken from [1]. >>> + >>> +---- foo.dts ----------------------------------------------------------------- >>> + /* FOO platform */ >>> + / { >>> + compatible = "corp,foo"; >>> + >>> + /* shared resources */ >>> + res: res { >>> + }; >>> + >>> + /* On chip peripherals */ >>> + ocp: ocp { >>> + /* peripherals that are always instantiated */ >>> + peripheral1 { ... }; >>> + } >>> + }; >>> +---- foo.dts ----------------------------------------------------------------- >>> + >>> +The overlay bar.dts, when loaded (and resolved as described in [2]) should >>> + >>> +---- bar.dts ----------------------------------------------------------------- >>> +/plugin/; /* allow undefined label references and record them */ >>> +/ { >>> + .... /* various properties for loader use; i.e. part id etc. */ >>> + fragment@0 { >>> + target = <&ocp>; >>> + __overlay__ { >>> + /* bar peripheral */ >>> + bar { >>> + compatible = "corp,bar"; >>> + ... /* various properties and child nodes */ >>> + } >>> + }; >>> + }; >>> +}; >>> +---- bar.dts ----------------------------------------------------------------- >>> + >>> +result in foo+bar.dts >>> + >>> +---- foo+bar.dts ------------------------------------------------------------- >>> + /* FOO platform + bar peripheral */ >>> + / { >>> + compatible = "corp,foo"; >>> + >>> + /* shared resources */ >>> + res: res { >>> + }; >>> + >>> + /* On chip peripherals */ >>> + ocp: ocp { >>> + /* peripherals that are always instantiated */ >>> + peripheral1 { ... }; >>> + >>> + /* bar peripheral */ >>> + bar { >>> + compatible = "corp,bar"; >>> + ... /* various properties and child nodes */ >>> + } >>> + } >>> + }; >>> +---- foo+bar.dts ------------------------------------------------------------- >>> + >>> +As a result of the the overlay, a new device node (bar) has been created >>> +so a bar platform device will be registered and if a matching device driver >>> +is loaded the device will be created as expected. >>> + >>> +Overlay in-kernel API >>> +-------------------------------- >>> + >>> +The API is quite easy to use. >>> + >>> +1. Call of_overlay_create() to create and apply an overlay. The return value >>> +is a cookie identifying this overlay. >>> + >>> +2. Call of_overlay_destroy() to remove and cleanup the overlay previously >>> +created via the call to of_overlay_create(). Removal of an overlay that >>> +is stacked by another will not be permitted. >>> + >>> +Finally, if you need to remove all overlays in one-go, just call >>> +of_overlay_destroy_all() which will remove every single one in the correct >>> +order. >>> + >>> +Overlay DTS Format >>> +------------------ >>> + >>> +The DTS of an overlay should have the following format: >>> + >>> +{ >>> + /* ignored properties by the overlay */ >>> + >>> + fragment@0 { /* first child node */ >>> + >>> + target=; /* phandle target of the overlay */ >>> + or >>> + target-path="/path"; /* target path of the overlay */ >>> + >>> + __overlay__ { >>> + property-a; /* add property-a to the target */ >>> + -property-b; /* remove property-b from target */ >>> + node-a { /* add to an existing, or create a node-a */ >>> + ... >>> + }; >>> + -node-b { /* remove an existing node-b */ >>> + ... >>> + }; >> >> -property-b and -node-b need to be removed from the example. >> > > OK > >>> + }; >>> + } >>> + fragment@1 { /* second child node */ >>> + ... >>> + }; >>> + /* more fragments follow */ >>> +} >>> + >>> +Using the non-phandle based target method allows one to use a base DT which does >>> +not contain a __symbols__ node, i.e. it was not compiled with the -@ option. >>> +The __symbols__ node is only required for the target= method, since it >>> +contains the information required to map from a phandle to a tree location. >>> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig >>> index 1a13f5b..aa315c4 100644 >>> --- a/drivers/of/Kconfig >>> +++ b/drivers/of/Kconfig >>> @@ -83,4 +83,11 @@ config OF_RESERVED_MEM >>> config OF_RESOLVE >>> bool >>> >>> +config OF_OVERLAY >>> + bool >>> + depends on OF >>> + select OF_DYNAMIC >>> + select OF_DEVICE >>> + select OF_RESOLVE >>> + >>> endmenu # OF >>> diff --git a/drivers/of/Makefile b/drivers/of/Makefile >>> index ca9209c..1bfe462 100644 >>> --- a/drivers/of/Makefile >>> +++ b/drivers/of/Makefile >>> @@ -14,6 +14,7 @@ obj-$(CONFIG_OF_PCI_IRQ) += of_pci_irq.o >>> obj-$(CONFIG_OF_MTD) += of_mtd.o >>> obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o >>> obj-$(CONFIG_OF_RESOLVE) += resolver.o >>> +obj-$(CONFIG_OF_OVERLAY) += overlay.o >>> >>> CFLAGS_fdt.o = -I$(src)/../../scripts/dtc/libfdt >>> CFLAGS_fdt_address.o = -I$(src)/../../scripts/dtc/libfdt >>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c >>> new file mode 100644 >>> index 0000000..ec7675d >>> --- /dev/null >>> +++ b/drivers/of/overlay.c >>> @@ -0,0 +1,656 @@ >>> +/* >>> + * Functions for working with device tree overlays >>> + * >>> + * Copyright (C) 2012 Pantelis Antoniou >>> + * Copyright (C) 2012 Texas Instruments Inc. >>> + * >>> + * This program is free software; you can redistribute it and/or >>> + * modify it under the terms of the GNU General Public License >>> + * version 2 as published by the Free Software Foundation. >>> + */ >>> +#undef DEBUG >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +#include "of_private.h" >>> + >>> +/** >>> + * struct of_overlay_info - Holds a single overlay info >>> + * @target: target of the overlay operation >>> + * @overlay: pointer to the overlay contents node >>> + * >>> + * Holds a single overlay state, including all the overlay logs & >>> + * records. >>> + */ >>> +struct of_overlay_info { >>> + struct device_node *target; >>> + struct device_node *overlay; >>> +}; >>> + >>> +/** >>> + * struct of_overlay - Holds a complete overlay transaction >>> + * @node: List on which we are located >>> + * @count: Count of ovinfo structures >>> + * @ovinfo: Overlay info array (count size) >>> + * @le_list: List of the overlay logs >> >> The documentation is out-of-date. Please supply a fixup patch. >> > > OK > >>> + * >>> + * Holds a complete overlay transaction >>> + */ >>> +struct of_overlay { >>> + int id; >>> + struct list_head node; >>> + int count; >>> + struct of_overlay_info *ovinfo_tab; >>> + struct of_changeset cset; >>> +}; >>> + >>> +static int of_overlay_apply_one(struct of_overlay *ov, >>> + struct device_node *target, const struct device_node *overlay); >>> + >>> +static int of_overlay_apply_single_property(struct of_overlay *ov, >>> + struct device_node *target, struct property *prop) >>> +{ >>> + struct property *propn, *tprop; >>> + >>> + /* NOTE: Multiple changes of single properties not supported */ >>> + tprop = of_find_property(target, prop->name, NULL); >>> + >>> + /* special properties are not meant to be updated (silent NOP) */ >>> + if (tprop && >>> + (!of_prop_cmp(prop->name, "name") || >>> + !of_prop_cmp(prop->name, "phandle") || >>> + !of_prop_cmp(prop->name, "linux,phandle"))) >>> + return 0; >> >> What is the reason for these tests being conditional on the presence of >> the property in the node? >> > > We want to avoid updating those properties when updating an existing node. > The case where we insert these properties is valid when adding a new node. So the need for the test goes away if the properties are duplicated with the code (instead of as separate changeset items), correct? I've already got a change that duplicates the properties when adding a new node. > >>> + >>> + propn = __of_prop_dup(prop, GFP_KERNEL); >>> + if (propn == NULL) >>> + return -ENOMEM; >>> + >>> + /* not found? add */ >>> + if (tprop == NULL) >>> + return of_changeset_add_property(&ov->cset, target, propn); >>> + >>> + /* found? update */ >>> + return of_changeset_update_property(&ov->cset, target, propn, tprop); >>> +} >>> + >>> +static int of_overlay_apply_single_device_node(struct of_overlay *ov, >>> + struct device_node *target, struct device_node *child) >>> +{ >>> + const char *cname; >>> + struct device_node *tchild; >>> + char *full_name; >>> + const char *suffix; >>> + int ret; >>> + >>> + /* special case for nodes with a suffix */ >>> + suffix = strrchr(child->full_name, '@'); >>> + if (suffix != NULL) { >>> + cname = kbasename(child->full_name); >>> + if (cname == NULL) >>> + return -ENOMEM; >>> + } else >>> + cname = child->name; >> >> So, if it has a unit address suffix, kbasename is used. If it doesn't >> have a suffix, child->name is used because it is cheaper that using >> kbasename? If that's the reason, I don't think the code complexity is >> worth it since the cost should be negligable. Can kbasename() be used >> always? >> > > Yes, that is the reason. kbasename can be used each time (I have to verify > to be sure). Let's just use kbasename() here then for now, particularly if we're going to try to be rid of assigning full_name at this point. > >>> + >>> + ret = 0; >>> + >>> + /* NOTE: Multiple mods of created nodes not supported */ >>> + tchild = of_get_child_by_name(target, cname); >>> + if (tchild != NULL) { >>> + >>> + /* apply overlay recursively */ >>> + ret = of_overlay_apply_one(ov, tchild, >>> + child); >>> + >>> + of_node_put(tchild); >>> + >>> + } else { >>> + full_name = kasprintf(GFP_KERNEL, "%s/%s", >>> + target->full_name, cname); >>> + if (full_name == NULL) >>> + return -ENOMEM; >>> + >>> + /* create empty tree as a target */ >>> + tchild = __of_node_alloc(full_name, GFP_KERNEL); >>> + >>> + /* free either way */ >>> + kfree(full_name); >>> + >>> + if (tchild == NULL) >>> + return -ENOMEM; >>> + >>> + /* point to parent */ >>> + tchild->parent = target; >> >> As discussed on IRC, if a node doesn't already exist in the tree, then >> the properties can be duplicated with the node. There is no need to do a >> of_changeset_add_property() on each and every node. That just makes the >> load on notifiers a lot heavier and adds more processing on add and >> remove. > > In theory yes. I have to verify whether this approach works. > >> >>> + >>> + /* apply the overlay */ >>> + ret = of_overlay_apply_one(ov, tchild, >>> + child); >>> + >>> + /* attach the node afterwards */ >>> + if (!ret) >>> + ret = of_changeset_attach_node(&ov->cset, tchild); >> >> It looks to me like the parent is getting added /after/ the child is >> added in the changeset stack. That doesn't look right to me. Wouldn't >> that also mean that the child node get registered on sysfs after the >> parent nodes? >> > > This is done on purpose. Remember that nothing is added to sysfs at this > point; you have to commit the changeset for sysfs nodes to be created. But doesn't that cause the child nodes to be added to sysfs before the parent nodes? I don't think that is valid. > Performing the node attach after performing the recursive call allows > us to get rid of the special casing for properties; the phandle ones in > particular. I think this issue goes away when properties are already added to a new node before attach. > >>> + >>> + } >>> + >>> + return ret; >>> +} >>> + >>> +/* >>> + * Apply a single overlay node recursively. >>> + * >>> + * Note that the in case of an error the target node is left >>> + * in a inconsistent state. Error recovery should be performed >>> + * by using the tree changes list. >>> + */ >>> +static int of_overlay_apply_one(struct of_overlay *ov, >>> + struct device_node *target, const struct device_node *overlay) >>> +{ >>> + struct device_node *child; >>> + struct property *prop; >>> + int prev_avail, prop_avail, pass; >>> + int ret; >>> + >>> + /* >>> + * Special consideration for status properties >>> + * >>> + * In order to make status property changes work >>> + * we have the following cases: >>> + * >>> + * Enabled device with status change to 'disabled' >>> + * -> Status property must be first on the record list >>> + * >>> + * Disabled device with status change to 'okay' >>> + * -> Status property must be last in the record list >>> + * >>> + * That way we don't need a special notifier for >>> + * device status change, a simple notifier on the status >>> + * property is enough. >>> + * >>> + */ >> >> Is this true anymore? The notifiers are held back until all the changes >> are made to the tree, so regardless of what order the notifiers are >> emitted in, the tree will always be in the correct state. >> > > The tree will be in the correct state. The semantics are iffy regarding > what happens when the order of notifiers is not defined. > > In theory it shouldn't matter, so I'll try to remove this. Thanks. > >>> + >>> + /* note that we require the existence of a status property */ >>> + prev_avail = of_device_is_available(target) && >>> + of_find_property(target, >>> + "compatible", NULL) && >>> + of_find_property(target, >>> + "status", NULL); >>> + >>> + /* we make two passes */ >>> + for (pass = 1; pass <= 2; pass++) { >>> + >>> + for_each_property_of_node(overlay, prop) { >>> + >>> + prop_avail = -1; >>> + >>> + if (of_prop_cmp(prop->name, "status") == 0) >>> + prop_avail = strcmp(prop->value, "okay") == 0 || >>> + strcmp(prop->value, "ok") == 0; >>> + >>> + /* skip activation property */ >>> + if (prev_avail == 0) { >>> + /* 0 -> 1, pass #1, skip */ >>> + if (pass == 1) { >>> + if (prop_avail == 1) >>> + continue; >>> + } else { >>> + /* 0 -> 1, pass #2, process */ >>> + if (prop_avail != 1) >>> + continue; >>> + } >>> + } else { >>> + if (pass == 1) { >>> + /* 1 -> 0, pass #1, process */ >>> + if (prop_avail != 0) >>> + continue; >>> + } else { >>> + /* 1 -> 0, pass #2, skip */ >>> + if (prop_avail == 0) >>> + continue; >>> + } >>> + } >>> + >>> + ret = of_overlay_apply_single_property(ov, >>> + target, prop); >>> + if (ret != 0) { >>> + pr_err("%s: Failed to apply prop @%s/%s\n", >>> + __func__, target->full_name, >>> + prop->name); >>> + return ret; >>> + } >>> + } >>> + } >>> + >>> + for_each_child_of_node(overlay, child) { >>> + ret = of_overlay_apply_single_device_node(ov, target, child); >>> + if (ret != 0) { >>> + pr_err("%s: Failed to apply single node @%s/%s\n", >>> + __func__, target->full_name, >>> + child->name); >>> + return ret; >>> + } >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +/** >>> + * of_overlay_apply - Apply @count overlays pointed at by @ovinfo_tab >>> + * @ov: Overlay to apply >>> + * >>> + * Applies the overlays given, while handling all error conditions >>> + * appropriately. Either the operation succeeds, or if it fails the >>> + * live tree is reverted to the state before the attempt. >>> + * Returns 0, or an error if the overlay attempt failed. >>> + */ >>> +static int of_overlay_apply(struct of_overlay *ov) >>> +{ >>> + struct of_overlay_info *ovinfo; >>> + int i, err; >>> + >>> + /* first we apply the overlays atomically */ >>> + for (i = 0; i < ov->count; i++) { >>> + >>> + ovinfo = &ov->ovinfo_tab[i]; >>> + >>> + err = of_overlay_apply_one(ov, ovinfo->target, >>> + ovinfo->overlay); >>> + if (err != 0) { >>> + pr_err("%s: overlay failed '%s'\n", >>> + __func__, ovinfo->target->full_name); >>> + return err; >>> + } >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +/* >>> + * Find the target node using a number of different strategies >>> + * in order of preference >>> + * >>> + * "target" property containing the phandle of the target >>> + * "target-path" property containing the path of the target >>> + * >>> + */ >>> +static struct device_node *find_target_node(struct device_node *info_node) >>> +{ >>> + const char *path; >>> + u32 val; >>> + int ret; >>> + >>> + /* first try to go by using the target as a phandle */ >>> + ret = of_property_read_u32(info_node, "target", &val); >>> + if (ret == 0) >>> + return of_find_node_by_phandle(val); >>> + >>> + /* now try to locate by path */ >>> + ret = of_property_read_string(info_node, "target-path", &path); >>> + if (ret == 0) >>> + return of_find_node_by_path(path); >>> + >>> + pr_err("%s: Failed to find target for node %p (%s)\n", __func__, >>> + info_node, info_node->name); >>> + >>> + return NULL; >>> +} >>> + >>> +/** >>> + * of_fill_overlay_info - Fill an overlay info structure >>> + * @ov Overlay to fill >>> + * @info_node: Device node containing the overlay >>> + * @ovinfo: Pointer to the overlay info structure to fill >>> + * >>> + * Fills an overlay info structure with the overlay information >>> + * from a device node. This device node must have a target property >>> + * which contains a phandle of the overlay target node, and an >>> + * __overlay__ child node which has the overlay contents. >>> + * Both ovinfo->target & ovinfo->overlay have their references taken. >>> + * >>> + * Returns 0 on success, or a negative error value. >>> + */ >>> +static int of_fill_overlay_info(struct of_overlay *ov, >>> + struct device_node *info_node, struct of_overlay_info *ovinfo) >>> +{ >>> + ovinfo->overlay = of_get_child_by_name(info_node, "__overlay__"); >>> + if (ovinfo->overlay == NULL) >>> + goto err_fail; >>> + >>> + ovinfo->target = find_target_node(info_node); >>> + if (ovinfo->target == NULL) >>> + goto err_fail; >>> + >>> + return 0; >>> + >>> +err_fail: >>> + of_node_put(ovinfo->target); >>> + of_node_put(ovinfo->overlay); >>> + >>> + memset(ovinfo, 0, sizeof(*ovinfo)); >>> + return -EINVAL; >>> +} >>> + >>> +/** >>> + * of_build_overlay_info - Build an overlay info array >>> + * @ov Overlay to build >>> + * @tree: Device node containing all the overlays >>> + * >>> + * Helper function that given a tree containing overlay information, >>> + * allocates and builds an overlay info array containing it, ready >>> + * for use using of_overlay_apply. >>> + * >>> + * Returns 0 on success with the @cntp @ovinfop pointers valid, >>> + * while on error a negative error value is returned. >>> + */ >>> +static int of_build_overlay_info(struct of_overlay *ov, >>> + struct device_node *tree) >>> +{ >>> + struct device_node *node; >>> + struct of_overlay_info *ovinfo; >>> + int cnt, err; >>> + >>> + /* worst case; every child is a node */ >>> + cnt = 0; >>> + for_each_child_of_node(tree, node) >>> + cnt++; >>> + >>> + ovinfo = kcalloc(cnt, sizeof(*ovinfo), GFP_KERNEL); >>> + if (ovinfo == NULL) >>> + return -ENOMEM; >>> + >>> + cnt = 0; >>> + for_each_child_of_node(tree, node) { >>> + >>> + memset(&ovinfo[cnt], 0, sizeof(*ovinfo)); >>> + err = of_fill_overlay_info(ov, node, &ovinfo[cnt]); >>> + if (err == 0) >>> + cnt++; >>> + } >>> + >>> + /* if nothing filled, return error */ >>> + if (cnt == 0) { >>> + kfree(ovinfo); >>> + return -ENODEV; >>> + } >>> + >>> + ov->count = cnt; >>> + ov->ovinfo_tab = ovinfo; >>> + >>> + return 0; >>> +} >>> + >>> +/** >>> + * of_free_overlay_info - Free an overlay info array >>> + * @ov Overlay to free the overlay info from >>> + * @ovinfo_tab: Array of overlay_info's to free >>> + * >>> + * Releases the memory of a previously allocated ovinfo array >>> + * by of_build_overlay_info. >>> + * Returns 0, or an error if the arguments are bogus. >>> + */ >>> +static int of_free_overlay_info(struct of_overlay *ov) >>> +{ >>> + struct of_overlay_info *ovinfo; >>> + int i; >>> + >>> + /* do it in reverse */ >>> + for (i = ov->count - 1; i >= 0; i--) { >>> + ovinfo = &ov->ovinfo_tab[i]; >>> + >>> + of_node_put(ovinfo->target); >>> + of_node_put(ovinfo->overlay); >>> + } >>> + kfree(ov->ovinfo_tab); >>> + >>> + return 0; >>> +} >>> + >>> +static LIST_HEAD(ov_list); >>> +static DEFINE_MUTEX(ov_lock); >>> +static DEFINE_IDR(ov_idr); >>> + >>> +/** >>> + * of_overlay_create - Create and apply an overlay >>> + * @tree: Device node containing all the overlays >>> + * >>> + * Creates and applies an overlay while also keeping track >>> + * of the overlay in a list. This list can be used to prevent >>> + * illegal overlay removals. >>> + * >>> + * Returns the id of the created overlay, or an negative error number >>> + */ >>> +int of_overlay_create(struct device_node *tree) >>> +{ >>> + struct of_overlay *ov; >>> + int err, id; >>> + >>> + /* allocate the overlay structure */ >>> + ov = kzalloc(sizeof(*ov), GFP_KERNEL); >>> + if (ov == NULL) >>> + return -ENOMEM; >>> + ov->id = -1; >>> + >>> + INIT_LIST_HEAD(&ov->node); >>> + mutex_lock(&ov_lock); >> >> Holding the of_mutex lock over the entire operation is probably >> sufficent. I don't think the locking needs the complexity of multiple >> mutexes. >> > > Hmm, probably yes. My only concern is that we'll need to hold of_mutex > for as long as we're performing the overlay_removal_check() below. > > This is potentially an expensive operation during which no other > changes to the tree can occur. > > If that's fine I'll go and remove the ov_lock. I'm fine with that. > >>> + >>> + of_changeset_init(&ov->cset); >>> + >>> + id = idr_alloc(&ov_idr, ov, 0, 0, GFP_KERNEL); >>> + if (id < 0) { >>> + pr_err("%s: idr_alloc() failed for tree@%s\n", >>> + __func__, tree->full_name); >>> + err = id; >>> + goto err_destroy_trans; >>> + } >>> + ov->id = id; >>> + >>> + /* build the overlay info structures */ >>> + err = of_build_overlay_info(ov, tree); >>> + if (err) { >>> + pr_err("%s: of_build_overlay_info() failed for tree@%s\n", >>> + __func__, tree->full_name); >>> + goto err_free_idr; >>> + } >>> + >>> + mutex_lock(&of_mutex); >>> + >>> + /* apply the overlay */ >>> + err = of_overlay_apply(ov); >>> + if (err) { >>> + pr_err("%s: of_overlay_apply() failed for tree@%s\n", >>> + __func__, tree->full_name); >>> + goto err_abort_trans; >>> + } >>> + >>> + /* apply the changeset */ >>> + err = of_changeset_apply(&ov->cset); >>> + if (err) { >>> + pr_err("%s: of_changeset_apply() failed for tree@%s\n", >>> + __func__, tree->full_name); >>> + goto err_revert_overlay; >>> + } >>> + >>> + mutex_unlock(&of_mutex); >>> + >>> + /* add to the tail of the overlay list */ >>> + list_add_tail(&ov->node, &ov_list); >>> + >>> + mutex_unlock(&ov_lock); >>> + >>> + return id; >>> + >>> +err_revert_overlay: >>> +err_abort_trans: >>> + of_free_overlay_info(ov); >>> + mutex_unlock(&of_mutex); >>> +err_free_idr: >>> + idr_remove(&ov_idr, ov->id); >>> +err_destroy_trans: >>> + of_changeset_destroy(&ov->cset); >>> + mutex_unlock(&ov_lock); >>> + kfree(ov); >>> + >>> + return err; >>> +} >>> +EXPORT_SYMBOL_GPL(of_overlay_create); >>> + >>> +/* check whether the given node, lies under the given tree */ >>> +static int overlay_subtree_check(struct device_node *tree, >>> + struct device_node *dn) >>> +{ >>> + struct device_node *child; >>> + >>> + /* match? */ >>> + if (tree == dn) >>> + return 1; >>> + >>> + for_each_child_of_node(tree, child) { >>> + if (overlay_subtree_check(child, dn)) >>> + return 1; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +/* check whether this overlay is the topmost */ >>> +static int overlay_is_topmost(struct of_overlay *ov, struct device_node *dn) >>> +{ >>> + struct of_overlay *ovt; >>> + struct of_changeset_entry *ce; >>> + >>> + list_for_each_entry_reverse(ovt, &ov_list, node) { >>> + >>> + /* if we hit ourselves, we're done */ >>> + if (ovt == ov) >>> + break; >>> + >>> + /* check against each subtree affected by this overlay */ >>> + list_for_each_entry(ce, &ovt->cset.entries, node) { >>> + if (overlay_subtree_check(ce->np, dn)) { >>> + pr_err("%s: #%d clashes #%d @%s\n", >>> + __func__, ov->id, ovt->id, >>> + dn->full_name); >>> + return 0; >>> + } >>> + } >>> + } >>> + >>> + /* overlay is topmost */ >>> + return 1; >>> +} >>> + >>> +/* >>> + * We can safely remove the overlay only if it's the top-most one. >>> + * Newly applied overlays are inserted at the tail of the overlay list, >>> + * so a top most overlay is the one that is closest to the tail. >>> + * >>> + * The topmost check is done by exploiting this property. For each >>> + * affected device node in the log list we check if this overlay is >>> + * the one closest to the tail. If another overlay has affected this >>> + * device node and is closest to the tail, then removal is not permited. >>> + */ >>> +static int overlay_removal_is_ok(struct of_overlay *ov) >>> +{ >>> + struct of_changeset_entry *ce; >>> + >>> + list_for_each_entry(ce, &ov->cset.entries, node) { >>> + if (!overlay_is_topmost(ov, ce->np)) { >>> + pr_err("%s: overlay #%d is not topmost\n", >>> + __func__, ov->id); >>> + return 0; >>> + } >>> + } >>> + >>> + return 1; >>> +} >>> + >>> +/** >>> + * of_overlay_destroy - Removes an overlay >>> + * @id: Overlay id number returned by a previous call to of_overlay_create >>> + * >>> + * Removes an overlay if it is permissible. >>> + * >>> + * Returns 0 on success, or an negative error number >>> + */ >>> +int of_overlay_destroy(int id) >>> +{ >>> + struct of_overlay *ov; >>> + int err; >>> + >>> + mutex_lock(&ov_lock); >>> + ov = idr_find(&ov_idr, id); >>> + if (ov == NULL) { >>> + err = -ENODEV; >>> + pr_err("%s: Could not find overlay #%d\n", >>> + __func__, id); >>> + goto out; >>> + } >>> + >>> + /* check whether the overlay is safe to remove */ >>> + if (!overlay_removal_is_ok(ov)) { >>> + err = -EBUSY; >>> + pr_err("%s: removal check failed for overlay #%d\n", >>> + __func__, id); >>> + goto out; >>> + } >>> + >>> + >>> + list_del(&ov->node); >>> + >>> + mutex_lock(&of_mutex); >>> + of_changeset_revert(&ov->cset); >>> + mutex_unlock(&of_mutex); >>> + >>> + of_free_overlay_info(ov); >>> + idr_remove(&ov_idr, id); >>> + of_changeset_destroy(&ov->cset); >>> + kfree(ov); >>> + >>> + err = 0; >>> + >>> +out: >>> + mutex_unlock(&ov_lock); >>> + >>> + return err; >>> +} >>> +EXPORT_SYMBOL_GPL(of_overlay_destroy); >>> + >>> +/** >>> + * of_overlay_destroy_all - Removes all overlays from the system >>> + * >>> + * Removes all overlays from the system in the correct order. >>> + * >>> + * Returns 0 on success, or an negative error number >>> + */ >>> +int of_overlay_destroy_all(void) >>> +{ >>> + struct of_overlay *ov, *ovn; >>> + >>> + mutex_lock(&ov_lock); >>> + >>> + /* the tail of list is guaranteed to be safe to remove */ >>> + list_for_each_entry_safe_reverse(ov, ovn, &ov_list, node) { >>> + list_del(&ov->node); >>> + >>> + mutex_lock(&of_mutex); >>> + of_changeset_revert(&ov->cset); >>> + mutex_unlock(&of_mutex); >>> + >>> + of_free_overlay_info(ov); >>> + idr_remove(&ov_idr, ov->id); >>> + kfree(ov); >>> + } >>> + >>> + >>> + mutex_unlock(&ov_lock); >>> + >>> + return 0; >>> +} >>> +EXPORT_SYMBOL_GPL(of_overlay_destroy_all); >>> diff --git a/include/linux/of.h b/include/linux/of.h >>> index 9ff1ec5..9e3e545 100644 >>> --- a/include/linux/of.h >>> +++ b/include/linux/of.h >>> @@ -23,6 +23,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> >>> #include >>> #include >>> @@ -873,4 +874,34 @@ static inline int of_changeset_update_property(struct of_changeset *ocs, >>> /* CONFIG_OF_RESOLVE api */ >>> extern int of_resolve_phandles(struct device_node *tree); >>> >>> +/** >>> + * Overlay support >>> + */ >>> + >>> +#ifdef CONFIG_OF_OVERLAY >>> + >>> +/* ID based overlays; the API for external users */ >>> +int of_overlay_create(struct device_node *tree); >>> +int of_overlay_destroy(int id); >>> +int of_overlay_destroy_all(void); >>> + >>> +#else >>> + >>> +static inline int of_overlay_create(struct device_node *tree) >>> +{ >>> + return -ENOTSUPP; >>> +} >>> + >>> +static inline int of_overlay_destroy(int id) >>> +{ >>> + return -ENOTSUPP; >>> +} >>> + >>> +static inline int of_overlay_destroy_all(void) >>> +{ >>> + return -ENOTSUPP; >>> +} >>> + >>> +#endif >>> + >>> #endif /* _LINUX_OF_H */ >>> -- >>> 1.7.12 > > I'll try to generate an incremental patch today if possible. Great! Here's my current tree: git://git.kernel.org/pub/scm/linux/kernel/git/glikely/linux devicetree/next-overlay g. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/