[PATCH 0/2] of: fix handling of '/' in path options

[PATCH 0/2] of: fix handling of '/' in path options

[Leif Lindholm]

Commit 7914a7c5651a ("of: support passing console options with
stdout-path") neglected to deal with '/'s appearing past the ':'
terminator.

This mini-series fixes this oversight and adds the tests to prove it.

Leif Lindholm (1):
  of: fix handling of '/' in options for of_find_node_by_path()

Peter Hurley (1):
  of: unittest: Add options string testcase variants

 drivers/of/base.c     | 23 +++++++++++++++--------
 drivers/of/unittest.c | 11 +++++++++++
 2 files changed, 26 insertions(+), 8 deletions(-)

-- 
2.1.4

--
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

[PATCH 1/2] of: fix handling of '/' in options for of_find_node_by_path()

[Leif Lindholm]

Ensure proper handling of paths with appended options (after ':'),
where those options may contain a '/'.

Fixes: 7914a7c5651a ("of: support passing console options with stdout-path")
Reported-by: Peter Hurley <peter@hurleysoftware.com>
Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 drivers/of/base.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 0a8aeb8..8b904e5 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -714,16 +714,17 @@ 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;
-	int term;
+	int len;
+	const char *end;
 
+	end = strchr(path, ':');
+	if (!end)
+		end = strchrnul(path, '/');
+
+	len = end - path;
 	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))
