Skip to contentSkip to conversation

dt: changeset fixes and cleanups

PATCH Superseded Rob Herring 3 participants 17 messages Aug 4, 2023 – Aug 18, 2023 Device tree

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Rob Herring

author maintainer
#0 lore raw

v2 cover dt: changeset fixes and cleanups

Geert's locking fix[1] prompted my closer look at __of_changeset_entry_apply() and related functions. The result is a couple of fixes I found and some refactoring that unifies the "old dynamic API" and the changeset API implementations.

[1] https://lore.kernel.org/all/c593d8389352c574b5be69d4ca4810da13326a50.1690533838.git.geert+renesas@glider.be/

---
Changes in v2:
- Rework debug printing to fix issues with pr_debug() not having a
return value with dynamic debug
- Split action print refactoring into separate patch from fix
- Make removing property from deadprops a helper function
- Rework __of_add_property()/__of_update_property() exit code
- Link to v1: https://lore.kernel.org/r/[email protected]

---
Rob Herring (6):
of: unittest: Fix EXPECT for parse_phandle_with_args_map() test
of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock
of: dynamic: Refactor changeset action printing to common helpers
of: dynamic: Fix race in getting old property when updating property
of: dynamic: Move dead property list check into property add/update functions
of: Refactor node and property manipulation function locking

 drivers/of/base.c     |  87 ++++++++++++++++++-----------
 drivers/of/dynamic.c  | 149 ++++++++++----------------------------------------
 drivers/of/unittest.c |   4 +-
 3 files changed, 86 insertions(+), 154 deletions(-)

---
base-commit: 66a4210bc82e024e6de0f94298ad9230984a5924
change-id: 20230801-dt-changeset-fixes-b76b88fecc43

Best regards,

Signature · 1 line
Rob Herring <[email protected]>
author maintainer
#1 lore raw

PATCH v2 1/6 of: unittest: Fix EXPECT for parse_phandle_with_args_map() test

Commit 12e17243d8a1 ("of: base: improve error msg in of_phandle_iterator_next()") added printing of the phandle value on error, but failed to update the unittest.

Fixes 12e17243d8a1 ("of: base: improve error msg in of_phandle_iterator_next()")
---
 drivers/of/unittest.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/of/unittest.c +2 -2
index e5b0eea8011c..d943bf87c94d 100644
@@ -664,12 +664,12 @@ static void __init of_unittest_parse_phandle_with_args_map(void)
 	memset(&args, 0, sizeof(args));
 
 	EXPECT_BEGIN(KERN_INFO,
-		     "OF: /testcase-data/phandle-tests/consumer-b: could not find phandle");
+		     "OF: /testcase-data/phandle-tests/consumer-b: could not find phandle 12345678");
 
 	rc = of_parse_phandle_with_args_map(np, "phandle-list-bad-phandle",
 					    "phandle", 0, &args);
 	EXPECT_END(KERN_INFO,
-		   "OF: /testcase-data/phandle-tests/consumer-b: could not find phandle");
+		   "OF: /testcase-data/phandle-tests/consumer-b: could not find phandle 12345678");
 
 	unittest(rc == -EINVAL, "expected:%i got:%i\n", -EINVAL, rc);
 
Signature · 1 line
2.40.1
Reviewed-by on v2 1/6
#2 lore raw

Re: [PATCH v2 1/6] of: unittest: Fix EXPECT for parse_phandle_with_args_map() test

Quoted 6 lines · Rob Herring

Commit 12e17243d8a1 ("of: base: improve error msg in of_phandle_iterator_next()") added printing of the phandle value on error, but failed to update the unittest.

Fixes 12e17243d8a1 ("of: base: improve error msg in of_phandle_iterator_next()")

Gr{oetje,eeting}s,

Geert

Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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
author maintainer
#3 lore raw

PATCH v2 2/6 of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock

In reply to Rob Herring

While originally it was fine to format strings using "%pOF" while holding devtree_lock, this now causes a deadlock. Lockdep reports:

    of_get_parent from of_fwnode_get_parent+0x18/0x24
    ^^^^^^^^^^^^^
    of_fwnode_get_parent from fwnode_count_parents+0xc/0x28
    fwnode_count_parents from fwnode_full_name_string+0x18/0xac
    fwnode_full_name_string from device_node_string+0x1a0/0x404
    device_node_string from pointer+0x3c0/0x534
    pointer from vsnprintf+0x248/0x36c
    vsnprintf from vprintk_store+0x130/0x3b4

Fix this by moving the printing in __of_changeset_entry_apply() outside the lock. As the only difference in the the multiple prints is the action name, use the existing "action_names" to refactor the prints into a single print.

Fixes a92eb7621b9fb2c2 ("lib/vsprintf: Make use of fwnode API to obtain node names and separators")

