[PATCH 0/3] Rework of_find_node_by_path() code

[PATCH 0/3] Rework of_find_node_by_path() code

[Grant Likely]

This series reworks the of_find_node_by_path() function to search by
path component instead of comparing the full path on every node. This
makes for a more efficient search and makes it possible to parse things
like aliases.

This is the second time I'm posting this series. I'm going to apply it
to my tree for the next merge window.

[PATCH 1/3] lib: add glibc style strchrnul() variant

[Grant Likely]

The strchrnul() variant helpfully returns a the end of the string
instead of a NULL if the requested character is not found. This can
simplify string parsing code since it doesn't need to expicitly check
for a NULL return. If a valid string pointer is passed in, then a valid
null terminated string will always come back out.

Signed-off-by: Grant Likely <grant.likely@linaro.org>
---
 include/linux/string.h |  3 +++
 lib/string.c           | 15 +++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index ac889c5ea11b..d36977e029af 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -52,6 +52,9 @@ extern int strncasecmp(const char *s1, const char *s2, size_t n);
 #ifndef __HAVE_ARCH_STRCHR
 extern char * strchr(const char *,int);
 #endif
+#ifndef __HAVE_ARCH_STRCHRNUL
+extern char * strchrnul(const char *,int);
+#endif
 #ifndef __HAVE_ARCH_STRNCHR
 extern char * strnchr(const char *, size_t, int);
 #endif
diff --git a/lib/string.c b/lib/string.c
index 9b1f9062a202..059636f92c26 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -301,6 +301,21 @@ char *strchr(const char *s, int c)
 EXPORT_SYMBOL(strchr);
 #endif
 
+#ifndef __HAVE_ARCH_STRCHRNUL
+/**
+ * strchr - Find the first occurrence of a character in a string
+ * @s: The string to be searched
+ * @c: The character to search for
+ */
+char *strchrnul(const char *s, int c)
+{
+	while (*s && *s != (char)c)
+		s++;
+	return (char *)s;
+}
+EXPORT_SYMBOL(strchrnul);
+#endif
+
 #ifndef __HAVE_ARCH_STRRCHR
 /**
  * strrchr - Find the last occurrence of a character in a string
-- 
1.9.1

Re: [PATCH 1/3] lib: add glibc style strchrnul() variant

[Frank Rowand]

On 5/13/2014 7:58 AM, Grant Likely wrote:
> The strchrnul() variant helpfully returns a the end of the string
> instead of a NULL if the requested character is not found. This can
> simplify string parsing code since it doesn't need to expicitly check
> for a NULL return. If a valid string pointer is passed in, then a valid
> null terminated string will always come back out.
> 
> Signed-off-by: Grant Likely <grant.likely@linaro.org>
> ---
>  include/linux/string.h |  3 +++
>  lib/string.c           | 15 +++++++++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/include/linux/string.h b/include/linux/string.h
> index ac889c5ea11b..d36977e029af 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -52,6 +52,9 @@ extern int strncasecmp(const char *s1, const char *s2, size_t n);
>  #ifndef __HAVE_ARCH_STRCHR
>  extern char * strchr(const char *,int);
>  #endif
> +#ifndef __HAVE_ARCH_STRCHRNUL
> +extern char * strchrnul(const char *,int);
> +#endif
>  #ifndef __HAVE_ARCH_STRNCHR
>  extern char * strnchr(const char *, size_t, int);
>  #endif
> diff --git a/lib/string.c b/lib/string.c
> index 9b1f9062a202..059636f92c26 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -301,6 +301,21 @@ char *strchr(const char *s, int c)
>  EXPORT_SYMBOL(strchr);
>  #endif
>  
> +#ifndef __HAVE_ARCH_STRCHRNUL
> +/**
> + * strchr - Find the first occurrence of a character in a string

s/strchr/strchrnul/

Add to the end of the description: if c is not found in s then return
a pointer to the null byte at the end of s

> + * @s: The string to be searched
> + * @c: The character to search for
> + */
> +char *strchrnul(const char *s, int c)
> +{
> +	while (*s && *s != (char)c)
> +		s++;
> +	return (char *)s;
> +}
> +EXPORT_SYMBOL(strchrnul);
> +#endif
> +
>  #ifndef __HAVE_ARCH_STRRCHR
>  /**
>   * strrchr - Find the last occurrence of a character in a string
> 

Re: [PATCH 1/3] lib: add glibc style strchrnul() variant

[Grant Likely]

On Thu, 15 May 2014 15:19:00 -0700, Frank Rowand <frowand.list@gmail.com> wrote:
> On 5/13/2014 7:58 AM, Grant Likely wrote:
> > The strchrnul() variant helpfully returns a the end of the string
> > instead of a NULL if the requested character is not found. This can
> > simplify string parsing code since it doesn't need to expicitly check
> > for a NULL return. If a valid string pointer is passed in, then a valid
> > null terminated string will always come back out.
> > 
> > Signed-off-by: Grant Likely <grant.likely@linaro.org>
> > ---
> >  include/linux/string.h |  3 +++
> >  lib/string.c           | 15 +++++++++++++++
> >  2 files changed, 18 insertions(+)
> > 
> > diff --git a/include/linux/string.h b/include/linux/string.h
> > index ac889c5ea11b..d36977e029af 100644
> > --- a/include/linux/string.h
> > +++ b/include/linux/string.h
> > @@ -52,6 +52,9 @@ extern int strncasecmp(const char *s1, const char *s2, size_t n);
> >  #ifndef __HAVE_ARCH_STRCHR
> >  extern char * strchr(const char *,int);
> >  #endif
> > +#ifndef __HAVE_ARCH_STRCHRNUL
> > +extern char * strchrnul(const char *,int);
> > +#endif
> >  #ifndef __HAVE_ARCH_STRNCHR
> >  extern char * strnchr(const char *, size_t, int);
> >  #endif
> > diff --git a/lib/string.c b/lib/string.c
> > index 9b1f9062a202..059636f92c26 100644
> > --- a/lib/string.c
> > +++ b/lib/string.c
> > @@ -301,6 +301,21 @@ char *strchr(const char *s, int c)
> >  EXPORT_SYMBOL(strchr);
> >  #endif
> >  
> > +#ifndef __HAVE_ARCH_STRCHRNUL
> > +/**
> > + * strchr - Find the first occurrence of a character in a string
> 
> s/strchr/strchrnul/
> 
> Add to the end of the description: if c is not found in s then return
> a pointer to the null byte at the end of s

Thanks, fixed.

> 
> > + * @s: The string to be searched
> > + * @c: The character to search for
> > + */
> > +char *strchrnul(const char *s, int c)
> > +{
> > +	while (*s && *s != (char)c)
> > +		s++;
> > +	return (char *)s;
> > +}
> > +EXPORT_SYMBOL(strchrnul);
> > +#endif
> > +
> >  #ifndef __HAVE_ARCH_STRRCHR
> >  /**
> >   * strrchr - Find the last occurrence of a character in a string
> > 
> 

[PATCH 2/3] of: Make of_find_node_by_path() handle /aliases

[Grant Likely]

Make of_find_node_by_path() handle aliases as prefixes. To make this
work the name search is refactored to search by path component instead
of by full string. This should be a more efficient search, and it makes
it possible to start a search at a subnode of a tree.

Signed-off-by: David Daney <david.daney@cavium.com>
Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
[grant.likely: Rework to not require allocating at runtime]
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Grant Likely <grant.likely@linaro.org>
---
 drivers/of/base.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 56 insertions(+), 4 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 6e240698353b..60089b9a3014 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -771,9 +771,38 @@ struct device_node *of_get_child_by_name(const struct device_node *node,
 }
 EXPORT_SYMBOL(of_get_child_by_name);
 
+static struct device_node *__of_find_node_by_path(struct device_node *parent,
+						const char *path)
+{
+	struct device_node *child;
+	int len = strchrnul(path, '/') - path;
+
+	if (!len)
+		return parent;
+
+	for_each_child_of_node(parent, child) {
+		const char *name = strrchr(child->full_name, '/');
+		if (WARN(!name, "malformed device_node %s\n", child->full_name))
+			continue;
+		name++;
+		if (strncmp(path, name, len) == 0 && (strlen(name) == len))
+			return child;
+	}
+	return NULL;
+}
+
 /**
  *	of_find_node_by_path - Find a node matching a full OF path
  *	@path:	The full path to match
+ *	@path: Either the full path to match, or if the path does not
+ *	       start with '/', the name of a property of the /aliases
+ *	       node (an alias).  In the case of an alias, the node
+ *	       matching the alias' value will be returned.
+ *
+ *	Valid paths:
+ *		/foo/bar	Full path
+ *		foo		Valid alias
+ *		foo/bar		Valid alias + relative path
  *
  *	Returns a node pointer with refcount incremented, use
  *	of_node_put() on it when done.
@@ -781,13 +810,36 @@ EXPORT_SYMBOL(of_get_child_by_name);
 struct device_node *of_find_node_by_path(const char *path)
 {
 	struct device_node *np = of_allnodes;
+	struct property *pp;
 	unsigned long flags;
 
+	/* The path could begin with an alias */
+	if (*path != '/') {
+		char *p = strchrnul(path, '/');
+		int len = p - path;
+
+		/* of_aliases must not be NULL */
+		if (!of_aliases)
+			return NULL;
+
+		np = NULL;
+		for_each_property_of_node(of_aliases, pp) {
+			if (strlen(pp->name) == len && !strncmp(pp->name, path, len)) {
+				np = of_find_node_by_path(pp->value);
+				break;
+			}
+		}
+		if (!np)
+			return NULL;
+		path = p;
+	}
+
+	/* Step down the tree matching path components */
 	raw_spin_lock_irqsave(&devtree_lock, flags);
