of: fix handling of '/' in path options

PATCH In review Leif Lindholm 4 participants 10 messages Mar 9, 2015 – Mar 22, 2015

Subsystems (2)

Stable branch Device tree

lore

Share & tools

lore

Leif Lindholm

author

2015-03-09 18:02 UTC #0 lore raw

cover of: fix handling of '/' in path options

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

Signature · 1 line

2.1.4

Leif Lindholm

author

2015-03-09 18:02 UTC #1 lore raw

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

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 <[email protected]>

Signed-off-by Leif Lindholm <[email protected]>

---
 drivers/of/base.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

drivers/of/base.c +15 -8

index 0a8aeb8..8b904e5 100644
@@ -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;

Signature · 1 line

2.1.4

Brian Norris

2015-03-17 19:30 UTC #2 lore raw

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

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 <[email protected]>

Cc [email protected]

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

drivers/of/base.c +1 -6

index adb8764861c0..966d6fdcf427 100644
@@ -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;

Signature · 1 line

1.9.1

Brian Norris

2015-03-17 19:30 UTC #3 lore raw

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

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 <[email protected]>

---
 drivers/of/unittest.c | 5 +++++
 1 file changed, 5 insertions(+)

drivers/of/unittest.c +5 -0

index aba8946cac46..52c45c7df07f 100644
@@ -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);

Signature · 1 line

1.9.1

Leif Lindholm

author Acked-by on v1 2/2

2015-03-17 21:12 UTC #4 lore raw

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

Quoted 30 lines · 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 <[email protected]>
---
 drivers/of/unittest.c | 5 +++++
 1 file changed, 5 insertions(+)
drivers/of/unittest.c +5 -0
index aba8946cac46..52c45c7df07f 100644
@@ -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);
Signature · 1 line
1.9.1

Acked-by Leif Lindholm <[email protected]>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Leif Lindholm

author Acked-by on v1 1/2

2015-03-17 21:11 UTC #5 lore raw

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

In reply to Brian Norris

Quoted 46 lines · 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 <[email protected]>
Cc [email protected]
---
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(-)
drivers/of/base.c +1 -6
index adb8764861c0..966d6fdcf427 100644
@@ -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;
 
Signature · 1 line
1.9.1

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

Acked-by Leif Lindholm <[email protected]>

Rob Herring

maintainer

2015-03-22 20:04 UTC #6 lore raw

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

In reply to Brian Norris

Quoted 13 lines · 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.

Quoted 5 lines

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 <[email protected]>
Cc [email protected]

Thanks. Applied both and in Linus' tree now.

Rob

Leif Lindholm

author

2015-03-09 18:02 UTC #7 lore raw

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

In reply to Leif Lindholm

From: Peter Hurley <[email protected]>

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

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/of/unittest.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

drivers/of/unittest.c +11 -0

index 0cf9a23..b2d7547 100644
@@ -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);

Signature · 1 line

2.1.4

Rob Herring

2015-03-11 12:49 UTC #8 lore raw

In reply to Leif Lindholm

Quoted 11 lines · 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

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

Rob

Quoted 8 lines

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

--
2.1.4

Leif Lindholm

author

2015-03-11 13:00 UTC #9 lore raw

Quoted 7 lines · Rob Herring

Quoted 6 lines · 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.

Quoted 7 lines

Quoted 5 lines
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