Re: [PATCH] of/pdt: allow DT device matching by fixing 'name'brokenness (v2)

[Grant Likely]

On Wed, Feb 23, 2011 at 03:03:57PM -0800, Andres Salomon wrote:
> 
> 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.
> 
> Note that this affects sparc by reverting dp->name back to what
> sparc was originally doing (looking at the name property).
> 
> v2: combine two patches and revert of_pdt_node_name to original version.
> 
> Signed-off-by: Andres Salomon <dilinger@xxxxxxxxxx>

Hi Andres, comments below.

g.

> ---
>  drivers/of/pdt.c |   42 +++++++++++++++---------------------------
>  1 files changed, 15 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
> index 28295d0..a7aa85e 100644
> --- a/drivers/of/pdt.c
> +++ b/drivers/of/pdt.c
> @@ -134,7 +134,7 @@ static char * __init of_pdt_get_one_property(phandle node, const char *name)
>  
>  static char * __init of_pdt_try_pkg2path(phandle node)
>  {
> -	char *res, *buf = NULL;
> +	char *buf = NULL;
>  	int len;
>  
>  	if (!of_pdt_prom_ops->pkg2path)
> @@ -147,29 +147,6 @@ static char * __init of_pdt_try_pkg2path(phandle node)
>  		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;
>  }
>  

It seems to me that of_pdt_build_full_name will still be missing the
'@<addr>' component on non-sparc non-olpc builds because it uses the
broken of_pdt_node_name().  That needs to be fixed too, even if there
are no current users (or removed).

> @@ -187,7 +164,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,11 +175,22 @@ 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)
> +static char * __init of_pdt_build_full_name(struct device_node *dp,
> +		phandle node)

Is dp->phandle not suitable here?

>  {
>  	int len, ourlen, plen;
>  	char *n;
>  
> +	/*
> +	 * 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').
> +	 */
> +	n = of_pdt_try_pkg2path(node);
> +	if (n)
> +		return n;
> +
> +	/* Older method for determining full name */
>  	plen = strlen(dp->parent->full_name);
>  	ourlen = strlen(of_pdt_node_name(dp));
>  	len = ourlen + plen + 2;
> @@ -243,7 +231,7 @@ static struct device_node * __init of_pdt_build_tree(struct device_node *parent,
>  #if defined(CONFIG_SPARC)
>  		dp->path_component_name = build_path_component(dp);
>  #endif

I still think it would be useful to remove the #if
defined(CONFIG_SPARC) from path_component_name, and it might be the
best way to solve my comment about broken of_pdt_node_name above.

> -		dp->full_name = of_pdt_build_full_name(dp);
> +		dp->full_name = of_pdt_build_full_name(dp, node);
>  
>  		dp->child = of_pdt_build_tree(dp,
>  				of_pdt_prom_ops->getchild(node), nextp);
> -- 
> 1.7.2.3
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/