-	for (; np; np = np->allnext) {
-		if (np->full_name && (of_node_cmp(np->full_name, path) == 0)
-		    && of_node_get(np))
-			break;
+	while (np && *path == '/') {
+		path++; /* Increment past '/' delimiter */
+		np = __of_find_node_by_path(np, path);
+		path = strchrnul(path, '/');
 	}
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 	return np;
-- 
1.9.1

Re: [PATCH 2/3] of: Make of_find_node_by_path() handle /aliases

[Frank Rowand]

On 5/13/2014 7:58 AM, Grant Likely wrote:
> Make of_find_node_by_path() handle aliases as prefixes. To make this
> work the name search is refactored to search by path component instead
> of by full string. This should be a more efficient search, and it makes
> it possible to start a search at a subnode of a tree.
> 
> Signed-off-by: David Daney <david.daney@cavium.com>
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> [grant.likely: Rework to not require allocating at runtime]
> Acked-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Grant Likely <grant.likely@linaro.org>
> ---
>  drivers/of/base.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 56 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 6e240698353b..60089b9a3014 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -771,9 +771,38 @@ struct device_node *of_get_child_by_name(const struct device_node *node,
>  }
>  EXPORT_SYMBOL(of_get_child_by_name);
>  
> +static struct device_node *__of_find_node_by_path(struct device_node *parent,
> +						const char *path)
> +{
> +	struct device_node *child;
> +	int len = strchrnul(path, '/') - path;
> +
> +	if (!len)
> +		return parent;

(!len) is true if the the final character of the path passed into of_find_node_by_path()
was "/".  Strictly speaking, ->full_name will never end with "/", so the return value
should be NULL, indicating that the match fails.

> +
> +	for_each_child_of_node(parent, child) {
> +		const char *name = strrchr(child->full_name, '/');
> +		if (WARN(!name, "malformed device_node %s\n", child->full_name))
> +			continue;
> +		name++;

Why go to the effort of finding the final component of child->full_name instead
of just using child->name?

> +		if (strncmp(path, name, len) == 0 && (strlen(name) == len))
> +			return child;
> +	}
> +	return NULL;
> +}
> +
>  /**
>   *	of_find_node_by_path - Find a node matching a full OF path
>   *	@path:	The full path to match
> + *	@path: Either the full path to match, or if the path does not

Delete the old @path description.


> + *	       start with '/', the name of a property of the /aliases
> + *	       node (an alias).  In the case of an alias, the node
> + *	       matching the alias' value will be returned.
> + *
> + *	Valid paths:
> + *		/foo/bar	Full path
> + *		foo		Valid alias
> + *		foo/bar		Valid alias + relative path
>   *
>   *	Returns a node pointer with refcount incremented, use
>   *	of_node_put() on it when done.
> @@ -781,13 +810,36 @@ EXPORT_SYMBOL(of_get_child_by_name);
>  struct device_node *of_find_node_by_path(const char *path)
>  {
>  	struct device_node *np = of_allnodes;
> +	struct property *pp;
>  	unsigned long flags;
>  
> +	/* The path could begin with an alias */
> +	if (*path != '/') {
> +		char *p = strchrnul(path, '/');
> +		int len = p - path;
> +
> +		/* of_aliases must not be NULL */
> +		if (!of_aliases)
> +			return NULL;
> +
> +		np = NULL;
> +		for_each_property_of_node(of_aliases, pp) {
> +			if (strlen(pp->name) == len && !strncmp(pp->name, path, len)) {
> +				np = of_find_node_by_path(pp->value);
> +				break;
> +			}
> +		}
> +		if (!np)
> +			return NULL;
> +		path = p;
> +	}
> +
> +	/* Step down the tree matching path components */
>  	raw_spin_lock_irqsave(&devtree_lock, flags);
> -	for (; np; np = np->allnext) {
> -		if (np->full_name && (of_node_cmp(np->full_name, path) == 0)
> -		    && of_node_get(np))
> -			break;
> +	while (np && *path == '/') {
> +		path++; /* Increment past '/' delimiter */
> +		np = __of_find_node_by_path(np, path);
> +		path = strchrnul(path, '/');
>  	}
>  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  	return np;
> 

Re: [PATCH 2/3] of: Make of_find_node_by_path() handle /aliases

[Grant Likely]

On Thu, 15 May 2014 19:51:17 -0700, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 5/13/2014 7:58 AM, Grant Likely wrote:
> > Make of_find_node_by_path() handle aliases as prefixes. To make this
> > work the name search is refactored to search by path component instead
> > of by full string. This should be a more efficient search, and it makes
> > it possible to start a search at a subnode of a tree.
> > 
> > Signed-off-by: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> > Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> > [grant.likely: Rework to not require allocating at runtime]
> > Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > Signed-off-by: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > ---
> >  drivers/of/base.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 56 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index 6e240698353b..60089b9a3014 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -771,9 +771,38 @@ struct device_node *of_get_child_by_name(const struct device_node *node,
> >  }
> >  EXPORT_SYMBOL(of_get_child_by_name);
> >  
> > +static struct device_node *__of_find_node_by_path(struct device_node *parent,
> > +						const char *path)
> > +{
> > +	struct device_node *child;
> > +	int len = strchrnul(path, '/') - path;
> > +
> > +	if (!len)
> > +		return parent;
> 
> (!len) is true if the the final character of the path passed into of_find_node_by_path()
> was "/".  Strictly speaking, ->full_name will never end with "/", so the return value
> should be NULL, indicating that the match fails.

Ah, good catch. I should add a test case for that.

> 
> > +
> > +	for_each_child_of_node(parent, child) {
> > +		const char *name = strrchr(child->full_name, '/');
> > +		if (WARN(!name, "malformed device_node %s\n", child->full_name))
> > +			continue;
> > +		name++;
> 
> Why go to the effort of finding the final component of child->full_name instead
> of just using child->name?

Because child->name and the final component of child->full_name is not
the same thing. child->name has the unit address stripped off. It is
part of how OpenFirmware expects to be used. New drivers are never
supposed to be matching by node name, but I don't currently have a good
way to go through any driver depending on the OpenFirmware behaviour.

Ideally I'd like to get rid of the difference.

> 
> > +		if (strncmp(path, name, len) == 0 && (strlen(name) == len))
> > +			return child;
> > +	}
> > +	return NULL;
> > +}
> > +
> >  /**
> >   *	of_find_node_by_path - Find a node matching a full OF path
> >   *	@path:	The full path to match
> > + *	@path: Either the full path to match, or if the path does not
> 
> Delete the old @path description.

Oops, thanks.

> 
> 
> > + *	       start with '/', the name of a property of the /aliases
> > + *	       node (an alias).  In the case of an alias, the node
> > + *	       matching the alias' value will be returned.
> > + *
> > + *	Valid paths:
> > + *		/foo/bar	Full path
> > + *		foo		Valid alias
> > + *		foo/bar		Valid alias + relative path
> >   *
> >   *	Returns a node pointer with refcount incremented, use
> >   *	of_node_put() on it when done.
> > @@ -781,13 +810,36 @@ EXPORT_SYMBOL(of_get_child_by_name);
> >  struct device_node *of_find_node_by_path(const char *path)
> >  {
> >  	struct device_node *np = of_allnodes;
> > +	struct property *pp;
> >  	unsigned long flags;
> >  
> > +	/* The path could begin with an alias */
> > +	if (*path != '/') {
> > +		char *p = strchrnul(path, '/');
> > +		int len = p - path;
> > +
> > +		/* of_aliases must not be NULL */
> > +		if (!of_aliases)
> > +			return NULL;
> > +
> > +		np = NULL;
> > +		for_each_property_of_node(of_aliases, pp) {
> > +			if (strlen(pp->name) == len && !strncmp(pp->name, path, len)) {
> > +				np = of_find_node_by_path(pp->value);
> > +				break;
> > +			}
> > +		}
> > +		if (!np)
> > +			return NULL;
> > +		path = p;
> > +	}
> > +
> > +	/* Step down the tree matching path components */
> >  	raw_spin_lock_irqsave(&devtree_lock, flags);
> > -	for (; np; np = np->allnext) {
> > -		if (np->full_name && (of_node_cmp(np->full_name, path) == 0)
> > -		    && of_node_get(np))
> > -			break;
> > +	while (np && *path == '/') {
> > +		path++; /* Increment past '/' delimiter */
> > +		np = __of_find_node_by_path(np, path);
> > +		path = strchrnul(path, '/');
> >  	}
> >  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> >  	return np;
> > 
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] of: Make of_find_node_by_path() handle /aliases

[Grant Likely]

On Fri, 16 May 2014 11:54:44 +0100, Grant Likely <grant.likely@linaro.org> wrote:
> On Thu, 15 May 2014 19:51:17 -0700, Frank Rowand <frowand.list@gmail.com> wrote:
> > On 5/13/2014 7:58 AM, Grant Likely wrote:
> > > Make of_find_node_by_path() handle aliases as prefixes. To make this
> > > work the name search is refactored to search by path component instead
> > > of by full string. This should be a more efficient search, and it makes
> > > it possible to start a search at a subnode of a tree.
> > > 
> > > Signed-off-by: David Daney <david.daney@cavium.com>
> > > Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> > > [grant.likely: Rework to not require allocating at runtime]
> > > Acked-by: Rob Herring <robh@kernel.org>
> > > Signed-off-by: Grant Likely <grant.likely@linaro.org>
> > > ---
> > >  drivers/of/base.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++----
> > >  1 file changed, 56 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > > index 6e240698353b..60089b9a3014 100644
> > > --- a/drivers/of/base.c
> > > +++ b/drivers/of/base.c
> > > @@ -771,9 +771,38 @@ struct device_node *of_get_child_by_name(const struct device_node *node,
> > >  }
> > >  EXPORT_SYMBOL(of_get_child_by_name);
> > >  
> > > +static struct device_node *__of_find_node_by_path(struct device_node *parent,
> > > +						const char *path)
> > > +{
> > > +	struct device_node *child;
> > > +	int len = strchrnul(path, '/') - path;
> > > +
> > > +	if (!len)
> > > +		return parent;
> > 
> > (!len) is true if the the final character of the path passed into of_find_node_by_path()
> > was "/".  Strictly speaking, ->full_name will never end with "/", so the return value
> > should be NULL, indicating that the match fails.
> 
> Ah, good catch. I should add a test case for that.

In my testing this looks okay. The while loop that calls into
__of_find_node_by_path() looks like this:

	while (np && *path == '/') {
		path++; /* Increment past '/' delimiter */
		np = __of_find_node_by_path(np, path);
		path = strchrnul(path, '/');
	}

If the path ends with a '/', then the loop will go around one more time.
The pointer will be incremented to point at the null character and len
will be null because strchrnul() will point at the last item.

I've added a couple of test cases to make sure it works correctly:

diff --git a/drivers/of/selftest.c b/drivers/of/selftest.c
index a9d00e8c17ea..10900b18fc06 100644
--- a/drivers/of/selftest.c
+++ b/drivers/of/selftest.c
@@ -40,6 +40,12 @@ static void __init of_selftest_find_node_by_name(void)
 	 	"find /testcase-data failed\n");
 	of_node_put(np);
 
+	/* Test if trailing '/' works */
+	np = of_find_node_by_path("/testcase-data/");
+	selftest(np && !strcmp("/testcase-data", np->full_name),
+	 	"find /testcase-data/ failed\n");
+	of_node_put(np);
+
 	np = of_find_node_by_path("/testcase-data/phandle-tests/consumer-a");
 	selftest(np && !strcmp("/testcase-data/phandle-tests/consumer-a", np->full_name),
 	 	"find /testcase-data/phandle-tests/consumer-a failed\n");
@@ -50,6 +56,12 @@ static void __init of_selftest_find_node_by_name(void)
 	 	"find testcase-alias failed\n");
 	of_node_put(np);
 
+	/* Test if trailing '/' works on aliases */
+	np = of_find_node_by_path("testcase-alias/");
+	selftest(np && !strcmp("/testcase-data", np->full_name),
+	 	"find testcase-alias/ failed\n");
+	of_node_put(np);
+
 	np = of_find_node_by_path("testcase-alias/phandle-tests/consumer-a");
 	selftest(np && !strcmp("/testcase-data/phandle-tests/consumer-a", np->full_name),
 	 	"find testcase-alias/phandle-tests/consumer-a failed\n");

g.


> 
> > 
> > > +
> > > +	for_each_child_of_node(parent, child) {
> > > +		const char *name = strrchr(child->full_name, '/');
> > > +		if (WARN(!name, "malformed device_node %s\n", child->full_name))
> > > +			continue;
> > > +		name++;
> > 
> > Why go to the effort of finding the final component of child->full_name instead
> > of just using child->name?
> 
> Because child->name and the final component of child->full_name is not
> the same thing. child->name has the unit address stripped off. It is
> part of how OpenFirmware expects to be used. New drivers are never
> supposed to be matching by node name, but I don't currently have a good
> way to go through any driver depending on the OpenFirmware behaviour.
> 
> Ideally I'd like to get rid of the difference.
> 
> > 
> > > +		if (strncmp(path, name, len) == 0 && (strlen(name) == len))
> > > +			return child;
> > > +	}
> > > +	return NULL;
> > > +}
> > > +
> > >  /**
> > >   *	of_find_node_by_path - Find a node matching a full OF path
> > >   *	@path:	The full path to match
> > > + *	@path: Either the full path to match, or if the path does not
> > 
> > Delete the old @path description.
> 
> Oops, thanks.
> 
> > 
> > 
> > > + *	       start with '/', the name of a property of the /aliases
> > > + *	       node (an alias).  In the case of an alias, the node
> > > + *	       matching the alias' value will be returned.
> > > + *
> > > + *	Valid paths:
> > > + *		/foo/bar	Full path
> > > + *		foo		Valid alias
> > > + *		foo/bar		Valid alias + relative path
> > >   *
> > >   *	Returns a node pointer with refcount incremented, use
> > >   *	of_node_put() on it when done.
> > > @@ -781,13 +810,36 @@ EXPORT_SYMBOL(of_get_child_by_name);
> > >  struct device_node *of_find_node_by_path(const char *path)
> > >  {
> > >  	struct device_node *np = of_allnodes;
> > > +	struct property *pp;
> > >  	unsigned long flags;
> > >  
> > > +	/* The path could begin with an alias */
> > > +	if (*path != '/') {
> > > +		char *p = strchrnul(path, '/');
> > > +		int len = p - path;
> > > +
> > > +		/* of_aliases must not be NULL */
> > > +		if (!of_aliases)
> > > +			return NULL;
> > > +
> > > +		np = NULL;
> > > +		for_each_property_of_node(of_aliases, pp) {
> > > +			if (strlen(pp->name) == len && !strncmp(pp->name, path, len)) {
> > > +				np = of_find_node_by_path(pp->value);
> > > +				break;
> > > +			}
> > > +		}
> > > +		if (!np)
> > > +			return NULL;
> > > +		path = p;
> > > +	}
> > > +
> > > +	/* Step down the tree matching path components */
> > >  	raw_spin_lock_irqsave(&devtree_lock, flags);
> > > -	for (; np; np = np->allnext) {
> > > -		if (np->full_name && (of_node_cmp(np->full_name, path) == 0)
> > > -		    && of_node_get(np))
> > > -			break;
> > > +	while (np && *path == '/') {
> > > +		path++; /* Increment past '/' delimiter */
> > > +		np = __of_find_node_by_path(np, path);
> > > +		path = strchrnul(path, '/');
> > >  	}
> > >  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> > >  	return np;
> > > 
> > 
> 

Re: [PATCH 2/3] of: Make of_find_node_by_path() handle /aliases

[Frank Rowand]

On 5/18/2014 2:27 AM, Grant Likely wrote:

> On Fri, 16 May 2014 11:54:44 +0100, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

>> On Thu, 15 May 2014 19:51:17 -0700, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

>>> On 5/13/2014 7:58 AM, Grant Likely wrote:

>>>> Make of_find_node_by_path() handle aliases as prefixes. To make this

>>>> work the name search is refactored to search by path component instead

>>>> of by full string. This should be a more efficient search, and it makes

>>>> it possible to start a search at a subnode of a tree.

>>>>

>>>> Signed-off-by: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>

>>>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>

>>>> [grant.likely: Rework to not require allocating at runtime]

>>>> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

>>>> Signed-off-by: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

>>>> ---

>>>>  drivers/of/base.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++----

>>>>  1 file changed, 56 insertions(+), 4 deletions(-)

>>>>

>>>> diff --git a/drivers/of/base.c b/drivers/of/base.c

>>>> index 6e240698353b..60089b9a3014 100644

>>>> --- a/drivers/of/base.c

>>>> +++ b/drivers/of/base.c

>>>> @@ -771,9 +771,38 @@ struct device_node *of_get_child_by_name(const struct device_node *node,

>>>>  }

>>>>  EXPORT_SYMBOL(of_get_child_by_name);

>>>>  

>>>> +static struct device_node *__of_find_node_by_path(struct device_node *parent,

>>>> +						const char *path)

>>>> +{

>>>> +	struct device_node *child;

>>>> +	int len = strchrnul(path, '/') - path;

>>>> +

>>>> +	if (!len)

>>>> +		return parent;

>>>

>>> (!len) is true if the the final character of the path passed into of_find_node_by_path()

>>> was "/".  Strictly speaking, ->full_name will never end with "/", so the return value

>>> should be NULL, indicating that the match fails.

>>

>> Ah, good catch. I should add a test case for that.

> 

> In my testing this looks okay. The while loop that calls into

> __of_find_node_by_path() looks like this:

> 

> 	while (np && *path == '/') {

> 		path++; /* Increment past '/' delimiter */

> 		np = __of_find_node_by_path(np, path);

> 		path = strchrnul(path, '/');

> 	}

> 

> If the path ends with a '/', then the loop will go around one more time.

> The pointer will be incremented to point at the null character and len

> will be null because strchrnul() will point at the last item.



Yes, that was my point.  The old version of of_find_node_by_path() would not

find a match if the path ended with a "/" (unless the full path was "/").

This patch series changes the behavior to be a match.


I will reply to this email with an additional patch that restores the

original behavior.


If you move the additional test cases you provide below and the test cases

in patch 3 to the beginning of the series, you can see the before and after

behavior of adding patch 1 and patch 2.


> 

> I've added a couple of test cases to make sure it works correctly:

> 

> diff --git a/drivers/of/selftest.c b/drivers/of/selftest.c

> index a9d00e8c17ea..10900b18fc06 100644

> --- a/drivers/of/selftest.c

> +++ b/drivers/of/selftest.c

> @@ -40,6 +40,12 @@ static void __init of_selftest_find_node_by_name(void)

>  	 	"find /testcase-data failed\n");

>  	of_node_put(np);

>  

> +	/* Test if trailing '/' works */

> +	np = of_find_node_by_path("/testcase-data/");

> +	selftest(np && !strcmp("/testcase-data", np->full_name),

> +	 	"find /testcase-data/ failed\n");

> +	of_node_put(np);

> +

>  	np = of_find_node_by_path("/testcase-data/phandle-tests/consumer-a");

>  	selftest(np && !strcmp("/testcase-data/phandle-tests/consumer-a", np->full_name),

>  	 	"find /testcase-data/phandle-tests/consumer-a failed\n");

> @@ -50,6 +56,12 @@ static void __init of_selftest_find_node_by_name(void)

>  	 	"find testcase-alias failed\n");

