OF: fix of_find_node_by_path() assumption that of_allnodes is root

RFC PATCH In review Frank Rowand 4 participants 10 messages Jun 13, 2014 – Jun 15, 2014 Device tree

lore

Share & tools

lore

Frank Rowand

author

2014-06-13 05:53 UTC #0 lore raw

RFC PATCH 1/1 OF: fix of_find_node_by_path() assumption that of_allnodes is root

From: Frank Rowand <frank.rowand-/[email protected]>

Pantelis Antoniou reports that of_find_node_by_path() is borked because of_allnodes is not guaranteed to contain the root of the tree after using any of the dynamic update functions because some other nodes ends up as of_allnodes.

Fixes c22e650e66b8 of: Make of_find_node_by_path() handle /aliases

Signed-off-by Frank Rowand <frank.rowand-/[email protected]>

---

Pantelis,

Can you test whether the problem you reported in irc?

This patch has not been tested, not even compile tested.

drivers/of/base.c | 8 6 + 2 - 0 !

 1 file changed, 6 insertions(+), 2 deletions(-)

drivers/of/base.c +6 -2

Index: b/drivers/of/base.c
===================================================================
@@ -1948,6 +1948,9 @@ int of_reconfig_notify(unsigned long act
 
 /**
  * of_attach_node - Plug a device node into the tree and global list.
+ *
+ * Put the device node after of_allnodes in the global list so that
+ * of_allnodes remains the root.
  */
 int of_attach_node(struct device_node *np)
 {

@@ -1958,11 +1961,12 @@ int of_attach_node(struct device_node *n
 	if (rc)
 		return rc;
 
+	BUG_ON(!of_allnodes);
 	raw_spin_lock_irqsave(&devtree_lock, flags);
 	np->sibling = np->parent->child;
-	np->allnext = of_allnodes;
+	np->allnext = of_allnodes->allnext;
 	np->parent->child = np;
-	of_allnodes = np;
+	of_allnodes->allnext = np;
 	of_node_clear_flag(np, OF_DETACHED);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);

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

Pantelis Antoniou

Tested-by on v1 1/1

2014-06-13 07:07 UTC #1 lore raw

Hi Frank,

Quoted 10 lines · Frank Rowand

From: Frank Rowand <frank.rowand-/[email protected]>
Pantelis Antoniou reports that of_find_node_by_path() is borked because of_allnodes is not guaranteed to contain the root of the tree after using any of the dynamic update functions because some other nodes ends up as of_allnodes.
Fixes c22e650e66b8 of: Make of_find_node_by_path() handle /aliases
Signed-off-by Frank Rowand <frank.rowand-/[email protected]>

Quoted 9 lines

---
Pantelis,
Can you test whether the problem you reported in irc?
This patch has not been tested, not even compile tested.

I can verify that this fixes the problem. Please make sure this gets applied to mainline otherwise every arch which updates DT on runtime is going to break in subtle ways.

Regards

-- Pantelis

Tested-by Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/[email protected]>

Quoted 34 lines

drivers/of/base.c | 8 6 + 2 - 0 !

1 file changed, 6 insertions(+), 2 deletions(-)

Index: b/drivers/of/base.c
===================================================================

drivers/of/base.c +6 -3

@@ -1948,6 +1948,9 @@ int of_reconfig_notify(unsigned long act
 
 /**
 * of_attach_node - Plug a device node into the tree and global list.
+ *
+ * Put the device node after of_allnodes in the global list so that
+ * of_allnodes remains the root.
 */
 int of_attach_node(struct device_node *np)
 {

@@ -1958,11 +1961,12 @@ int of_attach_node(struct device_node *n
 	if (rc)
 		return rc;
 
+	BUG_ON(!of_allnodes);
 	raw_spin_lock_irqsave(&devtree_lock, flags);
 	np->sibling = np->parent->child;
-	np->allnext = of_allnodes;
+	np->allnext = of_allnodes->allnext;
 	np->parent->child = np;
-	of_allnodes = np;
+	of_allnodes->allnext = np;
 	of_node_clear_flag(np, OF_DETACHED);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
--

Quoted 3 lines

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

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

Rob Herring

2014-06-13 13:52 UTC #2 lore raw

In reply to Frank Rowand

Quoted 8 lines · Frank Rowand

From: Frank Rowand <frank.rowand-/[email protected]>
Pantelis Antoniou reports that of_find_node_by_path() is borked because of_allnodes is not guaranteed to contain the root of the tree after using any of the dynamic update functions because some other nodes ends up as of_allnodes.
Fixes c22e650e66b8 of: Make of_find_node_by_path() handle /aliases

Is it not possible to do a fix in of_find_node_by_path instead? I just wonder if this could have any other unintended side-effects on of_attach_node's behavior.

Rob

Quoted 34 lines

Signed-off-by Frank Rowand <frank.rowand-/[email protected]>
---
Pantelis,
Can you test whether the problem you reported in irc?
This patch has not been tested, not even compile tested.
drivers/of/base.c | 8 6 + 2 - 0 !
 1 file changed, 6 insertions(+), 2 deletions(-)
Index: b/drivers/of/base.c
===================================================================
drivers/of/base.c +4 -0
@@ -1948,6 +1948,9 @@ int of_reconfig_notify(unsigned long act
 
 /**
  * of_attach_node - Plug a device node into the tree and global list.
+ *
+ * Put the device node after of_allnodes in the global list so that
+ * of_allnodes remains the root.
  */
 int of_attach_node(struct device_node *np)
 {
@@ -1958,11 +1961,12 @@ int of_attach_node(struct device_node *n
        if (rc)
                return rc;
 
+       BUG_ON(!of_allnodes);

drivers/of/base.c int of_attach_node(struct device_node *n · line 1972

Quoted 10 lines

drivers/of/base.c +2 -2

        raw_spin_lock_irqsave(&devtree_lock, flags);
        np->sibling = np->parent->child;
-       np->allnext = of_allnodes;
+       np->allnext = of_allnodes->allnext;
        np->parent->child = np;
-       of_allnodes = np;
+       of_allnodes->allnext = np;
        of_node_clear_flag(np, OF_DETACHED);
        raw_spin_unlock_irqrestore(&devtree_lock, flags);

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

Grant Likely

Acked-by

2014-06-13 15:06 UTC #3 lore raw

Quoted 8 lines · Rob Herring

Quoted 7 lines · Frank Rowand
From: Frank Rowand <[email protected]>
Pantelis Antoniou reports that of_find_node_by_path() is borked because of_allnodes is not guaranteed to contain the root of the tree after using any of the dynamic update functions because some other nodes ends up as of_allnodes.

Quoted 5 lines

Quoted 1 line
Fixes c22e650e66b8 of: Make of_find_node_by_path() handle /aliases
Is it not possible to do a fix in of_find_node_by_path instead? I just wonder if this could have any other unintended side-effects on of_attach_node's behavior.

Given that I'm going to replace the custom list with list_head which makes this problem go away, I think this solution is fine.

Acked-by Grant Likely <[email protected]>

Quoted 47 lines

Rob

Quoted 44 lines

Signed-off-by Frank Rowand <[email protected]>

---

Pantelis,

Can you test whether the problem you reported in irc?

This patch has not been tested, not even compile tested.

drivers/of/base.c | 8 6 + 2 - 0 !

 1 file changed, 6 insertions(+), 2 deletions(-)

Index: b/drivers/of/base.c
===================================================================

drivers/of/base.c +6 -2

@@ -1948,6 +1948,9 @@ int of_reconfig_notify(unsigned long act
 
 /**
  * of_attach_node - Plug a device node into the tree and global list.
+ *
+ * Put the device node after of_allnodes in the global list so that
+ * of_allnodes remains the root.
  */
 int of_attach_node(struct device_node *np)
 {

@@ -1958,11 +1961,12 @@ int of_attach_node(struct device_node *n
        if (rc)
                return rc;
 
+       BUG_ON(!of_allnodes);
        raw_spin_lock_irqsave(&devtree_lock, flags);
        np->sibling = np->parent->child;
-       np->allnext = of_allnodes;
+       np->allnext = of_allnodes->allnext;
        np->parent->child = np;
-       of_allnodes = np;
+       of_allnodes->allnext = np;
        of_node_clear_flag(np, OF_DETACHED);
        raw_spin_unlock_irqrestore(&devtree_lock, flags);

Grant Likely

2014-06-14 15:00 UTC #4 lore raw

Quoted 11 lines · Grant Likely

Quoted 10 lines · Rob Herring
Quoted 8 lines · Frank Rowand
From: Frank Rowand <frank.rowand-/[email protected]>
Pantelis Antoniou reports that of_find_node_by_path() is borked because of_allnodes is not guaranteed to contain the root of the tree after using any of the dynamic update functions because some other nodes ends up as of_allnodes.
Fixes c22e650e66b8 of: Make of_find_node_by_path() handle /aliases

Quoted 8 lines

Quoted 3 lines
Is it not possible to do a fix in of_find_node_by_path instead? I just wonder if this could have any other unintended side-effects on of_attach_node's behavior.
Given that I'm going to replace the custom list with list_head which makes this problem go away, I think this solution is fine.
Acked-by Grant Likely <[email protected]>

That said, the following might be better because it would guarantee that children always appear after parents...

Quoted 35 lines

Quoted 35 lines
Rob
Quoted 33 lines
Signed-off-by Frank Rowand <frank.rowand-/[email protected]>
---
Pantelis,
Can you test whether the problem you reported in irc?
This patch has not been tested, not even compile tested.
drivers/of/base.c | 8 6 + 2 - 0 !
 1 file changed, 6 insertions(+), 2 deletions(-)
Index: b/drivers/of/base.c
===================================================================
drivers/of/base.c +3 -0
@@ -1948,6 +1948,9 @@ int of_reconfig_notify(unsigned long act
 
 /**
  * of_attach_node - Plug a device node into the tree and global list.
+ *
+ * Put the device node after of_allnodes in the global list so that
+ * of_allnodes remains the root.
  */
 int of_attach_node(struct device_node *np)
 {
@@ -1958,11 +1961,12 @@ int of_attach_node(struct device_node *n
        if (rc)
                return rc;
 

Quoted 5 lines

Quoted 5 lines
drivers/of/base.c int of_attach_node(struct device_node *n · line 1967
Quoted 5 lines
drivers/of/base.c +2 -1
+       BUG_ON(!of_allnodes);
        raw_spin_lock_irqsave(&devtree_lock, flags);
        np->sibling = np->parent->child;
-       np->allnext = of_allnodes;
+       np->allnext = of_allnodes->allnext;

np->allnext = np->parent->allnext;
np->parent->allnext = np;

Quoted 6 lines

Quoted 6 lines
drivers/of/base.c int of_attach_node(struct device_node *n · line 1972
Quoted 6 lines
drivers/of/base.c +1 -1
        np->parent->child = np;
-       of_allnodes = np;
+       of_allnodes->allnext = np;
        of_node_clear_flag(np, OF_DETACHED);
        raw_spin_unlock_irqrestore(&devtree_lock, flags);
 

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

Frank Rowand

author

2014-06-15 03:36 UTC #5 lore raw

Quoted 16 lines · Grant Likely

Quoted 15 lines · Grant Likely
Quoted 13 lines · Rob Herring
Quoted 8 lines · Frank Rowand
From: Frank Rowand <frank.rowand-/[email protected]>
Pantelis Antoniou reports that of_find_node_by_path() is borked because of_allnodes is not guaranteed to contain the root of the tree after using any of the dynamic update functions because some other nodes ends up as of_allnodes.
Fixes c22e650e66b8 of: Make of_find_node_by_path() handle /aliases
Is it not possible to do a fix in of_find_node_by_path instead? I just wonder if this could have any other unintended side-effects on of_attach_node's behavior.

Quoted 7 lines

Quoted 4 lines
Given that I'm going to replace the custom list with list_head which makes this problem go away, I think this solution is fine.
Acked-by Grant Likely <[email protected]>
That said, the following might be better because it would guarantee that children always appear after parents...

< snip >

I agree, I like that better. Patch respun, v2 will be sent as a reply to this email.

Pantelis, can you test v2 please?

Thanks,

Frank

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

Frank Rowand

author

2014-06-15 03:39 UTC #6 lore raw

From: Frank Rowand <frank.rowand-/[email protected]>

of_find_node_by_path() is borked because of_allnodes is not guaranteed to contain the root of the tree after using any of the dynamic update functions because some other nodes ends up as of_allnodes.

Fixes c22e650e66b8 of: Make of_find_node_by_path() handle /aliases

Reported-by pantelis.antoniou-OWPKS81ov/FWk0Htik3J/[email protected]

Signed-off-by Frank Rowand <frank.rowand-/[email protected]>

---

v2:
Remove change to function header, no longer needed.
Remove BUG_ON(), no longer needed.
Add np as np->parent->allnext instead of np->parent->allnext, as suggested
by Grant Likely.

drivers/of/base.c | 4 2 + 2 - 0 !

 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/of/base.c +2 -2

Index: b/drivers/of/base.c
===================================================================
@@ -1960,9 +1960,9 @@ int of_attach_node(struct device_node *n
 
 	raw_spin_lock_irqsave(&devtree_lock, flags);
 	np->sibling = np->parent->child;
-	np->allnext = of_allnodes;
+	np->allnext = np->parent->allnext;
+	np->parent->allnext = np;
 	np->parent->child = np;
-	of_allnodes = np;
 	of_node_clear_flag(np, OF_DETACHED);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);

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

Frank Rowand

author

2014-06-15 03:42 UTC #7 lore raw

Quoted 9 lines · Frank Rowand

From: Frank Rowand <frank.rowand-/[email protected]>
of_find_node_by_path() is borked because of_allnodes is not guaranteed to contain the root of the tree after using any of the dynamic update functions because some other nodes ends up as of_allnodes.
Fixes c22e650e66b8 of: Make of_find_node_by_path() handle /aliases
Reported-by pantelis.antoniou-OWPKS81ov/FWk0Htik3J/[email protected]
Signed-off-by Frank Rowand <frank.rowand-/[email protected]>

Quoted 6 lines

---
v2:
Remove change to function header, no longer needed.
Remove BUG_ON(), no longer needed.
Add np as np->parent->allnext instead of np->parent->allnext, as suggested

Aaaargh, cut and paste error, that should be:

    Add np as np->parent->allnext instead of of_allnodes->allnext, as suggested

Quoted 13 lines

by Grant Likely.
drivers/of/base.c | 4 2 + 2 - 0 !
 1 file changed, 2 insertions(+), 2 deletions(-)
Index: b/drivers/of/base.c
===================================================================
drivers/of/base.c +0 -0
@@ -1960,9 +1960,9 @@ int of_attach_node(struct device_node *n
 

drivers/of/base.c int of_attach_node(struct device_node *n · line 1968

Quoted 10 lines

drivers/of/base.c +2 -2

 	raw_spin_lock_irqsave(&devtree_lock, flags);
 	np->sibling = np->parent->child;
-	np->allnext = of_allnodes;
+	np->allnext = np->parent->allnext;
+	np->parent->allnext = np;
 	np->parent->child = np;
-	of_allnodes = np;
 	of_node_clear_flag(np, OF_DETACHED);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);

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

Frank Rowand

author

2014-06-13 16:49 UTC #8 lore raw

In reply to Rob Herring

Quoted 11 lines · Rob Herring

Quoted 8 lines · Frank Rowand
From: Frank Rowand <[email protected]>
Pantelis Antoniou reports that of_find_node_by_path() is borked because of_allnodes is not guaranteed to contain the root of the tree after using any of the dynamic update functions because some other nodes ends up as of_allnodes.
Fixes c22e650e66b8 of: Make of_find_node_by_path() handle /aliases
Is it not possible to do a fix in of_find_node_by_path instead? I just

Yes, the code for that is in https://lkml.org/lkml/2014/5/20/758

Or as Grant said in his reply, just use the fix in of_attach_node() for now since he is going to replace the custom list.

Quoted 51 lines

wonder if this could have any other unintended side-effects on of_attach_node's behavior.
Rob
Quoted 34 lines
Signed-off-by Frank Rowand <[email protected]>
---
Pantelis,
Can you test whether the problem you reported in irc?
This patch has not been tested, not even compile tested.
drivers/of/base.c | 8 6 + 2 - 0 !
 1 file changed, 6 insertions(+), 2 deletions(-)
Index: b/drivers/of/base.c
===================================================================
drivers/of/base.c +4 -0
@@ -1948,6 +1948,9 @@ int of_reconfig_notify(unsigned long act
 
 /**
  * of_attach_node - Plug a device node into the tree and global list.
+ *
+ * Put the device node after of_allnodes in the global list so that
+ * of_allnodes remains the root.
  */
 int of_attach_node(struct device_node *np)
 {
@@ -1958,11 +1961,12 @@ int of_attach_node(struct device_node *n
        if (rc)
                return rc;
 
+       BUG_ON(!of_allnodes);
drivers/of/base.c int of_attach_node(struct device_node *n · line 1972
Quoted 10 lines
drivers/of/base.c +2 -2
        raw_spin_lock_irqsave(&devtree_lock, flags);
        np->sibling = np->parent->child;
-       np->allnext = of_allnodes;
+       np->allnext = of_allnodes->allnext;
        np->parent->child = np;
-       of_allnodes = np;
+       of_allnodes->allnext = np;
        of_node_clear_flag(np, OF_DETACHED);
        raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
.

Rob Herring

2014-06-13 21:12 UTC #9 lore raw

Quoted 9 lines · Frank Rowand

Quoted 8 lines · Rob Herring
Quoted 7 lines · Frank Rowand
From: Frank Rowand <frank.rowand-/[email protected]>
Pantelis Antoniou reports that of_find_node_by_path() is borked because of_allnodes is not guaranteed to contain the root of the tree after using any of the dynamic update functions because some other nodes ends up as of_allnodes.

Quoted 8 lines

Quoted 3 lines
Quoted 1 line
Fixes c22e650e66b8 of: Make of_find_node_by_path() handle /aliases
Is it not possible to do a fix in of_find_node_by_path instead? I just
Yes, the code for that is in https://lkml.org/lkml/2014/5/20/758
Or as Grant said in his reply, just use the fix in of_attach_node() for now since he is going to replace the custom list.

Okay, I've applied.

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