diff mbox

[REGRESSION,stable] "of: fix handling of '/' in options for of_find_node_by_path()" breaks stdout-path

Message ID 20150316181957.GJ4278@bivouac.eciton.net
State New
Headers show

Commit Message

Leif Lindholm March 16, 2015, 6:19 p.m. UTC 
Hi Hans,

On Mon, Mar 16, 2015 at 04:53:22PM +0100, Hans de Goede wrote:
> While updating my local working tree to 4.0-rc4 this morning I noticed that I no longer
> got any console (neither video output not serial console) on an Allwinner A20 ARM
> board.
> 
> This is caused by:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/of?id=106937e8ccdcf0f4b95fbf0fe9abd42766cade33
> 
> Reverting this commit fixes the serial console being gone for me.

Sorry about that.
 
> After this there still is an issue that tty0 no longer is seen as console, where it
> used to be properly used as console in 3.19, I'll investigate that further and send
> a separate mail about this.
> 
> Greg, this commit has a: "Cc: <stable@vger.kernel.org> # 3.19" please do not apply
> this commit to stable!
> 
> u-boot sets stdout-path to this value on the board in question:
> "/soc@01c00000/serial@01c28000:115200"
> 
> Looking at the first hunk of the patch in question, the problem is obvious:
> 
> @@ -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))
> 
> The new code to determine len will match (when starting at root) the name of
> all child nodes against: "soc@01c00000/serial@01c28000" as it checks for
> the ":" first and then uses everything before it. Where as the old code
> would match against: "soc@01c00000" which is the correct thing to do.
> 
> The best fix I can come up with is to check for both ":" and "/" and use
> the earlier one as end to calculate the length. I've not coded this out /
> tested this due to -ENOTIME. Note that I've also not audited the rest of
> the patch for similar issues.
> 
> I will happily test any patches to fix this.

Can you give this one a try for the first part of your problem?

And if you're happy with that, I can revert the previous version and
send a new combined fix.

Rob/Grant: am I OK to assume the existence of the phandle-tests
subnode for my unrelated test?

/
    Leif

From cbb150ddd277e5fe1c109e6a675f075f0611f71d Mon Sep 17 00:00:00 2001
From: Leif Lindholm <leif.lindholm@linaro.org>
Date: Mon, 16 Mar 2015 17:58:22 +0000
Subject: [PATCH] of: fix regression in of_find_node_opts_by_path()

This fixes a regression for dealing with paths that contain both a ':'
option separator and a '/' in the path preceding it. And adds a test
case to prove it.

Fixes: 106937e8ccdc ("of: fix handling of '/' in options for of_find_node_by_path()")
Reported-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 drivers/of/base.c     | 10 ++++------
 drivers/of/unittest.c |  5 +++++
 2 files changed, 9 insertions(+), 6 deletions(-)
diff mbox

Patch

diff --git a/drivers/of/base.c b/drivers/of/base.c
index adb8764..2ee7265 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -715,13 +715,11 @@  static struct device_node *__of_find_node_by_path(struct device_node *parent,
 {
 	struct device_node *child;
 	int len;
-	const char *end;
+	const char *p1, *p2;
 
-	end = strchr(path, ':');
-	if (!end)
-		end = strchrnul(path, '/');
-
-	len = end - path;
+	p1 = strchrnul(path, ':');
+	p2 = strchrnul(path, '/');
+	len = (p1 < p2 ? p1 : p2) - path;
 	if (!len)
 		return NULL;
 
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index aba8946..8d94349 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/phandle-tests: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);