>  	of_node_put(np);

>  

> +	/* Test if trailing '/' works on aliases */

> +	np = of_find_node_by_path("testcase-alias/");

> +	selftest(np && !strcmp("/testcase-data", np->full_name),

> +	 	"find testcase-alias/ failed\n");

> +	of_node_put(np);

> +

>  	np = of_find_node_by_path("testcase-alias/phandle-tests/consumer-a");

>  	selftest(np && !strcmp("/testcase-data/phandle-tests/consumer-a", np->full_name),

>  	 	"find testcase-alias/phandle-tests/consumer-a failed\n");

> 

> g.

> 

> 

>>

>>>

>>>> +

>>>> +	for_each_child_of_node(parent, child) {

>>>> +		const char *name = strrchr(child->full_name, '/');

>>>> +		if (WARN(!name, "malformed device_node %s\n", child->full_name))

>>>> +			continue;

>>>> +		name++;


< snip >

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] of: Make of_find_node_by_path() handle /aliases

[Frank Rowand]

On 5/20/2014 7:41 PM, Frank Rowand wrote:



< snip >



> I will reply to this email with an additional patch that restores the

> original behavior.



< snip >



From: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>

If __of_find_node_by_path() returns parent when the remaining portion of the
path is "/" then the behavior of of_find_node_by_path() has changed.

