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

[Pantelis Antoniou]

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. 

>> +
>> +	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).

>> +
>> +	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.

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.

>> +
>> +	}
>> +
>> +	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.

>> +
>> +	/* 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.

>> +
>> +	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.

Regards

â Pantelis

--
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/