[PATCH v3 0/4] of: remove *phandle properties from expanded device tree

[PATCH v3 0/4] of: remove *phandle properties from expanded device tree

[frowand.list-Re5JQEeQqe8AvxtiuMwx3w]

From: Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>

Remove "phandle" and "linux,phandle" properties from the internal
device tree.  The phandle will still be in the struct device_node
phandle field.

This is to resolve the issue found by Stephen Boyd [1] when he changed
the type of struct property.value from void * to const void *.  As
a result of the type change, the overlay code had compile errors
where the resolver updates phandle values.

  [1] http://lkml.iu.edu/hypermail/linux/kernel/1702.1/04160.html

Patch 1 is the phandle related changes.

Patches 2 - 4 are minor fixups for issues that became visible
while implementing patch 1.

Changes from v1:
   - patch 1: Remove check in __of_add_phandle_sysfs() that would not
     add a sysfs entry if IS_ENABLED(CONFIG_PPC_PSERIES)

Changes from v1:
   - Remove phandle properties in of_attach_node(), before attaching
     the node to the live tree.
   - Add the phandle sysfs entry for the node in of_attach_node().
   - When creating an overlay changeset, duplicate the node phandle in
     __of_node_dup().

Frank Rowand (4):
  of: remove *phandle properties from expanded device tree
  of: make __of_attach_node() static
  of: be consistent in form of file mode
  of: detect invalid phandle in overlay

 drivers/of/base.c       | 50 +++++++++++++++++++++++++++++++++++++++----
 drivers/of/dynamic.c    | 56 ++++++++++++++++++++++++++++++++++++-------------
 drivers/of/fdt.c        | 40 +++++++++++++++++++++--------------
 drivers/of/of_private.h |  2 +-
 drivers/of/overlay.c    |  8 ++++---
 drivers/of/resolver.c   | 23 +-------------------
 include/linux/of.h      |  1 +
 7 files changed, 120 insertions(+), 60 deletions(-)

-- 
Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>

--
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 v3 1/4] of: remove *phandle properties from expanded device tree

[frowand.list-Re5JQEeQqe8AvxtiuMwx3w]

From: Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>

Remove "phandle", "linux,phandle", and "ibm,phandle" properties from
the internal device tree.  The phandle will still be in the struct
device_node phandle field.

This is to resolve the issue found by Stephen Boyd [1] when he changed
the type of struct property.value from void * to const void *.  As
a result of the type change, the overlay code had compile errors
where the resolver updates phandle values.

  [1] http://lkml.iu.edu/hypermail/linux/kernel/1702.1/04160.html

- Add sysfs infrastructure to report np->phandle, as if it was a property.
- Do not create "phandle" "ibm,phandle", and "linux,phandle" properties
  in the expanded device tree.
- Remove phandle properties in of_attach_node(), for nodes dynamically
  attached to the live tree.  Add the phandle sysfs entry for these nodes.
- When creating an overlay changeset, duplicate the node phandle in
  __of_node_dup().
- Remove no longer needed checks to exclude "phandle" and "linux,phandle"
  properties in several locations.
- A side effect of these changes is that the obsolete "linux,phandle" and
  "ibm,phandle" properties will no longer appear in /proc/device-tree (they
  will appear as "phandle").

Signed-off-by: Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>
---
 drivers/of/base.c       | 48 ++++++++++++++++++++++++++++++++++++++++---
 drivers/of/dynamic.c    | 54 +++++++++++++++++++++++++++++++++++++------------
 drivers/of/fdt.c        | 40 +++++++++++++++++++++---------------
 drivers/of/of_private.h |  1 +
 drivers/of/overlay.c    |  4 +---
 drivers/of/resolver.c   | 23 +--------------------
 include/linux/of.h      |  1 +
 7 files changed, 114 insertions(+), 57 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index d7c4629a3a2d..41473b1e2445 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -116,6 +116,19 @@ static ssize_t of_node_property_read(struct file *filp, struct kobject *kobj,
 	return memory_read_from_buffer(buf, count, &offset, pp->value, pp->length);
 }
 
+static ssize_t of_node_phandle_read(struct file *filp, struct kobject *kobj,
+				struct bin_attribute *bin_attr, char *buf,
+				loff_t offset, size_t count)
+{
+	phandle phandle;
+	struct device_node *np;
+
+	np = container_of(bin_attr, struct device_node, attr_phandle);
+	phandle = cpu_to_be32(np->phandle);
+	return memory_read_from_buffer(buf, count, &offset, &phandle,
+				       sizeof(phandle));
+}
+
 /* always return newly allocated name, caller must free after use */
 static const char *safe_name(struct kobject *kobj, const char *orig_name)
 {
@@ -164,6 +177,35 @@ int __of_add_property_sysfs(struct device_node *np, struct property *pp)
 	return rc;
 }
 