Previously, adding an extraneous "/" on the end of a path would result
in of_find_node_by_path() not finding a match.

Signed-off-by: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
---
 drivers/of/base.c |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Index: b/drivers/of/base.c
===================================================================
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -778,7 +778,7 @@ static struct device_node *__of_find_nod
 	int len = strchrnul(path, '/') - path;
 
 	if (!len)
-		return parent;
+		return NULL;
 
 	for_each_child_of_node(parent, child) {
 		const char *name = strrchr(child->full_name, '/');
@@ -813,6 +813,17 @@ struct device_node *of_find_node_by_path
 	struct property *pp;
 	unsigned long flags;
 
+	if (strcmp(path, "/") == 0) {
+		raw_spin_lock_irqsave(&devtree_lock, flags);
+		for (; np; np = np->allnext) {
+			if (np->full_name && (of_node_cmp(np->full_name, path) == 0)
+			    && of_node_get(np))
+				break;
+		}
+		raw_spin_unlock_irqrestore(&devtree_lock, flags);
+		return np;
+	}
+
 	/* The path could begin with an alias */
 	if (*path != '/') {
 		char *p = strchrnul(path, '/');
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] of: Make of_find_node_by_path() handle /aliases

[Grant Likely]

On Tue, 20 May 2014 19:46:19 -0700, Frank Rowand <frowand.list@gmail.com> wrote:
> On 5/20/2014 7:41 PM, Frank Rowand wrote:
> < snip >
> > I will reply to this email with an additional patch that restores the
> > original behavior.
> < snip >
> From: Frank Rowand <frank.rowand@sonymobile.com>
> 
> If __of_find_node_by_path() returns parent when the remaining portion of the
> path is "/" then the behavior of of_find_node_by_path() has changed.
> 
> Previously, adding an extraneous "/" on the end of a path would result
> in of_find_node_by_path() not finding a match.
> 
> Signed-off-by: Frank Rowand <frank.rowand@sonymobile.com>
> ---
>  drivers/of/base.c |   13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> Index: b/drivers/of/base.c
> ===================================================================
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -778,7 +778,7 @@ static struct device_node *__of_find_nod
>  	int len = strchrnul(path, '/') - path;
>  
>  	if (!len)
> -		return parent;
> +		return NULL;
>  
>  	for_each_child_of_node(parent, child) {
>  		const char *name = strrchr(child->full_name, '/');
> @@ -813,6 +813,17 @@ struct device_node *of_find_node_by_path
>  	struct property *pp;
>  	unsigned long flags;
>  
> +	if (strcmp(path, "/") == 0) {
> +		raw_spin_lock_irqsave(&devtree_lock, flags);
> +		for (; np; np = np->allnext) {
> +			if (np->full_name && (of_node_cmp(np->full_name, path) == 0)
> +			    && of_node_get(np))
> +				break;
> +		}
> +		raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +		return np;
> +	}
> +

Special case for the root node? Could use a comment, and of_allnodes
will already point to the root node so this could simply be:

	if (strcmp(path, "/") == 0)
		return of_node_get(np);

Here's a complete patch:

commit adc96db6c39ef7b895e75d30dbc69781f6443f1d
Author: Grant Likely <grant.likely@linaro.org>
Date:   Thu May 22 11:55:31 2014 +0900

    fix trailing '/' case

diff --git a/drivers/of/base.c b/drivers/of/base.c
index c05a143b6a70..45ac44c8dfea 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -790,7 +790,7 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
 	int len = strchrnul(path, '/') - path;
 
 	if (!len)
-		return parent;
+		return NULL;
 
 	__for_each_child_of_node(parent, child) {
 		const char *name = strrchr(child->full_name, '/');
@@ -820,10 +820,13 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
  */
 struct device_node *of_find_node_by_path(const char *path)
 {
-	struct device_node *np = of_allnodes;
+	struct device_node *np = NULL;
 	struct property *pp;
 	unsigned long flags;
 
+	if (strcmp(path, "/") == 0)
+		return of_node_get(of_allnodes);
+
 	/* The path could begin with an alias */
 	if (*path != '/') {
 		char *p = strchrnul(path, '/');
@@ -833,7 +836,6 @@ struct device_node *of_find_node_by_path(const char *path)
 		if (!of_aliases)
 			return NULL;
 
-		np = NULL;
 		for_each_property_of_node(of_aliases, pp) {
 			if (strlen(pp->name) == len && !strncmp(pp->name, path, len)) {
 				np = of_find_node_by_path(pp->value);
@@ -847,6 +849,8 @@ struct device_node *of_find_node_by_path(const char *path)
 
 	/* Step down the tree matching path components */
 	raw_spin_lock_irqsave(&devtree_lock, flags);
+	if (!np)
+		np = of_node_get(of_allnodes);
 	while (np && *path == '/') {
 		path++; /* Increment past '/' delimiter */
 		np = __of_find_node_by_path(np, path);
diff --git a/drivers/of/selftest.c b/drivers/of/selftest.c
index 10900b18fc06..aeb0b5c8b21d 100644
--- a/drivers/of/selftest.c
+++ b/drivers/of/selftest.c
@@ -42,9 +42,7 @@ static void __init of_selftest_find_node_by_name(void)
 
 	/* Test if trailing '/' works */
 	np = of_find_node_by_path("/testcase-data/");
-	selftest(np && !strcmp("/testcase-data", np->full_name),
-		"find /testcase-data/ failed\n");
-	of_node_put(np);
+	selftest(!np, "trailing '/' on /testcase-data/ should fail\n");
 
 	np = of_find_node_by_path("/testcase-data/phandle-tests/consumer-a");
 	selftest(np && !strcmp("/testcase-data/phandle-tests/consumer-a", np->full_name),
@@ -58,9 +56,7 @@ static void __init of_selftest_find_node_by_name(void)
 
 	/* Test if trailing '/' works on aliases */
 	np = of_find_node_by_path("testcase-alias/");
-	selftest(np && !strcmp("/testcase-data", np->full_name),
-		"find testcase-alias/ failed\n");
-	of_node_put(np);
+	selftest(!np, "trailing '/' on testcase-alias/ should fail\n");
 
 	np = of_find_node_by_path("testcase-alias/phandle-tests/consumer-a");
 	selftest(np && !strcmp("/testcase-data/phandle-tests/consumer-a", np->full_name),

Re: [PATCH 2/3] of: Make of_find_node_by_path() handle /aliases

[Frank Rowand]

On 5/21/2014 8:13 PM, Grant Likely wrote:
> On Tue, 20 May 2014 19:46:19 -0700, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On 5/20/2014 7:41 PM, Frank Rowand wrote:
>> < snip >
>>> I will reply to this email with an additional patch that restores the
>>> original behavior.
>> < snip >
>> From: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
>>
>> If __of_find_node_by_path() returns parent when the remaining portion of the
>> path is "/" then the behavior of of_find_node_by_path() has changed.
>>
>> Previously, adding an extraneous "/" on the end of a path would result
>> in of_find_node_by_path() not finding a match.
>>
>> Signed-off-by: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
>> ---
>>  drivers/of/base.c |   13 ++++++++++++-
>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> Index: b/drivers/of/base.c
>> ===================================================================
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -778,7 +778,7 @@ static struct device_node *__of_find_nod
>>  	int len = strchrnul(path, '/') - path;
>>  
>>  	if (!len)
>> -		return parent;
>> +		return NULL;
>>  
>>  	for_each_child_of_node(parent, child) {
>>  		const char *name = strrchr(child->full_name, '/');
>> @@ -813,6 +813,17 @@ struct device_node *of_find_node_by_path
>>  	struct property *pp;
>>  	unsigned long flags;
>>  
>> +	if (strcmp(path, "/") == 0) {
>> +		raw_spin_lock_irqsave(&devtree_lock, flags);
>> +		for (; np; np = np->allnext) {
>> +			if (np->full_name && (of_node_cmp(np->full_name, path) == 0)
>> +			    && of_node_get(np))
>> +				break;
>> +		}
>> +		raw_spin_unlock_irqrestore(&devtree_lock, flags);
>> +		return np;
>> +	}
>> +
> 
> Special case for the root node? Could use a comment, and of_allnodes
> will already point to the root node so this could simply be:
> 
> 	if (strcmp(path, "/") == 0)
> 		return of_node_get(np);

Yes, your version below is better.  I was hoping there was a direct way
of getting the node for "/".  It works on my dragonboard.

The comment of why special case "/" is that for the path "/"
__of_find_node_by_path() will return NULL.

> 
> Here's a complete patch:
> 
> commit adc96db6c39ef7b895e75d30dbc69781f6443f1d
> Author: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Date:   Thu May 22 11:55:31 2014 +0900
> 
>     fix trailing '/' case
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index c05a143b6a70..45ac44c8dfea 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -790,7 +790,7 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
>  	int len = strchrnul(path, '/') - path;
>  
>  	if (!len)
> -		return parent;
> +		return NULL;
>  
>  	__for_each_child_of_node(parent, child) {
>  		const char *name = strrchr(child->full_name, '/');
> @@ -820,10 +820,13 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
>   */
>  struct device_node *of_find_node_by_path(const char *path)
>  {
> -	struct device_node *np = of_allnodes;
> +	struct device_node *np = NULL;
>  	struct property *pp;
>  	unsigned long flags;
>  
> +	if (strcmp(path, "/") == 0)
> +		return of_node_get(of_allnodes);
> +
>  	/* The path could begin with an alias */
>  	if (*path != '/') {
>  		char *p = strchrnul(path, '/');
> @@ -833,7 +836,6 @@ struct device_node *of_find_node_by_path(const char *path)
>  		if (!of_aliases)
>  			return NULL;
>  
> -		np = NULL;
>  		for_each_property_of_node(of_aliases, pp) {
>  			if (strlen(pp->name) == len && !strncmp(pp->name, path, len)) {
>  				np = of_find_node_by_path(pp->value);
> @@ -847,6 +849,8 @@ struct device_node *of_find_node_by_path(const char *path)
>  
>  	/* Step down the tree matching path components */
>  	raw_spin_lock_irqsave(&devtree_lock, flags);
> +	if (!np)
> +		np = of_node_get(of_allnodes);
>  	while (np && *path == '/') {
>  		path++; /* Increment past '/' delimiter */
>  		np = __of_find_node_by_path(np, path);
> diff --git a/drivers/of/selftest.c b/drivers/of/selftest.c
> index 10900b18fc06..aeb0b5c8b21d 100644
> --- a/drivers/of/selftest.c
> +++ b/drivers/of/selftest.c
> @@ -42,9 +42,7 @@ static void __init of_selftest_find_node_by_name(void)
>  
>  	/* Test if trailing '/' works */
>  	np = of_find_node_by_path("/testcase-data/");
> -	selftest(np && !strcmp("/testcase-data", np->full_name),
> -		"find /testcase-data/ failed\n");
> -	of_node_put(np);
> +	selftest(!np, "trailing '/' on /testcase-data/ should fail\n");
>  
>  	np = of_find_node_by_path("/testcase-data/phandle-tests/consumer-a");
>  	selftest(np && !strcmp("/testcase-data/phandle-tests/consumer-a", np->full_name),
> @@ -58,9 +56,7 @@ static void __init of_selftest_find_node_by_name(void)
>  
>  	/* Test if trailing '/' works on aliases */
>  	np = of_find_node_by_path("testcase-alias/");
> -	selftest(np && !strcmp("/testcase-data", np->full_name),
> -		"find testcase-alias/ failed\n");
> -	of_node_put(np);
> +	selftest(!np, "trailing '/' on testcase-alias/ should fail\n");
>  
>  	np = of_find_node_by_path("testcase-alias/phandle-tests/consumer-a");
>  	selftest(np && !strcmp("/testcase-data/phandle-tests/consumer-a", np->full_name),
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] of: Make of_find_node_by_path() handle /aliases

[Grant Likely]

On Tue, 20 May 2014 19:41:22 -0700, Frank Rowand <frowand.list@gmail.com> wrote:
> On 5/18/2014 2:27 AM, Grant Likely wrote:
> > On Fri, 16 May 2014 11:54:44 +0100, Grant Likely <grant.likely@linaro.org> wrote:
> >> On Thu, 15 May 2014 19:51:17 -0700, Frank Rowand <frowand.list@gmail.com> wrote:
> >>> On 5/13/2014 7:58 AM, Grant Likely wrote:
> >>>> Make of_find_node_by_path() handle aliases as prefixes. To make this
> >>>> work the name search is refactored to search by path component instead
> >>>> of by full string. This should be a more efficient search, and it makes
> >>>> it possible to start a search at a subnode of a tree.
> >>>>
> >>>> Signed-off-by: David Daney <david.daney@cavium.com>
> >>>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> >>>> [grant.likely: Rework to not require allocating at runtime]
> >>>> Acked-by: Rob Herring <robh@kernel.org>
> >>>> Signed-off-by: Grant Likely <grant.likely@linaro.org>
> >>>> ---
> >>>>  drivers/of/base.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++----
> >>>>  1 file changed, 56 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/of/base.c b/drivers/of/base.c
> >>>> index 6e240698353b..60089b9a3014 100644
> >>>> --- a/drivers/of/base.c
> >>>> +++ b/drivers/of/base.c
> >>>> @@ -771,9 +771,38 @@ struct device_node *of_get_child_by_name(const struct device_node *node,
> >>>>  }
> >>>>  EXPORT_SYMBOL(of_get_child_by_name);
> >>>>  
> >>>> +static struct device_node *__of_find_node_by_path(struct device_node *parent,
> >>>> +						const char *path)
> >>>> +{
> >>>> +	struct device_node *child;
> >>>> +	int len = strchrnul(path, '/') - path;
> >>>> +
> >>>> +	if (!len)
> >>>> +		return parent;
> >>>
> >>> (!len) is true if the the final character of the path passed into of_find_node_by_path()
> >>> was "/".  Strictly speaking, ->full_name will never end with "/", so the return value
> >>> should be NULL, indicating that the match fails.
> >>
> >> Ah, good catch. I should add a test case for that.
> > 
> > In my testing this looks okay. The while loop that calls into
> > __of_find_node_by_path() looks like this:
> > 
> > 	while (np && *path == '/') {
> > 		path++; /* Increment past '/' delimiter */
> > 		np = __of_find_node_by_path(np, path);
> > 		path = strchrnul(path, '/');
> > 	}
> > 
> > If the path ends with a '/', then the loop will go around one more time.
> > The pointer will be incremented to point at the null character and len
> > will be null because strchrnul() will point at the last item.
> 
> Yes, that was my point.  The old version of of_find_node_by_path() would not
> find a match if the path ended with a "/" (unless the full path was "/").
> This patch series changes the behavior to be a match.
> 
> I will reply to this email with an additional patch that restores the
> original behavior.
> 
> If you move the additional test cases you provide below and the test cases
> in patch 3 to the beginning of the series, you can see the before and after
> behavior of adding patch 1 and patch 2.

Ah, I see. That raises the question about what the behaviour /should/
be. Off the top of my head, matching against a trailing '/' seems to be
okay. Are there situations that you see or can think of where matching
would be the wrong thing to do?

g.

Re: [PATCH 2/3] of: Make of_find_node_by_path() handle /aliases

[Frank Rowand]

On 5/21/2014 6:16 PM, Grant Likely wrote:
> On Tue, 20 May 2014 19:41:22 -0700, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On 5/18/2014 2:27 AM, Grant Likely wrote:
>>> On Fri, 16 May 2014 11:54:44 +0100, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>> On Thu, 15 May 2014 19:51:17 -0700, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>> On 5/13/2014 7:58 AM, Grant Likely wrote:
>>>>>> Make of_find_node_by_path() handle aliases as prefixes. To make this
>>>>>> work the name search is refactored to search by path component instead
>>>>>> of by full string. This should be a more efficient search, and it makes
>>>>>> it possible to start a search at a subnode of a tree.
>>>>>>
>>>>>> Signed-off-by: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
>>>>>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>>>>>> [grant.likely: Rework to not require allocating at runtime]
>>>>>> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>>>>> Signed-off-by: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>>>> ---
>>>>>>  drivers/of/base.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++----
>>>>>>  1 file changed, 56 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>>>>>> index 6e240698353b..60089b9a3014 100644
>>>>>> --- a/drivers/of/base.c
>>>>>> +++ b/drivers/of/base.c
>>>>>> @@ -771,9 +771,38 @@ struct device_node *of_get_child_by_name(const struct device_node *node,
>>>>>>  }
>>>>>>  EXPORT_SYMBOL(of_get_child_by_name);
>>>>>>  
>>>>>> +static struct device_node *__of_find_node_by_path(struct device_node *parent,
>>>>>> +						const char *path)
>>>>>> +{
>>>>>> +	struct device_node *child;
>>>>>> +	int len = strchrnul(path, '/') - path;
>>>>>> +
>>>>>> +	if (!len)
>>>>>> +		return parent;
>>>>>
>>>>> (!len) is true if the the final character of the path passed into of_find_node_by_path()
>>>>> was "/".  Strictly speaking, ->full_name will never end with "/", so the return value
>>>>> should be NULL, indicating that the match fails.
>>>>
>>>> Ah, good catch. I should add a test case for that.
>>>
>>> In my testing this looks okay. The while loop that calls into
>>> __of_find_node_by_path() looks like this:
>>>
>>> 	while (np && *path == '/') {
>>> 		path++; /* Increment past '/' delimiter */
>>> 		np = __of_find_node_by_path(np, path);
>>> 		path = strchrnul(path, '/');
>>> 	}
>>>
>>> If the path ends with a '/', then the loop will go around one more time.
>>> The pointer will be incremented to point at the null character and len
>>> will be null because strchrnul() will point at the last item.
>>
>> Yes, that was my point.  The old version of of_find_node_by_path() would not
>> find a match if the path ended with a "/" (unless the full path was "/").
>> This patch series changes the behavior to be a match.
>>
>> I will reply to this email with an additional patch that restores the
>> original behavior.
>>
>> If you move the additional test cases you provide below and the test cases
>> in patch 3 to the beginning of the series, you can see the before and after
>> behavior of adding patch 1 and patch 2.
> 
> Ah, I see. That raises the question about what the behaviour /should/
> be. Off the top of my head, matching against a trailing '/' seems to be
> okay. Are there situations that you see or can think of where matching
> would be the wrong thing to do?

I have not thought of a case where matching against a trailing '/' would
hurt anything.  It just seemed better to be consistent in naming.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] of: Make of_find_node_by_path() handle /aliases

[Grant Likely]

On Thu, 22 May 2014 18:14:38 -0700, Frank Rowand <frowand.list@gmail.com> wrote:
> On 5/21/2014 6:16 PM, Grant Likely wrote:
> > On Tue, 20 May 2014 19:41:22 -0700, Frank Rowand <frowand.list@gmail.com> wrote:
> >> On 5/18/2014 2:27 AM, Grant Likely wrote:
> >>> On Fri, 16 May 2014 11:54:44 +0100, Grant Likely <grant.likely@linaro.org> wrote:
> >>>> On Thu, 15 May 2014 19:51:17 -0700, Frank Rowand <frowand.list@gmail.com> wrote:
> >>>>> On 5/13/2014 7:58 AM, Grant Likely wrote:
> >>>>>> Make of_find_node_by_path() handle aliases as prefixes. To make this
> >>>>>> work the name search is refactored to search by path component instead
> >>>>>> of by full string. This should be a more efficient search, and it makes
> >>>>>> it possible to start a search at a subnode of a tree.
> >>>>>>
> >>>>>> Signed-off-by: David Daney <david.daney@cavium.com>
> >>>>>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> >>>>>> [grant.likely: Rework to not require allocating at runtime]
> >>>>>> Acked-by: Rob Herring <robh@kernel.org>
> >>>>>> Signed-off-by: Grant Likely <grant.likely@linaro.org>
> >>>>>> ---
> >>>>>>  drivers/of/base.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++----
> >>>>>>  1 file changed, 56 insertions(+), 4 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/of/base.c b/drivers/of/base.c
> >>>>>> index 6e240698353b..60089b9a3014 100644
> >>>>>> --- a/drivers/of/base.c
> >>>>>> +++ b/drivers/of/base.c
> >>>>>> @@ -771,9 +771,38 @@ struct device_node *of_get_child_by_name(const struct device_node *node,
> >>>>>>  }
> >>>>>>  EXPORT_SYMBOL(of_get_child_by_name);
> >>>>>>  
> >>>>>> +static struct device_node *__of_find_node_by_path(struct device_node *parent,
> >>>>>> +						const char *path)
> >>>>>> +{
> >>>>>> +	struct device_node *child;
> >>>>>> +	int len = strchrnul(path, '/') - path;
> >>>>>> +
> >>>>>> +	if (!len)
> >>>>>> +		return parent;
> >>>>>
> >>>>> (!len) is true if the the final character of the path passed into of_find_node_by_path()
> >>>>> was "/".  Strictly speaking, ->full_name will never end with "/", so the return value
> >>>>> should be NULL, indicating that the match fails.
> >>>>
> >>>> Ah, good catch. I should add a test case for that.
> >>>
> >>> In my testing this looks okay. The while loop that calls into
> >>> __of_find_node_by_path() looks like this:
> >>>
> >>> 	while (np && *path == '/') {
> >>> 		path++; /* Increment past '/' delimiter */
> >>> 		np = __of_find_node_by_path(np, path);
> >>> 		path = strchrnul(path, '/');
> >>> 	}
> >>>
> >>> If the path ends with a '/', then the loop will go around one more time.
> >>> The pointer will be incremented to point at the null character and len
> >>> will be null because strchrnul() will point at the last item.
> >>
> >> Yes, that was my point.  The old version of of_find_node_by_path() would not
> >> find a match if the path ended with a "/" (unless the full path was "/").
> >> This patch series changes the behavior to be a match.
> >>
> >> I will reply to this email with an additional patch that restores the
> >> original behavior.
> >>
> >> If you move the additional test cases you provide below and the test cases
> >> in patch 3 to the beginning of the series, you can see the before and after
> >> behavior of adding patch 1 and patch 2.
> > 
> > Ah, I see. That raises the question about what the behaviour /should/
> > be. Off the top of my head, matching against a trailing '/' seems to be
> > okay. Are there situations that you see or can think of where matching
> > would be the wrong thing to do?
> 
> I have not thought of a case where matching against a trailing '/' would
> hurt anything.  It just seemed better to be consistent in naming.
> 

I've gone ahead and merged in the trailing '/' fix. It can be relaxed
later if deemed important.

g.

Re: [PATCH 2/3] of: Make of_find_node_by_path() handle /aliases

[Frank Rowand]

On 5/13/2014 7:58 AM, Grant Likely wrote:

> Make of_find_node_by_path() handle aliases as prefixes. To make this

> work the name search is refactored to search by path component instead

> of by full string. This should be a more efficient search, and it makes

> it possible to start a search at a subnode of a tree.

> 

> Signed-off-by: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>

> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>

> [grant.likely: Rework to not require allocating at runtime]

> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

> Signed-off-by: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

> ---

>  drivers/of/base.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++----

>  1 file changed, 56 insertions(+), 4 deletions(-)



Was this patch created against a tree that has modifications to device tree

locking?
  I get a hang due to deadlock when I apply it.  Patch to verify the
cause is below.




From: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>

Do not apply this patch -- it is not a valid fix.  The point of this patch
is to confirm the cause of a bug.

Problem seen on 3.15-rc1 with dragonboard platform patches added.

The dragonboard hangs on boot the very first time that of_find_node_by_path()
is called:

   of_find_node_by_path()
      raw_spin_lock_irqsave(&devtree_lock, flags)
      np = __of_find_node_by_path(np, path)
         for_each_child_of_node(parent, child) {

            // This is a define:
	    #define for_each_child_of_node(parent, child) \
	       for (child = of_get_next_child(parent, NULL); child != NULL; \

	          // Resulting in a deadlock when of_get_next_child()
		  // attempts to lock the same lock:
	          raw_spin_lock_irqsave(&devtree_lock, flags)

When this patch is applied, the first lock is commented out so the
deadlock does not occur and the boot completes.

Not-signed-off-by: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
---
 drivers/of/base.c |    4 ++++
 1 file changed, 4 insertions(+)

Index: b/drivers/of/base.c
===================================================================
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -835,13 +835,17 @@ struct device_node *of_find_node_by_path
 	}
 
 	/* Step down the tree matching path components */
+#if 0
 	raw_spin_lock_irqsave(&devtree_lock, flags);
+#endif
 	while (np && *path == '/') {
 		path++; /* Increment past '/' delimiter */
 		np = __of_find_node_by_path(np, path);
 		path = strchrnul(path, '/');
 	}
+#if 0
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+#endif
 	return np;
 }
 EXPORT_SYMBOL(of_find_node_by_path);
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] of: Make of_find_node_by_path() handle /aliases

[Grant Likely]

On Tue, 20 May 2014 19:55:45 -0700, Frank Rowand <frowand.list@gmail.com> wrote:
> On 5/13/2014 7:58 AM, Grant Likely wrote:
> 
> > Make of_find_node_by_path() handle aliases as prefixes. To make this
> 
> > work the name search is refactored to search by path component instead
> 
> > of by full string. This should be a more efficient search, and it makes
> 
> > it possible to start a search at a subnode of a tree.
> 
> > 
> 
> > Signed-off-by: David Daney <david.daney@cavium.com>
> 
> > Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> 
> > [grant.likely: Rework to not require allocating at runtime]
> 
> > Acked-by: Rob Herring <robh@kernel.org>
> 
> > Signed-off-by: Grant Likely <grant.likely@linaro.org>
> 
> > ---
> 
> >  drivers/of/base.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++----
> 
> >  1 file changed, 56 insertions(+), 4 deletions(-)
> 
> 
> 
> Was this patch created against a tree that has modifications to device tree
> 
> locking?
>   I get a hang due to deadlock when I apply it.  Patch to verify the
> cause is below.

Ummm... I may have forgotten to enable CONFIG_LOCKDEP when testing. Try
the following fix, and can you please pass me that brown paper bag.

g.

commit 35e9c5ae6c3d0b5eb91579f397d8e1ecb95ee711
Author: Grant Likely <grant.likely@linaro.org>
Date:   Thu May 22 01:04:17 2014 +0900

    dt: Create unlocked version of for_each_child_of_node()
    
    When iterating over nodes, sometimes it needs to be done when the DT
    lock is already held. This patch makes an unlocked version of the
    for_each_child_of_node() macro.
    
    Signed-off-by: Grant Likely <grant.likely@linaro.org>

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 8900d378c07e..c05a143b6a70 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -695,6 +695,22 @@ struct device_node *of_get_next_parent(struct device_node *node)
 }
 EXPORT_SYMBOL(of_get_next_parent);
 
+#define __for_each_child_of_node(parent, child) \
+	for (child = __of_get_next_child(parent, NULL); child != NULL; \
+	     child = __of_get_next_child(parent, child))
+static struct device_node *__of_get_next_child(const struct device_node *node,
+						struct device_node *prev)
+{
+	struct device_node *next;
+
+	next = prev ? prev->sibling : node->child;
+	for (; next; next = next->sibling)
+		if (of_node_get(next))
+			break;
+	of_node_put(prev);
+	return next;
+}
+
 /**
  *	of_get_next_child - Iterate a node childs
  *	@node:	parent node
@@ -710,11 +726,7 @@ struct device_node *of_get_next_child(const struct device_node *node,
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&devtree_lock, flags);
-	next = prev ? prev->sibling : node->child;
-	for (; next; next = next->sibling)
-		if (of_node_get(next))
-			break;
-	of_node_put(prev);
+	next = __of_get_next_child(node, prev);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 	return next;
 }
@@ -780,7 +792,7 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
 	if (!len)
 		return parent;
 
-	for_each_child_of_node(parent, child) {
+	__for_each_child_of_node(parent, child) {
 		const char *name = strrchr(child->full_name, '/');
 		if (WARN(!name, "malformed device_node %s\n", child->full_name))
 			continue;

Re: [PATCH 2/3] of: Make of_find_node_by_path() handle /aliases

[Frank Rowand]

On 5/21/2014 9:09 AM, Grant Likely wrote:
> On Tue, 20 May 2014 19:55:45 -0700, Frank Rowand <frowand.list@gmail.com> wrote:
>> On 5/13/2014 7:58 AM, Grant Likely wrote:
>>
>>> Make of_find_node_by_path() handle aliases as prefixes. To make this

< snip >

>>> ---
>>
>>>  drivers/of/base.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++----
>>
>>>  1 file changed, 56 insertions(+), 4 deletions(-)
>>
>>
>>
>> Was this patch created against a tree that has modifications to device tree
>>
>> locking?
>>   I get a hang due to deadlock when I apply it.  Patch to verify the
>> cause is below.
> 
> Ummm... I may have forgotten to enable CONFIG_LOCKDEP when testing. Try
> the following fix, and can you please pass me that brown paper bag.

Nononono, I need to keep the bag for my own future use :-)

The following fix looks good and boots successfully on the dragonboard.

-Frank

> 
> g.
> 
> commit 35e9c5ae6c3d0b5eb91579f397d8e1ecb95ee711
> Author: Grant Likely <grant.likely@linaro.org>
> Date:   Thu May 22 01:04:17 2014 +0900
> 
>     dt: Create unlocked version of for_each_child_of_node()
>     
>     When iterating over nodes, sometimes it needs to be done when the DT
>     lock is already held. This patch makes an unlocked version of the
>     for_each_child_of_node() macro.
>     
>     Signed-off-by: Grant Likely <grant.likely@linaro.org>

< snip >

[PATCH 3/3] of: Add a testcase for of_find_node_by_path()

[Grant Likely]

Add a testcase for the find_node_by_path() function to make sure it
handles all the valid scenarios.

Signed-off-by: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/of/selftest.c                       | 39 +++++++++++++++++++++++++++++
 drivers/of/testcase-data/tests-phandle.dtsi |  6 ++++-
 2 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/drivers/of/selftest.c b/drivers/of/selftest.c
index ae4450070503..f5b4dcffbe32 100644
--- a/drivers/of/selftest.c
+++ b/drivers/of/selftest.c
@@ -30,6 +30,43 @@ static struct selftest_results {
 	} \
 }
 
+static void __init of_selftest_find_node_by_name(void)
+{
+	struct device_node *np;
+
+	np = of_find_node_by_path("/testcase-data");
+	selftest(np && !strcmp("/testcase-data", np->full_name),
+		"find /testcase-data failed\n");
+	of_node_put(np);
+
+	np = of_find_node_by_path("/testcase-data/phandle-tests/consumer-a");
+	selftest(np && !strcmp("/testcase-data/phandle-tests/consumer-a", np->full_name),
+		"find /testcase-data/phandle-tests/consumer-a failed\n");
+	of_node_put(np);
+
+	np = of_find_node_by_path("testcase-alias");
+	selftest(np && !strcmp("/testcase-data", np->full_name),
+		"find testcase-alias failed\n");
+	of_node_put(np);
+
+	np = of_find_node_by_path("testcase-alias/phandle-tests/consumer-a");
+	selftest(np && !strcmp("/testcase-data/phandle-tests/consumer-a", np->full_name),
+		"find testcase-alias/phandle-tests/consumer-a failed\n");
+	of_node_put(np);
+
+	np = of_find_node_by_path("/testcase-data/missing-path");
+	selftest(!np, "non-existent path returned node %s\n", np->full_name);
+	of_node_put(np);
+
+	np = of_find_node_by_path("missing-alias");
+	selftest(!np, "non-existent alias returned node %s\n", np->full_name);
+	of_node_put(np);
+
+	np = of_find_node_by_path("testcase-alias/missing-path");
+	selftest(!np, "non-existent alias with relative path returned node %s\n", np->full_name);
+	of_node_put(np);
+}
+
 static void __init of_selftest_dynamic(void)
 {
 	struct device_node *np;
@@ -89,6 +126,7 @@ static void __init of_selftest_dynamic(void)
 	if (prop->value)
 		selftest(of_add_property(np, prop) == 0,
 			 "Adding a large property should have passed\n");
+
 }
 
 static void __init of_selftest_parse_phandle_with_args(void)
@@ -439,6 +477,7 @@ static int __init of_selftest(void)
 	of_node_put(np);
 
 	pr_info("start of selftest - you will see error messages\n");
+	of_selftest_find_node_by_name();
 	of_selftest_dynamic();
 	of_selftest_parse_phandle_with_args();
 	of_selftest_property_match_string();
diff --git a/drivers/of/testcase-data/tests-phandle.dtsi b/drivers/of/testcase-data/tests-phandle.dtsi
index 788a4c24b8f5..ce0fe083d406 100644
--- a/drivers/of/testcase-data/tests-phandle.dtsi
+++ b/drivers/of/testcase-data/tests-phandle.dtsi
@@ -1,6 +1,10 @@
 
 / {
-	testcase-data {
+	aliases {
+		testcase-alias = &testcase;
+	};
+
+	testcase: testcase-data {
 		security-password = "password";
 		duplicate-name = "duplicate";
 		duplicate-name { };
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html