---
v5 (v2 in this series):
- Move majority of refactoring to separate patch and minimize the fix to just moving the print out of the locked section.
v4:
- Add missing 'static' reported by 0-day
v3:
- Re-implement Geert's fix to move the prints rather than move the spinlock

v1 and v2 from Geert simply moved the devtree_lock into each case statement:

https://lore.kernel.org/all/c593d8389352c574b5be69d4ca4810da13326a50.1690533838.git.geert+renesas@glider.be/

---
 drivers/of/dynamic.c | 27 +++++----------------------
 1 file changed, 5 insertions(+), 22 deletions(-)
drivers/of/dynamic.c +5 -22
index e311d406b170..2f0eb0053773 100644
@@ -63,15 +63,13 @@ int of_reconfig_notifier_unregister(struct notifier_block *nb)
 }
 EXPORT_SYMBOL_GPL(of_reconfig_notifier_unregister);
 
-#ifdef DEBUG
-const char *action_names[] = {
+static const char *action_names[] = {
 	[OF_RECONFIG_ATTACH_NODE] = "ATTACH_NODE",
 	[OF_RECONFIG_DETACH_NODE] = "DETACH_NODE",
 	[OF_RECONFIG_ADD_PROPERTY] = "ADD_PROPERTY",
 	[OF_RECONFIG_REMOVE_PROPERTY] = "REMOVE_PROPERTY",
 	[OF_RECONFIG_UPDATE_PROPERTY] = "UPDATE_PROPERTY",
 };
-#endif
 
 int of_reconfig_notify(unsigned long action, struct of_reconfig_data *p)
 {
@@ -620,21 +618,9 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 		}
 
 		ret = __of_add_property(ce->np, ce->prop);
-		if (ret) {
-			pr_err("changeset: add_property failed @%pOF/%s\n",
-				ce->np,
-				ce->prop->name);
-			break;
-		}
 		break;
 	case OF_RECONFIG_REMOVE_PROPERTY:
 		ret = __of_remove_property(ce->np, ce->prop);
-		if (ret) {
-			pr_err("changeset: remove_property failed @%pOF/%s\n",
-				ce->np,
-				ce->prop->name);
-			break;
-		}
 		break;
 
 	case OF_RECONFIG_UPDATE_PROPERTY:
@@ -648,20 +634,17 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 		}
 
 		ret = __of_update_property(ce->np, ce->prop, &old_prop);
-		if (ret) {
-			pr_err("changeset: update_property failed @%pOF/%s\n",
-				ce->np,
-				ce->prop->name);
-			break;
-		}
 		break;
 	default:
 		ret = -EINVAL;
 	}
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
-	if (ret)
+	if (ret) {
+		pr_err("changeset: apply failed: cset<%p> %-15s %pOF:%s\n",
+		       ce, action_names[ce->action], ce->np, ce->prop->name);
 		return ret;
+	}
 
 	switch (ce->action) {
 	case OF_RECONFIG_ATTACH_NODE:
Signature · 1 line
2.40.1
#4 lore raw

Re: [PATCH v2 2/6] of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock

On Fri, Aug 04, 2023 at 04:41:52PM -0600, Rob Herring wrote:

...

Quoted 6 lines

v5 (v2 in this series):
v4:
v3:
v1 and v2 from Geert simply moved the devtree_lock into each case statement:
https://lore.kernel.org/all/c593d8389352c574b5be69d4ca4810da13326a50.1690533838.git.geert+renesas@glider.be/

Side note: More natural to use main versioning as of this series and put the other to the parentheses.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#5 lore raw

Re: [PATCH v2 2/6] of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock

In reply to Rob Herring

Hi Rob,

Quoted 17 lines · Rob Herring

While originally it was fine to format strings using "%pOF" while holding devtree_lock, this now causes a deadlock. Lockdep reports:

    of_get_parent from of_fwnode_get_parent+0x18/0x24
    ^^^^^^^^^^^^^
    of_fwnode_get_parent from fwnode_count_parents+0xc/0x28
    fwnode_count_parents from fwnode_full_name_string+0x18/0xac
    fwnode_full_name_string from device_node_string+0x1a0/0x404
    device_node_string from pointer+0x3c0/0x534
    pointer from vsnprintf+0x248/0x36c
    vsnprintf from vprintk_store+0x130/0x3b4

Fix this by moving the printing in __of_changeset_entry_apply() outside the lock. As the only difference in the the multiple prints is the action name, use the existing "action_names" to refactor the prints into a single print.

Quoted 7 lines
Fixes a92eb7621b9fb2c2 ("lib/vsprintf: Make use of fwnode API to obtain node names and separators")

---
v5 (v2 in this series):
- Move majority of refactoring to separate patch and minimize the fix to just moving the print out of the locked section.

Thanks for your patch!

Quoted 12 lines
drivers/of/dynamic.c +0 -5
@@ -648,20 +634,17 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
                }
 
                ret = __of_update_property(ce->np, ce->prop, &old_prop);
