Re: [PATCH v8 1/8] OF: Introduce DT overlay support. (v2)

From: Grant Likely
Date: Mon Nov 17 2014 - 17:55:08 EST

On Mon, Nov 17, 2014 at 2:35 PM, Pantelis Antoniou
<pantelis.antoniou@xxxxxxxxxxxx> wrote:
> Hi Grant,
>
>> On Nov 14, 2014, at 01:36 , Grant Likely <grant.likely@xxxxxxxxxxxx> wrote:
>>
>> On Tue, 28 Oct 2014 22:35:58 +0200
>> , Pantelis Antoniou <pantelis.antoniou@xxxxxxxxxxxx>
>> 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 <pantelis.antoniou@xxxxxxxxxxxx>
>>> ---
>>> 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>; /* 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=<phandle> 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 <panto@xxxxxxxxxxxxxxxxxxxxxxx>
>>> + * 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 <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/string.h>
>>> +#include <linux/ctype.h>
>>> +#include <linux/errno.h>
>>> +#include <linux/string.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/err.h>
>>> +
>>> +#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 <linux/spinlock.h>
>>> #include <linux/topology.h>
>>> #include <linux/notifier.h>
>>> +#include <linux/list.h>
>>>
>>> #include <asm/byteorder.h>
>>> #include <asm/errno.h>
>>> @@ -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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/