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

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

[Hans de Goede]

Hi All,

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.

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.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.

Regards,

Hans
--
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: [REGRESSION stable] "of: fix handling of '/' in options for of_find_node_by_path()" breaks stdout-path

[Leif Lindholm]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Leif Lindholm <leif.lindholm-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/of/base.c     | 10 ++++------
 drivers/of/unittest.c |  5 +++++
 2 files changed, 9 insertions(+), 6 deletions(-)

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

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

[Hans de Goede]

Hi,

On 16-03-15 19:19, Leif Lindholm wrote:
> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.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?

I've just given the below patch a test run and it resolves the issue for me.

Thanks & Regards,

Hans


> 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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Leif Lindholm <leif.lindholm-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>   drivers/of/base.c     | 10 ++++------
>   drivers/of/unittest.c |  5 +++++
>   2 files changed, 9 insertions(+), 6 deletions(-)
>
> 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);
>
--
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