-               if (ret) {
-                       pr_err("changeset: update_property failed @%pOF/%s\n",
-                               ce->np,
-                               ce->prop->name);
-                       break;

drivers/of/dynamic.c static int __of_changeset_entry_apply(struct of_changeset_entry *ce) · line 644

Quoted 10 lines
drivers/of/dynamic.c +2 -2
-               }
                break;
        default:
                ret = -EINVAL;
        }
        raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
-       if (ret)
+       if (ret) {
+               pr_err("changeset: apply failed: cset<%p> %-15s %pOF:%s\n",

Printing the cset pointer will (needlessly?) complicate the EXPECT_*() handling in the unit test.

drivers/of/dynamic.c static int __of_changeset_entry_apply(struct of_changeset_entry *ce) · line 645

Quoted 1 line
drivers/of/dynamic.c +1 -0
+                      ce, action_names[ce->action], ce->np, ce->prop->name);

This should check ce->action to avoid an out-of-bounds access beyond the end of action_names[].

drivers/of/dynamic.c static int __of_changeset_entry_apply(struct of_changeset_entry *ce) · line 650

Quoted 5 lines
drivers/of/dynamic.c +1 -0
                return ret;
+       }
 
        switch (ce->action) {
        case OF_RECONFIG_ATTACH_NODE:

The rest LGTM to me.

Gr{oetje,eeting}s,

Geert

Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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
author maintainer
#6 lore raw

Re: [PATCH v2 2/6] of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock

Quoted 50 lines · Geert Uytterhoeven

Hi Rob,

Quoted 17 lines · Rob Herring

While originally it was fine to format strings using "%pOF" while holding devtree_lock, this now causes a deadlock. Lockdep reports:

    of_get_parent from of_fwnode_get_parent+0x18/0x24
    ^^^^^^^^^^^^^
    of_fwnode_get_parent from fwnode_count_parents+0xc/0x28
    fwnode_count_parents from fwnode_full_name_string+0x18/0xac
    fwnode_full_name_string from device_node_string+0x1a0/0x404
    device_node_string from pointer+0x3c0/0x534
    pointer from vsnprintf+0x248/0x36c
    vsnprintf from vprintk_store+0x130/0x3b4

Fix this by moving the printing in __of_changeset_entry_apply() outside the lock. As the only difference in the the multiple prints is the action name, use the existing "action_names" to refactor the prints into a single print.

Quoted 7 lines
Fixes a92eb7621b9fb2c2 ("lib/vsprintf: Make use of fwnode API to obtain node names and separators")

---
v5 (v2 in this series):
- Move majority of refactoring to separate patch and minimize the fix to just moving the print out of the locked section.

Thanks for your patch!

Quoted 19 lines
drivers/of/dynamic.c +0 -6
@@ -648,20 +634,17 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
                }
 
                ret = __of_update_property(ce->np, ce->prop, &old_prop);
-               if (ret) {
-                       pr_err("changeset: update_property failed @%pOF/%s\n",
-                               ce->np,
-                               ce->prop->name);
-                       break;
-               }
                break;
        default:
                ret = -EINVAL;
        }
        raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
Quoted 6 lines

drivers/of/dynamic.c static int __of_changeset_entry_apply(struct of_changeset_entry *ce) · line 644

Quoted 3 lines
drivers/of/dynamic.c +2 -1
-       if (ret)
+       if (ret) {
+               pr_err("changeset: apply failed: cset<%p> %-15s %pOF:%s\n",

Printing the cset pointer will (needlessly?) complicate the EXPECT_*() handling in the unit test.

That's added largely because the other prints which I rework later in this series had them. Either printing the changeset ptr is useful or it isn't. I think people running the unittest and the post-processor can easily enough filter this out when looking at the results. Honestly, even I probably run it less than once a cycle.

Quoted 5 lines

drivers/of/dynamic.c static int __of_changeset_entry_apply(struct of_changeset_entry *ce) · line 645

Quoted 2 lines
drivers/of/dynamic.c +1 -0
+                      ce, action_names[ce->action], ce->np, ce->prop->name);

This should check ce->action to avoid an out-of-bounds access beyond the end of action_names[].

Indeed.

I think I'll add "invalid" to action_names names and then do something
like: "(ce->action < FOO) ? ce->action : 0".

Rob

#7 lore raw

Re: [PATCH v2 2/6] of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock

Hi Rob,

Quoted 56 lines · Rob Herring
Quoted 47 lines · Geert Uytterhoeven
Quoted 17 lines · Rob Herring

While originally it was fine to format strings using "%pOF" while holding devtree_lock, this now causes a deadlock. Lockdep reports:

    of_get_parent from of_fwnode_get_parent+0x18/0x24
    ^^^^^^^^^^^^^
    of_fwnode_get_parent from fwnode_count_parents+0xc/0x28
    fwnode_count_parents from fwnode_full_name_string+0x18/0xac
    fwnode_full_name_string from device_node_string+0x1a0/0x404
    device_node_string from pointer+0x3c0/0x534
    pointer from vsnprintf+0x248/0x36c
    vsnprintf from vprintk_store+0x130/0x3b4

Fix this by moving the printing in __of_changeset_entry_apply() outside the lock. As the only difference in the the multiple prints is the action name, use the existing "action_names" to refactor the prints into a single print.

Quoted 7 lines
Fixes a92eb7621b9fb2c2 ("lib/vsprintf: Make use of fwnode API to obtain node names and separators")

---
v5 (v2 in this series):
- Move majority of refactoring to separate patch and minimize the fix to just moving the print out of the locked section.

Thanks for your patch!

Quoted 19 lines
drivers/of/dynamic.c +0 -6
@@ -648,20 +634,17 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
                }
 
                ret = __of_update_property(ce->np, ce->prop, &old_prop);
