diff mbox

[v3,2/3] of: add optional options parameter to of_find_node_by_path()

Message ID 20141128113428.GM2361@bivouac.eciton.net (mailing list archive)
State New, archived
Headers show

Commit Message

Leif Lindholm Nov. 28, 2014, 11:34 a.m. UTC 
On Fri, Nov 28, 2014 at 12:44:03AM +0000, Grant Likely wrote:
> On Thu, 27 Nov 2014 17:56:06 +0000
> , Leif Lindholm <leif.lindholm@linaro.org>
>  wrote:
> > Update of_find_node_by_path():
> > 1) Rename function to of_find_node_opts_by_path(), adding an optional
> >    pointer argument. Provide a static inline wrapper version of
> >    of_find_node_by_path() which calls the new function with NULL as
> >    the optional argument.
> > 2) Ignore any part of the path beyond and including the ':' separator.
> > 3) Set the new provided pointer argument to the beginning of the string
> >    following the ':' separator.
> > 4: Add tests.
> > 
> > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > ---
> >  drivers/of/base.c     |   21 +++++++++++++++++----
> >  drivers/of/selftest.c |   12 ++++++++++++
> >  include/linux/of.h    |   14 +++++++++++++-
> >  3 files changed, 42 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index 3823edf..7f0e5f7 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -699,10 +699,15 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
> >  {
> >  	struct device_node *child;
> >  	int len = strchrnul(path, '/') - path;
> > +	int term;
> >  
> >  	if (!len)
> >  		return NULL;
> >  
> > +	term = strchrnul(path, ':') - path;
> > +	if (term < len)
> > +		len = term;
> > +
> >  	__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))
> > @@ -715,11 +720,14 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
> >  }
> >  
> >  /**
> > - *	of_find_node_by_path - Find a node matching a full OF path
> > + *	of_find_node_opts_by_path - Find a node matching a full OF path
> >   *	@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.
> > + *	@opts: Address of a pointer into which to store the start of
> > + *	       an options string appended to the end of the path with
> > + *	       a ':' separator.
> >   *
> >   *	Valid paths:
> >   *		/foo/bar	Full path
> > @@ -729,19 +737,24 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
> >   *	Returns a node pointer with refcount incremented, use
> >   *	of_node_put() on it when done.
> >   */
> > -struct device_node *of_find_node_by_path(const char *path)
> > +struct device_node *of_find_node_opts_by_path(const char *path, const char **opts)
> >  {
> >  	struct device_node *np = NULL;
> >  	struct property *pp;
> >  	unsigned long flags;
> > +	char *separator;
> >  
> >  	if (strcmp(path, "/") == 0)
> >  		return of_node_get(of_allnodes);
> >  
> > +	separator = strchr(path, ':');
> > +	if (separator && opts)
> > +		*opts = separator + 1;
> > +
> 
> What about when there are no opts? Do we require the caller to make sure
> opts is NULL before calling the function (which sounds like a good
> source of bugs) or do we clear it on successful return?
> 
> I think if opts is passed in, but there are no options, then it should
> set *opts = NULL.

Yeah, oops.

> There should be test cases for this also. Must set opts to NULL on
> successful return, and (I think) should leave opts alone on an
> unsuccessful search.

I would actually argue for always nuking the opts - since that could
(theoretically) prevent something working by accident in spite of
error conditions.

How about the below?

/
    Leif

From 2e1a44e539967d96366d074ae158092900e0c822 Mon Sep 17 00:00:00 2001
From: Leif Lindholm <leif.lindholm@linaro.org>
Date: Thu, 27 Nov 2014 09:24:31 +0000
Subject: [PATCH] of: add optional options parameter to of_find_node_by_path()

Update of_find_node_by_path():
1) Rename function to of_find_node_opts_by_path(), adding an optional
   pointer argument. Provide a static inline wrapper version of
   of_find_node_by_path() which calls the new function with NULL as
   the optional argument.
2) Ignore any part of the path beyond and including the ':' separator.
3) Set the new provided pointer argument to the beginning of the string
   following the ':' separator.
4: Add tests.

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 drivers/of/base.c     |   21 +++++++++++++++++----
 drivers/of/selftest.c |   17 +++++++++++++++++
 include/linux/of.h    |   14 +++++++++++++-
 3 files changed, 47 insertions(+), 5 deletions(-)

