* [PATCH v5] of/promtree: allow DT device matching by fixing 'name' brokenness
@ 2011-02-24 17:04 ` Grant Likely
0 siblings, 0 replies; 18+ messages in thread
From: Grant Likely @ 2011-02-24 17:04 UTC (permalink / raw)
To: dsd, Andres Salomon; +Cc: sparclinux, devicetree-discuss, linux-kernel, davem
From: Andres Salomon <dilinger@queued.net>
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 <dilinger@queued.net>
Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---
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(-)
diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
index 28295d0..ab54819 100644
--- a/drivers/of/pdt.c
+++ b/drivers/of/pdt.c
@@ -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;
+
+ 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;
}
-#else
+#else /* CONFIG_SPARC */
static inline void of_pdt_incr_unique_id(void *p) { }
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)
{
- return dp->name;
+ char *buf;
+ int len;
+
+ if (!of_pdt_prom_ops->pkg2path)
+ BUG();
+
+ if (of_pdt_prom_ops->pkg2path(dp->phandle, buf, 0, &len))
+ BUG();
+
+ buf = prom_early_alloc(len + 1);
+ if (!buf)
+ BUG();
+
+ 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.
- */
-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)
{
@@ -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;
*nextp = &dp->allnext;
-#if defined(CONFIG_SPARC)
- dp->path_component_name = build_path_component(dp);
-#endif
dp->full_name = of_pdt_build_full_name(dp);
dp->child = of_pdt_build_tree(dp,
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v5] of/promtree: allow DT device matching by fixing
2011-02-24 17:04 ` [PATCH v5] of/promtree: allow DT device matching by fixing 'name' brokenness Grant Likely
@ 2011-02-24 17:33 ` Andres Salomon
-1 siblings, 0 replies; 18+ messages in thread
From: Andres Salomon @ 2011-02-24 17:33 UTC (permalink / raw)
To: Grant Likely; +Cc: dsd, sparclinux, devicetree-discuss, linux-kernel, davem
On Thu, 24 Feb 2011 10:04:41 -0700
Grant Likely <grant.likely@secretlab.ca> wrote:
> From: Andres Salomon <dilinger@queued.net>
>
> 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 <dilinger@queued.net>
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> ---
>
> 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(-)
>
> diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
> index 28295d0..ab54819 100644
> --- a/drivers/of/pdt.c
> +++ b/drivers/of/pdt.c
> @@ -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;
> +
> + 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.
>
> -#else
> +#else /* CONFIG_SPARC */
>
> static inline void of_pdt_incr_unique_id(void *p) { }
> 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)
> {
> - 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.
> +
> + 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.
> +
> + buf = prom_early_alloc(len + 1);
> + if (!buf)
> + BUG();
Ditto.
> +
> + 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.
> - */
> -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) {
> @@ -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;
> *nextp = &dp->allnext;
>
> -#if defined(CONFIG_SPARC)
> - dp->path_component_name = build_path_component(dp);
> -#endif
I do like moving this out of here.
> dp->full_name = of_pdt_build_full_name(dp);
>
> dp->child = of_pdt_build_tree(dp,
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v5] of/promtree: allow DT device matching by fixing 'name' brokenness
@ 2011-02-24 17:33 ` Andres Salomon
0 siblings, 0 replies; 18+ messages in thread
From: Andres Salomon @ 2011-02-24 17:33 UTC (permalink / raw)
To: Grant Likely; +Cc: dsd, sparclinux, devicetree-discuss, linux-kernel, davem
On Thu, 24 Feb 2011 10:04:41 -0700
Grant Likely <grant.likely@secretlab.ca> wrote:
> From: Andres Salomon <dilinger@queued.net>
>
> 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 <dilinger@queued.net>
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> ---
>
> 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(-)
>
> diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
> index 28295d0..ab54819 100644
> --- a/drivers/of/pdt.c
> +++ b/drivers/of/pdt.c
> @@ -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;
> +
> + 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.
>
> -#else
> +#else /* CONFIG_SPARC */
>
> static inline void of_pdt_incr_unique_id(void *p) { }
> 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)
> {
> - 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.
> +
> + 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.
> +
> + buf = prom_early_alloc(len + 1);
> + if (!buf)
> + BUG();
Ditto.
> +
> + 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.
> - */
> -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) {
> @@ -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;
> *nextp = &dp->allnext;
>
> -#if defined(CONFIG_SPARC)
> - dp->path_component_name = build_path_component(dp);
> -#endif
I do like moving this out of here.
> dp->full_name = of_pdt_build_full_name(dp);
>
> dp->child = of_pdt_build_tree(dp,
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v5] of/promtree: allow DT device matching by fixing
2011-02-24 17:33 ` [PATCH v5] of/promtree: allow DT device matching by fixing 'name' brokenness Andres Salomon
@ 2011-02-24 19:42 ` Grant Likely
-1 siblings, 0 replies; 18+ messages in thread
From: Grant Likely @ 2011-02-24 19:42 UTC (permalink / raw)
To: Andres Salomon; +Cc: dsd, sparclinux, devicetree-discuss, linux-kernel, davem
On Thu, Feb 24, 2011 at 09:33:41AM -0800, Andres Salomon wrote:
> On Thu, 24 Feb 2011 10:04:41 -0700
> Grant Likely <grant.likely@secretlab.ca> wrote:
>
> > From: Andres Salomon <dilinger@queued.net>
> >
> > 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 <dilinger@queued.net>
> > Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> > ---
> >
> > 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(-)
> >
> > diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
> > index 28295d0..ab54819 100644
> > --- a/drivers/of/pdt.c
> > +++ b/drivers/of/pdt.c
> > @@ -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;
> > +
> > + 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.
>
>
> >
> > -#else
> > +#else /* CONFIG_SPARC */
> >
> > static inline void of_pdt_incr_unique_id(void *p) { }
> > 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)
> > {
> > - 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.
> Our options if any of
> this fail are to BUG (causing bootup to fail, most likely),
Absolutely boot will fail.
> 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.
>
>
>
> > +
> > + 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.
>
> 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.
>
>
>
> > +
> > + 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.
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v5] of/promtree: allow DT device matching by fixing 'name' brokenness
@ 2011-02-24 19:42 ` Grant Likely
0 siblings, 0 replies; 18+ messages in thread
From: Grant Likely @ 2011-02-24 19:42 UTC (permalink / raw)
To: Andres Salomon; +Cc: dsd, sparclinux, devicetree-discuss, linux-kernel, davem
On Thu, Feb 24, 2011 at 09:33:41AM -0800, Andres Salomon wrote:
> On Thu, 24 Feb 2011 10:04:41 -0700
> Grant Likely <grant.likely@secretlab.ca> wrote:
>
> > From: Andres Salomon <dilinger@queued.net>
> >
> > 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 <dilinger@queued.net>
> > Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> > ---
> >
> > 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(-)
> >
> > diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
> > index 28295d0..ab54819 100644
> > --- a/drivers/of/pdt.c
> > +++ b/drivers/of/pdt.c
> > @@ -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;
> > +
> > + 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.
>
>
> >
> > -#else
> > +#else /* CONFIG_SPARC */
> >
> > static inline void of_pdt_incr_unique_id(void *p) { }
> > 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)
> > {
> > - 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.
> Our options if any of
> this fail are to BUG (causing bootup to fail, most likely),
Absolutely boot will fail.
> 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.
>
>
>
> > +
> > + 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.
>
> 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.
>
>
>
> > +
> > + 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.
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v5] of/promtree: allow DT device matching by fixing
2011-02-24 19:42 ` [PATCH v5] of/promtree: allow DT device matching by fixing 'name' brokenness Grant Likely
@ 2011-02-24 20:01 ` Andres Salomon
-1 siblings, 0 replies; 18+ messages in thread
From: Andres Salomon @ 2011-02-24 20:01 UTC (permalink / raw)
To: Grant Likely; +Cc: dsd, sparclinux, devicetree-discuss, linux-kernel, davem
On Thu, 24 Feb 2011 12:42:34 -0700
Grant Likely <grant.likely@secretlab.ca> wrote:
> On Thu, Feb 24, 2011 at 09:33:41AM -0800, Andres Salomon wrote:
> > On Thu, 24 Feb 2011 10:04:41 -0700
> > Grant Likely <grant.likely@secretlab.ca> wrote:
> >
> > > From: Andres Salomon <dilinger@queued.net>
[...]
> > > 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) {
> > > - 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.
>
> > Our options if any of
> > this fail are to BUG (causing bootup to fail, most likely),
>
> Absolutely boot will fail.
Unnecessarily. OLPC will still boot if the DT creation fails. Even if
things are broken, it'll at least allow someone to report some errors
on the screen. On the other hand, BUGing this early in boot means that
all a user will see is a black screen (or the OFW "loading kernel"
message). The framebuffer won't have been initialized, so.. yeah.
If it's a programmer who's hacking on this stuff, that's just
annoying. If it's a user who has just upgraded their firmware and hit
a firmware bug in the field, that's much worse; it's
pretty much impossible to debug.
>
> > 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.
>
> >
> >
> >
> > > +
> > > + 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.
Ah, nice catch, thanks.
>
> >
> > 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'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).
>
> (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.
>
> >
> >
> >
> > > +
> > > + 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.
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v5] of/promtree: allow DT device matching by fixing 'name' brokenness
@ 2011-02-24 20:01 ` Andres Salomon
0 siblings, 0 replies; 18+ messages in thread
From: Andres Salomon @ 2011-02-24 20:01 UTC (permalink / raw)
To: Grant Likely; +Cc: dsd, sparclinux, devicetree-discuss, linux-kernel, davem
On Thu, 24 Feb 2011 12:42:34 -0700
Grant Likely <grant.likely@secretlab.ca> wrote:
> On Thu, Feb 24, 2011 at 09:33:41AM -0800, Andres Salomon wrote:
> > On Thu, 24 Feb 2011 10:04:41 -0700
> > Grant Likely <grant.likely@secretlab.ca> wrote:
> >
> > > From: Andres Salomon <dilinger@queued.net>
[...]
> > > 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) {
> > > - 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.
>
> > Our options if any of
> > this fail are to BUG (causing bootup to fail, most likely),
>
> Absolutely boot will fail.
Unnecessarily. OLPC will still boot if the DT creation fails. Even if
things are broken, it'll at least allow someone to report some errors
on the screen. On the other hand, BUGing this early in boot means that
all a user will see is a black screen (or the OFW "loading kernel"
message). The framebuffer won't have been initialized, so.. yeah.
If it's a programmer who's hacking on this stuff, that's just
annoying. If it's a user who has just upgraded their firmware and hit
a firmware bug in the field, that's much worse; it's
pretty much impossible to debug.
>
> > 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.
>
> >
> >
> >
> > > +
> > > + 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.
Ah, nice catch, thanks.
>
> >
> > 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'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).
>
> (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.
>
> >
> >
> >
> > > +
> > > + 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.
^ permalink raw reply [flat|nested] 18+ messages in thread[parent not found: <20110224120109.0ed9f473-pFFUokh25LWsTnJN9+BGXg@public.gmane.org>]
* Re: [PATCH v5] of/promtree: allow DT device matching by fixing 'name' brokenness
2011-02-24 20:01 ` [PATCH v5] of/promtree: allow DT device matching by fixing 'name' brokenness Andres Salomon
(?)
@ 2011-02-25 6:06 ` Grant Likely
-1 siblings, 0 replies; 18+ messages in thread
From: Grant Likely @ 2011-02-25 6:06 UTC (permalink / raw)
To: Andres Salomon
Cc: sparclinux-u79uwXL29TY76Z2rM5mHXA,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
davem-fT/PcQaiUtIeIZ0/mPfg9Q, dsd-2X9k7bc8m7Mdnm+yROfE0A,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
On Thu, Feb 24, 2011 at 1:01 PM, Andres Salomon <dilinger@queued.net> wrote:
> On Thu, 24 Feb 2011 12:42:34 -0700
> Grant Likely <grant.likely@secretlab.ca> wrote:
>> 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).
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.
diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
index 5ab3bd5..4d87b5d 100644
--- a/drivers/of/pdt.c
+++ b/drivers/of/pdt.c
@@ -65,17 +65,25 @@ static inline void irq_trans_init(struct device_node *dp) {
static char * __init of_pdt_build_full_name(struct device_node *dp)
{
+ static int failsafe_id = 0; /* for generating unique names on failure */
char *buf;
int len;
if (of_pdt_prom_ops->pkg2path(dp->phandle, NULL, 0, &len))
- BUG();
+ goto failsafe;
buf = prom_early_alloc(len + 1);
- if (!buf)
- BUG();
if (of_pdt_prom_ops->pkg2path(dp->phandle, buf, len, &len))
- BUG();
+ goto failsafe;
+ return buf;
+
+ failsafe:
+ buf = prom_early_alloc(strlen(dp->parent->full_name) +
+ strlen(dp->name) + 16);
+ sprintf(buf, "%s/%s@unknown%i",
+ of_node_is_root(dp->parent) ? "" : dp->parent->full_name,
+ dp->name, failsafe_id++);
+ pr_err("%s: pkg2path failed; assigning %s\n", __func__, buf);
return buf;
}
g.
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v5] of/promtree: allow DT device matching by fixing 'name' brokenness
@ 2011-02-25 6:06 ` Grant Likely
0 siblings, 0 replies; 18+ messages in thread
From: Grant Likely @ 2011-02-25 6:06 UTC (permalink / raw)
To: Andres Salomon
Cc: sparclinux-u79uwXL29TY76Z2rM5mHXA,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
davem-fT/PcQaiUtIeIZ0/mPfg9Q, dsd-2X9k7bc8m7Mdnm+yROfE0A,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
On Thu, Feb 24, 2011 at 1:01 PM, Andres Salomon <dilinger-pFFUokh25LWsTnJN9+BGXg@public.gmane.org> wrote:
> On Thu, 24 Feb 2011 12:42:34 -0700
> Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
>> 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).
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.
diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
index 5ab3bd5..4d87b5d 100644
--- a/drivers/of/pdt.c
+++ b/drivers/of/pdt.c
@@ -65,17 +65,25 @@ static inline void irq_trans_init(struct device_node *dp) {
static char * __init of_pdt_build_full_name(struct device_node *dp)
{
+ static int failsafe_id = 0; /* for generating unique names on failure */
char *buf;
int len;
if (of_pdt_prom_ops->pkg2path(dp->phandle, NULL, 0, &len))
- BUG();
+ goto failsafe;
buf = prom_early_alloc(len + 1);
- if (!buf)
- BUG();
if (of_pdt_prom_ops->pkg2path(dp->phandle, buf, len, &len))
- BUG();
+ goto failsafe;
+ return buf;
+
+ failsafe:
+ buf = prom_early_alloc(strlen(dp->parent->full_name) +
+ strlen(dp->name) + 16);
+ sprintf(buf, "%s/%s@unknown%i",
+ of_node_is_root(dp->parent) ? "" : dp->parent->full_name,
+ dp->name, failsafe_id++);
+ pr_err("%s: pkg2path failed; assigning %s\n", __func__, buf);
return buf;
}
g.
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v5] of/promtree: allow DT device matching by fixing 'name' brokenness
@ 2011-02-25 6:06 ` Grant Likely
0 siblings, 0 replies; 18+ messages in thread
From: Grant Likely @ 2011-02-25 6:06 UTC (permalink / raw)
To: Andres Salomon; +Cc: dsd, sparclinux, devicetree-discuss, linux-kernel, davem
On Thu, Feb 24, 2011 at 1:01 PM, Andres Salomon <dilinger@queued.net> wrote:
> On Thu, 24 Feb 2011 12:42:34 -0700
> Grant Likely <grant.likely@secretlab.ca> wrote:
>> 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).
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.
diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
index 5ab3bd5..4d87b5d 100644
--- a/drivers/of/pdt.c
+++ b/drivers/of/pdt.c
@@ -65,17 +65,25 @@ static inline void irq_trans_init(struct device_node *dp) {
static char * __init of_pdt_build_full_name(struct device_node *dp)
{
+ static int failsafe_id = 0; /* for generating unique names on failure */
char *buf;
int len;
if (of_pdt_prom_ops->pkg2path(dp->phandle, NULL, 0, &len))
- BUG();
+ goto failsafe;
buf = prom_early_alloc(len + 1);
- if (!buf)
- BUG();
if (of_pdt_prom_ops->pkg2path(dp->phandle, buf, len, &len))
- BUG();
+ goto failsafe;
+ return buf;
+
+ failsafe:
+ buf = prom_early_alloc(strlen(dp->parent->full_name) +
+ strlen(dp->name) + 16);
+ sprintf(buf, "%s/%s@unknown%i",
+ of_node_is_root(dp->parent) ? "" : dp->parent->full_name,
+ dp->name, failsafe_id++);
+ pr_err("%s: pkg2path failed; assigning %s\n", __func__, buf);
return buf;
}
g.
^ permalink raw reply related [flat|nested] 18+ messages in thread[parent not found: <AANLkTimc37=azh11sYYi6x7gTE=0ybud5tXgxa1TDUMT-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH v5] of/promtree: allow DT device matching by fixing
2011-02-25 6:06 ` Grant Likely
(?)
@ 2011-02-26 6:41 ` Andres Salomon
-1 siblings, 0 replies; 18+ messages in thread
From: Andres Salomon @ 2011-02-26 6:41 UTC (permalink / raw)
To: Grant Likely
Cc: sparclinux-u79uwXL29TY76Z2rM5mHXA,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
davem-fT/PcQaiUtIeIZ0/mPfg9Q, dsd-2X9k7bc8m7Mdnm+yROfE0A,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
On Thu, 24 Feb 2011 23:06:06 -0700
Grant Likely <grant.likely@secretlab.ca> wrote:
> On Thu, Feb 24, 2011 at 1:01 PM, Andres Salomon <dilinger@queued.net>
> wrote:
> > On Thu, 24 Feb 2011 12:42:34 -0700
> > Grant Likely <grant.likely@secretlab.ca> wrote:
> >> 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).
>
> 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.
Thanks, that looks good. Feel free to push into next.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5] of/promtree: allow DT device matching by fixing 'name' brokenness
@ 2011-02-26 6:41 ` Andres Salomon
0 siblings, 0 replies; 18+ messages in thread
From: Andres Salomon @ 2011-02-26 6:41 UTC (permalink / raw)
To: Grant Likely
Cc: sparclinux-u79uwXL29TY76Z2rM5mHXA,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
davem-fT/PcQaiUtIeIZ0/mPfg9Q, dsd-2X9k7bc8m7Mdnm+yROfE0A,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
On Thu, 24 Feb 2011 23:06:06 -0700
Grant Likely <grant.likely@secretlab.ca> wrote:
> On Thu, Feb 24, 2011 at 1:01 PM, Andres Salomon <dilinger@queued.net>
> wrote:
> > On Thu, 24 Feb 2011 12:42:34 -0700
> > Grant Likely <grant.likely@secretlab.ca> wrote:
> >> 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).
>
> 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.
Thanks, that looks good. Feel free to push into next.
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5] of/promtree: allow DT device matching by fixing 'name' brokenness
@ 2011-02-26 6:41 ` Andres Salomon
0 siblings, 0 replies; 18+ messages in thread
From: Andres Salomon @ 2011-02-26 6:41 UTC (permalink / raw)
To: Grant Likely; +Cc: dsd, sparclinux, devicetree-discuss, linux-kernel, davem
On Thu, 24 Feb 2011 23:06:06 -0700
Grant Likely <grant.likely@secretlab.ca> wrote:
> On Thu, Feb 24, 2011 at 1:01 PM, Andres Salomon <dilinger@queued.net>
> wrote:
> > On Thu, 24 Feb 2011 12:42:34 -0700
> > Grant Likely <grant.likely@secretlab.ca> wrote:
> >> 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).
>
> 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.
Thanks, that looks good. Feel free to push into next.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5] of/promtree: allow DT device matching by fixing
2011-02-24 19:42 ` [PATCH v5] of/promtree: allow DT device matching by fixing 'name' brokenness Grant Likely
@ 2011-02-24 23:27 ` Andres Salomon
-1 siblings, 0 replies; 18+ messages in thread
From: Andres Salomon @ 2011-02-24 23:27 UTC (permalink / raw)
To: Grant Likely; +Cc: dsd, sparclinux, devicetree-discuss, linux-kernel, davem
On Thu, 24 Feb 2011 12:42:34 -0700
Grant Likely <grant.likely@secretlab.ca> wrote:
> On Thu, Feb 24, 2011 at 09:33:41AM -0800, Andres Salomon wrote:
> > On Thu, 24 Feb 2011 10:04:41 -0700
> > Grant Likely <grant.likely@secretlab.ca> wrote:
> >
> > > From: Andres Salomon <dilinger@queued.net>
> > >
[...]
> 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.
I just tried it and it works for me. Dsd said he'd give it a try
with his DT matching stuff tomorrow.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5] of/promtree: allow DT device matching by fixing 'name' brokenness
@ 2011-02-24 23:27 ` Andres Salomon
0 siblings, 0 replies; 18+ messages in thread
From: Andres Salomon @ 2011-02-24 23:27 UTC (permalink / raw)
To: Grant Likely; +Cc: dsd, sparclinux, devicetree-discuss, linux-kernel, davem
On Thu, 24 Feb 2011 12:42:34 -0700
Grant Likely <grant.likely@secretlab.ca> wrote:
> On Thu, Feb 24, 2011 at 09:33:41AM -0800, Andres Salomon wrote:
> > On Thu, 24 Feb 2011 10:04:41 -0700
> > Grant Likely <grant.likely@secretlab.ca> wrote:
> >
> > > From: Andres Salomon <dilinger@queued.net>
> > >
[...]
> 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.
I just tried it and it works for me. Dsd said he'd give it a try
with his DT matching stuff tomorrow.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v6] of/pdt: allow DT device matching by fixing 'name'
2011-02-24 17:04 ` [PATCH v5] of/promtree: allow DT device matching by fixing 'name' brokenness Grant Likely
@ 2011-02-24 19:45 ` Andres Salomon
-1 siblings, 0 replies; 18+ messages in thread
From: Andres Salomon @ 2011-02-24 19:45 UTC (permalink / raw)
To: Grant Likely; +Cc: dsd, sparclinux, devicetree-discuss, linux-kernel, davem
Commit e2f2a93b 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 also changes OLPC behavior 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.
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
v6: rework functions based on Grant's suggestions; WARN_ON if no
pkg2path hook
Signed-off-by: Andres Salomon <dilinger@queued.net>
---
drivers/of/pdt.c | 125 ++++++++++++++++++++++++------------------------------
1 files changed, 55 insertions(+), 70 deletions(-)
diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
index 28295d0..2c6be9c 100644
--- a/drivers/of/pdt.c
+++ b/drivers/of/pdt.c
@@ -36,19 +36,68 @@ 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;
+
+ 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;
}
-#else
+#else /* CONFIG_SPARC */
static inline void of_pdt_incr_unique_id(void *p) { }
static inline void irq_trans_init(struct device_node *dp) { }
-static inline const char *of_pdt_node_name(struct device_node *dp)
+/*
+ * When fetching the full name we want the name we see with
+ * package-to-path (ie, '/foo/bar/battery@0') rather than what
+ * we see with the name property (ie, 'battery').
+ */
+static char * __init of_pdt_build_full_name(struct device_node *dp)
{
- return dp->name;
+ char *buf = NULL;
+ int len;
+
+ if (!of_pdt_prom_ops->pkg2path) {
+ /* pkg2path hook should be set for all non-sparc archs */
+ WARN_ON_ONCE(1);
+ goto fail;
+ }
+
+ if (of_pdt_prom_ops->pkg2path(dp->phandle, buf, 0, &len)) {
+ pr_err("PDT: package-to-path failed on %s\n", dp->name);
+ goto fail;
+ }
+
+ buf = prom_early_alloc(len + 1);
+ if (!buf)
+ /* we have larger problems than DT creation.. */
+ return NULL;
+
+ if (of_pdt_prom_ops->pkg2path(dp->phandle, buf, len, &len)) {
+ pr_err("PDT: package-to-path failed on %s\n", dp->name);
+ goto fail;
+ }
+ return buf;
+
+fail:
+ /* hobble along w/ dp->name if pkg2path fails; not ideal */
+ return (char *) dp->name;
}
#endif /* !CONFIG_SPARC */
@@ -132,47 +181,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.
- */
-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)
{
@@ -187,7 +195,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 +206,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 +228,6 @@ static struct device_node * __init of_pdt_build_tree(struct device_node *parent,
*(*nextp) = dp;
*nextp = &dp->allnext;
-#if defined(CONFIG_SPARC)
- dp->path_component_name = build_path_component(dp);
-#endif
dp->full_name = of_pdt_build_full_name(dp);
dp->child = of_pdt_build_tree(dp,
--
1.7.2.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v6] of/pdt: allow DT device matching by fixing 'name' brokenness
@ 2011-02-24 19:45 ` Andres Salomon
0 siblings, 0 replies; 18+ messages in thread
From: Andres Salomon @ 2011-02-24 19:45 UTC (permalink / raw)
To: Grant Likely; +Cc: dsd, sparclinux, devicetree-discuss, linux-kernel, davem
Commit e2f2a93b 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 also changes OLPC behavior 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.
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
v6: rework functions based on Grant's suggestions; WARN_ON if no
pkg2path hook
Signed-off-by: Andres Salomon <dilinger@queued.net>
---
drivers/of/pdt.c | 125 ++++++++++++++++++++++++------------------------------
1 files changed, 55 insertions(+), 70 deletions(-)
diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
index 28295d0..2c6be9c 100644
--- a/drivers/of/pdt.c
+++ b/drivers/of/pdt.c
@@ -36,19 +36,68 @@ 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;
+
+ 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;
}
-#else
+#else /* CONFIG_SPARC */
static inline void of_pdt_incr_unique_id(void *p) { }
static inline void irq_trans_init(struct device_node *dp) { }
-static inline const char *of_pdt_node_name(struct device_node *dp)
+/*
+ * When fetching the full name we want the name we see with
+ * package-to-path (ie, '/foo/bar/battery@0') rather than what
+ * we see with the name property (ie, 'battery').
+ */
+static char * __init of_pdt_build_full_name(struct device_node *dp)
{
- return dp->name;
+ char *buf = NULL;
+ int len;
+
+ if (!of_pdt_prom_ops->pkg2path) {
+ /* pkg2path hook should be set for all non-sparc archs */
+ WARN_ON_ONCE(1);
+ goto fail;
+ }
+
+ if (of_pdt_prom_ops->pkg2path(dp->phandle, buf, 0, &len)) {
+ pr_err("PDT: package-to-path failed on %s\n", dp->name);
+ goto fail;
+ }
+
+ buf = prom_early_alloc(len + 1);
+ if (!buf)
+ /* we have larger problems than DT creation.. */
+ return NULL;
+
+ if (of_pdt_prom_ops->pkg2path(dp->phandle, buf, len, &len)) {
+ pr_err("PDT: package-to-path failed on %s\n", dp->name);
+ goto fail;
+ }
+ return buf;
+
+fail:
+ /* hobble along w/ dp->name if pkg2path fails; not ideal */
+ return (char *) dp->name;
}
#endif /* !CONFIG_SPARC */
@@ -132,47 +181,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.
- */
-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)
{
@@ -187,7 +195,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 +206,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 +228,6 @@ static struct device_node * __init of_pdt_build_tree(struct device_node *parent,
*(*nextp) = dp;
*nextp = &dp->allnext;
-#if defined(CONFIG_SPARC)
- dp->path_component_name = build_path_component(dp);
-#endif
dp->full_name = of_pdt_build_full_name(dp);
dp->child = of_pdt_build_tree(dp,
--
1.7.2.3
^ permalink raw reply related [flat|nested] 18+ messages in thread