+/*
+ * In the imported device tree (fdt), phandle is a property.  In the
+ * internal data structure it is instead stored in the struct device_node.
+ * Make phandle visible in sysfs as if it was a property.
+ */
+int __of_add_phandle_sysfs(struct device_node *np)
+{
+	int rc;
+
+	if (!IS_ENABLED(CONFIG_SYSFS))
+		return 0;
+
+	if (!of_kset || !of_node_is_attached(np))
+		return 0;
+
+	if (!np->phandle || np->phandle == 0xffffffff)
+		return 0;
+
+	sysfs_bin_attr_init(&pp->attr);
+	np->attr_phandle.attr.name = "phandle";
+	np->attr_phandle.attr.mode = 0444;
+	np->attr_phandle.size = sizeof(np->phandle);
+	np->attr_phandle.read = of_node_phandle_read;
+
+	rc = sysfs_create_bin_file(&np->kobj, &np->attr_phandle);
+	WARN(rc, "error adding attribute phandle to node %s\n", np->full_name);
+	return rc;
+}
+
 int __of_attach_node_sysfs(struct device_node *np)
 {
 	const char *name;
@@ -193,6 +235,8 @@ int __of_attach_node_sysfs(struct device_node *np)
 	if (rc)
 		return rc;
 
+	__of_add_phandle_sysfs(np);
+
 	for_each_property_of_node(np, pp)
 		__of_add_property_sysfs(np, pp);
 
@@ -2097,9 +2141,7 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align))
 		int id, len;
 
 		/* Skip those we do not want to proceed */
