of/promtree: allow DT device matching by fixing 'name' brokenness

From: Andres Salomon <[email protected]>

Commit e2f2a93b, "of/promtree: add package-to-path support to pdt" changed dp->name from using the 'name' property to using package-to-path. This fixed /proc/device-tree creation by eliminating conflicts between names (the 'name' property provides names like 'battery', whereas package-to-path provides names like '/foo/bar/battery@0', which we stripped to 'battery@0'). However, it also breaks of_device_id table matching.

The fix that we _really_ wanted was to keep dp->name based upon the name property ('battery'), but based dp->full_name upon package-to-path ('battery@0'). This patch does just that.

This changes all users (except SPARC) of promtree to use the full result from package-to-path for full_name, rather than stripping the directory out. In practice, the strings end up being exactly the same; this change saves time, code, and memory.

SPARC continues to use the existing build_path_component() code.

v2: combine two patches and revert of_pdt_node_name to original version v3: use dp->phandle instead of passing around node
v4: warn/bail out for non-sparc archs if pkg2path is not set
v5: split of_pdt_build_full_name into sparc & non-sparc versions
BUG() when pkg2path doesn't work.

Signed-off-by Andres Salomon <[email protected]>

Signed-off-by Grant Likely <[email protected]>

---

Hi Andres,

This is what I think the patch should really look like. The of_pdt_node_name() stuff really doesn't make sense to keep around because what we *really* care about is the full name, not the path component name. Therefore I reorganized it so SPARC continues to use it's build_path_component() to build up the full path; but everyone else *must* provide a working mechanism to obtain the full path; be it an actual call to OFW pkg2path, or something else.

Please take a look at let me know what you think.

g.

 drivers/of/pdt.c |  110

++++++++++++++++++++---------------------------------- 1 files changed, 40 insertions(+), 70 deletions(-)

drivers/of/pdt.c +10 -2

index 28295d0..ab54819 100644
@@ -36,19 +36,53 @@ unsigned int of_pdt_unique_id __initdata;
 	(p)->unique_id = of_pdt_unique_id++; \
 } while (0)
 
-static inline const char *of_pdt_node_name(struct device_node *dp)
+static char * __init of_pdt_build_full_name(struct device_node *dp)
 {
-	return dp->path_component_name;
+	int len, ourlen, plen;
+	char *n;
+
+	dp->path_component_name = build_path_component(dp);
+
+	plen = strlen(dp->parent->full_name);
+	ourlen = strlen(dp->path_component_name);
+	len = ourlen + plen + 2;
+

drivers/of/pdt.c +1 -1

-#else
+#else /* CONFIG_SPARC */
 
 static inline void of_pdt_incr_unique_id(void *p) { }
 static inline void irq_trans_init(struct device_node *dp) { }

drivers/of/pdt.c +6 -28

+
+	if (of_pdt_prom_ops->pkg2path(dp->phandle, buf, len, &len)) {
+		pr_err("%s: package-to-path failed\n", __func__);
+		BUG();
+	}
+	return buf;
 }
 
 #endif /* !CONFIG_SPARC */

@@ -132,47 +166,6 @@ static char * __init
 of_pdt_get_one_property(phandle node, const char *name) return buf;
 }
 
-static char * __init of_pdt_try_pkg2path(phandle node)
-{
-	char *res, *buf = NULL;
-	int len;
-
-	if (!of_pdt_prom_ops->pkg2path)
-		return NULL;
-
-	if (of_pdt_prom_ops->pkg2path(node, buf, 0, &len))
-		return NULL;
-	buf = prom_early_alloc(len + 1);
-	if (of_pdt_prom_ops->pkg2path(node, buf, len, &len)) {
-		pr_err("%s: package-to-path failed\n", __func__);
-		return NULL;
-	}
-
-	res = strrchr(buf, '/');
-	if (!res) {
-		pr_err("%s: couldn't find / in %s\n", __func__, buf);
-		return NULL;
-	}
-	return res+1;
-}
-
-/*
- * When fetching the node's name, first try using package-to-path; if
- * that fails (either because the arch hasn't supplied a PROM
 callback,
- * or some other random failure), fall back to just looking at the

node's
- * 'name' property.
- */

drivers/of/pdt.c +0 -11

