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

[PATCH v6 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 and will still be displayed as if it is a property
in /proc/device_tree.

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 v5:
   - patch 1: populate_properties(), prop_is_phandle was declared
     at the wrong scope and thus was initialized before the for
     loop instead of each time through the loop.  This resulted
     in any property in a node after the phandle property not
     being unflattened.

Changes from v4:
   - rebase on 4.12-rc1
   - Add reason for "<NULL>" in of_attach_node()
   - Simplify and consolidate phandle detection logic in
     populate_properties().  This results in a change of behaviour,
     the value of property "ibm,phandle" will no longer override the
     value of properties "phandle" and "linux,phandle".

Changes from v3:
   - patch 1: fix incorrect variable name in __of_add_phandle_sysfs().
     Problem was reported by the kbuild test robot

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


*** BLURB HERE ***

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    | 57 +++++++++++++++++++++++++++++++++++++------------
 drivers/of/fdt.c        | 43 ++++++++++++++++++-------------------
 drivers/of/of_private.h |  2 +-
 drivers/of/overlay.c    |  8 ++++---
 drivers/of/resolver.c   | 23 +-------------------
 include/linux/of.h      |  1 +
 7 files changed, 118 insertions(+), 66 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 v6 1/4] of: remove *phandle properties from expanded device tree

[frowand.list]

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

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 and will still be displayed as if it is
a property in /proc/device_tree.

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").
- A side effect is that the value of property "ibm,phandle" will no longer
  override the value of properties "phandle" and "linux,phandle".

Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
 drivers/of/base.c       | 48 +++++++++++++++++++++++++++++++++++++++---
 drivers/of/dynamic.c    | 55 +++++++++++++++++++++++++++++++++++++------------
 drivers/of/fdt.c        | 43 +++++++++++++++++++-------------------
 drivers/of/of_private.h |  1 +
 drivers/of/overlay.c    |  4 +---
 drivers/of/resolver.c   | 23 +--------------------
 include/linux/of.h      |  1 +
 7 files changed, 112 insertions(+), 63 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 28d5f53bc631..941c9a03471d 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(&np->attr_phandle);
+	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);
 
@@ -2128,9 +2172,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..be320082178f 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,48 @@ 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;
 
