[PATCH 0/3] OF: kobj-ification fixes

[PATCH 0/3] OF: kobj-ification fixes

[Pantelis Antoniou]

The recent patchset by Grant converted the DT dynamic interface from /proc
to using sysfs.

This patchset addresses various problems that come up when using it as a
base for the subsequent dynamic overlays patches.

Pantelis Antoniou (3):
  of: Fix early OF builtup on kobj-ification
  OF: Add a allnodes pointer back to the tree
  OF: kobj ops should only happen on attached nodes

 drivers/of/base.c  | 112 ++++++++++++++++++++++++++++++++++++++++-------------
 drivers/of/fdt.c   |  10 +++--
 include/linux/of.h |   1 +
 3 files changed, 94 insertions(+), 29 deletions(-)

-- 
1.7.12

[PATCH 0/3] OF: kobj-ification fixes

[Pantelis Antoniou]

The recent patchset by Grant converted the DT dynamic interface from /proc
to using sysfs.

This patchset addresses various problems that come up when using it as a
base for the subsequent dynamic overlays patches.

Pantelis Antoniou (3):
  of: Fix early OF builtup on kobj-ification
  OF: Add a allnodes pointer back to the tree
  OF: kobj ops should only happen on attached nodes

 drivers/of/base.c  | 112 ++++++++++++++++++++++++++++++++++++++++-------------
 drivers/of/fdt.c   |  10 +++--
 include/linux/of.h |   1 +
 3 files changed, 94 insertions(+), 29 deletions(-)

-- 
1.7.12

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] of: Fix early OF builtup on kobj-ification

[Pantelis Antoniou]

When booting platforms that do very early OF initialization before
core_initcalls are performed of_init is called too late.

This results in a hard-hard without getting a chance to output anything.

Fixed by adding a flag that marks when init has been done, and
performing the per-node init at that time.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 drivers/of/base.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 734689b..a4f3dda 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -98,7 +98,7 @@ int __weak of_node_to_nid(struct device_node *np)
  */
 struct device_node *of_node_get(struct device_node *node)
 {
-	if (node)
+	if (node && of_kset)
 		kobject_get(&node->kobj);
 	return node;
 }
@@ -156,7 +156,7 @@ static void of_node_release(struct kobject *kobj)
  */
 void of_node_put(struct device_node *node)
 {
-	if (node)
+	if (node && of_kset)
 		kobject_put(&node->kobj);
 }
 EXPORT_SYMBOL(of_node_put);