-               if (ret) {
-                       pr_err("changeset: update_property failed @%pOF/%s\n",
-                               ce->np,
-                               ce->prop->name);
-                       break;
-               }
                break;
        default:
                ret = -EINVAL;
        }
        raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
Quoted 6 lines

drivers/of/dynamic.c static int __of_changeset_entry_apply(struct of_changeset_entry *ce) · line 644

Quoted 3 lines
drivers/of/dynamic.c +2 -1
-       if (ret)
+       if (ret) {
+               pr_err("changeset: apply failed: cset<%p> %-15s %pOF:%s\n",

Printing the cset pointer will (needlessly?) complicate the EXPECT_*() handling in the unit test.

Quoted 5 lines

That's added largely because the other prints which I rework later in this series had them. Either printing the changeset ptr is useful or it isn't. I think people running the unittest and the post-processor can easily enough filter this out when looking at the results. Honestly, even I probably run it less than once a cycle.

Do you have a use for printing the pointer value? And by default, it will be an obfuscated cookie anyway.

Quoted 9 lines
Quoted 4 lines

drivers/of/dynamic.c static int __of_changeset_entry_apply(struct of_changeset_entry *ce) · line 645

Quoted 1 line
drivers/of/dynamic.c +1 -0
+                      ce, action_names[ce->action], ce->np, ce->prop->name);

This should check ce->action to avoid an out-of-bounds access beyond the end of action_names[].

Indeed.

I think I'll add "invalid" to action_names names and then do something
like: "(ce->action < FOO) ? ce->action : 0".

OK, zero is invalid.

Gr{oetje,eeting}s,

Geert

Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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
author maintainer
#8 lore raw

Re: [PATCH v2 2/6] of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock

Quoted 60 lines · Geert Uytterhoeven

Hi Rob,

Quoted 56 lines · Rob Herring
Quoted 53 lines · Geert Uytterhoeven
Quoted 17 lines · Rob Herring

While originally it was fine to format strings using "%pOF" while holding devtree_lock, this now causes a deadlock. Lockdep reports:

    of_get_parent from of_fwnode_get_parent+0x18/0x24
    ^^^^^^^^^^^^^
    of_fwnode_get_parent from fwnode_count_parents+0xc/0x28
    fwnode_count_parents from fwnode_full_name_string+0x18/0xac
    fwnode_full_name_string from device_node_string+0x1a0/0x404
    device_node_string from pointer+0x3c0/0x534
    pointer from vsnprintf+0x248/0x36c
    vsnprintf from vprintk_store+0x130/0x3b4

Fix this by moving the printing in __of_changeset_entry_apply() outside the lock. As the only difference in the the multiple prints is the action name, use the existing "action_names" to refactor the prints into a single print.

Quoted 7 lines
Fixes a92eb7621b9fb2c2 ("lib/vsprintf: Make use of fwnode API to obtain node names and separators")

---
v5 (v2 in this series):
- Move majority of refactoring to separate patch and minimize the fix to just moving the print out of the locked section.

Thanks for your patch!

Quoted 12 lines
drivers/of/dynamic.c +0 -5
@@ -648,20 +634,17 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
                }
 
                ret = __of_update_property(ce->np, ce->prop, &old_prop);
-               if (ret) {
-                       pr_err("changeset: update_property failed @%pOF/%s\n",
-                               ce->np,
-                               ce->prop->name);
-                       break;

drivers/of/dynamic.c static int __of_changeset_entry_apply(struct of_changeset_entry *ce) · line 644

Quoted 10 lines
drivers/of/dynamic.c +2 -2
-               }
                break;
        default:
                ret = -EINVAL;
        }
        raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
