[PATCH] OF: kobj node lifecycle fixes

[PATCH] OF: kobj node lifecycle fixes

[Pantelis Antoniou]

After the move to having device nodes be proper kobjects the lifecycle
of the node needs to be controlled better.

At first convert of_add_node() in the unflattened functions to
of_init_node() which initializes the kobject so that of_node_get/put
work correctly even before of_init is called.

Afterwards introduce of_node_is_initialized & of_node_is_attached that
query the underlying kobject about the state (attached means kobj
is visible in sysfs)

Using that make sure the lifecycle of the tree is correct at all
times.

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

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 78cd546..39dda4c 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -245,45 +245,104 @@ 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);
+
+	BUG_ON(!of_node_is_initialized(np));
+
+	if (!of_kset) {
+		pr_warn("%s: of_node_add before of_init on %s\n",
+				__func__, np->full_name);
+		return 0;
+	}
+
 	mutex_lock(&of_aliases_mutex);
-	if (of_kset)
-		rc = __of_node_add(np);
+	rc = __of_node_add(np);
 	mutex_unlock(&of_aliases_mutex);
 	return rc;
 }
 
+/*
+ * Initialize a new device node
+ *
+ * At the moment it is just initializing the kobj of the node.
+ * This occurs during unflattening and when creating dynamic nodes.
+ */
+void of_node_init(struct device_node *np)
+{
+	kobject_init(&np->kobj, &of_node_ktype);
+}
+
 #if defined(CONFIG_OF_DYNAMIC)
 static void of_node_remove(struct device_node *np)
 {
 	struct property *pp;
 
-	for_each_property_of_node(np, pp)
-		sysfs_remove_bin_file(&np->kobj, &pp->attr);
+	BUG_ON(!of_node_is_initialized(np));
+
+	/* only remove properties if on sysfs */
+	if (of_node_is_attached(np)) {
+		for_each_property_of_node(np, pp)
+			sysfs_remove_bin_file(&np->kobj, &pp->attr);
+		/* delete from sysfs */
+		kobject_del(&np->kobj);
+	}
 
-	kobject_del(&np->kobj);
+	/* finally remove the kobj_init ref */
+	of_node_put(np);
 }
 #endif
 
+/* recursively attach the tree */
+static __init int __of_populate(struct device_node *np)
+{
+	struct device_node *child;
+	int rc;
+
+	/* add the parent first */
+	rc = __of_node_add(np);
+	if (rc)
+		return rc;
+
+	/* the children afterwards */
+	__for_each_child_of_node(np, child) {
+		rc = __of_populate(child);
+		if (rc)
+			return rc;
+	}
+
+	return 0;
+}
+
 static int __init of_init(void)
 {
 	struct device_node *np;
+	int rc;
 
 	of_kset = kset_create_and_add("devicetree", NULL, firmware_kobj);
 	if (!of_kset)
 		return -ENOMEM;
 
-	/* 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);
+
+	/* find root */
+	np = of_find_node_by_path("/");
+	if (np == NULL) {
+		rc = -EINVAL;
+		goto out;
+	}
+	/* populate */
+	rc = __of_populate(np);
+	of_node_put(np);
 
 	/* Symlink in /proc as required by userspace ABI */
-	if (of_allnodes)
-		proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
+	if (rc != 0)
+		goto out;
 
-	return 0;
+	proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
+
+out:
+	mutex_unlock(&of_aliases_mutex);
+
+	return rc;
 }
 core_initcall(of_init);
 