@@ -768,8 +769,12 @@ struct device_node *of_find_node_opts_by_path(const char *path, const char **opt
 
 	/* The path could begin with an alias */
 	if (*path != '/') {
-		char *p = strchrnul(path, '/');
-		int len = separator ? separator - path : p - path;
+		int len;
+		const char *p = separator;
+
+		if (!p)
+			p = strchrnul(path, '/');
+		len = p - path;
 
 		/* of_aliases must not be NULL */
 		if (!of_aliases)
@@ -794,6 +799,8 @@ struct device_node *of_find_node_opts_by_path(const char *path, const char **opt
 		path++; /* Increment past '/' delimiter */
 		np = __of_find_node_by_path(np, path);
 		path = strchrnul(path, '/');
+		if (separator && separator < path)
+			break;
 	}
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 	return np;
-- 
2.1.4

[PATCH 1/2] of: handle both '/' and ':' in path strings

[Brian Norris]

Commit 106937e8ccdc ("of: fix handling of '/' in options for
of_find_node_by_path()") caused a regression in OF handling of
stdout-path. While it fixes some cases which have '/' after the ':', it
breaks cases where there is more than one '/' *before* the ':'.

For example, it breaks this boot string

  stdout-path = "/rdb/serial@f040ab00:115200";

So rather than doing sequentialized checks (first for '/', then for ':';
or vice versa), to get the correct behavior we need to check for the
first occurrence of either one of them.

It so happens that the handy strcspn() helper can do just that.

Fixes: 106937e8ccdc ("of: fix handling of '/' in options for of_find_node_by_path()")
Signed-off-by: Brian Norris <computersforpeace@gmail.com>
Cc: stable@vger.kernel.org
---
This is for -stable only because the regression is marked for stable. Not sure
the first one deserves to go to -stable, actually...

 drivers/of/base.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index adb8764861c0..966d6fdcf427 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -715,13 +715,8 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
 {
 	struct device_node *child;
 	int len;
-	const char *end;
 
-	end = strchr(path, ':');
-	if (!end)
-		end = strchrnul(path, '/');
-
-	len = end - path;
+	len = strcspn(path, "/:");
 	if (!len)
 		return NULL;
 
-- 
1.9.1

[PATCH 2/2] of: unittest: Add option string test case with longer path

[Brian Norris]

There were regressions seen with commit 106937e8ccdc ("of: fix handling
of '/' in options for of_find_node_by_path()"), where we couldn't handle
extra '/' before the ':'. Let's test for this now.

Confirmed that this test fails without the previous patch and passes
when patched. All other tests pass.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/of/unittest.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index aba8946cac46..52c45c7df07f 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -97,6 +97,11 @@ static void __init of_selftest_find_node_by_name(void)
 		 "option path test, subcase #1 failed\n");
 	of_node_put(np);
 
+	np = of_find_node_opts_by_path("/testcase-data/testcase-device1:test/option", &options);
+	selftest(np && !strcmp("test/option", options),
+		 "option path test, subcase #2 failed\n");
+	of_node_put(np);
+
 	np = of_find_node_opts_by_path("/testcase-data:testoption", NULL);
 	selftest(np, "NULL option path test failed\n");
 	of_node_put(np);
-- 
1.9.1

Re: [PATCH 2/2] of: unittest: Add option string test case with longer path

[Leif Lindholm]

On Tue, Mar 17, 2015 at 12:30:32PM -0700, Brian Norris wrote:
> There were regressions seen with commit 106937e8ccdc ("of: fix handling
> of '/' in options for of_find_node_by_path()"), where we couldn't handle
> extra '/' before the ':'. Let's test for this now.
> 
> Confirmed that this test fails without the previous patch and passes
> when patched. All other tests pass.
> 
> Signed-off-by: Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/of/unittest.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> index aba8946cac46..52c45c7df07f 100644
> --- a/drivers/of/unittest.c
> +++ b/drivers/of/unittest.c
> @@ -97,6 +97,11 @@ static void __init of_selftest_find_node_by_name(void)
>  		 "option path test, subcase #1 failed\n");
>  	of_node_put(np);
>  
> +	np = of_find_node_opts_by_path("/testcase-data/testcase-device1:test/option", &options);
> +	selftest(np && !strcmp("test/option", options),
> +		 "option path test, subcase #2 failed\n");
> +	of_node_put(np);
> +
>  	np = of_find_node_opts_by_path("/testcase-data:testoption", NULL);
>  	selftest(np, "NULL option path test failed\n");
>  	of_node_put(np);
> -- 
> 1.9.1

Acked-by: Leif Lindholm <leif.lindholm-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
--
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 1/2] of: handle both '/' and ':' in path strings

[Leif Lindholm]

On Tue, Mar 17, 2015 at 12:30:31PM -0700, Brian Norris wrote:
> Commit 106937e8ccdc ("of: fix handling of '/' in options for
> of_find_node_by_path()") caused a regression in OF handling of
> stdout-path. While it fixes some cases which have '/' after the ':', it
> breaks cases where there is more than one '/' *before* the ':'.
> 
> For example, it breaks this boot string
> 
>   stdout-path = "/rdb/serial@f040ab00:115200";
> 
> So rather than doing sequentialized checks (first for '/', then for ':';
> or vice versa), to get the correct behavior we need to check for the
> first occurrence of either one of them.
> 
> It so happens that the handy strcspn() helper can do just that.
> 
> Fixes: 106937e8ccdc ("of: fix handling of '/' in options for of_find_node_by_path()")
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> Cc: stable@vger.kernel.org
> ---
> This is for -stable only because the regression is marked for stable. Not sure
> the first one deserves to go to -stable, actually...
> 
>  drivers/of/base.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index adb8764861c0..966d6fdcf427 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -715,13 +715,8 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,
>  {
>  	struct device_node *child;
>  	int len;
> -	const char *end;
>  
> -	end = strchr(path, ':');
> -	if (!end)
> -		end = strchrnul(path, '/');
> -
> -	len = end - path;
> +	len = strcspn(path, "/:");
>  	if (!len)
>  		return NULL;
>  
> -- 
> 1.9.1

Yeah, that's neater that the fix I sent out earlier today.

Acked-by: Leif Lindholm <leif.lindholm@linaro.org>

Re: [PATCH 1/2] of: handle both '/' and ':' in path strings

[Rob Herring]

On Tue, Mar 17, 2015 at 2:30 PM, Brian Norris
<computersforpeace@gmail.com> wrote:
> Commit 106937e8ccdc ("of: fix handling of '/' in options for
> of_find_node_by_path()") caused a regression in OF handling of
> stdout-path. While it fixes some cases which have '/' after the ':', it
> breaks cases where there is more than one '/' *before* the ':'.
>
> For example, it breaks this boot string
>
>   stdout-path = "/rdb/serial@f040ab00:115200";
>
> So rather than doing sequentialized checks (first for '/', then for ':';
> or vice versa), to get the correct behavior we need to check for the
> first occurrence of either one of them.
>
> It so happens that the handy strcspn() helper can do just that.
>
> Fixes: 106937e8ccdc ("of: fix handling of '/' in options for of_find_node_by_path()")
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> Cc: stable@vger.kernel.org

Thanks. Applied both and in Linus' tree now.

Rob

[PATCH 2/2] of: unittest: Add options string testcase variants

[Leif Lindholm]

From: Peter Hurley <peter@hurleysoftware.com>

Add testcase variants with '/' in the options string to test for
scan beyond end path name terminated by ':'.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/of/unittest.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 0cf9a23..b2d7547 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -92,6 +92,11 @@ static void __init of_selftest_find_node_by_name(void)
 		 "option path test failed\n");
 	of_node_put(np);
 
+	np = of_find_node_opts_by_path("/testcase-data:test/option", &options);
+	selftest(np && !strcmp("test/option", options),
+		 "option path test, subcase #1 failed\n");
+	of_node_put(np);
+
 	np = of_find_node_opts_by_path("/testcase-data:testoption", NULL);
 	selftest(np, "NULL option path test failed\n");
 	of_node_put(np);
@@ -102,6 +107,12 @@ static void __init of_selftest_find_node_by_name(void)
 		 "option alias path test failed\n");
 	of_node_put(np);
 
+	np = of_find_node_opts_by_path("testcase-alias:test/alias/option",
+				       &options);
+	selftest(np && !strcmp("test/alias/option", options),
+		 "option alias path test, subcase #1 failed\n");
+	of_node_put(np);
+
 	np = of_find_node_opts_by_path("testcase-alias:testaliasoption", NULL);
 	selftest(np, "NULL option alias path test failed\n");
 	of_node_put(np);
-- 
2.1.4

Re: [PATCH 0/2] of: fix handling of '/' in path options

[Rob Herring]

On Mon, Mar 9, 2015 at 1:03 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> Commit 7914a7c5651a ("of: support passing console options with
> stdout-path") neglected to deal with '/'s appearing past the ':'
> terminator.
>
> This mini-series fixes this oversight and adds the tests to prove it.
>
> Leif Lindholm (1):
>   of: fix handling of '/' in options for of_find_node_by_path()
>
> Peter Hurley (1):
>   of: unittest: Add options string testcase variants

Are there changes from the previous versions? I've already pulled those in.

Rob

>
>  drivers/of/base.c     | 23 +++++++++++++++--------
>  drivers/of/unittest.c | 11 +++++++++++
>  2 files changed, 26 insertions(+), 8 deletions(-)
>
> --
> 2.1.4
>

Re: [PATCH 0/2] of: fix handling of '/' in path options

[Leif Lindholm]

On Wed, Mar 11, 2015 at 07:49:33AM -0500, Rob Herring wrote:
> On Mon, Mar 9, 2015 at 1:03 PM, Leif Lindholm <leif.lindholm-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > Commit 7914a7c5651a ("of: support passing console options with
> > stdout-path") neglected to deal with '/'s appearing past the ':'
> > terminator.
> >
> > This mini-series fixes this oversight and adds the tests to prove it.
> >
> > Leif Lindholm (1):
> >   of: fix handling of '/' in options for of_find_node_by_path()
> >
> > Peter Hurley (1):
> >   of: unittest: Add options string testcase variants
> 
> Are there changes from the previous versions? I've already pulled those in.

No, just sent together as a series since I didn't realise you had.

/
    Leif
--
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