-       if (ret)
+       if (ret) {
+               pr_err("changeset: apply failed: cset<%p> %-15s %pOF:%s\n",

Printing the cset pointer will (needlessly?) complicate the EXPECT_*() handling in the unit test.

Quoted 7 lines
Quoted 5 lines

That's added largely because the other prints which I rework later in this series had them. Either printing the changeset ptr is useful or it isn't. I think people running the unittest and the post-processor can easily enough filter this out when looking at the results. Honestly, even I probably run it less than once a cycle.

Do you have a use for printing the pointer value?

I have no use for overlays in general, so no. :)

I'd assumed it was there to provide a changeset ID to tell which actions belong to the same changeset. But it's printing the changeset entry rather than the changeset, so I agree it is not really useful.

Rob

author maintainer
#9 lore raw

PATCH v2 3/6 of: dynamic: Refactor changeset action printing to common helpers

In reply to Rob Herring

Several places print the changeset action with node and property details. Refactor these into a common printing helper. The complicating factor is some prints are debug and some are errors. Solve this with a bit of preprocessor magic.

---
v2:
- Split out refactoring from fix in prior patch
- Avoid pr_cont and relying on pr_debug return value which dynamic debug doesn't have

---
 drivers/of/dynamic.c | 53 +++++++++++-----------------------------------------
 1 file changed, 11 insertions(+), 42 deletions(-)
drivers/of/dynamic.c +11 -42
index 2f0eb0053773..6eaa66b11a02 100644
@@ -71,27 +71,21 @@ static const char *action_names[] = {
 	[OF_RECONFIG_UPDATE_PROPERTY] = "UPDATE_PROPERTY",
 };
 
+#define _do_print(func, prefix, action, node, prop, ...) ({	\
+	func("changeset: " prefix "%-15s %pOF%s%s\n",		\
+	     ##__VA_ARGS__, action_names[action], node,		\
+	     prop ? ":" : "", prop ? prop->name : "");		\
+})
+#define of_changeset_action_err(...) _do_print(pr_err, __VA_ARGS__)
+#define of_changeset_action_debug(...) _do_print(pr_debug, __VA_ARGS__)
+
 int of_reconfig_notify(unsigned long action, struct of_reconfig_data *p)
 {
 	int rc;
-#ifdef DEBUG
 	struct of_reconfig_data *pr = p;
 
-	switch (action) {
-	case OF_RECONFIG_ATTACH_NODE:
-	case OF_RECONFIG_DETACH_NODE:
-		pr_debug("notify %-15s %pOF\n", action_names[action],
-			pr->dn);
-		break;
-	case OF_RECONFIG_ADD_PROPERTY:
-	case OF_RECONFIG_REMOVE_PROPERTY:
-	case OF_RECONFIG_UPDATE_PROPERTY:
-		pr_debug("notify %-15s %pOF:%s\n", action_names[action],
-			pr->dn, pr->prop->name);
-		break;
+	of_changeset_action_debug("notify: ", action, pr->dn, pr->prop);
 
-	}
-#endif
 	rc = blocking_notifier_call_chain(&of_reconfig_chain, action, p);
 	return notifier_to_errno(rc);
 }
@@ -502,30 +496,6 @@ static void __of_changeset_entry_destroy(struct of_changeset_entry *ce)
 	kfree(ce);
 }
 
