[v2,4/4] of: improve reporting invalid overlay target path

Message ID 1518425496-12159-5-git-send-email-frowand.list@gmail.com
State Superseded
Headers show
Series
  • of: change overlay apply input data from unflattened
Related show

Commit Message

Frank Rowand Feb. 12, 2018, 8:51 a.m.
From: Frank Rowand <frank.rowand@sony.com>

Errors while developing the patch to create of_overlay_fdt_apply()
exposed inadequate error messages to debug problems when overlay
devicetree fragment nodes contain an invalid target path.  Improve
the messages in find_target_node() to remedy this.

Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---

Changes from v1:
  - split out from patch 1/2

 drivers/of/overlay.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

Comments

Geert Uytterhoeven Feb. 12, 2018, 9 a.m. | #1
Hi Frank,

On Mon, Feb 12, 2018 at 9:51 AM,  <frowand.list@gmail.com> wrote:
> From: Frank Rowand <frank.rowand@sony.com>
>
> Errors while developing the patch to create of_overlay_fdt_apply()
> exposed inadequate error messages to debug problems when overlay
> devicetree fragment nodes contain an invalid target path.  Improve
> the messages in find_target_node() to remedy this.
>
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>

Thanks for your patch!

> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -488,17 +488,26 @@ static int build_changeset(struct overlay_changeset *ovcs)
>   */
>  static struct device_node *find_target_node(struct device_node *info_node)
>  {
> +       struct device_node *node;
>         const char *path;
>         u32 val;
>         int ret;
>
>         ret = of_property_read_u32(info_node, "target", &val);
> -       if (!ret)
> -               return of_find_node_by_phandle(val);
> +       if (!ret) {
> +               node = of_find_node_by_phandle(val);
> +               if (!node)
> +                       pr_err("target node find by phandle failed\n");

Do you want to print the actual node, cfr. below?

> +               return node;
> +       }
>
>         ret = of_property_read_string(info_node, "target-path", &path);
> -       if (!ret)
> -               return of_find_node_by_path(path);
> +       if (!ret) {
> +               node =  of_find_node_by_path(path);
> +               if (!node)
> +                       pr_err("target node find by path failed\n");

Likewise.

> +               return node;
> +       }
>
>         pr_err("Failed to find target for node %p (%s)\n",
>                 info_node, info_node->name);

This one prints more info.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Frank Rowand Feb. 12, 2018, 8:51 p.m. | #2
On 02/12/18 01:00, Geert Uytterhoeven wrote:
> Hi Frank,
> 
> On Mon, Feb 12, 2018 at 9:51 AM,  <frowand.list@gmail.com> wrote:
>> From: Frank Rowand <frank.rowand@sony.com>
>>
>> Errors while developing the patch to create of_overlay_fdt_apply()
>> exposed inadequate error messages to debug problems when overlay
>> devicetree fragment nodes contain an invalid target path.  Improve
>> the messages in find_target_node() to remedy this.
>>
>> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> 
> Thanks for your patch!
> 
>> --- a/drivers/of/overlay.c
>> +++ b/drivers/of/overlay.c
>> @@ -488,17 +488,26 @@ static int build_changeset(struct overlay_changeset *ovcs)
>>   */
>>  static struct device_node *find_target_node(struct device_node *info_node)
>>  {
>> +       struct device_node *node;
>>         const char *path;
>>         u32 val;
>>         int ret;
>>
>>         ret = of_property_read_u32(info_node, "target", &val);
>> -       if (!ret)
>> -               return of_find_node_by_phandle(val);
>> +       if (!ret) {
>> +               node = of_find_node_by_phandle(val);
>> +               if (!node)
>> +                       pr_err("target node find by phandle failed\n");
> 
> Do you want to print the actual node, cfr. below?

That seems like a good idea.  The name printed below is the (short) node
name of the fragment, which seems useful.

Will do, but will remove the pointer to the node, which is more of an
internal debugging detail.


>> +               return node;
>> +       }
>>
>>         ret = of_property_read_string(info_node, "target-path", &path);
>> -       if (!ret)
>> -               return of_find_node_by_path(path);
>> +       if (!ret) {
>> +               node =  of_find_node_by_path(path);
>> +               if (!node)
>> +                       pr_err("target node find by path failed\n");
> 
> Likewise.

Will do.


>> +               return node;
>> +       }
>>
>>         pr_err("Failed to find target for node %p (%s)\n",
>>                 info_node, info_node->name);
> 
> This one prints more info.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> 

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

Patch

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 5f6c5569e97d..01362e7e98e3 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -488,17 +488,26 @@  static int build_changeset(struct overlay_changeset *ovcs)
  */
 static struct device_node *find_target_node(struct device_node *info_node)
 {
+	struct device_node *node;
 	const char *path;
 	u32 val;
 	int ret;
 
 	ret = of_property_read_u32(info_node, "target", &val);
-	if (!ret)
-		return of_find_node_by_phandle(val);
+	if (!ret) {
+		node = of_find_node_by_phandle(val);
+		if (!node)
+			pr_err("target node find by phandle failed\n");
+		return node;
+	}
 
 	ret = of_property_read_string(info_node, "target-path", &path);
-	if (!ret)
-		return of_find_node_by_path(path);
+	if (!ret) {
+		node =  of_find_node_by_path(path);
+		if (!node)
+			pr_err("target node find by path failed\n");
+		return node;
+	}
 
 	pr_err("Failed to find target for node %p (%s)\n",
 		info_node, info_node->name);