Comments

Grant Likely Nov. 28, 2014, 3:25 p.m. UTC | #1 
On Fri, 28 Nov 2014 11:34:28 +0000
, Leif Lindholm <leif.lindholm@linaro.org>
 wrote:
> On Fri, Nov 28, 2014 at 12:44:03AM +0000, Grant Likely wrote:
> > > +	separator = strchr(path, ':');
> > > +	if (separator && opts)
> > > +		*opts = separator + 1;
> > > +
> > 
> > What about when there are no opts? Do we require the caller to make sure
> > opts is NULL before calling the function (which sounds like a good
> > source of bugs) or do we clear it on successful return?
> > 
> > I think if opts is passed in, but there are no options, then it should
> > set *opts = NULL.
> 
> Yeah, oops.
> 
> > There should be test cases for this also. Must set opts to NULL on
> > successful return, and (I think) should leave opts alone on an
> > unsuccessful search.
> 
> I would actually argue for always nuking the opts - since that could
> (theoretically) prevent something working by accident in spite of
> error conditions.
> 
> How about the below?

Perfect, applied with one fixup below...

> 
> /
>     Leif
> 
> From 2e1a44e539967d96366d074ae158092900e0c822 Mon Sep 17 00:00:00 2001
> From: Leif Lindholm <leif.lindholm@linaro.org>
> Date: Thu, 27 Nov 2014 09:24:31 +0000
> Subject: [PATCH] of: add optional options parameter to of_find_node_by_path()
> 
> Update of_find_node_by_path():
> 1) Rename function to of_find_node_opts_by_path(), adding an optional
>    pointer argument. Provide a static inline wrapper version of
>    of_find_node_by_path() which calls the new function with NULL as
>    the optional argument.
> 2) Ignore any part of the path beyond and including the ':' separator.
> 3) Set the new provided pointer argument to the beginning of the string
>    following the ':' separator.
> 4: Add tests.
> 
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
> @@ -729,19 +737,24 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
>   *	Returns a node pointer with refcount incremented, use
>   *	of_node_put() on it when done.
>   */
> -struct device_node *of_find_node_by_path(const char *path)
> +struct device_node *of_find_node_opts_by_path(const char *path, const char **opts)
>  {
>  	struct device_node *np = NULL;
>  	struct property *pp;
>  	unsigned long flags;
> +	char *separator;

const char *separator.

Thanks,
g.
Grant Likely Nov. 28, 2014, 3:33 p.m. UTC | #2 
On Fri, Nov 28, 2014 at 3:25 PM, Grant Likely <grant.likely@linaro.org> wrote:
> On Fri, 28 Nov 2014 11:34:28 +0000
> , Leif Lindholm <leif.lindholm@linaro.org>
>  wrote:
>> On Fri, Nov 28, 2014 at 12:44:03AM +0000, Grant Likely wrote:
>> > > + separator = strchr(path, ':');
>> > > + if (separator && opts)
>> > > +         *opts = separator + 1;
>> > > +
>> >
>> > What about when there are no opts? Do we require the caller to make sure
>> > opts is NULL before calling the function (which sounds like a good
>> > source of bugs) or do we clear it on successful return?
>> >
>> > I think if opts is passed in, but there are no options, then it should
>> > set *opts = NULL.
>>
>> Yeah, oops.
>>
>> > There should be test cases for this also. Must set opts to NULL on
>> > successful return, and (I think) should leave opts alone on an
>> > unsuccessful search.
>>
>> I would actually argue for always nuking the opts - since that could
>> (theoretically) prevent something working by accident in spite of
>> error conditions.
>>
>> How about the below?
>
> Perfect, applied with one fixup below...

And by the way, let me say well done on this patch. It is elegantly
implemented within the framework already there.

g.
diff mbox

Patch

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 3823edf..99f0fd9 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -699,10 +699,15 @@  static struct device_node *__of_find_node_by_path(struct device_node *parent,
 {
 	struct device_node *child;
 	int len = strchrnul(path, '/') - path;
+	int term;
 
 	if (!len)
 		return NULL;
 
+	term = strchrnul(path, ':') - path;
+	if (term < len)
+		len = term;
+
 	__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))
@@ -715,11 +720,14 @@  static struct device_node *__of_find_node_by_path(struct device_node *parent,
 }
 
 /**
- *	of_find_node_by_path - Find a node matching a full OF path
+ *	of_find_node_opts_by_path - Find a node matching a full OF path
  *	@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.
+ *	@opts: Address of a pointer into which to store the start of
+ *	       an options string appended to the end of the path with
+ *	       a ':' separator.
  *
  *	Valid paths:
  *		/foo/bar	Full path
@@ -729,19 +737,24 @@  static struct device_node *__of_find_node_by_path(struct device_node *parent,
  *	Returns a node pointer with refcount incremented, use
  *	of_node_put() on it when done.
  */
-struct device_node *of_find_node_by_path(const char *path)
+struct device_node *of_find_node_opts_by_path(const char *path, const char **opts)
 {
 	struct device_node *np = NULL;
 	struct property *pp;
 	unsigned long flags;
+	char *separator;
 
 	if (strcmp(path, "/") == 0)
 		return of_node_get(of_allnodes);
 
+	separator = strchr(path, ':');
+	if (opts)
+		*opts = separator ? separator + 1 : NULL;
+
 	/* The path could begin with an alias */
 	if (*path != '/') {
 		char *p = strchrnul(path, '/');
-		int len = p - path;
+		int len = separator ? separator - path : p - path;
 
 		/* of_aliases must not be NULL */
 		if (!of_aliases)
@@ -770,7 +783,7 @@  struct device_node *of_find_node_by_path(const char *path)
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 	return np;
 }
-EXPORT_SYMBOL(of_find_node_by_path);
+EXPORT_SYMBOL(of_find_node_opts_by_path);
 
 /**
  *	of_find_node_by_name - Find a node by its "name" property
diff --git a/drivers/of/selftest.c b/drivers/of/selftest.c
index e2d79af..721b2a4 100644
--- a/drivers/of/selftest.c
+++ b/drivers/of/selftest.c
@@ -43,6 +43,7 @@  static bool selftest_live_tree;
 static void __init of_selftest_find_node_by_name(void)
 {
 	struct device_node *np;
+	const char *options;
 
 	np = of_find_node_by_path("/testcase-data");
 	selftest(np && !strcmp("/testcase-data", np->full_name),
@@ -83,6 +84,22 @@  static void __init of_selftest_find_node_by_name(void)
 	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);
+
+	np = of_find_node_opts_by_path("/testcase-data:testoption", &options);
+	selftest(np && !strcmp("testoption", options),
+		 "option path test failed\n");
+	of_node_put(np);
+
+	np = of_find_node_opts_by_path("testcase-alias:testaliasoption",
+				       &options);
+	selftest(np && !strcmp("testaliasoption", options),
+		 "option alias path test failed\n");
+	of_node_put(np);
+
+	options = "testoption";
+	np = of_find_node_opts_by_path("testcase-alias", &options);
+	selftest(np && !options, "option clearing test failed\n");
+	of_node_put(np);
 }
 
 static void __init of_selftest_dynamic(void)
diff --git a/include/linux/of.h b/include/linux/of.h
index 29f0adc..a36be70 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -228,7 +228,13 @@  extern struct device_node *of_find_matching_node_and_match(
 	const struct of_device_id *matches,
 	const struct of_device_id **match);
 
-extern struct device_node *of_find_node_by_path(const char *path);
+extern struct device_node *of_find_node_opts_by_path(const char *path,
+	const char **opts);
+static inline struct device_node *of_find_node_by_path(const char *path)
+{
+	return of_find_node_opts_by_path(path, NULL);
+}
+
 extern struct device_node *of_find_node_by_phandle(phandle handle);
 extern struct device_node *of_get_parent(const struct device_node *node);
 extern struct device_node *of_get_next_parent(struct device_node *node);
@@ -385,6 +391,12 @@  static inline struct device_node *of_find_node_by_path(const char *path)
 	return NULL;
 }
 
+static inline struct device_node *of_find_node_opts_by_path(const char *path,
+	const char **opts)
+{
+	return NULL;
+}
+
 static inline struct device_node *of_get_parent(const struct device_node *node)
 {
 	return NULL;