-		if (!strcmp(pp->name, "name") ||
-		    !strcmp(pp->name, "phandle") ||
-		    !strcmp(pp->name, "linux,phandle"))
+		if (!strcmp(pp->name, "name"))
 			continue;
 
 		np = of_find_node_by_path(pp->value);
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 888fdbc09992..59545b8fbf46 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -218,19 +218,6 @@ int of_property_notify(int action, struct device_node *np,
 
 void __of_attach_node(struct device_node *np)
 {
-	const __be32 *phandle;
-	int sz;
-
-	np->name = __of_get_property(np, "name", NULL) ? : "<NULL>";
-	np->type = __of_get_property(np, "device_type", NULL) ? : "<NULL>";
-
-	phandle = __of_get_property(np, "phandle", &sz);
-	if (!phandle)
-		phandle = __of_get_property(np, "linux,phandle", &sz);
-	if (IS_ENABLED(CONFIG_PPC_PSERIES) && !phandle)
-		phandle = __of_get_property(np, "ibm,phandle", &sz);
-	np->phandle = (phandle && (sz >= 4)) ? be32_to_cpup(phandle) : 0;
-
 	np->child = NULL;
 	np->sibling = np->parent->child;
 	np->parent->child = np;
@@ -243,11 +230,47 @@ void __of_attach_node(struct device_node *np)
 int of_attach_node(struct device_node *np)
 {
 	struct of_reconfig_data rd;
+	struct property *prev;
+	struct property *prop;
+	struct property *save_next;
 	unsigned long flags;
+	const __be32 *phandle;
+	int sz;
 
 	memset(&rd, 0, sizeof(rd));
 	rd.dn = np;
 
+	np->name = __of_get_property(np, "name", NULL) ? : "<NULL>";
+	np->type = __of_get_property(np, "device_type", NULL) ? : "<NULL>";
+
+	phandle = __of_get_property(np, "phandle", &sz);
+	if (!phandle)
+		phandle = __of_get_property(np, "linux,phandle", &sz);
+	if (IS_ENABLED(CONFIG_PPC_PSERIES) && !phandle)
+		phandle = __of_get_property(np, "ibm,phandle", &sz);
+	np->phandle = (phandle && (sz >= 4)) ? be32_to_cpup(phandle) : 0;
+
+	/* remove phandle properties from node */
+	prev = NULL;
+	for (prop = np->properties; prop != NULL; ) {
+		save_next = prop->next;
+		if (!strcmp(prop->name, "phandle") ||
+		    !strcmp(prop->name, "ibm,phandle") ||
+		    !strcmp(prop->name, "linux,phandle")) {
+			if (prev)
+				prev->next = prop->next;
+			else
+				np->properties = prop->next;
+			prop->next = np->deadprops;
+			np->deadprops = prop;
+		} else {
+			prev = prop;
+		}
+		prop = save_next;
+	}
+
+	__of_add_phandle_sysfs(np);
+
 	mutex_lock(&of_mutex);
 	raw_spin_lock_irqsave(&devtree_lock, flags);
 	__of_attach_node(np);
@@ -429,6 +452,7 @@ struct device_node *__of_node_dup(const struct device_node *np, const char *fmt,
 	/* Iterate over and duplicate all properties */
 	if (np) {
 		struct property *pp, *new_pp;
+		node->phandle = np->phandle;
 		for_each_property_of_node(np, pp) {
 			new_pp = __of_prop_dup(pp, GFP_KERNEL);
 			if (!new_pp)
@@ -441,6 +465,10 @@ struct device_node *__of_node_dup(const struct device_node *np, const char *fmt,
 			}
 		}
 	}
+
+	node->name = __of_get_property(node, "name", NULL) ? : "<NULL>";
+	node->type = __of_get_property(node, "device_type", NULL) ? : "<NULL>";
+
 	return node;
 
  err_prop:
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index e5ce4b59e162..270f31b0c259 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -181,6 +181,8 @@ static void populate_properties(const void *blob,
 		const __be32 *val;
 		const char *pname;
 		u32 sz;
+		int prop_is_phandle = 0;
+		int prop_is_ibm_phandle = 0;
 
 		val = fdt_getprop_by_offset(blob, cur, &pname, &sz);
 		if (!val) {
@@ -196,11 +198,6 @@ static void populate_properties(const void *blob,
 		if (!strcmp(pname, "name"))
 			has_name = true;
 
-		pp = unflatten_dt_alloc(mem, sizeof(struct property),
-					__alignof__(struct property));
-		if (dryrun)
-			continue;
-
 		/* We accept flattened tree phandles either in
 		 * ePAPR-style "phandle" properties, or the
 		 * legacy "linux,phandle" properties.  If both
@@ -208,23 +205,34 @@ static void populate_properties(const void *blob,
 		 * will get weird. Don't do that.
 		 */
 		if (!strcmp(pname, "phandle") ||
-		    !strcmp(pname, "linux,phandle")) {
-			if (!np->phandle)
-				np->phandle = be32_to_cpup(val);
-		}
+		    !strcmp(pname, "linux,phandle"))
+			prop_is_phandle = 1;
 
 		/* And we process the "ibm,phandle" property
 		 * used in pSeries dynamic device tree
 		 * stuff
 		 */
-		if (!strcmp(pname, "ibm,phandle"))
-			np->phandle = be32_to_cpup(val);
+		if (!strcmp(pname, "ibm,phandle")) {
+			prop_is_phandle = 1;
+			prop_is_ibm_phandle = 1;
+		}
+
+		if (!prop_is_phandle)
+			pp = unflatten_dt_alloc(mem, sizeof(struct property),
+						__alignof__(struct property));
 
-		pp->name   = (char *)pname;
-		pp->length = sz;
-		pp->value  = (__be32 *)val;
-		*pprev     = pp;
-		pprev      = &pp->next;
+		if (dryrun)
+			continue;
+
+		if (!prop_is_phandle) {
+			pp->name   = (char *)pname;
+			pp->length = sz;
+			pp->value  = (__be32 *)val;
+			*pprev     = pp;
+			pprev      = &pp->next;
+		} else if (prop_is_ibm_phandle || !np->phandle) {
+			np->phandle = be32_to_cpup(val);
+		}
 	}
 
 	/* With version 0x10 we may not have the name property,
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 18bbb4517e25..33f11a5384f3 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -47,6 +47,7 @@ extern int of_property_notify(int action, struct device_node *np,
 extern void of_node_release(struct kobject *kobj);
 extern int __of_changeset_apply(struct of_changeset *ocs);
 extern int __of_changeset_revert(struct of_changeset *ocs);
+extern int __of_add_phandle_sysfs(struct device_node *np);
 #else /* CONFIG_OF_DYNAMIC */
 static inline int of_property_notify(int action, struct device_node *np,
 				     struct property *prop, struct property *old_prop)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 7827786718d8..ca0b85f5deb1 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -101,9 +101,7 @@ static int of_overlay_apply_single_property(struct of_overlay *ov,
 	tprop = of_find_property(target, prop->name, NULL);
 
 	/* special properties are not meant to be updated (silent NOP) */
-	if (of_prop_cmp(prop->name, "name") == 0 ||
-	    of_prop_cmp(prop->name, "phandle") == 0 ||
-	    of_prop_cmp(prop->name, "linux,phandle") == 0)
+	if (!of_prop_cmp(prop->name, "name"))
 		return 0;
 
 	propn = __of_prop_dup(prop, GFP_KERNEL);
diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
index 7ae9863cb0a4..624cbaeae0a4 100644
--- a/drivers/of/resolver.c
+++ b/drivers/of/resolver.c
@@ -71,30 +71,11 @@ static void adjust_overlay_phandles(struct device_node *overlay,
 		int phandle_delta)
 {
 	struct device_node *child;
-	struct property *prop;
-	phandle phandle;
 
 	/* adjust node's phandle in node */
 	if (overlay->phandle != 0 && overlay->phandle != OF_PHANDLE_ILLEGAL)
 		overlay->phandle += phandle_delta;
 
-	/* copy adjusted phandle into *phandle properties */
-	for_each_property_of_node(overlay, prop) {
-
-		if (of_prop_cmp(prop->name, "phandle") &&
-		    of_prop_cmp(prop->name, "linux,phandle"))
-			continue;
-
-		if (prop->length < 4)
-			continue;
-
-		phandle = be32_to_cpup(prop->value);
-		if (phandle == OF_PHANDLE_ILLEGAL)
-			continue;
-
-		*(uint32_t *)prop->value = cpu_to_be32(overlay->phandle);
-	}
-
 	for_each_child_of_node(overlay, child)
 		adjust_overlay_phandles(child, phandle_delta);
 }
@@ -197,9 +178,7 @@ static int adjust_local_phandle_references(struct device_node *local_fixups,
 	for_each_property_of_node(local_fixups, prop_fix) {
 
 		/* skip properties added automatically */
-		if (!of_prop_cmp(prop_fix->name, "name") ||
-		    !of_prop_cmp(prop_fix->name, "phandle") ||
-		    !of_prop_cmp(prop_fix->name, "linux,phandle"))
+		if (!of_prop_cmp(prop_fix->name, "name"))
 			continue;
 
 		if ((prop_fix->length % 4) != 0 || prop_fix->length == 0)
diff --git a/include/linux/of.h b/include/linux/of.h
index 21e6323de0f3..4e86e05853f3 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -61,6 +61,7 @@ struct device_node {
 	struct	kobject kobj;
 	unsigned long _flags;
 	void	*data;
+	struct bin_attribute attr_phandle;
 #if defined(CONFIG_SPARC)
 	const char *path_component_name;
 	unsigned int unique_id;
-- 
Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>

--
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 v3 1/4] of: remove *phandle properties from expanded device tree

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 8074 bytes --]

Hi Frank,

[auto build test ERROR on robh/for-next]
[also build test ERROR on v4.11-rc8 next-20170428]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/frowand-list-gmail-com/of-remove-phandle-properties-from-expanded-device-tree/20170426-000149
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: s390-allmodconfig (attached as .config)
compiler: s390x-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=s390 

All errors (new ones prefixed by >>):

   In file included from include/linux/kobject.h:21:0,
                    from include/linux/device.h:17,
                    from include/linux/node.h:17,
                    from include/linux/cpu.h:16,
                    from drivers/of/base.c:25:
   drivers/of/base.c: In function '__of_add_phandle_sysfs':
>> drivers/of/base.c:198:23: error: 'pp' undeclared (first use in this function)
     sysfs_bin_attr_init(&pp->attr);
                          ^
   include/linux/sysfs.h:54:3: note: in definition of macro 'sysfs_attr_init'
     (attr)->key = &__key;    \
      ^~~~
   drivers/of/base.c:198:2: note: in expansion of macro 'sysfs_bin_attr_init'
     sysfs_bin_attr_init(&pp->attr);
     ^~~~~~~~~~~~~~~~~~~
   drivers/of/base.c:198:23: note: each undeclared identifier is reported only once for each function it appears in
     sysfs_bin_attr_init(&pp->attr);
                          ^
   include/linux/sysfs.h:54:3: note: in definition of macro 'sysfs_attr_init'
     (attr)->key = &__key;    \
      ^~~~
   drivers/of/base.c:198:2: note: in expansion of macro 'sysfs_bin_attr_init'
     sysfs_bin_attr_init(&pp->attr);
     ^~~~~~~~~~~~~~~~~~~

vim +/pp +198 drivers/of/base.c

    19	 */
    20	
    21	#define pr_fmt(fmt)	"OF: " fmt
    22	
    23	#include <linux/console.h>
    24	#include <linux/ctype.h>
  > 25	#include <linux/cpu.h>
    26	#include <linux/module.h>
    27	#include <linux/of.h>
    28	#include <linux/of_device.h>
    29	#include <linux/of_graph.h>
    30	#include <linux/spinlock.h>
    31	#include <linux/slab.h>
    32	#include <linux/string.h>
    33	#include <linux/proc_fs.h>
    34	
    35	#include "of_private.h"
    36	
    37	LIST_HEAD(aliases_lookup);
    38	
    39	struct device_node *of_root;
    40	EXPORT_SYMBOL(of_root);
    41	struct device_node *of_chosen;
    42	struct device_node *of_aliases;
    43	struct device_node *of_stdout;
    44	static const char *of_stdout_options;
    45	
    46	struct kset *of_kset;
    47	
    48	/*
    49	 * Used to protect the of_aliases, to hold off addition of nodes to sysfs.
    50	 * This mutex must be held whenever modifications are being made to the
    51	 * device tree. The of_{attach,detach}_node() and
    52	 * of_{add,remove,update}_property() helpers make sure this happens.
    53	 */
    54	DEFINE_MUTEX(of_mutex);
    55	
    56	/* use when traversing tree through the child, sibling,
    57	 * or parent members of struct device_node.
    58	 */
    59	DEFINE_RAW_SPINLOCK(devtree_lock);
    60	
    61	int of_n_addr_cells(struct device_node *np)
    62	{
    63		const __be32 *ip;
    64	
    65		do {
    66			if (np->parent)
    67				np = np->parent;
    68			ip = of_get_property(np, "#address-cells", NULL);
    69			if (ip)
    70				return be32_to_cpup(ip);
    71		} while (np->parent);
    72		/* No #address-cells property for the root node */
    73		return OF_ROOT_NODE_ADDR_CELLS_DEFAULT;
    74	}
    75	EXPORT_SYMBOL(of_n_addr_cells);
    76	
    77	int of_n_size_cells(struct device_node *np)
    78	{
    79		const __be32 *ip;
    80	
    81		do {
    82			if (np->parent)
    83				np = np->parent;
    84			ip = of_get_property(np, "#size-cells", NULL);
    85			if (ip)
    86				return be32_to_cpup(ip);
    87		} while (np->parent);
    88		/* No #size-cells property for the root node */
    89		return OF_ROOT_NODE_SIZE_CELLS_DEFAULT;
    90	}
    91	EXPORT_SYMBOL(of_n_size_cells);
    92	
    93	#ifdef CONFIG_NUMA
    94	int __weak of_node_to_nid(struct device_node *np)
    95	{
    96		return NUMA_NO_NODE;
    97	}
    98	#endif
    99	
   100	#ifndef CONFIG_OF_DYNAMIC
   101	static void of_node_release(struct kobject *kobj)
   102	{
   103		/* Without CONFIG_OF_DYNAMIC, no nodes gets freed */
   104	}
   105	#endif /* CONFIG_OF_DYNAMIC */
   106	
   107	struct kobj_type of_node_ktype = {
   108		.release = of_node_release,
   109	};
   110	
   111	static ssize_t of_node_property_read(struct file *filp, struct kobject *kobj,
   112					struct bin_attribute *bin_attr, char *buf,
   113					loff_t offset, size_t count)
   114	{
   115		struct property *pp = container_of(bin_attr, struct property, attr);
   116		return memory_read_from_buffer(buf, count, &offset, pp->value, pp->length);
   117	}
   118	
   119	static ssize_t of_node_phandle_read(struct file *filp, struct kobject *kobj,
   120					struct bin_attribute *bin_attr, char *buf,
   121					loff_t offset, size_t count)
   122	{
   123		phandle phandle;
   124		struct device_node *np;
   125	
   126		np = container_of(bin_attr, struct device_node, attr_phandle);
   127		phandle = cpu_to_be32(np->phandle);
   128		return memory_read_from_buffer(buf, count, &offset, &phandle,
   129					       sizeof(phandle));
   130	}
   131	
   132	/* always return newly allocated name, caller must free after use */
   133	static const char *safe_name(struct kobject *kobj, const char *orig_name)
   134	{
   135		const char *name = orig_name;
   136		struct kernfs_node *kn;
   137		int i = 0;
   138	
   139		/* don't be a hero. After 16 tries give up */
   140		while (i < 16 && (kn = sysfs_get_dirent(kobj->sd, name))) {
   141			sysfs_put(kn);
   142			if (name != orig_name)
   143				kfree(name);
   144			name = kasprintf(GFP_KERNEL, "%s#%i", orig_name, ++i);
   145		}
   146	
   147		if (name == orig_name) {
   148			name = kstrdup(orig_name, GFP_KERNEL);
   149		} else {
   150			pr_warn("Duplicate name in %s, renamed to \"%s\"\n",
   151				kobject_name(kobj), name);
   152		}
   153		return name;
   154	}
   155	
   156	int __of_add_property_sysfs(struct device_node *np, struct property *pp)
   157	{
   158		int rc;
   159	
   160		/* Important: Don't leak passwords */
   161		bool secure = strncmp(pp->name, "security-", 9) == 0;
   162	
   163		if (!IS_ENABLED(CONFIG_SYSFS))
   164			return 0;
   165	
   166		if (!of_kset || !of_node_is_attached(np))
   167			return 0;
   168	
   169		sysfs_bin_attr_init(&pp->attr);
   170		pp->attr.attr.name = safe_name(&np->kobj, pp->name);
   171		pp->attr.attr.mode = secure ? S_IRUSR : S_IRUGO;
   172		pp->attr.size = secure ? 0 : pp->length;
   173		pp->attr.read = of_node_property_read;
   174	
   175		rc = sysfs_create_bin_file(&np->kobj, &pp->attr);
   176		WARN(rc, "error adding attribute %s to node %s\n", pp->name, np->full_name);
   177		return rc;
   178	}
   179	
   180	/*
   181	 * In the imported device tree (fdt), phandle is a property.  In the
   182	 * internal data structure it is instead stored in the struct device_node.
   183	 * Make phandle visible in sysfs as if it was a property.
   184	 */
   185	int __of_add_phandle_sysfs(struct device_node *np)
   186	{
   187		int rc;
   188	
   189		if (!IS_ENABLED(CONFIG_SYSFS))
   190			return 0;
   191	
   192		if (!of_kset || !of_node_is_attached(np))
   193			return 0;
   194	
   195		if (!np->phandle || np->phandle == 0xffffffff)
   196			return 0;
   197	
 > 198		sysfs_bin_attr_init(&pp->attr);
   199		np->attr_phandle.attr.name = "phandle";
   200		np->attr_phandle.attr.mode = 0444;
   201		np->attr_phandle.size = sizeof(np->phandle);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 45067 bytes --]

Re: [PATCH v3 1/4] of: remove *phandle properties from expanded device tree

[Frank Rowand]

On 04/29/17 17:22, kbuild test robot wrote:
> Hi Frank,
> 
> [auto build test ERROR on robh/for-next]
> [also build test ERROR on v4.11-rc8 next-20170428]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/frowand-list-gmail-com/of-remove-phandle-properties-from-expanded-device-tree/20170426-000149
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
> config: s390-allmodconfig (attached as .config)
> compiler: s390x-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
> reproduce:
>         wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         make.cross ARCH=s390 
> 
> All errors (new ones prefixed by >>):
> 
>    In file included from include/linux/kobject.h:21:0,
>                     from include/linux/device.h:17,
>                     from include/linux/node.h:17,
>                     from include/linux/cpu.h:16,
>                     from drivers/of/base.c:25:
>    drivers/of/base.c: In function '__of_add_phandle_sysfs':
>>> drivers/of/base.c:198:23: error: 'pp' undeclared (first use in this function)
>      sysfs_bin_attr_init(&pp->attr);
>                           ^

Thanks for the report!

A patch to fix this is now submitted to Rob.

-Frank

>    include/linux/sysfs.h:54:3: note: in definition of macro 'sysfs_attr_init'
>      (attr)->key = &__key;    \
>       ^~~~
>    drivers/of/base.c:198:2: note: in expansion of macro 'sysfs_bin_attr_init'
>      sysfs_bin_attr_init(&pp->attr);
>      ^~~~~~~~~~~~~~~~~~~
>    drivers/of/base.c:198:23: note: each undeclared identifier is reported only once for each function it appears in
>      sysfs_bin_attr_init(&pp->attr);
>                           ^
>    include/linux/sysfs.h:54:3: note: in definition of macro 'sysfs_attr_init'
>      (attr)->key = &__key;    \
>       ^~~~
>    drivers/of/base.c:198:2: note: in expansion of macro 'sysfs_bin_attr_init'
>      sysfs_bin_attr_init(&pp->attr);
>      ^~~~~~~~~~~~~~~~~~~
> 
> vim +/pp +198 drivers/of/base.c
> 
>     19	 */
>     20	
>     21	#define pr_fmt(fmt)	"OF: " fmt
>     22	
>     23	#include <linux/console.h>
>     24	#include <linux/ctype.h>
>   > 25	#include <linux/cpu.h>
>     26	#include <linux/module.h>
>     27	#include <linux/of.h>
>     28	#include <linux/of_device.h>
>     29	#include <linux/of_graph.h>
>     30	#include <linux/spinlock.h>
>     31	#include <linux/slab.h>
>     32	#include <linux/string.h>
>     33	#include <linux/proc_fs.h>
>     34	
>     35	#include "of_private.h"
>     36	
>     37	LIST_HEAD(aliases_lookup);
>     38	
>     39	struct device_node *of_root;
>     40	EXPORT_SYMBOL(of_root);
>     41	struct device_node *of_chosen;
>     42	struct device_node *of_aliases;
>     43	struct device_node *of_stdout;
>     44	static const char *of_stdout_options;
>     45	
>     46	struct kset *of_kset;
>     47	
>     48	/*
>     49	 * Used to protect the of_aliases, to hold off addition of nodes to sysfs.
>     50	 * This mutex must be held whenever modifications are being made to the
>     51	 * device tree. The of_{attach,detach}_node() and
>     52	 * of_{add,remove,update}_property() helpers make sure this happens.
>     53	 */
>     54	DEFINE_MUTEX(of_mutex);
>     55	
>     56	/* use when traversing tree through the child, sibling,
>     57	 * or parent members of struct device_node.
>     58	 */
>     59	DEFINE_RAW_SPINLOCK(devtree_lock);
>     60	
>     61	int of_n_addr_cells(struct device_node *np)
>     62	{
>     63		const __be32 *ip;
>     64	
>     65		do {
>     66			if (np->parent)
>     67				np = np->parent;
>     68			ip = of_get_property(np, "#address-cells", NULL);
>     69			if (ip)
>     70				return be32_to_cpup(ip);
>     71		} while (np->parent);
>     72		/* No #address-cells property for the root node */
>     73		return OF_ROOT_NODE_ADDR_CELLS_DEFAULT;
>     74	}
>     75	EXPORT_SYMBOL(of_n_addr_cells);
>     76	
>     77	int of_n_size_cells(struct device_node *np)
>     78	{
>     79		const __be32 *ip;
>     80	
>     81		do {
>     82			if (np->parent)
>     83				np = np->parent;
>     84			ip = of_get_property(np, "#size-cells", NULL);
>     85			if (ip)
>     86				return be32_to_cpup(ip);
>     87		} while (np->parent);
>     88		/* No #size-cells property for the root node */
>     89		return OF_ROOT_NODE_SIZE_CELLS_DEFAULT;
>     90	}
>     91	EXPORT_SYMBOL(of_n_size_cells);
>     92	
>     93	#ifdef CONFIG_NUMA
>     94	int __weak of_node_to_nid(struct device_node *np)
>     95	{
>     96		return NUMA_NO_NODE;
>     97	}
>     98	#endif
>     99	
>    100	#ifndef CONFIG_OF_DYNAMIC
>    101	static void of_node_release(struct kobject *kobj)
>    102	{
>    103		/* Without CONFIG_OF_DYNAMIC, no nodes gets freed */
>    104	}
>    105	#endif /* CONFIG_OF_DYNAMIC */
>    106	
>    107	struct kobj_type of_node_ktype = {
>    108		.release = of_node_release,
>    109	};
>    110	
>    111	static ssize_t of_node_property_read(struct file *filp, struct kobject *kobj,
>    112					struct bin_attribute *bin_attr, char *buf,
>    113					loff_t offset, size_t count)
>    114	{
>    115		struct property *pp = container_of(bin_attr, struct property, attr);
>    116		return memory_read_from_buffer(buf, count, &offset, pp->value, pp->length);
>    117	}
>    118	
>    119	static ssize_t of_node_phandle_read(struct file *filp, struct kobject *kobj,
>    120					struct bin_attribute *bin_attr, char *buf,
>    121					loff_t offset, size_t count)
>    122	{
>    123		phandle phandle;
>    124		struct device_node *np;
>    125	
>    126		np = container_of(bin_attr, struct device_node, attr_phandle);
>    127		phandle = cpu_to_be32(np->phandle);
>    128		return memory_read_from_buffer(buf, count, &offset, &phandle,
>    129					       sizeof(phandle));
>    130	}
>    131	
>    132	/* always return newly allocated name, caller must free after use */
>    133	static const char *safe_name(struct kobject *kobj, const char *orig_name)
>    134	{
>    135		const char *name = orig_name;
>    136		struct kernfs_node *kn;
>    137		int i = 0;
>    138	
>    139		/* don't be a hero. After 16 tries give up */
>    140		while (i < 16 && (kn = sysfs_get_dirent(kobj->sd, name))) {
>    141			sysfs_put(kn);
>    142			if (name != orig_name)
>    143				kfree(name);
>    144			name = kasprintf(GFP_KERNEL, "%s#%i", orig_name, ++i);
>    145		}
>    146	
>    147		if (name == orig_name) {
>    148			name = kstrdup(orig_name, GFP_KERNEL);
>    149		} else {
>    150			pr_warn("Duplicate name in %s, renamed to \"%s\"\n",
>    151				kobject_name(kobj), name);
>    152		}
>    153		return name;
>    154	}
>    155	
>    156	int __of_add_property_sysfs(struct device_node *np, struct property *pp)
>    157	{
>    158		int rc;
>    159	
>    160		/* Important: Don't leak passwords */
>    161		bool secure = strncmp(pp->name, "security-", 9) == 0;
>    162	
>    163		if (!IS_ENABLED(CONFIG_SYSFS))
>    164			return 0;
>    165	
>    166		if (!of_kset || !of_node_is_attached(np))
>    167			return 0;
>    168	
>    169		sysfs_bin_attr_init(&pp->attr);
>    170		pp->attr.attr.name = safe_name(&np->kobj, pp->name);
>    171		pp->attr.attr.mode = secure ? S_IRUSR : S_IRUGO;
>    172		pp->attr.size = secure ? 0 : pp->length;
>    173		pp->attr.read = of_node_property_read;
>    174	
>    175		rc = sysfs_create_bin_file(&np->kobj, &pp->attr);
>    176		WARN(rc, "error adding attribute %s to node %s\n", pp->name, np->full_name);
>    177		return rc;
>    178	}
>    179	
>    180	/*
>    181	 * In the imported device tree (fdt), phandle is a property.  In the
>    182	 * internal data structure it is instead stored in the struct device_node.
>    183	 * Make phandle visible in sysfs as if it was a property.
>    184	 */
>    185	int __of_add_phandle_sysfs(struct device_node *np)
>    186	{
>    187		int rc;
>    188	
>    189		if (!IS_ENABLED(CONFIG_SYSFS))
>    190			return 0;
>    191	
>    192		if (!of_kset || !of_node_is_attached(np))
>    193			return 0;
>    194	
>    195		if (!np->phandle || np->phandle == 0xffffffff)
>    196			return 0;
>    197	
>  > 198		sysfs_bin_attr_init(&pp->attr);
>    199		np->attr_phandle.attr.name = "phandle";
>    200		np->attr_phandle.attr.mode = 0444;
>    201		np->attr_phandle.size = sizeof(np->phandle);
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
> 

--
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 v3 2/4] of: make __of_attach_node() static

[frowand.list-Re5JQEeQqe8AvxtiuMwx3w]

From: Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>

__of_attach_node() is not used outside of drivers/of/dynamic.c.  Make
it static and remove it from drivers/of/of_private.h.

Signed-off-by: Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>
---
 drivers/of/dynamic.c    | 2 +-
 drivers/of/of_private.h | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 59545b8fbf46..0b9cf6d5914c 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -216,7 +216,7 @@ int of_property_notify(int action, struct device_node *np,
 	return of_reconfig_notify(action, &pr);
 }
 
-void __of_attach_node(struct device_node *np)
+static void __of_attach_node(struct device_node *np)
 {
 	np->child = NULL;
 	np->sibling = np->parent->child;
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 33f11a5384f3..b1ff70e1fdda 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -79,7 +79,6 @@ extern int __of_update_property(struct device_node *np,
 extern void __of_update_property_sysfs(struct device_node *np,
 		struct property *newprop, struct property *oldprop);
 
-extern void __of_attach_node(struct device_node *np);
 extern int __of_attach_node_sysfs(struct device_node *np);
 extern void __of_detach_node(struct device_node *np);
 extern void __of_detach_node_sysfs(struct device_node *np);
-- 
Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>

--
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 v3 3/4] of: be consistent in form of file mode

[frowand.list-Re5JQEeQqe8AvxtiuMwx3w]

From: Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>

checkpatch whined about using S_IRUGO instead of octal equivalent
when adding phandle sysfs code, so used octal in that patch.
Change other instances of the S_* constants in the same file to
the octal form.

Signed-off-by: Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>
---
 drivers/of/base.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 41473b1e2445..9fe42346925b 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -168,7 +168,7 @@ int __of_add_property_sysfs(struct device_node *np, struct property *pp)
 
 	sysfs_bin_attr_init(&pp->attr);
 	pp->attr.attr.name = safe_name(&np->kobj, pp->name);
-	pp->attr.attr.mode = secure ? S_IRUSR : S_IRUGO;
+	pp->attr.attr.mode = secure ? 0400 : 0444;
 	pp->attr.size = secure ? 0 : pp->length;
 	pp->attr.read = of_node_property_read;
 
-- 
Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>

--
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 v3 0/4] of: remove *phandle properties from expanded device tree

[Frank Rowand]

On 04/25/17 01:47, frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> From: Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>
> 
> Remove "phandle" and "linux,phandle" properties from the internal
> device tree.  The phandle will still be in the struct device_node
> phandle field.
> 
> This is to resolve the issue found by Stephen Boyd [1] when he changed
> the type of struct property.value from void * to const void *.  As
> a result of the type change, the overlay code had compile errors
> where the resolver updates phandle values.
> 
>   [1] http://lkml.iu.edu/hypermail/linux/kernel/1702.1/04160.html
> 
> Patch 1 is the phandle related changes.
> 
> Patches 2 - 4 are minor fixups for issues that became visible
> while implementing patch 1.
> 
> Changes from v1:

          ^^^^^^^^
  Changes from v2


>    - patch 1: Remove check in __of_add_phandle_sysfs() that would not
>      add a sysfs entry if IS_ENABLED(CONFIG_PPC_PSERIES)
> 
> Changes from v1:
>    - Remove phandle properties in of_attach_node(), before attaching
>      the node to the live tree.
>    - Add the phandle sysfs entry for the node in of_attach_node().
>    - When creating an overlay changeset, duplicate the node phandle in
>      __of_node_dup().
> 
> Frank Rowand (4):
>   of: remove *phandle properties from expanded device tree
>   of: make __of_attach_node() static
>   of: be consistent in form of file mode
>   of: detect invalid phandle in overlay
> 
>  drivers/of/base.c       | 50 +++++++++++++++++++++++++++++++++++++++----
>  drivers/of/dynamic.c    | 56 ++++++++++++++++++++++++++++++++++++-------------
>  drivers/of/fdt.c        | 40 +++++++++++++++++++++--------------
>  drivers/of/of_private.h |  2 +-
>  drivers/of/overlay.c    |  8 ++++---
>  drivers/of/resolver.c   | 23 +-------------------
>  include/linux/of.h      |  1 +
>  7 files changed, 120 insertions(+), 60 deletions(-)
> 

--
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 v3 4/4] of: detect invalid phandle in overlay

[frowand.list]

From: Frank Rowand <frank.rowand@sony.com>

Overlays are not allowed to modify phandle values of previously existing
nodes because there is no information available to allow fixup up
properties that use the previously existing phandle.

Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
 drivers/of/overlay.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index ca0b85f5deb1..20ab49d2f7a4 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -130,6 +130,10 @@ static int of_overlay_apply_single_device_node(struct of_overlay *ov,
 	/* NOTE: Multiple mods of created nodes not supported */
 	tchild = of_get_child_by_name(target, cname);
 	if (tchild != NULL) {
+		/* new overlay phandle value conflicts with existing value */
+		if (child->phandle)
+			return -EINVAL;
+
 		/* apply overlay recursively */
 		ret = of_overlay_apply_one(ov, tchild, child);
 		of_node_put(tchild);
-- 
Frank Rowand <frank.rowand@sony.com>