-static char * __init of_pdt_build_name(phandle node)
-{
-	char *buf;
-
-	buf = of_pdt_try_pkg2path(node);
-	if (!buf)
-		buf = of_pdt_get_one_property(node, "name");
-
-	return buf;
-}
-
 static struct device_node * __init of_pdt_create_node(phandle node,
 						    struct

device_node *parent) {

drivers/of/pdt.c +1 -21

@@ -187,7 +180,7 @@ static struct device_node * __init
 of_pdt_create_node(phandle node, 
 	kref_init(&dp->kref);
 
-	dp->name = of_pdt_build_name(node);
+	dp->name = of_pdt_get_one_property(node, "name");
 	dp->type = of_pdt_get_one_property(node, "device_type");
 	dp->phandle = node;
 

@@ -198,26 +191,6 @@ static struct device_node * __init
 of_pdt_create_node(phandle node, return dp;
 }
 
-static char * __init of_pdt_build_full_name(struct device_node *dp)
-{
-	int len, ourlen, plen;
-	char *n;
-
-	plen = strlen(dp->parent->full_name);
-	ourlen = strlen(of_pdt_node_name(dp));
-	len = ourlen + plen + 2;
-
-	n = prom_early_alloc(len);
-	strcpy(n, dp->parent->full_name);
-	if (!of_node_is_root(dp->parent)) {
-		strcpy(n + plen, "/");
-		plen++;
-	}
-	strcpy(n + plen, of_pdt_node_name(dp));
-
-	return n;
-}
-
 static struct device_node * __init of_pdt_build_tree(struct
 device_node *parent, phandle node,
 						   struct

device_node ***nextp) @@ -240,9 +213,6 @@ static struct device_node * __init of_pdt_build_tree(struct device_node *parent, *(*nextp) = dp;

drivers/of/pdt.c +0 -0

 		dp->full_name = of_pdt_build_full_name(dp);
 
 		dp->child = of_pdt_build_tree(dp,

Quoted 70 lines · On Thu, 24 Feb 2011 10:04:41 -0700 Grant Likely <[email protected]>…

From: Andres Salomon <[email protected]>

Commit e2f2a93b, "of/promtree: add package-to-path support to pdt" changed dp->name from using the 'name' property to using package-to-path. This fixed /proc/device-tree creation by eliminating conflicts between names (the 'name' property provides names like 'battery', whereas package-to-path provides names like '/foo/bar/battery@0', which we stripped to 'battery@0'). However, it also breaks of_device_id table matching.

The fix that we _really_ wanted was to keep dp->name based upon the name property ('battery'), but based dp->full_name upon package-to-path ('battery@0'). This patch does just that.

This changes all users (except SPARC) of promtree to use the full result from package-to-path for full_name, rather than stripping the directory out. In practice, the strings end up being exactly the same; this change saves time, code, and memory.

SPARC continues to use the existing build_path_component() code.

v2: combine two patches and revert of_pdt_node_name to original version v3: use dp->phandle instead of passing around node
v4: warn/bail out for non-sparc archs if pkg2path is not set
v5: split of_pdt_build_full_name into sparc & non-sparc versions
BUG() when pkg2path doesn't work.

Signed-off-by Andres Salomon <[email protected]>

Signed-off-by Grant Likely <[email protected]>

---

Hi Andres,

This is what I think the patch should really look like. The of_pdt_node_name() stuff really doesn't make sense to keep around because what we *really* care about is the full name, not the path component name. Therefore I reorganized it so SPARC continues to use it's build_path_component() to build up the full path; but everyone else *must* provide a working mechanism to obtain the full path; be it an actual call to OFW pkg2path, or something else.

Please take a look at let me know what you think.

g.

 drivers/of/pdt.c |  110

++++++++++++++++++++---------------------------------- 1 files changed, 40 insertions(+), 70 deletions(-)

drivers/of/pdt.c +10 -2

index 28295d0..ab54819 100644
@@ -36,19 +36,53 @@ unsigned int of_pdt_unique_id __initdata;
 	(p)->unique_id = of_pdt_unique_id++; \
 } while (0)
 
-static inline const char *of_pdt_node_name(struct device_node *dp)
+static char * __init of_pdt_build_full_name(struct device_node *dp)
 {
-	return dp->path_component_name;
+	int len, ourlen, plen;
+	char *n;
+
+	dp->path_component_name = build_path_component(dp);
+
+	plen = strlen(dp->parent->full_name);
+	ourlen = strlen(dp->path_component_name);
+	len = ourlen + plen + 2;
+

drivers/of/pdt.c unsigned int of_pdt_unique_id __initdata; · line 59

Quoted 10 lines

drivers/of/pdt.c +9 -0

+	n = prom_early_alloc(len);
+	strcpy(n, dp->parent->full_name);
+	if (!of_node_is_root(dp->parent)) {
+		strcpy(n + plen, "/");
+		plen++;
+	}
+	strcpy(n + plen, dp->path_component_name);
+
+	return n;
 }

Sure, this part is fine. The reason why this was originally in 2 separate patches (and what I'd been attempting to do all along) was to keep the bugfix portion of it to a minimal size, and to have a separate generic cleanup patch that changed around more stuff.

Quoted 7 lines

drivers/of/pdt.c +1 -1

-#else
+#else /* CONFIG_SPARC */
 
 static inline void of_pdt_incr_unique_id(void *p) { }
 static inline void irq_trans_init(struct device_node *dp) { }
 

drivers/of/pdt.c unsigned int of_pdt_unique_id __initdata; · line 72

Quoted 9 lines

drivers/of/pdt.c +6 -2

-static inline const char *of_pdt_node_name(struct device_node *dp)
+static char * __init of_pdt_build_full_name(struct device_node *dp)
 {
-	return dp->name;
+	char *buf;
+	int len;
+
+	if (!of_pdt_prom_ops->pkg2path)
+		BUG();

I'd rather see WARN_ON() here.

Our options if any of
this fail are to BUG (causing bootup to fail, most likely),

WARN and
return NULL (causing the DT creation to not happen, which may affect things differently depending upon what the DT is being used for), or WARN and return dp->name (which would cause the DT to be created, albeit not correctly. The more I think about it, the more I like that last option. If the DT is being used for booting, that option would provide the best attempt at actually booting up.

drivers/of/pdt.c unsigned int of_pdt_unique_id __initdata; · line 75

Quoted 6 lines

drivers/of/pdt.c +3 -0

+
+	if (of_pdt_prom_ops->pkg2path(dp->phandle, buf, 0, &len))
+		BUG();

Well, no, if pkg2path fails, we shouldn't really BUG(). There may be some reason why the call fails for only a particular node (for example, perhaps an invalid phandle gets passed, or there's a bug in the firmware for that particular node). I'd rather disable the DT or just disable the node and have the machine at least bring up a framebuffer console with kernel logs showing some kind of warning/error message.

drivers/of/pdt.c unsigned int of_pdt_unique_id __initdata; · line 79

Quoted 7 lines

drivers/of/pdt.c +4 -0

+
+	buf = prom_early_alloc(len + 1);
+	if (!buf)
+		BUG();

Ditto.

Quoted 4 lines · Andres Salomon

Quoted 1 line · On Thu, 24 Feb 2011 10:04:41 -0700 Grant Likely <[email protected]>…

From: Andres Salomon <[email protected]>

Quoted 13 lines

drivers/of/pdt.c unsigned int of_pdt_unique_id __initdata; · line 72

Quoted 11 lines

static inline void irq_trans_init(struct device_node *dp) { }

-static inline const char *of_pdt_node_name(struct device_node *dp) +static char * __init of_pdt_build_full_name(struct device_node *dp) {

drivers/of/pdt.c +5 -1

-	return dp->name;
+	char *buf;
+	int len;
+
+	if (!of_pdt_prom_ops->pkg2path)
+		BUG();

I'd rather see WARN_ON() here.

This one needs to be BUG. It is a *requirement* for the platform to supply a pkg2path hook. Although, I think I'll remove this line anyway; if a new platform doesn't supply the hook, then it isn't even going to boot, so no point bothering to execute this test for each an every node in the tree.

Quoted 2 lines

Our options if any of
this fail are to BUG (causing bootup to fail, most likely),

Absolutely boot will fail.

Quoted 8 lines

WARN and
return NULL (causing the DT creation to not happen, which may affect things differently depending upon what the DT is being used for), or WARN and return dp->name (which would cause the DT to be created, albeit not correctly. The more I think about it, the more I like that last option. If the DT is being used for booting, that option would provide the best attempt at actually booting up.

I'm adamant on this point. pkg2path *must* return a value path string. If it does not, then the node must not get registered. I've taken a look at the sparc code, and it really looks like the assignment of full_name can be moved into of_pdt_create_node. None of the SPARC *_path_component functions seem to depend on the sibling or allnodes pointers being populated.

Quoted 3 lines

Quoted 3 lines

drivers/of/pdt.c unsigned int of_pdt_unique_id __initdata; · line 75

Quoted 3 lines

drivers/of/pdt.c +3 -0

+
+	if (of_pdt_prom_ops->pkg2path(dp->phandle, buf, 0, &len))
+		BUG();

I just noticed another potential bug here. buf is unassigned and random here. Even though length is passed as '0', a random value is effectively getting passed as the buf pointer. It should be passed as NULL to avoid any accidents. Changed in my version of the patch.

Quoted 9 lines

Well, no, if pkg2path fails, we shouldn't really BUG(). There may be some reason why the call fails for only a particular node (for example, perhaps an invalid phandle gets passed, or there's a bug in the firmware for that particular node). I'd rather disable the DT or just disable the node and have the machine at least bring up a framebuffer console with kernel logs showing some kind of warning/error message.

If firmware is buggy, then pkg2path must deal with it. It is not okay for it to return NULL. (I know that pkg2path is an OFW command, but in this context it really means the linux wrapper to pkg2path which has the semantics, "give me the unique, full and accurate path for this node"). If OFW pkg2path doesn't work, then the platform code must work around it. I'm pushing back on this because I do not want to see platform workarounds in the common code.

(I'd almost rather see all promtree users adopt the sparc approach of building up from known data instead of calling into higher level OFW functions, but that's an orthogonal debate).

*however* if you really want to implement a dp->name fallback, then I would be okay with something like this: At least it ensures every full_path string is unique in the system.

static int broken_unique_id = 0;
sprintf(buf, "%s/%s-broken%i", parent->full_name, dp->name, broken_unique_id++); pr_err("pkg2path busted; ugly name workaround applied\n");

For now I'm inclined to keep the BUG(), and I'll let you handle any workarounds in the OLPC code if needed.

Quoted 9 lines

drivers/of/pdt.c unsigned int of_pdt_unique_id __initdata; · line 79

Quoted 7 lines

drivers/of/pdt.c +4 -0

+
+	buf = prom_early_alloc(len + 1);
+	if (!buf)
+		BUG();

Ditto.

If we cannot allocate ram at this point, we've got much bigger problems. BUG is appropriate here.... actually, if prom_early_alloc() cannot provide memory, it should fail hard like sparc prom_64 does. Care to craft a separate patch to make that change for OLPC? (for 2.6.39; no rush)

I've pushed the latest version out to my devicetree/experimental branch on git://git.secretlab.ca/git/linux-2.6. Give it a spin and make sure everything is still working for you.

g.

Quoted 7 lines · On Thu, 24 Feb 2011 12:42:34 -0700 Grant Likely…

If firmware is buggy, then pkg2path must deal with it.  It is not okay for it to return NULL.  (I know that pkg2path is an OFW command, but in this context it really means the linux wrapper to pkg2path which has the semantics, "give me the unique, full and accurate path for this node").  If OFW pkg2path doesn't work, then the platform code must work around it.  I'm pushing back on this because I do not want to see platform workarounds in the common code.

I'm fine with that, I just don't want to see BUG() happening that early.  I think a workaround should be handled in common code.  I agree that heroic workarounds for firmware bugs should be handled in arch-specific pkg2path hooks, but a simple workaround in common code is better than just crashing early in boot (imo).

Quoted 11 lines · Andres Salomon

Quoted 8 lines · On Thu, 24 Feb 2011 12:42:34 -0700 Grant Likely <[email protected]>…

If firmware is buggy, then pkg2path must deal with it.  It is not okay for it to return NULL.  (I know that pkg2path is an OFW command, but in this context it really means the linux wrapper to pkg2path which has the semantics, "give me the unique, full and accurate path for this node").  If OFW pkg2path doesn't work, then the platform code must work around it.  I'm pushing back on this because I do not want to see platform workarounds in the common code.

Quoted 5 lines

I'm fine with that, I just don't want to see BUG() happening that early.  I think a workaround should be handled in common code.  I agree that heroic workarounds for firmware bugs should be handled in arch-specific pkg2path hooks, but a simple workaround in common code is better than just crashing early in boot (imo).

Alright, you've swayed me a bit. I've made this change and pushed it out to devicetree/experimental. I've also picked up your other patch.
Let me know if it works for you.

Quoted 5 lines · Andres Salomon

Quoted 2 lines · On Thu, 24 Feb 2011 10:04:41 -0700 Grant Likely <[email protected]>…

From: Andres Salomon <[email protected]>

I've pushed the latest version out to my devicetree/experimental branch on git://git.secretlab.ca/git/linux-2.6. Give it a spin and make sure everything is still working for you.

g.