@@ -202,9 +202,16 @@ static const char *safe_name(struct kobject *kobj, const char *orig_name)
 static int __of_add_property(struct device_node *np, struct property *pp)
 {
 	int rc;
+	bool secure;
+
+	/* note that we don't take a lock here */
+
+	/* fake the return while of_init is not yet called */
+	if (!of_kset)
+		return 0;
 
 	/* Important: Don't leak passwords */
-	bool secure = strncmp(pp->name, "security-", 9) == 0;
+	secure = strncmp(pp->name, "security-", 9) == 0;
 
 	pp->attr.attr.name = safe_name(&np->kobj, pp->name);
 	pp->attr.attr.mode = secure ? S_IRUSR : S_IRUGO;
@@ -222,6 +229,12 @@ static int __of_node_add(struct device_node *np)
 	struct property *pp;
 	int rc;
 
+	/* note that we don't take a lock here */
+
+	/* fake the return while of_init is not yet called */
+	if (!of_kset)
+		return 0;
+
 	np->kobj.kset = of_kset;
 	if (!np->parent) {
 		/* Nodes without parents are new top level trees */
@@ -245,11 +258,14 @@ static int __of_node_add(struct device_node *np)
 int of_node_add(struct device_node *np)
 {
 	int rc = 0;
-	kobject_init(&np->kobj, &of_node_ktype);
+
+	/* fake the return while of_init is not yet called */
 	mutex_lock(&of_aliases_mutex);
+	kobject_init(&np->kobj, &of_node_ktype);
 	if (of_kset)
 		rc = __of_node_add(np);
 	mutex_unlock(&of_aliases_mutex);
+
 	return rc;
 }
 
@@ -275,14 +291,16 @@ static int __init of_init(void)
 
 	/* Make sure all nodes added before this time get added to sysfs */
 	mutex_lock(&of_aliases_mutex);
+
 	for_each_of_allnodes(np)
 		__of_node_add(np);
-	mutex_unlock(&of_aliases_mutex);
 
 	/* Symlink in /proc as required by userspace ABI */
 	if (of_allnodes)
 		proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
 
+	mutex_unlock(&of_aliases_mutex);
+
 	return 0;
 }
 core_initcall(of_init);
-- 
1.7.12

Re: [PATCH 1/3] of: Fix early OF builtup on kobj-ification

[Koen Kooi]

Op 10 dec. 2013, om 15:13 heeft Pantelis Antoniou <panto@antoniou-consulting.com> het volgende geschreven:

> When booting platforms that do very early OF initialization before
> core_initcalls are performed of_init is called too late.
> 
> This results in a hard-hard without getting a chance to output anything.

I think that should be 'hard-hang'.

regards,

Koen

> 
> Fixed by adding a flag that marks when init has been done, and
> performing the per-node init at that time.
> 
> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> ---
> drivers/of/base.c | 28 +++++++++++++++++++++++-----
> 1 file changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 734689b..a4f3dda 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -98,7 +98,7 @@ int __weak of_node_to_nid(struct device_node *np)
>  */
> struct device_node *of_node_get(struct device_node *node)
> {
> -	if (node)
> +	if (node && of_kset)
> 		kobject_get(&node->kobj);
> 	return node;
> }
> @@ -156,7 +156,7 @@ static void of_node_release(struct kobject *kobj)
>  */
> void of_node_put(struct device_node *node)
> {
> -	if (node)
> +	if (node && of_kset)
> 		kobject_put(&node->kobj);
> }
> EXPORT_SYMBOL(of_node_put);
> @@ -202,9 +202,16 @@ static const char *safe_name(struct kobject *kobj, const char *orig_name)
> static int __of_add_property(struct device_node *np, struct property *pp)
> {
> 	int rc;
> +	bool secure;
> +
> +	/* note that we don't take a lock here */
> +
> +	/* fake the return while of_init is not yet called */
> +	if (!of_kset)
> +		return 0;
> 
> 	/* Important: Don't leak passwords */
> -	bool secure = strncmp(pp->name, "security-", 9) == 0;
> +	secure = strncmp(pp->name, "security-", 9) == 0;
> 
> 	pp->attr.attr.name = safe_name(&np->kobj, pp->name);
> 	pp->attr.attr.mode = secure ? S_IRUSR : S_IRUGO;
> @@ -222,6 +229,12 @@ static int __of_node_add(struct device_node *np)
> 	struct property *pp;
> 	int rc;
> 
> +	/* note that we don't take a lock here */
> +
> +	/* fake the return while of_init is not yet called */
> +	if (!of_kset)
> +		return 0;
> +
> 	np->kobj.kset = of_kset;
> 	if (!np->parent) {
> 		/* Nodes without parents are new top level trees */
> @@ -245,11 +258,14 @@ static int __of_node_add(struct device_node *np)
> int of_node_add(struct device_node *np)
> {
> 	int rc = 0;
> -	kobject_init(&np->kobj, &of_node_ktype);
> +
> +	/* fake the return while of_init is not yet called */
> 	mutex_lock(&of_aliases_mutex);
> +	kobject_init(&np->kobj, &of_node_ktype);
> 	if (of_kset)
> 		rc = __of_node_add(np);
> 	mutex_unlock(&of_aliases_mutex);
> +
> 	return rc;
> }
> 
> @@ -275,14 +291,16 @@ static int __init of_init(void)
> 
> 	/* Make sure all nodes added before this time get added to sysfs */
> 	mutex_lock(&of_aliases_mutex);
> +
> 	for_each_of_allnodes(np)
> 		__of_node_add(np);
> -	mutex_unlock(&of_aliases_mutex);
> 
> 	/* Symlink in /proc as required by userspace ABI */
> 	if (of_allnodes)
> 		proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
> 
> +	mutex_unlock(&of_aliases_mutex);
> +
> 	return 0;
> }
> core_initcall(of_init);
> -- 
> 1.7.12
> 
> 

Re: [PATCH 1/3] of: Fix early OF builtup on kobj-ification

[Koen Kooi]

Op 10 dec. 2013, om 15:13 heeft Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org> het volgende geschreven:

> When booting platforms that do very early OF initialization before
> core_initcalls are performed of_init is called too late.
> 
> This results in a hard-hard without getting a chance to output anything.

I think that should be 'hard-hang'.

regards,

Koen

> 
> Fixed by adding a flag that marks when init has been done, and
> performing the per-node init at that time.
> 
> Signed-off-by: Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>
> ---
> drivers/of/base.c | 28 +++++++++++++++++++++++-----
> 1 file changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 734689b..a4f3dda 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -98,7 +98,7 @@ int __weak of_node_to_nid(struct device_node *np)
>  */
> struct device_node *of_node_get(struct device_node *node)
> {
> -	if (node)
> +	if (node && of_kset)
> 		kobject_get(&node->kobj);
> 	return node;
> }
> @@ -156,7 +156,7 @@ static void of_node_release(struct kobject *kobj)
>  */
> void of_node_put(struct device_node *node)
> {
> -	if (node)
> +	if (node && of_kset)
> 		kobject_put(&node->kobj);
> }
> EXPORT_SYMBOL(of_node_put);
> @@ -202,9 +202,16 @@ static const char *safe_name(struct kobject *kobj, const char *orig_name)
> static int __of_add_property(struct device_node *np, struct property *pp)
> {
> 	int rc;
> +	bool secure;
> +
> +	/* note that we don't take a lock here */
> +
> +	/* fake the return while of_init is not yet called */
> +	if (!of_kset)
> +		return 0;
> 
> 	/* Important: Don't leak passwords */
> -	bool secure = strncmp(pp->name, "security-", 9) == 0;
> +	secure = strncmp(pp->name, "security-", 9) == 0;
> 
> 	pp->attr.attr.name = safe_name(&np->kobj, pp->name);
> 	pp->attr.attr.mode = secure ? S_IRUSR : S_IRUGO;
> @@ -222,6 +229,12 @@ static int __of_node_add(struct device_node *np)
> 	struct property *pp;
> 	int rc;
> 
> +	/* note that we don't take a lock here */
> +
> +	/* fake the return while of_init is not yet called */
> +	if (!of_kset)
> +		return 0;
> +
> 	np->kobj.kset = of_kset;
> 	if (!np->parent) {
> 		/* Nodes without parents are new top level trees */
> @@ -245,11 +258,14 @@ static int __of_node_add(struct device_node *np)
> int of_node_add(struct device_node *np)
> {
> 	int rc = 0;
> -	kobject_init(&np->kobj, &of_node_ktype);
> +
> +	/* fake the return while of_init is not yet called */
> 	mutex_lock(&of_aliases_mutex);
> +	kobject_init(&np->kobj, &of_node_ktype);
> 	if (of_kset)
> 		rc = __of_node_add(np);
> 	mutex_unlock(&of_aliases_mutex);
> +
> 	return rc;
> }
> 
> @@ -275,14 +291,16 @@ static int __init of_init(void)
> 
> 	/* Make sure all nodes added before this time get added to sysfs */
> 	mutex_lock(&of_aliases_mutex);
> +
> 	for_each_of_allnodes(np)
> 		__of_node_add(np);
> -	mutex_unlock(&of_aliases_mutex);
> 
> 	/* Symlink in /proc as required by userspace ABI */
> 	if (of_allnodes)
> 		proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
> 
> +	mutex_unlock(&of_aliases_mutex);
> +
> 	return 0;
> }
> core_initcall(of_init);
> -- 
> 1.7.12
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] of: Fix early OF builtup on kobj-ification

[Grant Likely]

On Tue, Dec 10, 2013 at 2:13 PM, Pantelis Antoniou
<panto@antoniou-consulting.com> wrote:
> When booting platforms that do very early OF initialization before
> core_initcalls are performed of_init is called too late.
>
> This results in a hard-hard without getting a chance to output anything.
>
> Fixed by adding a flag that marks when init has been done, and
> performing the per-node init at that time.
>
> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> ---
>  drivers/of/base.c | 28 +++++++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 734689b..a4f3dda 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -98,7 +98,7 @@ int __weak of_node_to_nid(struct device_node *np)
>   */
>  struct device_node *of_node_get(struct device_node *node)
>  {
> -       if (node)
> +       if (node && of_kset)
>                 kobject_get(&node->kobj);
>         return node;
>  }
> @@ -156,7 +156,7 @@ static void of_node_release(struct kobject *kobj)
>   */
>  void of_node_put(struct device_node *node)
>  {
> -       if (node)
> +       if (node && of_kset)
>                 kobject_put(&node->kobj);
>  }

As discussed on IRC, I don't think this is required.

>  EXPORT_SYMBOL(of_node_put);
> @@ -202,9 +202,16 @@ static const char *safe_name(struct kobject *kobj, const char *orig_name)
>  static int __of_add_property(struct device_node *np, struct property *pp)
>  {
>         int rc;
> +       bool secure;
> +
> +       /* note that we don't take a lock here */
> +
> +       /* fake the return while of_init is not yet called */
> +       if (!of_kset)
> +               return 0;

__of_add_property is called from three functions; __of_node_add(),
of_add_property() and of_modify_property(). __of_node_add is never
called on an unattached node. If the other two are getting called
early, then I think they should make the check. I'll modify my patch
to reflect that.

>
>         /* Important: Don't leak passwords */
> -       bool secure = strncmp(pp->name, "security-", 9) == 0;
> +       secure = strncmp(pp->name, "security-", 9) == 0;
>
>         pp->attr.attr.name = safe_name(&np->kobj, pp->name);
>         pp->attr.attr.mode = secure ? S_IRUSR : S_IRUGO;
> @@ -222,6 +229,12 @@ static int __of_node_add(struct device_node *np)
>         struct property *pp;
>         int rc;
>
> +       /* note that we don't take a lock here */
> +
> +       /* fake the return while of_init is not yet called */
> +       if (!of_kset)
> +               return 0;
> +

__of_node_add() never gets called before of_kset is initialized.

>         np->kobj.kset = of_kset;
>         if (!np->parent) {
>                 /* Nodes without parents are new top level trees */
> @@ -245,11 +258,14 @@ static int __of_node_add(struct device_node *np)
>  int of_node_add(struct device_node *np)
>  {
>         int rc = 0;
> -       kobject_init(&np->kobj, &of_node_ktype);
> +
> +       /* fake the return while of_init is not yet called */
>         mutex_lock(&of_aliases_mutex);
> +       kobject_init(&np->kobj, &of_node_ktype);

Why the kobject_init() move?

>         if (of_kset)
>                 rc = __of_node_add(np);
>         mutex_unlock(&of_aliases_mutex);
> +
>         return rc;
>  }
>
> @@ -275,14 +291,16 @@ static int __init of_init(void)
>
>         /* Make sure all nodes added before this time get added to sysfs */
>         mutex_lock(&of_aliases_mutex);
> +
>         for_each_of_allnodes(np)
>                 __of_node_add(np);
> -       mutex_unlock(&of_aliases_mutex);
>
>         /* Symlink in /proc as required by userspace ABI */
>         if (of_allnodes)
>                 proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
>
> +       mutex_unlock(&of_aliases_mutex);
> +
>         return 0;
>  }
>  core_initcall(of_init);
> --
> 1.7.12
>

[PATCH 2/3] OF: Add a allnodes pointer back to the tree

[Pantelis Antoniou]

As it stands right now there's no way for a node to point back
to allnodes that the top level tree tracker for now.

This is problematic when using unflatten_dt_node more than once
because is leads to the nodes to be linked incorrectly.

Keep track of the root allnodes on each device_node and modify
unflaten_dt_node to match.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 drivers/of/base.c  |  1 +
 drivers/of/fdt.c   | 10 +++++++---
 include/linux/of.h |  1 +
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index a4f3dda..db0de86 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1791,6 +1791,7 @@ int of_attach_node(struct device_node *np)
 	np->allnext = of_allnodes;
 	np->parent->child = np;
 	of_allnodes = np;
+	np->allnodes = &of_allnodes;
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
 	of_node_add(np);
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index d5d62cd..b09b1c2 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -145,6 +145,7 @@ static void *unflatten_dt_alloc(void **mem, unsigned long size,
  * @p: pointer to node in flat tree
  * @dad: Parent struct device_node
  * @allnextpp: pointer to ->allnext from last allocated device_node
+ * @mynodes: The device_node tree
  * @fpsize: Size of the node path up at the current depth.
  */
 static void * unflatten_dt_node(struct boot_param_header *blob,
@@ -152,6 +153,7 @@ static void * unflatten_dt_node(struct boot_param_header *blob,
 				void **p,
 				struct device_node *dad,
 				struct device_node ***allnextpp,
+				struct device_node **mynodes,
 				unsigned long fpsize)
 {
 	struct device_node *np;
@@ -327,6 +329,7 @@ static void * unflatten_dt_node(struct boot_param_header *blob,
 		if (!np->type)
 			np->type = "<NULL>";
 
+		np->allnodes = mynodes;
 		of_node_add(np);
 	}
 	while (tag == OF_DT_BEGIN_NODE || tag == OF_DT_NOP) {
@@ -334,7 +337,7 @@ static void * unflatten_dt_node(struct boot_param_header *blob,
 			*p += 4;
 		else
 			mem = unflatten_dt_node(blob, mem, p, np, allnextpp,
-						fpsize);
+						mynodes, fpsize);
 		tag = be32_to_cpup(*p);
 	}
 	if (tag != OF_DT_END_NODE) {
@@ -384,7 +387,8 @@ static void __unflatten_device_tree(struct boot_param_header *blob,
 
 	/* First pass, scan for size */
 	start = ((void *)blob) + be32_to_cpu(blob->off_dt_struct);
-	size = (unsigned long)unflatten_dt_node(blob, 0, &start, NULL, NULL, 0);
+	size = (unsigned long)unflatten_dt_node(blob, 0, &start, NULL, NULL,
+			mynodes, 0);
 	size = ALIGN(size, 4);
 
 	pr_debug("  size is %lx, allocating...\n", size);
@@ -399,7 +403,7 @@ static void __unflatten_device_tree(struct boot_param_header *blob,
 
 	/* Second pass, do actual unflattening */
 	start = ((void *)blob) + be32_to_cpu(blob->off_dt_struct);
-	unflatten_dt_node(blob, mem, &start, NULL, &allnextp, 0);
+	unflatten_dt_node(blob, mem, &start, NULL, &allnextp, mynodes, 0);
 	if (be32_to_cpup(start) != OF_DT_END)
 		pr_warning("Weird tag at end of tree: %08x\n", be32_to_cpup(start));
 	if (be32_to_cpup(mem + size) != 0xdeadbeef)
diff --git a/include/linux/of.h b/include/linux/of.h
index f285222..c5ca1f2 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -57,6 +57,7 @@ struct device_node {
 	struct	device_node *sibling;
 	struct	device_node *next;	/* next device of same type */
 	struct	device_node *allnext;	/* next in list of all nodes */
+	struct  device_node **allnodes;	/* pointer to the tree */
 	struct	kobject kobj;
 	unsigned long _flags;
 	void	*data;
-- 
1.7.12

[PATCH 2/3] OF: Add a allnodes pointer back to the tree

[Pantelis Antoniou]

As it stands right now there's no way for a node to point back
to allnodes that the top level tree tracker for now.

This is problematic when using unflatten_dt_node more than once
because is leads to the nodes to be linked incorrectly.

Keep track of the root allnodes on each device_node and modify
unflaten_dt_node to match.

Signed-off-by: Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>
---
 drivers/of/base.c  |  1 +
 drivers/of/fdt.c   | 10 +++++++---
 include/linux/of.h |  1 +
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index a4f3dda..db0de86 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1791,6 +1791,7 @@ int of_attach_node(struct device_node *np)
 	np->allnext = of_allnodes;
 	np->parent->child = np;
 	of_allnodes = np;
+	np->allnodes = &of_allnodes;
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
 	of_node_add(np);
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index d5d62cd..b09b1c2 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -145,6 +145,7 @@ static void *unflatten_dt_alloc(void **mem, unsigned long size,
  * @p: pointer to node in flat tree
  * @dad: Parent struct device_node
  * @allnextpp: pointer to ->allnext from last allocated device_node
+ * @mynodes: The device_node tree
  * @fpsize: Size of the node path up at the current depth.
  */
 static void * unflatten_dt_node(struct boot_param_header *blob,
@@ -152,6 +153,7 @@ static void * unflatten_dt_node(struct boot_param_header *blob,
 				void **p,
 				struct device_node *dad,
 				struct device_node ***allnextpp,
+				struct device_node **mynodes,
 				unsigned long fpsize)
 {
 	struct device_node *np;
@@ -327,6 +329,7 @@ static void * unflatten_dt_node(struct boot_param_header *blob,
 		if (!np->type)
 			np->type = "<NULL>";
 
+		np->allnodes = mynodes;
 		of_node_add(np);
 	}
 	while (tag == OF_DT_BEGIN_NODE || tag == OF_DT_NOP) {
@@ -334,7 +337,7 @@ static void * unflatten_dt_node(struct boot_param_header *blob,
 			*p += 4;
 		else
 			mem = unflatten_dt_node(blob, mem, p, np, allnextpp,
-						fpsize);
+						mynodes, fpsize);
 		tag = be32_to_cpup(*p);
 	}
 	if (tag != OF_DT_END_NODE) {
@@ -384,7 +387,8 @@ static void __unflatten_device_tree(struct boot_param_header *blob,
 
 	/* First pass, scan for size */
 	start = ((void *)blob) + be32_to_cpu(blob->off_dt_struct);
-	size = (unsigned long)unflatten_dt_node(blob, 0, &start, NULL, NULL, 0);
+	size = (unsigned long)unflatten_dt_node(blob, 0, &start, NULL, NULL,
+			mynodes, 0);
 	size = ALIGN(size, 4);
 
 	pr_debug("  size is %lx, allocating...\n", size);
@@ -399,7 +403,7 @@ static void __unflatten_device_tree(struct boot_param_header *blob,
 
 	/* Second pass, do actual unflattening */
 	start = ((void *)blob) + be32_to_cpu(blob->off_dt_struct);
-	unflatten_dt_node(blob, mem, &start, NULL, &allnextp, 0);
+	unflatten_dt_node(blob, mem, &start, NULL, &allnextp, mynodes, 0);
 	if (be32_to_cpup(start) != OF_DT_END)
 		pr_warning("Weird tag at end of tree: %08x\n", be32_to_cpup(start));
 	if (be32_to_cpup(mem + size) != 0xdeadbeef)
diff --git a/include/linux/of.h b/include/linux/of.h
index f285222..c5ca1f2 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -57,6 +57,7 @@ struct device_node {
 	struct	device_node *sibling;
 	struct	device_node *next;	/* next device of same type */
 	struct	device_node *allnext;	/* next in list of all nodes */
+	struct  device_node **allnodes;	/* pointer to the tree */
 	struct	kobject kobj;
 	unsigned long _flags;
 	void	*data;
-- 
1.7.12

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] OF: Add a allnodes pointer back to the tree

[Grant Likely]

On Tue, 10 Dec 2013 16:13:52 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
> As it stands right now there's no way for a node to point back
> to allnodes that the top level tree tracker for now.
> 
> This is problematic when using unflatten_dt_node more than once
> because is leads to the nodes to be linked incorrectly.
> 
> Keep track of the root allnodes on each device_node and modify
> unflaten_dt_node to match.

After our conversation on IRC and the decision to split unflattening the
tree from attaching it to be visible to the entire kernel, I'm going to
hold off on this patch. I suspect it will change in the process.

g.

Re: [PATCH 2/3] OF: Add a allnodes pointer back to the tree

[Grant Likely]

On Tue, 10 Dec 2013 16:13:52 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
> As it stands right now there's no way for a node to point back
> to allnodes that the top level tree tracker for now.
> 
> This is problematic when using unflatten_dt_node more than once
> because is leads to the nodes to be linked incorrectly.
> 
> Keep track of the root allnodes on each device_node and modify
> unflaten_dt_node to match.

After our conversation on IRC and the decision to split unflattening the
tree from attaching it to be visible to the entire kernel, I'm going to
hold off on this patch. I suspect it will change in the process.

g.

Re: [PATCH 2/3] OF: Add a allnodes pointer back to the tree

[Pantelis Antoniou]

Hi Grant,

On Dec 12, 2013, at 1:20 PM, Grant Likely wrote:

> On Tue, 10 Dec 2013 16:13:52 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
>> As it stands right now there's no way for a node to point back
>> to allnodes that the top level tree tracker for now.
>> 
>> This is problematic when using unflatten_dt_node more than once
>> because is leads to the nodes to be linked incorrectly.
>> 
>> Keep track of the root allnodes on each device_node and modify
>> unflaten_dt_node to match.
> 
> After our conversation on IRC and the decision to split unflattening the
> tree from attaching it to be visible to the entire kernel, I'm going to
> hold off on this patch. I suspect it will change in the process.
> 

OK,

I'm taking a look right now; should have something done on the weekend.

The whole idea is splitting unflattening from attaching to the live tree.

Should be straight forward enough.
 
> .
> 

Regards

-- Pantelis

Re: [PATCH 2/3] OF: Add a allnodes pointer back to the tree

[Pantelis Antoniou]

Hi Grant,

On Dec 12, 2013, at 1:20 PM, Grant Likely wrote:

> On Tue, 10 Dec 2013 16:13:52 +0200, Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org> wrote:
>> As it stands right now there's no way for a node to point back
>> to allnodes that the top level tree tracker for now.
>> 
>> This is problematic when using unflatten_dt_node more than once
>> because is leads to the nodes to be linked incorrectly.
>> 
>> Keep track of the root allnodes on each device_node and modify
>> unflaten_dt_node to match.
> 
> After our conversation on IRC and the decision to split unflattening the
> tree from attaching it to be visible to the entire kernel, I'm going to
> hold off on this patch. I suspect it will change in the process.
> 

OK,

I'm taking a look right now; should have something done on the weekend.

The whole idea is splitting unflattening from attaching to the live tree.

Should be straight forward enough.
 
> .
> 

Regards

-- Pantelis

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] OF: kobj ops should only happen on attached nodes

[Pantelis Antoniou]

kobject operation should only take place on already attached (live)
device nodes. The same restriction applies to the invocation of
device tree notifiers.

Introduce a simple of_node_is_attached() test which tests whether
the given node is attached via means of the allnodes member.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 drivers/of/base.c | 87 ++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 64 insertions(+), 23 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index db0de86..c6299bd 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -89,6 +89,13 @@ int __weak of_node_to_nid(struct device_node *np)
 #endif
 
 #if defined(CONFIG_OF_DYNAMIC)
+
+/* simple test for figuring out if node is attached */
+static inline int of_node_is_attached(struct device_node *np)
+{
+	return np && np->allnodes == &of_allnodes;
+}
+
 /**
  *	of_node_get - Increment refcount of a node
  *	@node:	Node to inc refcount, NULL is supported to
@@ -98,7 +105,7 @@ int __weak of_node_to_nid(struct device_node *np)
  */
 struct device_node *of_node_get(struct device_node *node)
 {
-	if (node && of_kset)
+	if (node && of_kset && of_node_is_attached(node))
 		kobject_get(&node->kobj);
 	return node;
 }
@@ -156,7 +163,7 @@ static void of_node_release(struct kobject *kobj)
  */
 void of_node_put(struct device_node *node)
 {
-	if (node && of_kset)
+	if (node && of_kset && of_node_is_attached(node))
 		kobject_put(&node->kobj);
 }
 EXPORT_SYMBOL(of_node_put);
@@ -210,6 +217,10 @@ static int __of_add_property(struct device_node *np, struct property *pp)
 	if (!of_kset)
 		return 0;
 
+	/* don't do anything with a detached node */
+	if (!of_node_is_attached(np))
+		return 0;
+
 	/* Important: Don't leak passwords */
 	secure = strncmp(pp->name, "security-", 9) == 0;
 
@@ -235,6 +246,9 @@ static int __of_node_add(struct device_node *np)
 	if (!of_kset)
 		return 0;
 
+	if (!of_node_is_attached(np))
+		return 0;
+
 	np->kobj.kset = of_kset;
 	if (!np->parent) {
 		/* Nodes without parents are new top level trees */
@@ -259,6 +273,9 @@ int of_node_add(struct device_node *np)
 {
 	int rc = 0;
 
+	if (!of_node_is_attached(np))
+		return 0;
+
 	/* fake the return while of_init is not yet called */
 	mutex_lock(&of_aliases_mutex);
 	kobject_init(&np->kobj, &of_node_ktype);
@@ -274,10 +291,16 @@ static void of_node_remove(struct device_node *np)
 {
 	struct property *pp;
 
+	/* don't do anything with a detached node */
+	if (!of_node_is_attached(np))
+		return;
+
 	for_each_property_of_node(np, pp)
 		sysfs_remove_bin_file(&np->kobj, &pp->attr);
 
 	kobject_del(&np->kobj);
+
+	of_node_put(np);
 }
 #endif
 
@@ -1626,9 +1649,11 @@ int of_add_property(struct device_node *np, struct property *prop)
 	unsigned long flags;
 	int rc;
 
-	rc = of_property_notify(OF_RECONFIG_ADD_PROPERTY, np, prop);
-	if (rc)
-		return rc;
+	if (of_node_is_attached(np)) {
+		rc = of_property_notify(OF_RECONFIG_ADD_PROPERTY, np, prop);
+		if (rc)
+			return rc;
+	}
 
 	prop->next = NULL;
 	raw_spin_lock_irqsave(&devtree_lock, flags);
@@ -1664,9 +1689,11 @@ int of_remove_property(struct device_node *np, struct property *prop)
 	int found = 0;
 	int rc;
 
-	rc = of_property_notify(OF_RECONFIG_REMOVE_PROPERTY, np, prop);
-	if (rc)
-		return rc;
+	if (of_node_is_attached(np)) {
+		rc = of_property_notify(OF_RECONFIG_REMOVE_PROPERTY, np, prop);
+		if (rc)
+			return rc;
+	}
 
 	raw_spin_lock_irqsave(&devtree_lock, flags);
 	next = &np->properties;
@@ -1686,7 +1713,8 @@ int of_remove_property(struct device_node *np, struct property *prop)
 	if (!found)
 		return -ENODEV;
 
-	sysfs_remove_bin_file(&np->kobj, &prop->attr);
+	if (of_node_is_attached(np))
+		sysfs_remove_bin_file(&np->kobj, &prop->attr);
 
 	return 0;
 }
@@ -1706,9 +1734,11 @@ int of_update_property(struct device_node *np, struct property *newprop)
 	unsigned long flags;
 	int rc, found = 0;
 
-	rc = of_property_notify(OF_RECONFIG_UPDATE_PROPERTY, np, newprop);
-	if (rc)
-		return rc;
+	if (of_node_is_attached(np)) {
+		rc = of_property_notify(OF_RECONFIG_UPDATE_PROPERTY, np, newprop);
+		if (rc)
+			return rc;
+	}
 
 	if (!newprop->name)
 		return -EINVAL;
@@ -1736,9 +1766,11 @@ int of_update_property(struct device_node *np, struct property *newprop)
 	if (!found)
 		return -ENODEV;
 
-	/* Update the sysfs attribute */
-	sysfs_remove_bin_file(&np->kobj, &oldprop->attr);
-	__of_add_property(np, newprop);
+	if (of_node_is_attached(np)) {
+		/* Update the sysfs attribute */
+		sysfs_remove_bin_file(&np->kobj, &oldprop->attr);
+		__of_add_property(np, newprop);
+	}
 
 	return 0;
 }
@@ -1782,10 +1814,6 @@ int of_attach_node(struct device_node *np)
 	unsigned long flags;
 	int rc;
 
-	rc = of_reconfig_notify(OF_RECONFIG_ATTACH_NODE, np);
-	if (rc)
-		return rc;
-
 	raw_spin_lock_irqsave(&devtree_lock, flags);
 	np->sibling = np->parent->child;
 	np->allnext = of_allnodes;
@@ -1794,7 +1822,18 @@ int of_attach_node(struct device_node *np)
 	np->allnodes = &of_allnodes;
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
-	of_node_add(np);
+	rc = of_node_add(np);
+	if (rc != 0)
+		goto err_detach;
+
+	rc = of_reconfig_notify(OF_RECONFIG_ATTACH_NODE, np);
+	if (rc)
+		goto err_detach;
+
+	return 0;
+
+err_detach:
+	of_node_remove(np);
 	return 0;
 }
 
@@ -1810,9 +1849,11 @@ int of_detach_node(struct device_node *np)
 	unsigned long flags;
 	int rc = 0;
 
-	rc = of_reconfig_notify(OF_RECONFIG_DETACH_NODE, np);
-	if (rc)
-		return rc;
+	if (of_node_is_attached(np)) {
+		rc = of_reconfig_notify(OF_RECONFIG_DETACH_NODE, np);
+		if (rc)
+			return rc;
+	}
 
 	raw_spin_lock_irqsave(&devtree_lock, flags);
 
-- 
1.7.12

[PATCH 3/3] OF: kobj ops should only happen on attached nodes

[Pantelis Antoniou]

kobject operation should only take place on already attached (live)
device nodes. The same restriction applies to the invocation of
device tree notifiers.

Introduce a simple of_node_is_attached() test which tests whether
the given node is attached via means of the allnodes member.

Signed-off-by: Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>
---
 drivers/of/base.c | 87 ++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 64 insertions(+), 23 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index db0de86..c6299bd 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -89,6 +89,13 @@ int __weak of_node_to_nid(struct device_node *np)
 #endif
 
 #if defined(CONFIG_OF_DYNAMIC)
+
+/* simple test for figuring out if node is attached */
+static inline int of_node_is_attached(struct device_node *np)
+{
+	return np && np->allnodes == &of_allnodes;
+}
+
 /**
  *	of_node_get - Increment refcount of a node
  *	@node:	Node to inc refcount, NULL is supported to
@@ -98,7 +105,7 @@ int __weak of_node_to_nid(struct device_node *np)
  */
 struct device_node *of_node_get(struct device_node *node)
 {
-	if (node && of_kset)
+	if (node && of_kset && of_node_is_attached(node))
 		kobject_get(&node->kobj);
 	return node;
 }
@@ -156,7 +163,7 @@ static void of_node_release(struct kobject *kobj)
  */
 void of_node_put(struct device_node *node)
 {
-	if (node && of_kset)
+	if (node && of_kset && of_node_is_attached(node))
 		kobject_put(&node->kobj);
 }
 EXPORT_SYMBOL(of_node_put);
@@ -210,6 +217,10 @@ static int __of_add_property(struct device_node *np, struct property *pp)
 	if (!of_kset)
 		return 0;
 
+	/* don't do anything with a detached node */
+	if (!of_node_is_attached(np))
+		return 0;
+
 	/* Important: Don't leak passwords */
 	secure = strncmp(pp->name, "security-", 9) == 0;
 
@@ -235,6 +246,9 @@ static int __of_node_add(struct device_node *np)
 	if (!of_kset)
 		return 0;
 
+	if (!of_node_is_attached(np))
+		return 0;
+
 	np->kobj.kset = of_kset;
 	if (!np->parent) {
 		/* Nodes without parents are new top level trees */
@@ -259,6 +273,9 @@ int of_node_add(struct device_node *np)
 {
 	int rc = 0;
 
+	if (!of_node_is_attached(np))
+		return 0;
+
 	/* fake the return while of_init is not yet called */
 	mutex_lock(&of_aliases_mutex);
 	kobject_init(&np->kobj, &of_node_ktype);
@@ -274,10 +291,16 @@ static void of_node_remove(struct device_node *np)
 {
 	struct property *pp;
 
+	/* don't do anything with a detached node */
+	if (!of_node_is_attached(np))
+		return;
+
 	for_each_property_of_node(np, pp)
 		sysfs_remove_bin_file(&np->kobj, &pp->attr);
 
 	kobject_del(&np->kobj);
+
+	of_node_put(np);
 }
 #endif
 
@@ -1626,9 +1649,11 @@ int of_add_property(struct device_node *np, struct property *prop)
 	unsigned long flags;
 	int rc;
 
-	rc = of_property_notify(OF_RECONFIG_ADD_PROPERTY, np, prop);
-	if (rc)
-		return rc;
+	if (of_node_is_attached(np)) {
+		rc = of_property_notify(OF_RECONFIG_ADD_PROPERTY, np, prop);
+		if (rc)
+			return rc;
+	}
 
 	prop->next = NULL;
 	raw_spin_lock_irqsave(&devtree_lock, flags);
@@ -1664,9 +1689,11 @@ int of_remove_property(struct device_node *np, struct property *prop)
 	int found = 0;
 	int rc;
 
-	rc = of_property_notify(OF_RECONFIG_REMOVE_PROPERTY, np, prop);
-	if (rc)
-		return rc;
+	if (of_node_is_attached(np)) {
+		rc = of_property_notify(OF_RECONFIG_REMOVE_PROPERTY, np, prop);
+		if (rc)
+			return rc;
+	}
 
 	raw_spin_lock_irqsave(&devtree_lock, flags);
 	next = &np->properties;
@@ -1686,7 +1713,8 @@ int of_remove_property(struct device_node *np, struct property *prop)
 	if (!found)
 		return -ENODEV;
 
-	sysfs_remove_bin_file(&np->kobj, &prop->attr);
+	if (of_node_is_attached(np))
+		sysfs_remove_bin_file(&np->kobj, &prop->attr);
 
 	return 0;
 }
@@ -1706,9 +1734,11 @@ int of_update_property(struct device_node *np, struct property *newprop)
 	unsigned long flags;
 	int rc, found = 0;
 
-	rc = of_property_notify(OF_RECONFIG_UPDATE_PROPERTY, np, newprop);
-	if (rc)
-		return rc;
+	if (of_node_is_attached(np)) {
+		rc = of_property_notify(OF_RECONFIG_UPDATE_PROPERTY, np, newprop);
+		if (rc)
+			return rc;
+	}
 
 	if (!newprop->name)
 		return -EINVAL;
@@ -1736,9 +1766,11 @@ int of_update_property(struct device_node *np, struct property *newprop)
 	if (!found)
 		return -ENODEV;
 
-	/* Update the sysfs attribute */
-	sysfs_remove_bin_file(&np->kobj, &oldprop->attr);
-	__of_add_property(np, newprop);
+	if (of_node_is_attached(np)) {
+		/* Update the sysfs attribute */
+		sysfs_remove_bin_file(&np->kobj, &oldprop->attr);
+		__of_add_property(np, newprop);
+	}
 
 	return 0;
 }
@@ -1782,10 +1814,6 @@ int of_attach_node(struct device_node *np)
 	unsigned long flags;
 	int rc;
 
-	rc = of_reconfig_notify(OF_RECONFIG_ATTACH_NODE, np);
-	if (rc)
-		return rc;
-
 	raw_spin_lock_irqsave(&devtree_lock, flags);
 	np->sibling = np->parent->child;
 	np->allnext = of_allnodes;
@@ -1794,7 +1822,18 @@ int of_attach_node(struct device_node *np)
 	np->allnodes = &of_allnodes;
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
-	of_node_add(np);
+	rc = of_node_add(np);
+	if (rc != 0)
+		goto err_detach;
+
+	rc = of_reconfig_notify(OF_RECONFIG_ATTACH_NODE, np);
+	if (rc)
+		goto err_detach;
+
+	return 0;
+
+err_detach:
+	of_node_remove(np);
 	return 0;
 }
 
@@ -1810,9 +1849,11 @@ int of_detach_node(struct device_node *np)
 	unsigned long flags;
 	int rc = 0;
 
-	rc = of_reconfig_notify(OF_RECONFIG_DETACH_NODE, np);
-	if (rc)
-		return rc;
+	if (of_node_is_attached(np)) {
+		rc = of_reconfig_notify(OF_RECONFIG_DETACH_NODE, np);
+		if (rc)
+			return rc;
+	}
 
 	raw_spin_lock_irqsave(&devtree_lock, flags);
 
-- 
1.7.12

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] OF: kobj ops should only happen on attached nodes

[Grant Likely]

On Tue, 10 Dec 2013 16:13:53 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
> kobject operation should only take place on already attached (live)
> device nodes. The same restriction applies to the invocation of
> device tree notifiers.
> 
> Introduce a simple of_node_is_attached() test which tests whether
> the given node is attached via means of the allnodes member.
> 
> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> ---
>  drivers/of/base.c | 87 ++++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 64 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index db0de86..c6299bd 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -89,6 +89,13 @@ int __weak of_node_to_nid(struct device_node *np)
>  #endif
>  
>  #if defined(CONFIG_OF_DYNAMIC)
> +
> +/* simple test for figuring out if node is attached */
> +static inline int of_node_is_attached(struct device_node *np)
> +{
> +	return np && np->allnodes == &of_allnodes;
> +}
> +
>  /**
>   *	of_node_get - Increment refcount of a node
>   *	@node:	Node to inc refcount, NULL is supported to
> @@ -98,7 +105,7 @@ int __weak of_node_to_nid(struct device_node *np)
>   */
>  struct device_node *of_node_get(struct device_node *node)
>  {
> -	if (node && of_kset)
> +	if (node && of_kset && of_node_is_attached(node))
>  		kobject_get(&node->kobj);

Remind me, what is the reason for not wanting to refcount unattached
nodes? I would think that doing a refcount is still desirable, even if
it doesn't show up in sysfs.

I know we talked about this on IRC, but I don't remember the conclusion
we came to.

>  	return node;
>  }
> @@ -156,7 +163,7 @@ static void of_node_release(struct kobject *kobj)
>   */
>  void of_node_put(struct device_node *node)
>  {
> -	if (node && of_kset)
> +	if (node && of_kset && of_node_is_attached(node))
>  		kobject_put(&node->kobj);
>  }
>  EXPORT_SYMBOL(of_node_put);
> @@ -210,6 +217,10 @@ static int __of_add_property(struct device_node *np, struct property *pp)
>  	if (!of_kset)
>  		return 0;
>  
> +	/* don't do anything with a detached node */
> +	if (!of_node_is_attached(np))
> +		return 0;
> +

I'm concerned about this whole approach. It "feels" wrong to me.
Perhaps the mistake is to having the unflatten routine be the one also
adding the nodes to the tree. The whole thing could be sidestepped if
unflattening the data structure was a completely separate step from
attaching it to the live tree.

We could rework fdt.c (and pdt.c) to not touch allnodes at all and then
have a separate function that walks the unflattened device tree and
attaches the lot at once. That would make it easier for you to grab an
unflattened tree and work with it before adding nodes to the core tree.

>  	/* Important: Don't leak passwords */
>  	secure = strncmp(pp->name, "security-", 9) == 0;
>  
> @@ -235,6 +246,9 @@ static int __of_node_add(struct device_node *np)
>  	if (!of_kset)
>  		return 0;
>  
> +	if (!of_node_is_attached(np))
> +		return 0;
> +
>  	np->kobj.kset = of_kset;
>  	if (!np->parent) {
>  		/* Nodes without parents are new top level trees */
> @@ -259,6 +273,9 @@ int of_node_add(struct device_node *np)
>  {
>  	int rc = 0;
>  
> +	if (!of_node_is_attached(np))
> +		return 0;
> +
>  	/* fake the return while of_init is not yet called */
>  	mutex_lock(&of_aliases_mutex);
>  	kobject_init(&np->kobj, &of_node_ktype);
> @@ -274,10 +291,16 @@ static void of_node_remove(struct device_node *np)
>  {
>  	struct property *pp;
>  
> +	/* don't do anything with a detached node */
> +	if (!of_node_is_attached(np))
> +		return;
> +
>  	for_each_property_of_node(np, pp)
>  		sysfs_remove_bin_file(&np->kobj, &pp->attr);
>  
>  	kobject_del(&np->kobj);
> +
> +	of_node_put(np);
>  }
>  #endif
>  
> @@ -1626,9 +1649,11 @@ int of_add_property(struct device_node *np, struct property *prop)
>  	unsigned long flags;
>  	int rc;
>  
> -	rc = of_property_notify(OF_RECONFIG_ADD_PROPERTY, np, prop);
> -	if (rc)
> -		return rc;
> +	if (of_node_is_attached(np)) {
> +		rc = of_property_notify(OF_RECONFIG_ADD_PROPERTY, np, prop);
> +		if (rc)
> +			return rc;
> +	}
>  
>  	prop->next = NULL;
>  	raw_spin_lock_irqsave(&devtree_lock, flags);
> @@ -1664,9 +1689,11 @@ int of_remove_property(struct device_node *np, struct property *prop)
>  	int found = 0;
>  	int rc;
>  
> -	rc = of_property_notify(OF_RECONFIG_REMOVE_PROPERTY, np, prop);
> -	if (rc)
> -		return rc;
> +	if (of_node_is_attached(np)) {
> +		rc = of_property_notify(OF_RECONFIG_REMOVE_PROPERTY, np, prop);
> +		if (rc)
> +			return rc;
> +	}
>  
>  	raw_spin_lock_irqsave(&devtree_lock, flags);
>  	next = &np->properties;
> @@ -1686,7 +1713,8 @@ int of_remove_property(struct device_node *np, struct property *prop)
>  	if (!found)
>  		return -ENODEV;
>  
> -	sysfs_remove_bin_file(&np->kobj, &prop->attr);
> +	if (of_node_is_attached(np))
> +		sysfs_remove_bin_file(&np->kobj, &prop->attr);
>  
>  	return 0;
>  }
> @@ -1706,9 +1734,11 @@ int of_update_property(struct device_node *np, struct property *newprop)
>  	unsigned long flags;
>  	int rc, found = 0;
>  
> -	rc = of_property_notify(OF_RECONFIG_UPDATE_PROPERTY, np, newprop);
> -	if (rc)
> -		return rc;
> +	if (of_node_is_attached(np)) {
> +		rc = of_property_notify(OF_RECONFIG_UPDATE_PROPERTY, np, newprop);
> +		if (rc)
> +			return rc;
> +	}
>  
>  	if (!newprop->name)
>  		return -EINVAL;
> @@ -1736,9 +1766,11 @@ int of_update_property(struct device_node *np, struct property *newprop)
>  	if (!found)
>  		return -ENODEV;
>  
> -	/* Update the sysfs attribute */
> -	sysfs_remove_bin_file(&np->kobj, &oldprop->attr);
> -	__of_add_property(np, newprop);
> +	if (of_node_is_attached(np)) {
> +		/* Update the sysfs attribute */
> +		sysfs_remove_bin_file(&np->kobj, &oldprop->attr);
> +		__of_add_property(np, newprop);
> +	}
>  
>  	return 0;
>  }
> @@ -1782,10 +1814,6 @@ int of_attach_node(struct device_node *np)
>  	unsigned long flags;
>  	int rc;
>  
> -	rc = of_reconfig_notify(OF_RECONFIG_ATTACH_NODE, np);
> -	if (rc)
> -		return rc;
> -
>  	raw_spin_lock_irqsave(&devtree_lock, flags);
>  	np->sibling = np->parent->child;
>  	np->allnext = of_allnodes;
> @@ -1794,7 +1822,18 @@ int of_attach_node(struct device_node *np)
>  	np->allnodes = &of_allnodes;
>  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  
> -	of_node_add(np);
> +	rc = of_node_add(np);
> +	if (rc != 0)
> +		goto err_detach;
> +
> +	rc = of_reconfig_notify(OF_RECONFIG_ATTACH_NODE, np);
> +	if (rc)
> +		goto err_detach;
> +

I'm pretty sure the notifier is where it is to allow a notifier callback
to intercepted it before it gets attached to the tree. I suspect this
change will break prom_reconfig_notifier. Notifying after
attachment, or before removal will require a separate set of notifiers.

> +	return 0;
> +
> +err_detach:
> +	of_node_remove(np);
>  	return 0;
>  }
>  
> @@ -1810,9 +1849,11 @@ int of_detach_node(struct device_node *np)
>  	unsigned long flags;
>  	int rc = 0;
>  
> -	rc = of_reconfig_notify(OF_RECONFIG_DETACH_NODE, np);
> -	if (rc)
> -		return rc;
> +	if (of_node_is_attached(np)) {
> +		rc = of_reconfig_notify(OF_RECONFIG_DETACH_NODE, np);
> +		if (rc)
> +			return rc;
> +	}
>  
>  	raw_spin_lock_irqsave(&devtree_lock, flags);
>  
> -- 
> 1.7.12
> 

Re: [PATCH 3/3] OF: kobj ops should only happen on attached nodes

[Grant Likely]

On Tue, 10 Dec 2013 16:13:53 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
> kobject operation should only take place on already attached (live)
> device nodes. The same restriction applies to the invocation of
> device tree notifiers.
> 
> Introduce a simple of_node_is_attached() test which tests whether
> the given node is attached via means of the allnodes member.
> 
> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> ---
>  drivers/of/base.c | 87 ++++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 64 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index db0de86..c6299bd 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -89,6 +89,13 @@ int __weak of_node_to_nid(struct device_node *np)
>  #endif
>  
>  #if defined(CONFIG_OF_DYNAMIC)
> +
> +/* simple test for figuring out if node is attached */
> +static inline int of_node_is_attached(struct device_node *np)
> +{
> +	return np && np->allnodes == &of_allnodes;
> +}
> +
>  /**
>   *	of_node_get - Increment refcount of a node
>   *	@node:	Node to inc refcount, NULL is supported to
> @@ -98,7 +105,7 @@ int __weak of_node_to_nid(struct device_node *np)
>   */
>  struct device_node *of_node_get(struct device_node *node)
>  {
> -	if (node && of_kset)
> +	if (node && of_kset && of_node_is_attached(node))
>  		kobject_get(&node->kobj);

Remind me, what is the reason for not wanting to refcount unattached
nodes? I would think that doing a refcount is still desirable, even if
it doesn't show up in sysfs.

I know we talked about this on IRC, but I don't remember the conclusion
we came to.

>  	return node;
>  }
> @@ -156,7 +163,7 @@ static void of_node_release(struct kobject *kobj)
>   */
>  void of_node_put(struct device_node *node)
>  {
> -	if (node && of_kset)
> +	if (node && of_kset && of_node_is_attached(node))
>  		kobject_put(&node->kobj);
>  }
>  EXPORT_SYMBOL(of_node_put);
> @@ -210,6 +217,10 @@ static int __of_add_property(struct device_node *np, struct property *pp)
>  	if (!of_kset)
>  		return 0;
>  
> +	/* don't do anything with a detached node */
> +	if (!of_node_is_attached(np))
> +		return 0;
> +

I'm concerned about this whole approach. It "feels" wrong to me.
Perhaps the mistake is to having the unflatten routine be the one also
adding the nodes to the tree. The whole thing could be sidestepped if
unflattening the data structure was a completely separate step from
attaching it to the live tree.

We could rework fdt.c (and pdt.c) to not touch allnodes at all and then
have a separate function that walks the unflattened device tree and
attaches the lot at once. That would make it easier for you to grab an
unflattened tree and work with it before adding nodes to the core tree.

>  	/* Important: Don't leak passwords */
>  	secure = strncmp(pp->name, "security-", 9) == 0;
>  
> @@ -235,6 +246,9 @@ static int __of_node_add(struct device_node *np)
>  	if (!of_kset)
>  		return 0;
>  
> +	if (!of_node_is_attached(np))
> +		return 0;
> +
>  	np->kobj.kset = of_kset;
>  	if (!np->parent) {
>  		/* Nodes without parents are new top level trees */
> @@ -259,6 +273,9 @@ int of_node_add(struct device_node *np)
>  {
>  	int rc = 0;
>  
> +	if (!of_node_is_attached(np))
> +		return 0;
> +
>  	/* fake the return while of_init is not yet called */
>  	mutex_lock(&of_aliases_mutex);
>  	kobject_init(&np->kobj, &of_node_ktype);
> @@ -274,10 +291,16 @@ static void of_node_remove(struct device_node *np)
>  {
>  	struct property *pp;
>  
> +	/* don't do anything with a detached node */
> +	if (!of_node_is_attached(np))
> +		return;
> +
>  	for_each_property_of_node(np, pp)
>  		sysfs_remove_bin_file(&np->kobj, &pp->attr);
>  
>  	kobject_del(&np->kobj);
> +
> +	of_node_put(np);
>  }
>  #endif
>  
> @@ -1626,9 +1649,11 @@ int of_add_property(struct device_node *np, struct property *prop)
>  	unsigned long flags;
>  	int rc;
>  
> -	rc = of_property_notify(OF_RECONFIG_ADD_PROPERTY, np, prop);
> -	if (rc)
> -		return rc;
> +	if (of_node_is_attached(np)) {
> +		rc = of_property_notify(OF_RECONFIG_ADD_PROPERTY, np, prop);
> +		if (rc)
> +			return rc;
> +	}
>  
>  	prop->next = NULL;
>  	raw_spin_lock_irqsave(&devtree_lock, flags);
> @@ -1664,9 +1689,11 @@ int of_remove_property(struct device_node *np, struct property *prop)
>  	int found = 0;
>  	int rc;
>  
> -	rc = of_property_notify(OF_RECONFIG_REMOVE_PROPERTY, np, prop);
> -	if (rc)
> -		return rc;
> +	if (of_node_is_attached(np)) {
> +		rc = of_property_notify(OF_RECONFIG_REMOVE_PROPERTY, np, prop);
> +		if (rc)
> +			return rc;
> +	}
>  
>  	raw_spin_lock_irqsave(&devtree_lock, flags);
>  	next = &np->properties;
> @@ -1686,7 +1713,8 @@ int of_remove_property(struct device_node *np, struct property *prop)
>  	if (!found)
>  		return -ENODEV;
>  
> -	sysfs_remove_bin_file(&np->kobj, &prop->attr);
> +	if (of_node_is_attached(np))
> +		sysfs_remove_bin_file(&np->kobj, &prop->attr);
>  
>  	return 0;
>  }
> @@ -1706,9 +1734,11 @@ int of_update_property(struct device_node *np, struct property *newprop)
>  	unsigned long flags;
>  	int rc, found = 0;
>  
> -	rc = of_property_notify(OF_RECONFIG_UPDATE_PROPERTY, np, newprop);
> -	if (rc)
> -		return rc;
> +	if (of_node_is_attached(np)) {
> +		rc = of_property_notify(OF_RECONFIG_UPDATE_PROPERTY, np, newprop);
> +		if (rc)
> +			return rc;
> +	}
>  
>  	if (!newprop->name)
>  		return -EINVAL;
> @@ -1736,9 +1766,11 @@ int of_update_property(struct device_node *np, struct property *newprop)
>  	if (!found)
>  		return -ENODEV;
>  
> -	/* Update the sysfs attribute */
> -	sysfs_remove_bin_file(&np->kobj, &oldprop->attr);
> -	__of_add_property(np, newprop);
> +	if (of_node_is_attached(np)) {
> +		/* Update the sysfs attribute */
> +		sysfs_remove_bin_file(&np->kobj, &oldprop->attr);
> +		__of_add_property(np, newprop);
> +	}
>  
>  	return 0;
>  }
> @@ -1782,10 +1814,6 @@ int of_attach_node(struct device_node *np)
>  	unsigned long flags;
>  	int rc;
>  
> -	rc = of_reconfig_notify(OF_RECONFIG_ATTACH_NODE, np);
> -	if (rc)
> -		return rc;
> -
>  	raw_spin_lock_irqsave(&devtree_lock, flags);
>  	np->sibling = np->parent->child;
>  	np->allnext = of_allnodes;
> @@ -1794,7 +1822,18 @@ int of_attach_node(struct device_node *np)
>  	np->allnodes = &of_allnodes;
>  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  
> -	of_node_add(np);
> +	rc = of_node_add(np);
> +	if (rc != 0)
> +		goto err_detach;
> +
> +	rc = of_reconfig_notify(OF_RECONFIG_ATTACH_NODE, np);
> +	if (rc)
> +		goto err_detach;
> +

I'm pretty sure the notifier is where it is to allow a notifier callback
to intercepted it before it gets attached to the tree. I suspect this
change will break prom_reconfig_notifier. Notifying after
attachment, or before removal will require a separate set of notifiers.

> +	return 0;
> +
> +err_detach:
> +	of_node_remove(np);
>  	return 0;
>  }
>  
> @@ -1810,9 +1849,11 @@ int of_detach_node(struct device_node *np)
>  	unsigned long flags;
>  	int rc = 0;
>  
> -	rc = of_reconfig_notify(OF_RECONFIG_DETACH_NODE, np);
> -	if (rc)
> -		return rc;
> +	if (of_node_is_attached(np)) {
> +		rc = of_reconfig_notify(OF_RECONFIG_DETACH_NODE, np);
> +		if (rc)
> +			return rc;
> +	}
>  
>  	raw_spin_lock_irqsave(&devtree_lock, flags);
>  
> -- 
> 1.7.12
>