@@ -1587,6 +1646,10 @@ static int of_property_notify(int action, struct device_node *np,
 {
 	struct of_prop_reconfig pr;
 
+	/* only call notifiers if the node is attached */
+	if (!of_node_is_attached(np))
+		return 0;
+
 	pr.dn = np;
 	pr.prop = prop;
 	return of_reconfig_notify(action, &pr);
@@ -1626,11 +1689,8 @@ int of_add_property(struct device_node *np, struct property *prop)
 	*next = prop;
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
-	/* at early boot, bail hear and defer setup to of_init() */
-	if (!of_kset)
-		return 0;
-
-	__of_add_property(np, prop);
+	if (of_node_is_attached(np))
+		__of_add_property(np, prop);
 
 	return 0;
 }
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index d5d62cd..3ca75fb 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -327,7 +327,8 @@ static void * unflatten_dt_node(struct boot_param_header *blob,
 		if (!np->type)
 			np->type = "<NULL>";
 
-		of_node_add(np);
+		/* initialize node (do not add) */
+		of_node_init(np);
 	}
 	while (tag == OF_DT_BEGIN_NODE || tag == OF_DT_NOP) {
 		if (tag == OF_DT_NOP)
diff --git a/include/linux/of.h b/include/linux/of.h
index f285222..f13e773 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -76,6 +76,21 @@ struct of_phandle_args {
 
 extern int of_node_add(struct device_node *node);
 
+/* initialize a node */
+extern void of_node_init(struct device_node *node);
+
+/* true when node is initialized */
+static inline int of_node_is_initialized(struct device_node *node)
+{
+	return node && node->kobj.state_initialized;
+}
+
+/* true when node is attached (i.e. present on sysfs) */
+static inline int of_node_is_attached(struct device_node *node)
+{
+	return node && node->kobj.state_in_sysfs;
+}
+
 #ifdef CONFIG_OF_DYNAMIC
 extern struct device_node *of_node_get(struct device_node *node);
 extern void of_node_put(struct device_node *node);
-- 
1.7.12

[PATCH] OF: kobj node lifecycle fixes

[Pantelis Antoniou]

After the move to having device nodes be proper kobjects the lifecycle
of the node needs to be controlled better.

At first convert of_add_node() in the unflattened functions to
of_init_node() which initializes the kobject so that of_node_get/put
work correctly even before of_init is called.

Afterwards introduce of_node_is_initialized & of_node_is_attached that
query the underlying kobject about the state (attached means kobj
is visible in sysfs)

Using that make sure the lifecycle of the tree is correct at all
times.

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

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 78cd546..39dda4c 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -245,45 +245,104 @@ 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);
+
+	BUG_ON(!of_node_is_initialized(np));
+
+	if (!of_kset) {
+		pr_warn("%s: of_node_add before of_init on %s\n",
+				__func__, np->full_name);
+		return 0;
+	}
+
 	mutex_lock(&of_aliases_mutex);
-	if (of_kset)
-		rc = __of_node_add(np);
+	rc = __of_node_add(np);
 	mutex_unlock(&of_aliases_mutex);
 	return rc;
 }
 
+/*
+ * Initialize a new device node
+ *
+ * At the moment it is just initializing the kobj of the node.
+ * This occurs during unflattening and when creating dynamic nodes.
+ */
+void of_node_init(struct device_node *np)
+{
+	kobject_init(&np->kobj, &of_node_ktype);
+}
+
 #if defined(CONFIG_OF_DYNAMIC)
 static void of_node_remove(struct device_node *np)
 {
 	struct property *pp;
 
-	for_each_property_of_node(np, pp)
-		sysfs_remove_bin_file(&np->kobj, &pp->attr);
+	BUG_ON(!of_node_is_initialized(np));
+
+	/* only remove properties if on sysfs */
+	if (of_node_is_attached(np)) {
+		for_each_property_of_node(np, pp)
+			sysfs_remove_bin_file(&np->kobj, &pp->attr);
+		/* delete from sysfs */
+		kobject_del(&np->kobj);
+	}
 
-	kobject_del(&np->kobj);
+	/* finally remove the kobj_init ref */
+	of_node_put(np);
 }
 #endif
 
+/* recursively attach the tree */
+static __init int __of_populate(struct device_node *np)
+{
+	struct device_node *child;
+	int rc;
+
+	/* add the parent first */
+	rc = __of_node_add(np);
+	if (rc)
+		return rc;
+
+	/* the children afterwards */
+	__for_each_child_of_node(np, child) {
+		rc = __of_populate(child);
+		if (rc)
+			return rc;
+	}
+
+	return 0;
+}
+
 static int __init of_init(void)
 {
 	struct device_node *np;
+	int rc;
 
 	of_kset = kset_create_and_add("devicetree", NULL, firmware_kobj);
 	if (!of_kset)
 		return -ENOMEM;
 
-	/* 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);
+
+	/* find root */
+	np = of_find_node_by_path("/");
+	if (np == NULL) {
+		rc = -EINVAL;
+		goto out;
+	}
+	/* populate */
+	rc = __of_populate(np);
+	of_node_put(np);
 
 	/* Symlink in /proc as required by userspace ABI */
-	if (of_allnodes)
-		proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
+	if (rc != 0)
+		goto out;
 
-	return 0;
+	proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
+
+out:
+	mutex_unlock(&of_aliases_mutex);
+
+	return rc;
 }
 core_initcall(of_init);
 
@@ -1587,6 +1646,10 @@ static int of_property_notify(int action, struct device_node *np,
 {
 	struct of_prop_reconfig pr;
 
+	/* only call notifiers if the node is attached */
+	if (!of_node_is_attached(np))
+		return 0;
+
 	pr.dn = np;
 	pr.prop = prop;
 	return of_reconfig_notify(action, &pr);
@@ -1626,11 +1689,8 @@ int of_add_property(struct device_node *np, struct property *prop)
 	*next = prop;
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
-	/* at early boot, bail hear and defer setup to of_init() */
-	if (!of_kset)
-		return 0;
-
-	__of_add_property(np, prop);
+	if (of_node_is_attached(np))
+		__of_add_property(np, prop);
 
 	return 0;
 }
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index d5d62cd..3ca75fb 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -327,7 +327,8 @@ static void * unflatten_dt_node(struct boot_param_header *blob,
 		if (!np->type)
 			np->type = "<NULL>";
 
-		of_node_add(np);
+		/* initialize node (do not add) */
+		of_node_init(np);
 	}
 	while (tag == OF_DT_BEGIN_NODE || tag == OF_DT_NOP) {
 		if (tag == OF_DT_NOP)
diff --git a/include/linux/of.h b/include/linux/of.h
index f285222..f13e773 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -76,6 +76,21 @@ struct of_phandle_args {
 
 extern int of_node_add(struct device_node *node);
 
+/* initialize a node */
+extern void of_node_init(struct device_node *node);
+
+/* true when node is initialized */
+static inline int of_node_is_initialized(struct device_node *node)
+{
+	return node && node->kobj.state_initialized;
+}
+
+/* true when node is attached (i.e. present on sysfs) */
+static inline int of_node_is_attached(struct device_node *node)
+{
+	return node && node->kobj.state_in_sysfs;
+}
+
 #ifdef CONFIG_OF_DYNAMIC
 extern struct device_node *of_node_get(struct device_node *node);
 extern void of_node_put(struct device_node *node);
-- 
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] OF: kobj node lifecycle fixes

[Grant Likely]

On Fri, 13 Dec 2013 20:08:59 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
> After the move to having device nodes be proper kobjects the lifecycle
> of the node needs to be controlled better.
> 
> At first convert of_add_node() in the unflattened functions to
> of_init_node() which initializes the kobject so that of_node_get/put
> work correctly even before of_init is called.
> 
> Afterwards introduce of_node_is_initialized & of_node_is_attached that
> query the underlying kobject about the state (attached means kobj
> is visible in sysfs)
> 
> Using that make sure the lifecycle of the tree is correct at all
> times.
> 
> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>

Merged with some rework, thanks

g.

> ---
>  drivers/of/base.c  | 96 ++++++++++++++++++++++++++++++++++++++++++++----------
>  drivers/of/fdt.c   |  3 +-
>  include/linux/of.h | 15 +++++++++
>  3 files changed, 95 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 78cd546..39dda4c 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -245,45 +245,104 @@ 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);
> +
> +	BUG_ON(!of_node_is_initialized(np));
> +
> +	if (!of_kset) {
> +		pr_warn("%s: of_node_add before of_init on %s\n",
> +				__func__, np->full_name);
> +		return 0;
> +	}
> +
>  	mutex_lock(&of_aliases_mutex);
> -	if (of_kset)
> -		rc = __of_node_add(np);
> +	rc = __of_node_add(np);
>  	mutex_unlock(&of_aliases_mutex);
>  	return rc;
>  }
>  
> +/*
> + * Initialize a new device node
> + *
> + * At the moment it is just initializing the kobj of the node.
> + * This occurs during unflattening and when creating dynamic nodes.
> + */
> +void of_node_init(struct device_node *np)
> +{
> +	kobject_init(&np->kobj, &of_node_ktype);
> +}
> +
>  #if defined(CONFIG_OF_DYNAMIC)
>  static void of_node_remove(struct device_node *np)
>  {
>  	struct property *pp;
>  
> -	for_each_property_of_node(np, pp)
> -		sysfs_remove_bin_file(&np->kobj, &pp->attr);
> +	BUG_ON(!of_node_is_initialized(np));
> +
> +	/* only remove properties if on sysfs */
> +	if (of_node_is_attached(np)) {
> +		for_each_property_of_node(np, pp)
> +			sysfs_remove_bin_file(&np->kobj, &pp->attr);
> +		/* delete from sysfs */
> +		kobject_del(&np->kobj);
> +	}
>  
> -	kobject_del(&np->kobj);
> +	/* finally remove the kobj_init ref */
> +	of_node_put(np);
>  }
>  #endif
>  
> +/* recursively attach the tree */
> +static __init int __of_populate(struct device_node *np)
> +{
> +	struct device_node *child;
> +	int rc;
> +
> +	/* add the parent first */
> +	rc = __of_node_add(np);
> +	if (rc)
> +		return rc;
> +
> +	/* the children afterwards */
> +	__for_each_child_of_node(np, child) {
> +		rc = __of_populate(child);
> +		if (rc)
> +			return rc;
> +	}
> +
> +	return 0;
> +}
> +
>  static int __init of_init(void)
>  {
>  	struct device_node *np;
> +	int rc;
>  
>  	of_kset = kset_create_and_add("devicetree", NULL, firmware_kobj);
>  	if (!of_kset)
>  		return -ENOMEM;
>  
> -	/* 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);
> +
> +	/* find root */
> +	np = of_find_node_by_path("/");
> +	if (np == NULL) {
> +		rc = -EINVAL;
> +		goto out;
> +	}
> +	/* populate */
> +	rc = __of_populate(np);
> +	of_node_put(np);
>  
>  	/* Symlink in /proc as required by userspace ABI */
> -	if (of_allnodes)
> -		proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
> +	if (rc != 0)
> +		goto out;
>  
> -	return 0;
> +	proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
> +
> +out:
> +	mutex_unlock(&of_aliases_mutex);
> +
> +	return rc;
>  }
>  core_initcall(of_init);
>  
> @@ -1587,6 +1646,10 @@ static int of_property_notify(int action, struct device_node *np,
>  {
>  	struct of_prop_reconfig pr;
>  
> +	/* only call notifiers if the node is attached */
> +	if (!of_node_is_attached(np))
> +		return 0;
> +
>  	pr.dn = np;
>  	pr.prop = prop;
>  	return of_reconfig_notify(action, &pr);
> @@ -1626,11 +1689,8 @@ int of_add_property(struct device_node *np, struct property *prop)
>  	*next = prop;
>  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  
> -	/* at early boot, bail hear and defer setup to of_init() */
> -	if (!of_kset)
> -		return 0;
> -
> -	__of_add_property(np, prop);
> +	if (of_node_is_attached(np))
> +		__of_add_property(np, prop);
>  
>  	return 0;
>  }
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index d5d62cd..3ca75fb 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -327,7 +327,8 @@ static void * unflatten_dt_node(struct boot_param_header *blob,
>  		if (!np->type)
>  			np->type = "<NULL>";
>  
> -		of_node_add(np);
> +		/* initialize node (do not add) */
> +		of_node_init(np);
>  	}
>  	while (tag == OF_DT_BEGIN_NODE || tag == OF_DT_NOP) {
>  		if (tag == OF_DT_NOP)
> diff --git a/include/linux/of.h b/include/linux/of.h
> index f285222..f13e773 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -76,6 +76,21 @@ struct of_phandle_args {
>  
>  extern int of_node_add(struct device_node *node);
>  
> +/* initialize a node */
> +extern void of_node_init(struct device_node *node);
> +
> +/* true when node is initialized */
> +static inline int of_node_is_initialized(struct device_node *node)
> +{
> +	return node && node->kobj.state_initialized;
> +}
> +
> +/* true when node is attached (i.e. present on sysfs) */
> +static inline int of_node_is_attached(struct device_node *node)
> +{
> +	return node && node->kobj.state_in_sysfs;
> +}
> +
>  #ifdef CONFIG_OF_DYNAMIC
>  extern struct device_node *of_node_get(struct device_node *node);
>  extern void of_node_put(struct device_node *node);
> -- 
> 1.7.12
> 

Re: [PATCH] OF: kobj node lifecycle fixes

[Grant Likely]

On Fri, 13 Dec 2013 20:08:59 +0200, Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org> wrote:
> After the move to having device nodes be proper kobjects the lifecycle
> of the node needs to be controlled better.
> 
> At first convert of_add_node() in the unflattened functions to
> of_init_node() which initializes the kobject so that of_node_get/put
> work correctly even before of_init is called.
> 
> Afterwards introduce of_node_is_initialized & of_node_is_attached that
> query the underlying kobject about the state (attached means kobj
> is visible in sysfs)
> 
> Using that make sure the lifecycle of the tree is correct at all
> times.
> 
> Signed-off-by: Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>

Merged with some rework, thanks

g.

> ---
>  drivers/of/base.c  | 96 ++++++++++++++++++++++++++++++++++++++++++++----------
>  drivers/of/fdt.c   |  3 +-
>  include/linux/of.h | 15 +++++++++
>  3 files changed, 95 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 78cd546..39dda4c 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -245,45 +245,104 @@ 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);
> +
> +	BUG_ON(!of_node_is_initialized(np));
> +
> +	if (!of_kset) {
> +		pr_warn("%s: of_node_add before of_init on %s\n",
> +				__func__, np->full_name);
> +		return 0;
> +	}
> +
>  	mutex_lock(&of_aliases_mutex);
> -	if (of_kset)
> -		rc = __of_node_add(np);
> +	rc = __of_node_add(np);
>  	mutex_unlock(&of_aliases_mutex);
>  	return rc;
>  }
>  
> +/*
> + * Initialize a new device node
> + *
> + * At the moment it is just initializing the kobj of the node.
> + * This occurs during unflattening and when creating dynamic nodes.
> + */
> +void of_node_init(struct device_node *np)
> +{
> +	kobject_init(&np->kobj, &of_node_ktype);
> +}
> +
>  #if defined(CONFIG_OF_DYNAMIC)
>  static void of_node_remove(struct device_node *np)
>  {
>  	struct property *pp;
>  
> -	for_each_property_of_node(np, pp)
> -		sysfs_remove_bin_file(&np->kobj, &pp->attr);
> +	BUG_ON(!of_node_is_initialized(np));
> +
> +	/* only remove properties if on sysfs */
> +	if (of_node_is_attached(np)) {
> +		for_each_property_of_node(np, pp)
> +			sysfs_remove_bin_file(&np->kobj, &pp->attr);
> +		/* delete from sysfs */
> +		kobject_del(&np->kobj);
> +	}
>  
> -	kobject_del(&np->kobj);
> +	/* finally remove the kobj_init ref */
> +	of_node_put(np);
>  }
>  #endif
>  
> +/* recursively attach the tree */
> +static __init int __of_populate(struct device_node *np)
> +{
> +	struct device_node *child;
> +	int rc;
> +
> +	/* add the parent first */
> +	rc = __of_node_add(np);
> +	if (rc)
> +		return rc;
> +
> +	/* the children afterwards */
> +	__for_each_child_of_node(np, child) {
> +		rc = __of_populate(child);
> +		if (rc)
> +			return rc;
> +	}
> +
> +	return 0;
> +}
> +
>  static int __init of_init(void)
>  {
>  	struct device_node *np;
> +	int rc;
>  
>  	of_kset = kset_create_and_add("devicetree", NULL, firmware_kobj);
>  	if (!of_kset)
>  		return -ENOMEM;
>  
> -	/* 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);
> +
> +	/* find root */
> +	np = of_find_node_by_path("/");
> +	if (np == NULL) {
> +		rc = -EINVAL;
> +		goto out;
> +	}
> +	/* populate */
> +	rc = __of_populate(np);
> +	of_node_put(np);
>  
>  	/* Symlink in /proc as required by userspace ABI */
> -	if (of_allnodes)
> -		proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
> +	if (rc != 0)
> +		goto out;
>  
> -	return 0;
> +	proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
> +
> +out:
> +	mutex_unlock(&of_aliases_mutex);
> +
> +	return rc;
>  }
>  core_initcall(of_init);
>  
> @@ -1587,6 +1646,10 @@ static int of_property_notify(int action, struct device_node *np,
>  {
>  	struct of_prop_reconfig pr;
>  
> +	/* only call notifiers if the node is attached */
> +	if (!of_node_is_attached(np))
> +		return 0;
> +
>  	pr.dn = np;
>  	pr.prop = prop;
>  	return of_reconfig_notify(action, &pr);
> @@ -1626,11 +1689,8 @@ int of_add_property(struct device_node *np, struct property *prop)
>  	*next = prop;
>  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  
> -	/* at early boot, bail hear and defer setup to of_init() */
> -	if (!of_kset)
> -		return 0;
> -
> -	__of_add_property(np, prop);
> +	if (of_node_is_attached(np))
> +		__of_add_property(np, prop);
>  
>  	return 0;
>  }
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index d5d62cd..3ca75fb 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -327,7 +327,8 @@ static void * unflatten_dt_node(struct boot_param_header *blob,
>  		if (!np->type)
>  			np->type = "<NULL>";
>  
> -		of_node_add(np);
> +		/* initialize node (do not add) */
> +		of_node_init(np);
>  	}
>  	while (tag == OF_DT_BEGIN_NODE || tag == OF_DT_NOP) {
>  		if (tag == OF_DT_NOP)
> diff --git a/include/linux/of.h b/include/linux/of.h
> index f285222..f13e773 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -76,6 +76,21 @@ struct of_phandle_args {
>  
>  extern int of_node_add(struct device_node *node);
>  
> +/* initialize a node */
> +extern void of_node_init(struct device_node *node);
> +
> +/* true when node is initialized */
> +static inline int of_node_is_initialized(struct device_node *node)
> +{
> +	return node && node->kobj.state_initialized;
> +}
> +
> +/* true when node is attached (i.e. present on sysfs) */
> +static inline int of_node_is_attached(struct device_node *node)
> +{
> +	return node && node->kobj.state_in_sysfs;
> +}
> +
>  #ifdef CONFIG_OF_DYNAMIC
>  extern struct device_node *of_node_get(struct device_node *node);
>  extern void of_node_put(struct device_node *node);
> -- 
> 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] OF: kobj node lifecycle fixes

[Grant Likely]

On Thu, 13 Mar 2014 08:30:37 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote:
> Hi Grant,
> 
> On Mar 13, 2014, at 12:49 AM, Grant Likely wrote:
> 
> Looks good; booted and tested overlays with no issues.

Thanks.

g.

Re: [PATCH] OF: kobj node lifecycle fixes

[Pantelis Antoniou]

Hi Grant,

On Mar 13, 2014, at 12:49 AM, Grant Likely wrote:

> From: Pantelis Antoniou <panto@antoniou-consulting.com>
> 
> After the move to having device nodes be proper kobjects the lifecycle
> of the node needs to be controlled better.
> 
> At first convert of_add_node() in the unflattened functions to
> of_init_node() which initializes the kobject so that of_node_get/put
> work correctly even before of_init is called.
> 
> Afterwards introduce of_node_is_initialized & of_node_is_attached that
> query the underlying kobject about the state (attached means kobj
> is visible in sysfs)
> 
> Using that make sure the lifecycle of the tree is correct at all
> times.
> 
> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> [grant.likely: moved of_node_init() calls, fixed up locking, and
>               dropped __of_populate() hunks]
> Signed-off-by: Grant Likely <grant.likely@linaro.org>
> ---
> 
> I've reworked Pantelis' patch to work on my tree. Here's the result.
> I've already got this in my tree and will be pushing it out shortly.
> 
> g.
> 
> drivers/of/base.c  | 35 ++++++++++++++++++++++++++---------
> drivers/of/fdt.c   |  3 +--
> drivers/of/pdt.c   |  3 +--
> include/linux/of.h | 19 +++++++++++++++++++
> 4 files changed, 47 insertions(+), 13 deletions(-)
> 

Looks good; booted and tested overlays with no issues.

Regards

-- Pantelis

Re: [PATCH] OF: kobj node lifecycle fixes

[Pantelis Antoniou]

Hi Grant,

On Mar 13, 2014, at 12:49 AM, Grant Likely wrote:

> From: Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>
> 
> After the move to having device nodes be proper kobjects the lifecycle
> of the node needs to be controlled better.
> 
> At first convert of_add_node() in the unflattened functions to
> of_init_node() which initializes the kobject so that of_node_get/put
> work correctly even before of_init is called.
> 
> Afterwards introduce of_node_is_initialized & of_node_is_attached that
> query the underlying kobject about the state (attached means kobj
> is visible in sysfs)
> 
> Using that make sure the lifecycle of the tree is correct at all
> times.
> 
> Signed-off-by: Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>
> [grant.likely: moved of_node_init() calls, fixed up locking, and
>               dropped __of_populate() hunks]
> Signed-off-by: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> 
> I've reworked Pantelis' patch to work on my tree. Here's the result.
> I've already got this in my tree and will be pushing it out shortly.
> 
> g.
> 
> drivers/of/base.c  | 35 ++++++++++++++++++++++++++---------
> drivers/of/fdt.c   |  3 +--
> drivers/of/pdt.c   |  3 +--
> include/linux/of.h | 19 +++++++++++++++++++
> 4 files changed, 47 insertions(+), 13 deletions(-)
> 

Looks good; booted and tested overlays with no issues.

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] OF: kobj node lifecycle fixes

[Grant Likely]

From: Pantelis Antoniou <panto@antoniou-consulting.com>

After the move to having device nodes be proper kobjects the lifecycle
of the node needs to be controlled better.

At first convert of_add_node() in the unflattened functions to
of_init_node() which initializes the kobject so that of_node_get/put
work correctly even before of_init is called.

Afterwards introduce of_node_is_initialized & of_node_is_attached that
query the underlying kobject about the state (attached means kobj
is visible in sysfs)

Using that make sure the lifecycle of the tree is correct at all
times.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
[grant.likely: moved of_node_init() calls, fixed up locking, and
               dropped __of_populate() hunks]
Signed-off-by: Grant Likely <grant.likely@linaro.org>
---

I've reworked Pantelis' patch to work on my tree. Here's the result.
I've already got this in my tree and will be pushing it out shortly.

g.

 drivers/of/base.c  | 35 ++++++++++++++++++++++++++---------
 drivers/of/fdt.c   |  3 +--
 drivers/of/pdt.c   |  3 +--
 include/linux/of.h | 19 +++++++++++++++++++
 4 files changed, 47 insertions(+), 13 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index ed3e70b84957..7083fad079a6 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -246,10 +246,19 @@ 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);
+
+	BUG_ON(!of_node_is_initialized(np));
+
+	/*
+	 * Grab the mutex here so that in a race condition between of_init() and
+	 * of_node_add(), node addition will still be consistent.
+	 */
 	mutex_lock(&of_aliases_mutex);
 	if (of_kset)
 		rc = __of_node_add(np);
+	else
+		/* This scenario may be perfectly valid, but report it anyway */
+		pr_info("of_node_add(%s) before of_init()\n", np->full_name);
 	mutex_unlock(&of_aliases_mutex);
 	return rc;
 }
@@ -259,10 +268,17 @@ static void of_node_remove(struct device_node *np)
 {
 	struct property *pp;
 
-	for_each_property_of_node(np, pp)
-		sysfs_remove_bin_file(&np->kobj, &pp->attr);
+	BUG_ON(!of_node_is_initialized(np));
+
+	/* only remove properties if on sysfs */
+	if (of_node_is_attached(np)) {
+		for_each_property_of_node(np, pp)
+			sysfs_remove_bin_file(&np->kobj, &pp->attr);
+		kobject_del(&np->kobj);
+	}
 
-	kobject_del(&np->kobj);
+	/* finally remove the kobj_init ref */
+	of_node_put(np);
 }
 #endif
 
@@ -1631,6 +1647,10 @@ static int of_property_notify(int action, struct device_node *np,
 {
 	struct of_prop_reconfig pr;
 
+	/* only call notifiers if the node is attached */
+	if (!of_node_is_attached(np))
+		return 0;
+
 	pr.dn = np;
 	pr.prop = prop;
 	return of_reconfig_notify(action, &pr);
@@ -1682,11 +1702,8 @@ int of_add_property(struct device_node *np, struct property *prop)
 	if (rc)
 		return rc;
 
-	/* at early boot, bail hear and defer setup to of_init() */
-	if (!of_kset)
-		return 0;
-
-	__of_add_property_sysfs(np, prop);
+	if (of_node_is_attached(np));
+		__of_add_property_sysfs(np, prop);
 
 	return rc;
 }
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 96ad1ab7f9d6..70ccc36513e7 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -202,6 +202,7 @@ static void * unflatten_dt_node(struct boot_param_header *blob,
 				__alignof__(struct device_node));
 	if (allnextpp) {
 		char *fn;
+		of_node_init(np);
 		np->full_name = fn = ((char *)np) + sizeof(*np);
 		if (new_format) {
 			/* rebuild full path for new format */
@@ -326,8 +327,6 @@ static void * unflatten_dt_node(struct boot_param_header *blob,
 			np->name = "<NULL>";
 		if (!np->type)
 			np->type = "<NULL>";
-
-		of_node_add(np);
 	}
 	while (tag == OF_DT_BEGIN_NODE || tag == OF_DT_NOP) {
 		if (tag == OF_DT_NOP)
diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
index e64fa3d3da5f..36b4035881b0 100644
--- a/drivers/of/pdt.c
+++ b/drivers/of/pdt.c
@@ -176,6 +176,7 @@ static struct device_node * __init of_pdt_create_node(phandle node,
 		return NULL;
 
 	dp = prom_early_alloc(sizeof(*dp));
+	of_node_init(dp);
 	of_pdt_incr_unique_id(dp);
 	dp->parent = parent;
 
@@ -213,7 +214,6 @@ static struct device_node * __init of_pdt_build_tree(struct device_node *parent,
 		*nextp = &dp->allnext;
 
 		dp->full_name = of_pdt_build_full_name(dp);
-		of_node_add(dp);
 
 		dp->child = of_pdt_build_tree(dp,
 				of_pdt_prom_ops->getchild(node), nextp);
@@ -244,7 +244,6 @@ void __init of_pdt_build_devicetree(phandle root_node, struct of_pdt_ops *ops)
 	of_allnodes->path_component_name = "";
 #endif
 	of_allnodes->full_name = "/";
-	of_node_add(of_allnodes);
 
 	nextp = &of_allnodes->allnext;
 	of_allnodes->child = of_pdt_build_tree(of_allnodes,
diff --git a/include/linux/of.h b/include/linux/of.h
index 257994a420f3..a8b9dad90c64 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -76,6 +76,25 @@ struct of_phandle_args {
 
 extern int of_node_add(struct device_node *node);
 
+/* initialize a node */
+extern struct kobj_type of_node_ktype;
+static inline void of_node_init(struct device_node *node)
+{
+	kobject_init(&node->kobj, &of_node_ktype);
+}
+
+/* true when node is initialized */
+static inline int of_node_is_initialized(struct device_node *node)
+{
+	return node && node->kobj.state_initialized;
+}
+
+/* true when node is attached (i.e. present on sysfs) */
+static inline int of_node_is_attached(struct device_node *node)
+{
+	return node && node->kobj.state_in_sysfs;
+}
+
 #ifdef CONFIG_OF_DYNAMIC
 extern struct device_node *of_node_get(struct device_node *node);
 extern void of_node_put(struct device_node *node);
-- 
1.8.3.2

[PATCH] OF: kobj node lifecycle fixes

[Grant Likely]

From: Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>

After the move to having device nodes be proper kobjects the lifecycle
of the node needs to be controlled better.

At first convert of_add_node() in the unflattened functions to
of_init_node() which initializes the kobject so that of_node_get/put
work correctly even before of_init is called.

Afterwards introduce of_node_is_initialized & of_node_is_attached that
query the underlying kobject about the state (attached means kobj
is visible in sysfs)

Using that make sure the lifecycle of the tree is correct at all
times.

Signed-off-by: Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>
[grant.likely: moved of_node_init() calls, fixed up locking, and
               dropped __of_populate() hunks]
Signed-off-by: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---

I've reworked Pantelis' patch to work on my tree. Here's the result.
I've already got this in my tree and will be pushing it out shortly.

g.

 drivers/of/base.c  | 35 ++++++++++++++++++++++++++---------
 drivers/of/fdt.c   |  3 +--
 drivers/of/pdt.c   |  3 +--
 include/linux/of.h | 19 +++++++++++++++++++
 4 files changed, 47 insertions(+), 13 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index ed3e70b84957..7083fad079a6 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -246,10 +246,19 @@ 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);
+
+	BUG_ON(!of_node_is_initialized(np));
+
+	/*
+	 * Grab the mutex here so that in a race condition between of_init() and
+	 * of_node_add(), node addition will still be consistent.
+	 */
 	mutex_lock(&of_aliases_mutex);
 	if (of_kset)
 		rc = __of_node_add(np);
+	else
+		/* This scenario may be perfectly valid, but report it anyway */
+		pr_info("of_node_add(%s) before of_init()\n", np->full_name);
 	mutex_unlock(&of_aliases_mutex);
 	return rc;
 }
@@ -259,10 +268,17 @@ static void of_node_remove(struct device_node *np)
 {
 	struct property *pp;
 
-	for_each_property_of_node(np, pp)
-		sysfs_remove_bin_file(&np->kobj, &pp->attr);
+	BUG_ON(!of_node_is_initialized(np));
+
+	/* only remove properties if on sysfs */
+	if (of_node_is_attached(np)) {
+		for_each_property_of_node(np, pp)
+			sysfs_remove_bin_file(&np->kobj, &pp->attr);
+		kobject_del(&np->kobj);
+	}
 
-	kobject_del(&np->kobj);
+	/* finally remove the kobj_init ref */
+	of_node_put(np);
 }
 #endif
 
@@ -1631,6 +1647,10 @@ static int of_property_notify(int action, struct device_node *np,
 {
 	struct of_prop_reconfig pr;
 
+	/* only call notifiers if the node is attached */
+	if (!of_node_is_attached(np))
+		return 0;
+
 	pr.dn = np;
 	pr.prop = prop;
 	return of_reconfig_notify(action, &pr);
@@ -1682,11 +1702,8 @@ int of_add_property(struct device_node *np, struct property *prop)
 	if (rc)
 		return rc;
 
-	/* at early boot, bail hear and defer setup to of_init() */
-	if (!of_kset)
-		return 0;
-
-	__of_add_property_sysfs(np, prop);
+	if (of_node_is_attached(np));
+		__of_add_property_sysfs(np, prop);
 
 	return rc;
 }
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 96ad1ab7f9d6..70ccc36513e7 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -202,6 +202,7 @@ static void * unflatten_dt_node(struct boot_param_header *blob,
 				__alignof__(struct device_node));
 	if (allnextpp) {
 		char *fn;
+		of_node_init(np);
 		np->full_name = fn = ((char *)np) + sizeof(*np);
 		if (new_format) {
 			/* rebuild full path for new format */
@@ -326,8 +327,6 @@ static void * unflatten_dt_node(struct boot_param_header *blob,
 			np->name = "<NULL>";
 		if (!np->type)
 			np->type = "<NULL>";
-
-		of_node_add(np);
 	}
 	while (tag == OF_DT_BEGIN_NODE || tag == OF_DT_NOP) {
 		if (tag == OF_DT_NOP)
diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
index e64fa3d3da5f..36b4035881b0 100644
--- a/drivers/of/pdt.c
+++ b/drivers/of/pdt.c
@@ -176,6 +176,7 @@ static struct device_node * __init of_pdt_create_node(phandle node,
 		return NULL;
 
 	dp = prom_early_alloc(sizeof(*dp));
+	of_node_init(dp);
 	of_pdt_incr_unique_id(dp);
 	dp->parent = parent;
 
@@ -213,7 +214,6 @@ static struct device_node * __init of_pdt_build_tree(struct device_node *parent,
 		*nextp = &dp->allnext;
 
 		dp->full_name = of_pdt_build_full_name(dp);
-		of_node_add(dp);
 
 		dp->child = of_pdt_build_tree(dp,
 				of_pdt_prom_ops->getchild(node), nextp);
@@ -244,7 +244,6 @@ void __init of_pdt_build_devicetree(phandle root_node, struct of_pdt_ops *ops)
 	of_allnodes->path_component_name = "";
 #endif
 	of_allnodes->full_name = "/";
-	of_node_add(of_allnodes);
 
 	nextp = &of_allnodes->allnext;
 	of_allnodes->child = of_pdt_build_tree(of_allnodes,
diff --git a/include/linux/of.h b/include/linux/of.h
index 257994a420f3..a8b9dad90c64 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -76,6 +76,25 @@ struct of_phandle_args {
 
 extern int of_node_add(struct device_node *node);
 
+/* initialize a node */
+extern struct kobj_type of_node_ktype;
+static inline void of_node_init(struct device_node *node)
+{
+	kobject_init(&node->kobj, &of_node_ktype);
+}
+
+/* true when node is initialized */
+static inline int of_node_is_initialized(struct device_node *node)
+{
+	return node && node->kobj.state_initialized;
+}
+
+/* true when node is attached (i.e. present on sysfs) */
+static inline int of_node_is_attached(struct device_node *node)
+{
+	return node && node->kobj.state_in_sysfs;
+}
+
 #ifdef CONFIG_OF_DYNAMIC
 extern struct device_node *of_node_get(struct device_node *node);
 extern void of_node_put(struct device_node *node);
-- 
1.8.3.2

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