+	/* use "<NULL>" to be consistent with populate_node() */
+	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 +453,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 +466,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 3080d9dd031d..f4c75a781c80 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -183,6 +183,7 @@ static void populate_properties(const void *blob,
 		const __be32 *val;
 		const char *pname;
 		u32 sz;
+		bool prop_is_phandle = false;
 
 		val = fdt_getprop_by_offset(blob, cur, &pname, &sz);
 		if (!val) {
@@ -198,35 +199,33 @@ 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 (strcmp(pname, "phandle") &&
+		    strcmp(pname, "linux,phandle") &&
+		    strcmp(pname, "ibm,phandle"))
+			pp = unflatten_dt_alloc(mem, sizeof(struct property),
+						__alignof__(struct property));
+		else
+			prop_is_phandle = true;
+
 		if (dryrun)
 			continue;
 
-		/* We accept flattened tree phandles either in
-		 * ePAPR-style "phandle" properties, or the
-		 * legacy "linux,phandle" properties.  If both
-		 * appear and have different values, things
-		 * will get weird. Don't do that.
+		/* We accept flattened tree phandles in ePAPR-style "phandle"
+		 * property, the legacy "linux,phandle" property, or the
+		 * "ibm,phandle" property used in pSeries dynamic device tree.
+		 * If more than one of them appear and have different values,
+		 * things will get weird. Don't do that.
 		 */
-		if (!strcmp(pname, "phandle") ||
-		    !strcmp(pname, "linux,phandle")) {
+		if (prop_is_phandle) {
 			if (!np->phandle)
 				np->phandle = be32_to_cpup(val);
+		} else {
+			pp->name   = (char *)pname;
+			pp->length = sz;
+			pp->value  = (__be32 *)val;
+			*pprev     = pp;
+			pprev      = &pp->next;
 		}
-
-		/* 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);
-
-		pp->name   = (char *)pname;
-		pp->length = sz;
-		pp->value  = (__be32 *)val;
-		*pprev     = pp;
-		pprev      = &pp->next;
 	}
 
 	/* 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 4ebb0149d118..1a041411b219 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 771f4844c781..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;
-
-		*(__be32 *)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 50fcdb54087f..4e483782d124 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@sony.com>

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

[Michael Ellerman]

Hi Frank,

frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org writes:
> 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 and will still be displayed as if it is
> a property in /proc/device_tree.
>
> 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").

Sorry but I don't think that can work for us.

Our DLPAR (ie. CPU/memory/device hotplug) stuff on PowerVM uses
"ibm,phandle", and it's not the same thing as "phandle" /
"linux,phandle".

I don't know the code well myself, but the spec (PAPR) says:

  Note: If the “ibm,phandle” property exists, there are two “phandle”
  namespaces which must be kept separate. One is that actually used by
  the OF client interface, the other is properties in the device tree
  making reference to device tree nodes. These requirements are written
  to maintain backward compatibility with older FW versions predating
  these requirements; if the “ibm,phandle” property is not present, the
  OS may assume that any device tree properties which refer to this node
  will have a phandle value matching that returned by client interface
  services.

I have systems here that still use "ibm,phandle". I also see at least
some of the userspace code that looks for "ibm,phandle", and nothing
else.

The note above actually implies that the current Linux code is wrong,
when it uses "ibm,phandle" as the value of np->phandle.

So sorry that's a big mess, but we can't just rip out those properties.

I think the minimal change would be to treat "ibm,phandle" like a normal
property, I think that would allow our tools to keep working?


The other thing that worries me is that by renaming (effectively)
"linux,phandle" to "phandle", we lose the ability to accurately
regenerate the device tree from /proc/device-tree. In theory it
shouldn't matter, but I worry that in practice something will break.

What if we just kept a single bit flag somewhere indicating if the name of
the phandle property we found was "phandle" or "linux,phandle", and
create the sysfs phandle using that name?

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

[Frank Rowand]

Hi Rob,

Michael has an issue that means this patch series is not OK in the
current form.  I will work on a v7 to see if I can resolve the
issue.

-Frank


On 06/20/17 21:57, Michael Ellerman wrote:
> Hi Frank,
> 
> frowand.list@gmail.com writes:
>> From: Frank Rowand <frank.rowand@sony.com>
>>
>> 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 and will still be displayed as if it is
>> a property in /proc/device_tree.
>>
>> 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").
> 
> Sorry but I don't think that can work for us.
> 
> Our DLPAR (ie. CPU/memory/device hotplug) stuff on PowerVM uses
> "ibm,phandle", and it's not the same thing as "phandle" /
> "linux,phandle".
> 
> I don't know the code well myself, but the spec (PAPR) says:
> 
>   Note: If the “ibm,phandle” property exists, there are two “phandle”
>   namespaces which must be kept separate. One is that actually used by
>   the OF client interface, the other is properties in the device tree
>   making reference to device tree nodes. These requirements are written
>   to maintain backward compatibility with older FW versions predating
>   these requirements; if the “ibm,phandle” property is not present, the
>   OS may assume that any device tree properties which refer to this node
>   will have a phandle value matching that returned by client interface
>   services.
> 
> I have systems here that still use "ibm,phandle". I also see at least
> some of the userspace code that looks for "ibm,phandle", and nothing
> else.
> 
> The note above actually implies that the current Linux code is wrong,
> when it uses "ibm,phandle" as the value of np->phandle.
> 
> So sorry that's a big mess, but we can't just rip out those properties.
> 
> I think the minimal change would be to treat "ibm,phandle" like a normal
> property, I think that would allow our tools to keep working?
> 
> 
> The other thing that worries me is that by renaming (effectively)
> "linux,phandle" to "phandle", we lose the ability to accurately
> regenerate the device tree from /proc/device-tree. In theory it
> shouldn't matter, but I worry that in practice something will break.
> 
> What if we just kept a single bit flag somewhere indicating if the name of
> the phandle property we found was "phandle" or "linux,phandle", and
> create the sysfs phandle using that name?
> 
> cheers
> 

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

[Frank Rowand]

On 06/20/17 23:18, Frank Rowand wrote:
> Hi Rob,
> 
> Michael has an issue that means this patch series is not OK in the
> current form.  I will work on a v7 to see if I can resolve the
> issue.
> 
> -Frank

< snip >

Hi Rob,

The issue is in patch 1.  Patches 2 - 4 are small independent patches
that are not dependent on patch 1, so I just sent them as individual
patches.  Version 7 of this series will be just patch 1.

-Frank

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

[Frank Rowand]

adding Ben and Paul.

Hi Michael,

On 06/20/17 21:57, Michael Ellerman wrote:
> Hi Frank,
> 
> frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org writes:
>> 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 and will still be displayed as if it is
>> a property in /proc/device_tree.
>>
>> 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").
> 
> Sorry but I don't think that can work for us.
> 
> Our DLPAR (ie. CPU/memory/device hotplug) stuff on PowerVM uses
> "ibm,phandle", and it's not the same thing as "phandle" /
> "linux,phandle".
> 
> I don't know the code well myself, but the spec (PAPR) says:

This is the LoPAPR, section 2.1.4, R1-2.1.4-3

   https://members.openpowerfoundation.org/document/dl/469


>   Note: If the “ibm,phandle” property exists, there are two “phandle”
>   namespaces which must be kept separate. One is that actually used by
>   the OF client interface, the other is properties in the device tree
>   making reference to device tree nodes. These requirements are written
>   to maintain backward compatibility with older FW versions predating
>   these requirements; if the “ibm,phandle” property is not present, the
>   OS may assume that any device tree properties which refer to this node
>   will have a phandle value matching that returned by client interface
>   services.
> 
> I have systems here that still use "ibm,phandle". I also see at least
> some of the userspace code that looks for "ibm,phandle", and nothing
> else.
> 
> The note above actually implies that the current Linux code is wrong,
> when it uses "ibm,phandle" as the value of np->phandle.

My interpretation of the LoPAPR R1-2.1.4-1 and R1-2.1.4-2 is that the
ibm-phandle property it the node's phandle value that other nodes may
refer to.  Thus this is the value that should be placed in np->phandle,
which is the value that will be used to find a node based on its
phandle value.  Which is the way the drivers/of/fdt.c currently works:

                /* We accept flattened tree phandles either in
                 * ePAPR-style "phandle" properties, or the
                 * legacy "linux,phandle" properties.  If both
                 * appear and have different values, things
                 * will get weird. Don't do that.
                 */
                if (!strcmp(pname, "phandle") ||
                    !strcmp(pname, "linux,phandle")) {
                        if (!np->phandle)
                                np->phandle = be32_to_cpup(val);
                }

                /* 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);

My interpratation of R1-2.1.4-1 through R1-2.1.4-3 is that the
"ibm,phandle" property is relevant to the contents of the Linux
kernel device tree and that the "phandle returned by a client
interface service" is not relevant to the Linux kernel device
tree.  I would not expect the powerpc code to expose the
device tree code to a "phandle returned by a client
interface service".  Is that correct?

The current code which chooses which value potentially ends up in
np->phandle seems to involve a little bit of cargo cult coding.
This code has been adapted and combined from several locations,
see commits:

   dfbd4c6eff35f1b1065cca046003cc9d7ff27222

then earlier:

   04b954a673dd02f585a2769c4945a43880faa989
   6016a363f6b56b46b24655bcfc0499b715851cf3
   e6a6928c3ea1d0195ed75a091e345696b916c09b
   bbd33931a08362f78266a4016211a35947b91041

I would like for the code that sets the value of np->phandle
to simply say:

   if name is "phandle", "linux,phandle", or "ibm,phandle" then
      np->phandle = the value

Does anyone know if the additional logic in the current code is
needed?

Beyond the issue of the value of np->phandle, there is the issue of
exposing the unflattened properties to user space in
/proc/device-tree.  Michael reports that there are user space
programs that access "ibm,phandle" specifically, so that property
name must continue to be exposed under /proc/device-tree.  Rob
was hoping that we could collapse the properties "linux,phandle"
and "phandle" to be the single property "phandle" under
/proc/device-tree.  Are there any problems with this plan?


> So sorry that's a big mess, but we can't just rip out those properties.
> 
> I think the minimal change would be to treat "ibm,phandle" like a normal
> property, I think that would allow our tools to keep working?

Rob was hoping that we could collapse the properties "linux,phandle"
and "phandle" to be the single property "phandle" under
/proc/device-tree.  The next step after my current patch would be
to modify the Linux kernel build system to use the dtc command line
option to not generate both the "phandle" and the "linux,phandle"
properties, but instead just generate the "phandle" property.
This would result in slightly smaller FDT files.

Michael is a little worried about collapsing "phandle" and
"linux,phandle" to be just "phandle" (see below).
Does anyone else have an opinion on this?


> The other thing that worries me is that by renaming (effectively)
> "linux,phandle" to "phandle", we lose the ability to accurately
> regenerate the device tree from /proc/device-tree. In theory it
> shouldn't matter, but I worry that in practice something will break.
> 
> What if we just kept a single bit flag somewhere indicating if the name of
> the phandle property we found was "phandle" or "linux,phandle", and
> create the sysfs phandle using that name?

Another nit with my current patch is that if a node has an "ibm,phandle"
but no "linux,phandle" and no "phandle" then a "phandle" property is
created for the node under /proc/device-tree.  Is this a potential
issue?  My feeling is that it should not be harmful.

-Frank

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

[Rob Herring]

On Wed, Jun 21, 2017 at 02:57:35PM +1000, Michael Ellerman wrote:
> Hi Frank,
> 
> frowand.list@gmail.com writes:
> > From: Frank Rowand <frank.rowand@sony.com>
> >
> > 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 and will still be displayed as if it is
> > a property in /proc/device_tree.
> >
> > 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").
> 
> Sorry but I don't think that can work for us.
> 
> Our DLPAR (ie. CPU/memory/device hotplug) stuff on PowerVM uses
> "ibm,phandle", and it's not the same thing as "phandle" /
> "linux,phandle".
> 
> I don't know the code well myself, but the spec (PAPR) says:
> 
>   Note: If the “ibm,phandle” property exists, there are two “phandle”
>   namespaces which must be kept separate. One is that actually used by
>   the OF client interface, the other is properties in the device tree
>   making reference to device tree nodes. These requirements are written
>   to maintain backward compatibility with older FW versions predating
>   these requirements; if the “ibm,phandle” property is not present, the
>   OS may assume that any device tree properties which refer to this node
>   will have a phandle value matching that returned by client interface
>   services.
> 
> I have systems here that still use "ibm,phandle". I also see at least
> some of the userspace code that looks for "ibm,phandle", and nothing
> else.
> 
> The note above actually implies that the current Linux code is wrong,
> when it uses "ibm,phandle" as the value of np->phandle.
> 
> So sorry that's a big mess, but we can't just rip out those properties.
> 
> I think the minimal change would be to treat "ibm,phandle" like a normal
> property, I think that would allow our tools to keep working?
> 
> 
> The other thing that worries me is that by renaming (effectively)
> "linux,phandle" to "phandle", we lose the ability to accurately
> regenerate the device tree from /proc/device-tree. In theory it
> shouldn't matter, but I worry that in practice something will break.

The only scenario I can come up with is booting a kernel with this 
change and kexec'ing to a kernel older than 2010 (when ePAPR phandle was 
added). Some how I doubt that would work without this change.

> What if we just kept a single bit flag somewhere indicating if the name of
> the phandle property we found was "phandle" or "linux,phandle", and
> create the sysfs phandle using that name?

I'd like to move towards dropping 'linux,phandle' including changing 
dtc to stop generating both properties by default. Perhaps we should 
just be more explicit that we are doing that. Stop exposing it first and 
then change how phandles are stored/managed a cycle later. Then we can 
easily revert it if needed.

Rob

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

[Frank Rowand]

Hi Michael,

In Rob's reply to you email, he said:

   I'd like to move towards dropping 'linux,phandle' including changing 
   dtc to stop generating both properties by default. Perhaps we should 
   just be more explicit that we are doing that. Stop exposing it first and 
   then change how phandles are stored/managed a cycle later. Then we can 
   easily revert it if needed.

My current plan is to follow that suggestion.  So fixing my patches that
started this thread will wait for a later cycle.  But I would like to
try to address the issues with property "ibm,phandle" that you raised
below _before_ fixing my patches.  Those fixes should help me understand
"ibm,phandle" better, and hopefully when I update my patches I will not
cause further "ibm,phandle" issues.

More comments from me, embedded below.


On 06/20/17 21:57, Michael Ellerman wrote:
> Hi Frank,
> 
> frowand.list@gmail.com writes:
>> From: Frank Rowand <frank.rowand@sony.com>
>>
>> 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 and will still be displayed as if it is
>> a property in /proc/device_tree.
>>
>> 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").
> 
> Sorry but I don't think that can work for us.
> 
> Our DLPAR (ie. CPU/memory/device hotplug) stuff on PowerVM uses
> "ibm,phandle", and it's not the same thing as "phandle" /
> "linux,phandle".
> 
> I don't know the code well myself, but the spec (PAPR) says:
> 
>   Note: If the “ibm,phandle” property exists, there are two “phandle”
>   namespaces which must be kept separate. One is that actually used by
>   the OF client interface, the other is properties in the device tree
>   making reference to device tree nodes. These requirements are written
>   to maintain backward compatibility with older FW versions predating
>   these requirements; if the “ibm,phandle” property is not present, the
>   OS may assume that any device tree properties which refer to this node
>   will have a phandle value matching that returned by client interface
>   services.
> 
> I have systems here that still use "ibm,phandle". I also see at least
> some of the userspace code that looks for "ibm,phandle", and nothing
> else.
> 
> The note above actually implies that the current Linux code is wrong,
> when it uses "ibm,phandle" as the value of np->phandle.

This is where I get lost, and need your help and expertise.

If I do the naive search:

   git grep '"ibm,phandle"'


I find it used in three files.

(1) drivers/of/fdt.c, which is part of what my patches modified.

(2) drivers/misc/cxl/of.c reads (and optionally reports the value of)
"ibm,phandle", but does not use the value for anything else.

(3) drivers/of/dynamic.c gets and uses the value of "ibm,phandle"
only if properties "phandle" and "linux,phandle" do not exist.
If either of those two properties exist, their value is used
preferentially to the value of "ibm,phandle".  So in this case
it seems that folding the value of "ibm,phandle" into "phandle"
would work properly.

Is the "ibm,phandle" property used elsewhere in a way that my
naive search missed it?  Should it be used in a place that is
not using it?


> So sorry that's a big mess, but we can't just rip out those properties.

You provide some useful references and information above, but I do not
understand what I am reading.  :-)


> I think the minimal change would be to treat "ibm,phandle" like a normal
> property, I think that would allow our tools to keep working?

This sounds reasonable.  Since userspace is using the "ibm,phandle"
property we need to continue to expose that property to userspace.

If userspace needs to differentiate between "phandle" and "ibm,phandle"
then we will need to keep "ibm,phandle" for as long as userspace needs
it.  Otherwise, there is a possibility that we could update userspace,
wait a long time, then remove "ibm,phandle".


> The other thing that worries me is that by renaming (effectively)
> "linux,phandle" to "phandle", we lose the ability to accurately
> regenerate the device tree from /proc/device-tree. In theory it
> shouldn't matter, but I worry that in practice something will break.

Hopefully we will flush any such problem out with Rob's suggestion of doing
the easily reversible removal of generating the "linux,phandle" property
when we compile device trees.  Then wait a while before removing it from
the kernel.


> What if we just kept a single bit flag somewhere indicating if the name of
> the phandle property we found was "phandle" or "linux,phandle", and
> create the sysfs phandle using that name?
> 
> cheers
> 

[PATCH v6 2/4] of: make __of_attach_node() static

[frowand.list]

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

__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@sony.com>
---
 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 be320082178f..3367ed2da9ad 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 1a041411b219..73da291a51cd 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -91,7 +91,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@sony.com>

Re: [PATCH v6 2/4] of: make __of_attach_node() static

[Stephen Boyd]

On 06/20, frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> 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>
> ---

Reviewed-by: Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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 v6 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 941c9a03471d..a4e2159c8671 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

[PATCH v6 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>