-#ifdef DEBUG
-static void __of_changeset_entry_dump(struct of_changeset_entry *ce)
-{
-	switch (ce->action) {
-	case OF_RECONFIG_ADD_PROPERTY:
-	case OF_RECONFIG_REMOVE_PROPERTY:
-	case OF_RECONFIG_UPDATE_PROPERTY:
-		pr_debug("cset<%p> %-15s %pOF/%s\n", ce, action_names[ce->action],
-			ce->np, ce->prop->name);
-		break;
-	case OF_RECONFIG_ATTACH_NODE:
-	case OF_RECONFIG_DETACH_NODE:
-		pr_debug("cset<%p> %-15s %pOF\n", ce, action_names[ce->action],
-			ce->np);
-		break;
-	}
-}
-#else
-static inline void __of_changeset_entry_dump(struct of_changeset_entry *ce)
-{
-	/* empty */
-}
-#endif
-
 static void __of_changeset_entry_invert(struct of_changeset_entry *ce,
 					  struct of_changeset_entry *rce)
 {
@@ -597,7 +567,7 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 	unsigned long flags;
 	int ret = 0;
 
-	__of_changeset_entry_dump(ce);
+	of_changeset_action_debug("applying: cset<%p> ", ce->action, ce->np, ce->prop, ce);
 
 	raw_spin_lock_irqsave(&devtree_lock, flags);
 	switch (ce->action) {
@@ -641,8 +611,7 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
 	if (ret) {
-		pr_err("changeset: apply failed: cset<%p> %-15s %pOF:%s\n",
-		       ce, action_names[ce->action], ce->np, ce->prop->name);
+		of_changeset_action_err("apply failed: cset<%p> ", ce->action, ce->np, ce->prop, ce);
 		return ret;
 	}
 
Signature · 1 line
2.40.1
author maintainer
#10 lore raw

PATCH v2 4/6 of: dynamic: Fix race in getting old property when updating property

In reply to Rob Herring

__of_update_property() returns the existing property if there is one, but that value is never added to the changeset. Updates work because the existing property is also retrieved in of_changeset_action(), but that is racy as of_changeset_action() doesn't hold any locks. The property could be changed before the changeset is applied.

---
 drivers/of/dynamic.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)
drivers/of/dynamic.c +2 -5
index 6eaa66b11a02..fbc7c29896a2 100644
@@ -563,7 +563,7 @@ static int __of_changeset_entry_notify(struct of_changeset_entry *ce,
 
 static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 {
-	struct property *old_prop, **propp;
+	struct property **propp;
 	unsigned long flags;
 	int ret = 0;
 
@@ -603,7 +603,7 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 			}
 		}
 
-		ret = __of_update_property(ce->np, ce->prop, &old_prop);
+		ret = __of_update_property(ce->np, ce->prop, &ce->old_prop);
 		break;
 	default:
 		ret = -EINVAL;
@@ -904,9 +904,6 @@ int of_changeset_action(struct of_changeset *ocs, unsigned long action,
 	ce->np = of_node_get(np);
 	ce->prop = prop;
 
-	if (action == OF_RECONFIG_UPDATE_PROPERTY && prop)
-		ce->old_prop = of_find_property(np, prop->name, NULL);
-
 	/* add it to the list */
 	list_add_tail(&ce->node, &ocs->entries);
 	return 0;
Signature · 1 line
2.40.1
author maintainer
#11 lore raw

PATCH v2 5/6 of: dynamic: Move dead property list check into property add/update functions

In reply to Rob Herring

The changeset code checks for a property in the deadprops list when adding/updating a property, but of_add_property() and of_update_property() do not. As the users of these functions are pretty simple, they have not hit this scenario or else the property lists would get corrupted.

---
v2:
- Add helper to remove property from deadprops list

---
 drivers/of/base.c    | 19 +++++++++++++++++++
 drivers/of/dynamic.c | 19 -------------------
 2 files changed, 19 insertions(+), 19 deletions(-)
drivers/of/base.c +19 -0
index e235f3a57ea8..c63a4cde281e 100644
@@ -1530,6 +1530,21 @@ int of_count_phandle_with_args(const struct device_node *np, const char *list_na
 }
 EXPORT_SYMBOL(of_count_phandle_with_args);
 
+static void __of_remove_dead_property(struct device_node *np, struct property *prop)
+{
+	struct property **next;
+
+	/* If the property is in deadprops then it must be removed */
+	for (next = &np->deadprops; *next; next = &(*next)->next) {
+		if (*next != prop)
+			continue;
+
+		*next = prop->next;
+		prop->next = NULL;
+		break;
+	}
+}
+
 /**
  * __of_add_property - Add a property to a node without lock operations
  * @np:		Caller's Device Node
@@ -1539,6 +1554,8 @@ int __of_add_property(struct device_node *np, struct property *prop)
 {
 	struct property **next;
 
+	__of_remove_dead_property(np, prop);
+
 	prop->next = NULL;
 	next = &np->properties;
 	while (*next) {
@@ -1641,6 +1658,8 @@ int __of_update_property(struct device_node *np, struct property *newprop,
 {
 	struct property **next, *oldprop;
 
+	__of_remove_dead_property(np, newprop);
+
 	for (next = &np->properties; *next; next = &(*next)->next) {
 		if (of_prop_cmp((*next)->name, newprop->name) == 0)
 			break;
drivers/of/dynamic.c +0 -19
index fbc7c29896a2..769869e8b847 100644
@@ -563,7 +563,6 @@ static int __of_changeset_entry_notify(struct of_changeset_entry *ce,
 
 static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 {
-	struct property **propp;
 	unsigned long flags;
 	int ret = 0;
 
@@ -578,15 +577,6 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 		__of_detach_node(ce->np);
 		break;
 	case OF_RECONFIG_ADD_PROPERTY:
-		/* If the property is in deadprops then it must be removed */
-		for (propp = &ce->np->deadprops; *propp; propp = &(*propp)->next) {
-			if (*propp == ce->prop) {
-				*propp = ce->prop->next;
-				ce->prop->next = NULL;
-				break;
-			}
-		}
-
 		ret = __of_add_property(ce->np, ce->prop);
 		break;
 	case OF_RECONFIG_REMOVE_PROPERTY:
@@ -594,15 +584,6 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 		break;
 
 	case OF_RECONFIG_UPDATE_PROPERTY:
-		/* If the property is in deadprops then it must be removed */
-		for (propp = &ce->np->deadprops; *propp; propp = &(*propp)->next) {
-			if (*propp == ce->prop) {
-				*propp = ce->prop->next;
-				ce->prop->next = NULL;
-				break;
-			}
-		}
-
 		ret = __of_update_property(ce->np, ce->prop, &ce->old_prop);
 		break;
 	default:
Signature · 1 line
2.40.1
#12 lore raw

Re: [PATCH v2 5/6] of: dynamic: Move dead property list check into property add/update functions

Quoted 5 lines · Rob Herring

The changeset code checks for a property in the deadprops list when adding/updating a property, but of_add_property() and of_update_property() do not. As the users of these functions are pretty simple, they have not hit this scenario or else the property lists would get corrupted.

Suggested-by ? :-)

...

drivers/of/base.c int of_count_phandle_with_args(const struct device_node *np, const char *list_na · line 1544

Quoted 12 lines
drivers/of/base.c +12 -0
+static void __of_remove_dead_property(struct device_node *np, struct property *prop)
+{
+	struct property **next;
+
+	/* If the property is in deadprops then it must be removed */
+	for (next = &np->deadprops; *next; next = &(*next)->next) {
+		if (*next != prop)
+			continue;
+
+		*next = prop->next;
+		prop->next = NULL;
+		break;

Why not

		if (*next == prop) {
			*next = prop->next;
			prop->next = NULL;
			break;
		}

which seems to me clearer?

Quoted 2 lines
drivers/of/base.c +2 -0
+	}
+}
Signature · 2 lines
With Best Regards,
Andy Shevchenko
author maintainer
#13 lore raw

Re: [PATCH v2 5/6] of: dynamic: Move dead property list check into property add/update functions

Quoted 8 lines · Andy Shevchenko
Quoted 5 lines · Rob Herring

The changeset code checks for a property in the deadprops list when adding/updating a property, but of_add_property() and of_update_property() do not. As the users of these functions are pretty simple, they have not hit this scenario or else the property lists would get corrupted.

Suggested-by ? :-)

Humm, by me? The change in behavior and point of this patch comes from me. You've provided review comments which will get covered by a Reviewed-by I presume.

Quoted 18 lines

...

drivers/of/base.c int of_count_phandle_with_args(const struct device_node *np, const char *list_na · line 1544

Quoted 12 lines
drivers/of/base.c +12 -0
+static void __of_remove_dead_property(struct device_node *np, struct property *prop)
+{
+	struct property **next;
+
+	/* If the property is in deadprops then it must be removed */
+	for (next = &np->deadprops; *next; next = &(*next)->next) {
+		if (*next != prop)
+			continue;
+
+		*next = prop->next;
+		prop->next = NULL;
+		break;

Why not

Quoted 7 lines
		if (*next == prop) {
			*next = prop->next;
			prop->next = NULL;
			break;
		}

which seems to me clearer?

Sure. I like the style I wrote, but whichever way ends the discussion is fine for me.

Rob

#14 lore raw

Re: [PATCH v2 5/6] of: dynamic: Move dead property list check into property add/update functions

Quoted 2 lines · Rob Herring
Quoted 1 line · Andy Shevchenko

On Fri, Aug 04, 2023 at 04:41:55PM -0600, Rob Herring wrote:

...

Quoted 5 lines
Quoted 1 line
Suggested-by ? :-)

Humm, by me? The change in behavior and point of this patch comes from me. You've provided review comments which will get covered by a Reviewed-by I presume.

OK!

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author maintainer
#15 lore raw

PATCH v2 6/6 of: Refactor node and property manipulation function locking

In reply to Rob Herring

All callers of __of_{add,remove,update}_property() and __of_{attach,detach}_node() wrap the call with the devtree_lock spinlock. Let's move the spinlock into the functions. This allows moving the sysfs update functions into those functions as well.

---
v2:
- Re-arrange exit handling

---
 drivers/of/base.c    | 68 +++++++++++++++++++++++++++-------------------------
 drivers/of/dynamic.c | 51 +++++++++++++--------------------------
 2 files changed, 51 insertions(+), 68 deletions(-)
drivers/of/base.c +35 -33
index c63a4cde281e..3ec134429f11 100644
@@ -1552,21 +1552,32 @@ static void __of_remove_dead_property(struct device_node *np, struct property *p
  */
 int __of_add_property(struct device_node *np, struct property *prop)
 {
+	int rc = 0;
+	unsigned long flags;
 	struct property **next;
 
+	raw_spin_lock_irqsave(&devtree_lock, flags);
+
 	__of_remove_dead_property(np, prop);
 
 	prop->next = NULL;
 	next = &np->properties;
 	while (*next) {
-		if (strcmp(prop->name, (*next)->name) == 0)
+		if (strcmp(prop->name, (*next)->name) == 0) {
 			/* duplicate ! don't insert it */
-			return -EEXIST;
-
+			rc = -EEXIST;
+			goto out_unlock;
+		}
 		next = &(*next)->next;
 	}
 	*next = prop;
 
+out_unlock:
+	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+	if (rc)
+		return rc;
+
+	__of_add_property_sysfs(np, prop);
 	return 0;
 }
 
@@ -1577,23 +1588,12 @@ int __of_add_property(struct device_node *np, struct property *prop)
  */
 int of_add_property(struct device_node *np, struct property *prop)
 {
-	unsigned long flags;
 	int rc;
 
 	mutex_lock(&of_mutex);
-
-	raw_spin_lock_irqsave(&devtree_lock, flags);
 	rc = __of_add_property(np, prop);
-	raw_spin_unlock_irqrestore(&devtree_lock, flags);
-
-	if (!rc)
-		__of_add_property_sysfs(np, prop);
-
 	mutex_unlock(&of_mutex);
 
-	if (!rc)
-		of_property_notify(OF_RECONFIG_ADD_PROPERTY, np, prop, NULL);
-
 	return rc;
 }
 EXPORT_SYMBOL_GPL(of_add_property);
@@ -1601,19 +1601,30 @@ EXPORT_SYMBOL_GPL(of_add_property);
 int __of_remove_property(struct device_node *np, struct property *prop)
 {
 	struct property **next;
+	unsigned long flags;
+	int rc = 0;
+
+	raw_spin_lock_irqsave(&devtree_lock, flags);
 
 	for (next = &np->properties; *next; next = &(*next)->next) {
 		if (*next == prop)
 			break;
 	}
-	if (*next == NULL)
-		return -ENODEV;
-
+	if (*next == NULL) {
+		rc = -ENODEV;
+		goto out_unlock;
+	}
 	/* found the node */
 	*next = prop->next;
 	prop->next = np->deadprops;
 	np->deadprops = prop;
 
+out_unlock:
+	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+	if (rc)
+		return rc;
+
+	__of_remove_property_sysfs(np, prop);
 	return 0;
 }
 
@@ -1629,21 +1640,13 @@ int __of_remove_property(struct device_node *np, struct property *prop)
  */
 int of_remove_property(struct device_node *np, struct property *prop)
 {
-	unsigned long flags;
 	int rc;
 
 	if (!prop)
 		return -ENODEV;
 
 	mutex_lock(&of_mutex);
-
-	raw_spin_lock_irqsave(&devtree_lock, flags);
 	rc = __of_remove_property(np, prop);
-	raw_spin_unlock_irqrestore(&devtree_lock, flags);
-
-	if (!rc)
-		__of_remove_property_sysfs(np, prop);
-
 	mutex_unlock(&of_mutex);
 
 	if (!rc)
@@ -1657,6 +1660,9 @@ int __of_update_property(struct device_node *np, struct property *newprop,
 		struct property **oldpropp)
 {
 	struct property **next, *oldprop;
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&devtree_lock, flags);
 
 	__of_remove_dead_property(np, newprop);
 
@@ -1678,6 +1684,10 @@ int __of_update_property(struct device_node *np, struct property *newprop,
 		*next = newprop;
 	}
 
+	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+
+	__of_update_property_sysfs(np, newprop, oldprop);
+
 	return 0;
 }
 
@@ -1693,21 +1703,13 @@ int __of_update_property(struct device_node *np, struct property *newprop,
 int of_update_property(struct device_node *np, struct property *newprop)
 {
 	struct property *oldprop;
-	unsigned long flags;
 	int rc;
 
 	if (!newprop->name)
 		return -EINVAL;
 
 	mutex_lock(&of_mutex);
-
-	raw_spin_lock_irqsave(&devtree_lock, flags);
 	rc = __of_update_property(np, newprop, &oldprop);
-	raw_spin_unlock_irqrestore(&devtree_lock, flags);
-
-	if (!rc)
-		__of_update_property_sysfs(np, newprop, oldprop);
-
 	mutex_unlock(&of_mutex);
 
 	if (!rc)
drivers/of/dynamic.c +16 -35

125 lines not on this page · raw message

Signature · 1 line
2.40.1
#16 lore raw

Re: [PATCH v2 6/6] of: Refactor node and property manipulation function locking

Quoted 4 lines · Rob Herring

All callers of __of_{add,remove,update}_property() and __of_{attach,detach}_node() wrap the call with the devtree_lock spinlock. Let's move the spinlock into the functions. This allows moving the sysfs update functions into those functions as well.

...

Quoted 4 lines
drivers/of/base.c +0 -0
 	for (next = &np->properties; *next; next = &(*next)->next) {
 		if (*next == prop)
 			break;
 	}

drivers/of/base.c EXPORT_SYMBOL_GPL(of_add_property); · line 1619

Quoted 10 lines
drivers/of/base.c +4 -3
-	if (*next == NULL)
-		return -ENODEV;
-
+	if (*next == NULL) {
+		rc = -ENODEV;
+		goto out_unlock;
+	}
 	/* found the node */
 	*next = prop->next;
 	prop->next = np->deadprops;

But this also looks like a dup for deadprop, maybe something like

static int __of_remove_property_dead_or_alive() // pun intended.

?

Signature · 2 lines
With Best Regards,
Andy Shevchenko