devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/6] dt: changeset fixes and cleanups
@ 2023-08-18 20:40 Rob Herring
  2023-08-18 20:40 ` [PATCH v3 1/6] of: unittest: Fix EXPECT for parse_phandle_with_args_map() test Rob Herring
                   ` (5 more replies)
  0 siblings, 6 replies; 19+ messages in thread
From: Rob Herring @ 2023-08-18 20:40 UTC (permalink / raw)
  To: Frank Rowand, Enrico Weigelt, metux IT consult, Rafael J. Wysocki,
	Sakari Ailus, Petr Mladek, Andy Shevchenko, Geert Uytterhoeven
  Cc: devicetree, linux-kernel

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/

Signed-off-by: Rob Herring <robh@kernel.org>
---
Changes in v3:
- Drop print changeset entry pointers
- Add bounds check for action value
- Further rework deadprops helper to remove a property from either list
- Keep existing style for deadprops loop
- Link to v2: https://lore.kernel.org/r/20230801-dt-changeset-fixes-v2-0-c2b701579dee@kernel.org

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/20230801-dt-changeset-fixes-v1-0-b5203e3fc22f@kernel.org

---
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     |  92 +++++++++++++++++-------------
 drivers/of/dynamic.c  | 153 +++++++++++---------------------------------------
 drivers/of/unittest.c |   4 +-
 3 files changed, 88 insertions(+), 161 deletions(-)
---
base-commit: 66a4210bc82e024e6de0f94298ad9230984a5924
change-id: 20230801-dt-changeset-fixes-b76b88fecc43

Best regards,
-- 
Rob Herring <robh@kernel.org>


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v3 1/6] of: unittest: Fix EXPECT for parse_phandle_with_args_map() test
  2023-08-18 20:40 [PATCH v3 0/6] dt: changeset fixes and cleanups Rob Herring
@ 2023-08-18 20:40 ` Rob Herring
  2023-08-18 20:40 ` [PATCH v3 2/6] of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock Rob Herring
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 19+ messages in thread
From: Rob Herring @ 2023-08-18 20:40 UTC (permalink / raw)
  To: Frank Rowand, Enrico Weigelt, metux IT consult, Rafael J. Wysocki,
	Sakari Ailus, Petr Mladek, Andy Shevchenko, Geert Uytterhoeven
  Cc: devicetree, linux-kernel

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()")
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/of/unittest.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index e5b0eea8011c..d943bf87c94d 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -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);
 

-- 
2.40.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v3 2/6] of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock
  2023-08-18 20:40 [PATCH v3 0/6] dt: changeset fixes and cleanups Rob Herring
  2023-08-18 20:40 ` [PATCH v3 1/6] of: unittest: Fix EXPECT for parse_phandle_with_args_map() test Rob Herring
@ 2023-08-18 20:40 ` Rob Herring
  2023-08-21 11:52   ` Geert Uytterhoeven
  2023-08-18 20:40 ` [PATCH v3 3/6] of: dynamic: Refactor changeset action printing to common helpers Rob Herring
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 19+ messages in thread
From: Rob Herring @ 2023-08-18 20:40 UTC (permalink / raw)
  To: Frank Rowand, Enrico Weigelt, metux IT consult, Rafael J. Wysocki,
	Sakari Ailus, Petr Mladek, Andy Shevchenko, Geert Uytterhoeven
  Cc: devicetree, linux-kernel

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")
Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Rob Herring <robh@kernel.org>
---
v6 (v3 in this series):
 - Add check on 'action' bounds. As action is only set in
   of_changeset_action(), add the check there.
 - Drop printing the changeset entry pointer
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 | 31 +++++++++----------------------
 1 file changed, 9 insertions(+), 22 deletions(-)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index e311d406b170..4999636eaa92 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -63,15 +63,14 @@ 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[] = {
+	[0] = "INVALID",
 	[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 +619,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 +635,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: %-15s %pOF:%s\n",
+		       action_names[ce->action], ce->np, ce->prop->name);
 		return ret;
+	}
 
 	switch (ce->action) {
 	case OF_RECONFIG_ATTACH_NODE:
@@ -947,6 +931,9 @@ int of_changeset_action(struct of_changeset *ocs, unsigned long action,
 	if (!ce)
 		return -ENOMEM;
 
+	if (WARN_ON(action >= ARRAY_SIZE(action_names)))
+		return -EINVAL;
+
 	/* get a reference to the node */
 	ce->action = action;
 	ce->np = of_node_get(np);

-- 
2.40.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v3 3/6] of: dynamic: Refactor changeset action printing to common helpers
  2023-08-18 20:40 [PATCH v3 0/6] dt: changeset fixes and cleanups Rob Herring
  2023-08-18 20:40 ` [PATCH v3 1/6] of: unittest: Fix EXPECT for parse_phandle_with_args_map() test Rob Herring
  2023-08-18 20:40 ` [PATCH v3 2/6] of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock Rob Herring
@ 2023-08-18 20:40 ` Rob Herring
  2023-08-21 12:09   ` Geert Uytterhoeven
  2023-08-18 20:40 ` [PATCH v3 4/6] of: dynamic: Fix race in getting old property when updating property Rob Herring
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 19+ messages in thread
From: Rob Herring @ 2023-08-18 20:40 UTC (permalink / raw)
  To: Frank Rowand, Enrico Weigelt, metux IT consult, Rafael J. Wysocki,
	Sakari Ailus, Petr Mladek, Andy Shevchenko, Geert Uytterhoeven
  Cc: devicetree, linux-kernel

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.

Signed-off-by: Rob Herring <robh@kernel.org>
---
v3:
 - Drop printing changeset entry pointers
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(-)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 4999636eaa92..17a0727d3f9b 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -72,27 +72,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);
 }
@@ -503,30 +497,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)
 {
@@ -598,7 +568,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: ", ce->action, ce->np, ce->prop);
 
 	raw_spin_lock_irqsave(&devtree_lock, flags);
 	switch (ce->action) {
@@ -642,8 +612,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: %-15s %pOF:%s\n",
-		       action_names[ce->action], ce->np, ce->prop->name);
+		of_changeset_action_err("apply failed: ", ce->action, ce->np, ce->prop);
 		return ret;
 	}
 

-- 
2.40.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v3 4/6] of: dynamic: Fix race in getting old property when updating property
  2023-08-18 20:40 [PATCH v3 0/6] dt: changeset fixes and cleanups Rob Herring
                   ` (2 preceding siblings ...)
  2023-08-18 20:40 ` [PATCH v3 3/6] of: dynamic: Refactor changeset action printing to common helpers Rob Herring
@ 2023-08-18 20:40 ` Rob Herring
  2023-08-21 12:53   ` Geert Uytterhoeven
  2023-08-18 20:41 ` [PATCH v3 5/6] of: dynamic: Move dead property list check into property add/update functions Rob Herring
  2023-08-18 20:41 ` [PATCH v3 6/6] of: Refactor node and property manipulation function locking Rob Herring
  5 siblings, 1 reply; 19+ messages in thread
From: Rob Herring @ 2023-08-18 20:40 UTC (permalink / raw)
  To: Frank Rowand, Enrico Weigelt, metux IT consult, Rafael J. Wysocki,
	Sakari Ailus, Petr Mladek, Andy Shevchenko, Geert Uytterhoeven
  Cc: devicetree, linux-kernel

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

Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/of/dynamic.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 17a0727d3f9b..1876af5b01fc 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -564,7 +564,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;
 
@@ -604,7 +604,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;
@@ -908,9 +908,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;

-- 
2.40.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v3 5/6] of: dynamic: Move dead property list check into property add/update functions
  2023-08-18 20:40 [PATCH v3 0/6] dt: changeset fixes and cleanups Rob Herring
                   ` (3 preceding siblings ...)
  2023-08-18 20:40 ` [PATCH v3 4/6] of: dynamic: Fix race in getting old property when updating property Rob Herring
@ 2023-08-18 20:41 ` Rob Herring
  2023-08-21 10:49   ` Andy Shevchenko
  2023-08-21 13:05   ` Geert Uytterhoeven
  2023-08-18 20:41 ` [PATCH v3 6/6] of: Refactor node and property manipulation function locking Rob Herring
  5 siblings, 2 replies; 19+ messages in thread
From: Rob Herring @ 2023-08-18 20:41 UTC (permalink / raw)
  To: Frank Rowand, Enrico Weigelt, metux IT consult, Rafael J. Wysocki,
	Sakari Ailus, Petr Mladek, Andy Shevchenko, Geert Uytterhoeven
  Cc: devicetree, linux-kernel

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.

With this there are 3 cases of removing a property from either deadprops
or properties lists, so add a helper to find and remove a matching
property.

Signed-off-by: Rob Herring <robh@kernel.org>
---
v3:
 - Make the helper remove a property from either deadprops or properties
   lists
 - Keep existing style in deadprops loop
v2:
 - Add helper to remove property from deadprops list
---
 drivers/of/base.c    | 37 ++++++++++++++++++++++++-------------
 drivers/of/dynamic.c | 19 -------------------
 2 files changed, 24 insertions(+), 32 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index e235f3a57ea8..3f9ddd18ff4b 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1530,6 +1530,20 @@ int of_count_phandle_with_args(const struct device_node *np, const char *list_na
 }
 EXPORT_SYMBOL(of_count_phandle_with_args);
 
+static struct property *__of_remove_property_from_list(struct property **list, struct property *prop)
+{
+	struct property **next;
+
+	for (next = list; *next; next = &(*next)->next) {
+		if (*next == prop) {
+			*next = prop->next;
+			prop->next = NULL;
+			return prop;
+		}
+	}
+	return NULL;
+}
+
 /**
  * __of_add_property - Add a property to a node without lock operations
  * @np:		Caller's Device Node
@@ -1539,6 +1553,8 @@ int __of_add_property(struct device_node *np, struct property *prop)
 {
 	struct property **next;
 
+	__of_remove_property_from_list(&np->deadprops, prop);
+
 	prop->next = NULL;
 	next = &np->properties;
 	while (*next) {
@@ -1583,21 +1599,14 @@ EXPORT_SYMBOL_GPL(of_add_property);
 
 int __of_remove_property(struct device_node *np, struct property *prop)
 {
-	struct property **next;
-
-	for (next = &np->properties; *next; next = &(*next)->next) {
-		if (*next == prop)
-			break;
+	if (__of_remove_property_from_list(&np->properties, prop)) {
+		/* Found the property, add it to deadprops list */
+		prop->next = np->deadprops;
+		np->deadprops = prop;
+		return 0;
 	}
-	if (*next == NULL)
-		return -ENODEV;
-
-	/* found the node */
-	*next = prop->next;
-	prop->next = np->deadprops;
-	np->deadprops = prop;
 
-	return 0;
+	return -ENODEV;
 }
 
 /**
@@ -1641,6 +1650,8 @@ int __of_update_property(struct device_node *np, struct property *newprop,
 {
 	struct property **next, *oldprop;
 
+	__of_remove_property_from_list(&np->deadprops, newprop);
+
 	for (next = &np->properties; *next; next = &(*next)->next) {
 		if (of_prop_cmp((*next)->name, newprop->name) == 0)
 			break;
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 1876af5b01fc..d3ad970ad7b8 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -564,7 +564,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;
 
@@ -579,15 +578,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:
@@ -595,15 +585,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:

-- 
2.40.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v3 6/6] of: Refactor node and property manipulation function locking
  2023-08-18 20:40 [PATCH v3 0/6] dt: changeset fixes and cleanups Rob Herring
                   ` (4 preceding siblings ...)
  2023-08-18 20:41 ` [PATCH v3 5/6] of: dynamic: Move dead property list check into property add/update functions Rob Herring
@ 2023-08-18 20:41 ` Rob Herring
  2023-08-21 10:51   ` Andy Shevchenko
  2023-08-21 13:18   ` Geert Uytterhoeven
  5 siblings, 2 replies; 19+ messages in thread
From: Rob Herring @ 2023-08-18 20:41 UTC (permalink / raw)
  To: Frank Rowand, Enrico Weigelt, metux IT consult, Rafael J. Wysocki,
	Sakari Ailus, Petr Mladek, Andy Shevchenko, Geert Uytterhoeven
  Cc: devicetree, linux-kernel

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.

Signed-off-by: Rob Herring <robh@kernel.org>
---
v3:
 - Rebase due to changes in prior patch
v2:
 - Re-arrange exit handling
---
 drivers/of/base.c    | 65 ++++++++++++++++++++++++++--------------------------
 drivers/of/dynamic.c | 51 +++++++++++++----------------------------
 2 files changed, 49 insertions(+), 67 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 3f9ddd18ff4b..4b24bed894ad 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1551,21 +1551,32 @@ static struct property *__of_remove_property_from_list(struct property **list, s
  */
 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_property_from_list(&np->deadprops, 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;
 }
 
@@ -1576,37 +1587,36 @@ 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);
 
 int __of_remove_property(struct device_node *np, struct property *prop)
 {
+	unsigned long flags;
+	int rc = -ENODEV;
+
+	raw_spin_lock_irqsave(&devtree_lock, flags);
+
 	if (__of_remove_property_from_list(&np->properties, prop)) {
 		/* Found the property, add it to deadprops list */
 		prop->next = np->deadprops;
 		np->deadprops = prop;
-		return 0;
+		rc = 0;
 	}
 
-	return -ENODEV;
+	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+	if (rc)
+		return rc;
+
+	__of_remove_property_sysfs(np, prop);
+	return 0;
 }
 
 /**
@@ -1621,21 +1631,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)
@@ -1649,6 +1651,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_property_from_list(&np->deadprops, newprop);
 
@@ -1670,6 +1675,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;
 }
 
@@ -1685,21 +1694,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)
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index d3ad970ad7b8..9252641de507 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -198,6 +198,9 @@ static void __of_attach_node(struct device_node *np)
 {
 	const __be32 *phandle;
 	int sz;
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&devtree_lock, flags);
 
 	if (!of_node_check_flag(np, OF_OVERLAY)) {
 		np->name = __of_get_property(np, "name", NULL);
@@ -220,6 +223,10 @@ static void __of_attach_node(struct device_node *np)
 	np->parent->child = np;
 	of_node_clear_flag(np, OF_DETACHED);
 	np->fwnode.flags |= FWNODE_FLAG_NOT_DEVICE;
+
+	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+
+	__of_attach_node_sysfs(np);
 }
 
 /**
@@ -229,17 +236,12 @@ static void __of_attach_node(struct device_node *np)
 int of_attach_node(struct device_node *np)
 {
 	struct of_reconfig_data rd;
-	unsigned long flags;
 
 	memset(&rd, 0, sizeof(rd));
 	rd.dn = np;
 
 	mutex_lock(&of_mutex);
-	raw_spin_lock_irqsave(&devtree_lock, flags);
 	__of_attach_node(np);
-	raw_spin_unlock_irqrestore(&devtree_lock, flags);
-
-	__of_attach_node_sysfs(np);
 	mutex_unlock(&of_mutex);
 
 	of_reconfig_notify(OF_RECONFIG_ATTACH_NODE, &rd);
@@ -250,13 +252,15 @@ int of_attach_node(struct device_node *np)
 void __of_detach_node(struct device_node *np)
 {
 	struct device_node *parent;
+	unsigned long flags;
 
-	if (WARN_ON(of_node_check_flag(np, OF_DETACHED)))
-		return;
+	raw_spin_lock_irqsave(&devtree_lock, flags);
 
 	parent = np->parent;
-	if (WARN_ON(!parent))
+	if (WARN_ON(of_node_check_flag(np, OF_DETACHED) || !parent)) {
+		raw_spin_unlock_irqrestore(&devtree_lock, flags);
 		return;
+	}
 
 	if (parent->child == np)
 		parent->child = np->sibling;
@@ -273,6 +277,10 @@ void __of_detach_node(struct device_node *np)
 
 	/* race with of_find_node_by_phandle() prevented by devtree_lock */
 	__of_phandle_cache_inv_entry(np->phandle);
+
+	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+
+	__of_detach_node_sysfs(np);
 }
 
 /**
@@ -282,17 +290,12 @@ void __of_detach_node(struct device_node *np)
 int of_detach_node(struct device_node *np)
 {
 	struct of_reconfig_data rd;
-	unsigned long flags;
 
 	memset(&rd, 0, sizeof(rd));
 	rd.dn = np;
 
 	mutex_lock(&of_mutex);
-	raw_spin_lock_irqsave(&devtree_lock, flags);
 	__of_detach_node(np);
-	raw_spin_unlock_irqrestore(&devtree_lock, flags);
-
-	__of_detach_node_sysfs(np);
 	mutex_unlock(&of_mutex);
 
 	of_reconfig_notify(OF_RECONFIG_DETACH_NODE, &rd);
@@ -564,12 +567,10 @@ static int __of_changeset_entry_notify(struct of_changeset_entry *ce,
 
 static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 {
-	unsigned long flags;
 	int ret = 0;
 
 	of_changeset_action_debug("applying: ", ce->action, ce->np, ce->prop);
 
-	raw_spin_lock_irqsave(&devtree_lock, flags);
 	switch (ce->action) {
 	case OF_RECONFIG_ATTACH_NODE:
 		__of_attach_node(ce->np);
@@ -590,32 +591,12 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 	default:
 		ret = -EINVAL;
 	}
-	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
 	if (ret) {
 		of_changeset_action_err("apply failed: ", ce->action, ce->np, ce->prop);
 		return ret;
 	}
 
-	switch (ce->action) {
-	case OF_RECONFIG_ATTACH_NODE:
-		__of_attach_node_sysfs(ce->np);
-		break;
-	case OF_RECONFIG_DETACH_NODE:
-		__of_detach_node_sysfs(ce->np);
-		break;
-	case OF_RECONFIG_ADD_PROPERTY:
-		/* ignore duplicate names */
-		__of_add_property_sysfs(ce->np, ce->prop);
-		break;
-	case OF_RECONFIG_REMOVE_PROPERTY:
-		__of_remove_property_sysfs(ce->np, ce->prop);
-		break;
-	case OF_RECONFIG_UPDATE_PROPERTY:
-		__of_update_property_sysfs(ce->np, ce->prop, ce->old_prop);
-		break;
-	}
-
 	return 0;
 }
 

-- 
2.40.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH v3 5/6] of: dynamic: Move dead property list check into property add/update functions
  2023-08-18 20:41 ` [PATCH v3 5/6] of: dynamic: Move dead property list check into property add/update functions Rob Herring
@ 2023-08-21 10:49   ` Andy Shevchenko
  2023-08-21 10:52     ` Andy Shevchenko
  2023-08-21 12:24     ` Rob Herring
  2023-08-21 13:05   ` Geert Uytterhoeven
  1 sibling, 2 replies; 19+ messages in thread
From: Andy Shevchenko @ 2023-08-21 10:49 UTC (permalink / raw)
  To: Rob Herring
  Cc: Frank Rowand, Enrico Weigelt, metux IT consult, Rafael J. Wysocki,
	Sakari Ailus, Petr Mladek, Geert Uytterhoeven, devicetree,
	linux-kernel

On Fri, Aug 18, 2023 at 03:41:00PM -0500, Rob Herring wrote:
> 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.
> 
> With this there are 3 cases of removing a property from either deadprops
> or properties lists, so add a helper to find and remove a matching
> property.

...

> v3:
>  - Keep existing style in deadprops loop

Not sure where exactly in the code that one, but...

...

>  int __of_remove_property(struct device_node *np, struct property *prop)
>  {
> -	struct property **next;
> -
> -	for (next = &np->properties; *next; next = &(*next)->next) {
> -		if (*next == prop)
> -			break;
> +	if (__of_remove_property_from_list(&np->properties, prop)) {
> +		/* Found the property, add it to deadprops list */
> +		prop->next = np->deadprops;
> +		np->deadprops = prop;
> +		return 0;
>  	}
> -	if (*next == NULL)
> -		return -ENODEV;
> -
> -	/* found the node */
> -	*next = prop->next;
> -	prop->next = np->deadprops;
> -	np->deadprops = prop;
>  
> -	return 0;
> +	return -ENODEV;
>  }


...if it's this one, I don't see how it's better than

	if (!__of_remove_property_from_list(&np->properties, prop))
		return -ENODEV;

	/* Found the property, add it to deadprops list */
	prop->next = np->deadprops;
	np->deadprops = prop;
	return 0;

Note, with --patience in use it may produce even nice-looking diff.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v3 6/6] of: Refactor node and property manipulation function locking
  2023-08-18 20:41 ` [PATCH v3 6/6] of: Refactor node and property manipulation function locking Rob Herring
@ 2023-08-21 10:51   ` Andy Shevchenko
  2023-08-21 13:18   ` Geert Uytterhoeven
  1 sibling, 0 replies; 19+ messages in thread
From: Andy Shevchenko @ 2023-08-21 10:51 UTC (permalink / raw)
  To: Rob Herring
  Cc: Frank Rowand, Enrico Weigelt, metux IT consult, Rafael J. Wysocki,
	Sakari Ailus, Petr Mladek, Geert Uytterhoeven, devicetree,
	linux-kernel

On Fri, Aug 18, 2023 at 03:41:01PM -0500, Rob Herring wrote:
> 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.

With help of cleanup.h this may be made shorter and less error-prone.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v3 5/6] of: dynamic: Move dead property list check into property add/update functions
  2023-08-21 10:49   ` Andy Shevchenko
@ 2023-08-21 10:52     ` Andy Shevchenko
  2023-08-21 12:24     ` Rob Herring
  1 sibling, 0 replies; 19+ messages in thread
From: Andy Shevchenko @ 2023-08-21 10:52 UTC (permalink / raw)
  To: Rob Herring
  Cc: Frank Rowand, Enrico Weigelt, metux IT consult, Rafael J. Wysocki,
	Sakari Ailus, Petr Mladek, Geert Uytterhoeven, devicetree,
	linux-kernel

On Mon, Aug 21, 2023 at 01:49:20PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 18, 2023 at 03:41:00PM -0500, Rob Herring wrote:

...

> > v3:
> >  - Keep existing style in deadprops loop
> 
> Not sure where exactly in the code that one, but...

> ...if it's this one, I don't see how it's better than

Also check the comment for the next patch.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v3 2/6] of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock
  2023-08-18 20:40 ` [PATCH v3 2/6] of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock Rob Herring
@ 2023-08-21 11:52   ` Geert Uytterhoeven
  0 siblings, 0 replies; 19+ messages in thread
From: Geert Uytterhoeven @ 2023-08-21 11:52 UTC (permalink / raw)
  To: Rob Herring
  Cc: Frank Rowand, Enrico Weigelt, metux IT consult, Rafael J. Wysocki,
	Sakari Ailus, Petr Mladek, Andy Shevchenko, Geert Uytterhoeven,
	devicetree, linux-kernel

Hi Rob,

Thanks for the update!

On Fri, Aug 18, 2023 at 10:41 PM Rob Herring <robh@kernel.org> wrote:
> 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

scripts/checkpatch.pl says: WARNING: Possible repeated word: '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")
> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> v6 (v3 in this series):
>  - Add check on 'action' bounds. As action is only set in
>    of_changeset_action(), add the check there.
>  - Drop printing the changeset entry pointer

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v3 3/6] of: dynamic: Refactor changeset action printing to common helpers
  2023-08-18 20:40 ` [PATCH v3 3/6] of: dynamic: Refactor changeset action printing to common helpers Rob Herring
@ 2023-08-21 12:09   ` Geert Uytterhoeven
  0 siblings, 0 replies; 19+ messages in thread
From: Geert Uytterhoeven @ 2023-08-21 12:09 UTC (permalink / raw)
  To: Rob Herring
  Cc: Frank Rowand, Enrico Weigelt, metux IT consult, Rafael J. Wysocki,
	Sakari Ailus, Petr Mladek, Andy Shevchenko, Geert Uytterhoeven,
	devicetree, linux-kernel

Hi Rob,

Thanks for the update!

On Fri, Aug 18, 2023 at 10:41 PM Rob Herring <robh@kernel.org> wrote:
> 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.
>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> v3:
>  - Drop printing changeset entry pointers

I think it would be good to mention this in the actual patch
description, too.

> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c

> @@ -598,7 +568,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: ", ce->action, ce->np, ce->prop);

s/applying/apply/ ?

The rest LGTM, so
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v3 5/6] of: dynamic: Move dead property list check into property add/update functions
  2023-08-21 10:49   ` Andy Shevchenko
  2023-08-21 10:52     ` Andy Shevchenko
@ 2023-08-21 12:24     ` Rob Herring
  2023-08-21 12:35       ` Andy Shevchenko
  1 sibling, 1 reply; 19+ messages in thread
From: Rob Herring @ 2023-08-21 12:24 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Frank Rowand, Enrico Weigelt, metux IT consult, Rafael J. Wysocki,
	Sakari Ailus, Petr Mladek, Geert Uytterhoeven, devicetree,
	linux-kernel

On Mon, Aug 21, 2023 at 5:49 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Aug 18, 2023 at 03:41:00PM -0500, Rob Herring wrote:
> > 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.
> >
> > With this there are 3 cases of removing a property from either deadprops
> > or properties lists, so add a helper to find and remove a matching
> > property.
>
> ...
>
> > v3:
> >  - Keep existing style in deadprops loop
>
> Not sure where exactly in the code that one, but...

That was your previous comment...

>
> ...
>
> >  int __of_remove_property(struct device_node *np, struct property *prop)
> >  {
> > -     struct property **next;
> > -
> > -     for (next = &np->properties; *next; next = &(*next)->next) {
> > -             if (*next == prop)
> > -                     break;
> > +     if (__of_remove_property_from_list(&np->properties, prop)) {
> > +             /* Found the property, add it to deadprops list */
> > +             prop->next = np->deadprops;
> > +             np->deadprops = prop;
> > +             return 0;
> >       }
> > -     if (*next == NULL)
> > -             return -ENODEV;
> > -
> > -     /* found the node */
> > -     *next = prop->next;
> > -     prop->next = np->deadprops;
> > -     np->deadprops = prop;
> >
> > -     return 0;
> > +     return -ENODEV;
> >  }
>
>
> ...if it's this one, I don't see how it's better than
>
>         if (!__of_remove_property_from_list(&np->properties, prop))
>                 return -ENODEV;

Because this way doesn't work well when we move the spinlock in here.
Maybe cleanup.h will help, but I'm not going to do that now. If we do,
then I'll do it for the whole subsystem/file.

Rob

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v3 5/6] of: dynamic: Move dead property list check into property add/update functions
  2023-08-21 12:24     ` Rob Herring
@ 2023-08-21 12:35       ` Andy Shevchenko
  0 siblings, 0 replies; 19+ messages in thread
From: Andy Shevchenko @ 2023-08-21 12:35 UTC (permalink / raw)
  To: Rob Herring
  Cc: Frank Rowand, Enrico Weigelt, metux IT consult, Rafael J. Wysocki,
	Sakari Ailus, Petr Mladek, Geert Uytterhoeven, devicetree,
	linux-kernel

On Mon, Aug 21, 2023 at 07:24:43AM -0500, Rob Herring wrote:
> On Mon, Aug 21, 2023 at 5:49 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, Aug 18, 2023 at 03:41:00PM -0500, Rob Herring wrote:

...

> > > v3:
> > >  - Keep existing style in deadprops loop
> >
> > Not sure where exactly in the code that one, but...
> 
> That was your previous comment...

I admit that I haven't heard about cleanup.h that time.

...

> > >  int __of_remove_property(struct device_node *np, struct property *prop)
> > >  {
> > > -     struct property **next;
> > > -
> > > -     for (next = &np->properties; *next; next = &(*next)->next) {
> > > -             if (*next == prop)
> > > -                     break;
> > > +     if (__of_remove_property_from_list(&np->properties, prop)) {
> > > +             /* Found the property, add it to deadprops list */
> > > +             prop->next = np->deadprops;
> > > +             np->deadprops = prop;
> > > +             return 0;
> > >       }
> > > -     if (*next == NULL)
> > > -             return -ENODEV;
> > > -
> > > -     /* found the node */
> > > -     *next = prop->next;
> > > -     prop->next = np->deadprops;
> > > -     np->deadprops = prop;
> > >
> > > -     return 0;
> > > +     return -ENODEV;
> > >  }
> >
> >
> > ...if it's this one, I don't see how it's better than
> >
> >         if (!__of_remove_property_from_list(&np->properties, prop))
> >                 return -ENODEV;
> 
> Because this way doesn't work well when we move the spinlock in here.
> Maybe cleanup.h will help, but I'm not going to do that now. If we do,
> then I'll do it for the whole subsystem/file.

Fair enough.

Although we may also use goto approach in the next patch. Anyway,
I leave it to you for what you think is the best.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v3 4/6] of: dynamic: Fix race in getting old property when updating property
  2023-08-18 20:40 ` [PATCH v3 4/6] of: dynamic: Fix race in getting old property when updating property Rob Herring
@ 2023-08-21 12:53   ` Geert Uytterhoeven
  0 siblings, 0 replies; 19+ messages in thread
From: Geert Uytterhoeven @ 2023-08-21 12:53 UTC (permalink / raw)
  To: Rob Herring
  Cc: Frank Rowand, Enrico Weigelt, metux IT consult, Rafael J. Wysocki,
	Sakari Ailus, Petr Mladek, Andy Shevchenko, devicetree,
	linux-kernel

Hi Rob,

Thanks for your patch!

On Fri, Aug 18, 2023 at 10:41 PM Rob Herring <robh@kernel.org> wrote:
> __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

Perhaps s/is also retrieved/was also retrieved before/, as
of_overlay_apply() calls build_changeset() before
__of_changeset_apply_entries()?

> racy as of_changeset_action() doesn't hold any locks. The property could
> be changed before the changeset is applied.
>
> Signed-off-by: Rob Herring <robh@kernel.org>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v3 5/6] of: dynamic: Move dead property list check into property add/update functions
  2023-08-18 20:41 ` [PATCH v3 5/6] of: dynamic: Move dead property list check into property add/update functions Rob Herring
  2023-08-21 10:49   ` Andy Shevchenko
@ 2023-08-21 13:05   ` Geert Uytterhoeven
  2023-08-21 14:23     ` Rob Herring
  1 sibling, 1 reply; 19+ messages in thread
From: Geert Uytterhoeven @ 2023-08-21 13:05 UTC (permalink / raw)
  To: Rob Herring
  Cc: Frank Rowand, Enrico Weigelt, metux IT consult, Rafael J. Wysocki,
	Sakari Ailus, Petr Mladek, Andy Shevchenko, Geert Uytterhoeven,
	devicetree, linux-kernel

Hi Rob,

Thanks for your patch!


On Fri, Aug 18, 2023 at 10:41 PM Rob Herring <robh@kernel.org> wrote:
> 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.
>
> With this there are 3 cases of removing a property from either deadprops
> or properties lists, so add a helper to find and remove a matching
> property.
>
> Signed-off-by: Rob Herring <robh@kernel.org>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Perhaps this needs a Fixes tag?

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v3 6/6] of: Refactor node and property manipulation function locking
  2023-08-18 20:41 ` [PATCH v3 6/6] of: Refactor node and property manipulation function locking Rob Herring
  2023-08-21 10:51   ` Andy Shevchenko
@ 2023-08-21 13:18   ` Geert Uytterhoeven
  2023-08-21 19:00     ` Rob Herring
  1 sibling, 1 reply; 19+ messages in thread
From: Geert Uytterhoeven @ 2023-08-21 13:18 UTC (permalink / raw)
  To: Rob Herring
  Cc: Frank Rowand, Enrico Weigelt, metux IT consult, Rafael J. Wysocki,
	Sakari Ailus, Petr Mladek, Andy Shevchenko, Geert Uytterhoeven,
	devicetree, linux-kernel

Hi Rob,

On Fri, Aug 18, 2023 at 10:41 PM Rob Herring <robh@kernel.org> wrote:
> 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.
>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> v3:
>  - Rebase due to changes in prior patch

Thanks for your patch!

> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c

> @@ -1576,37 +1587,36 @@ 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);

The notify block should be kept.

The rest LGTM, although I have some second thoughts about more
functions with a double underscore now taking devtree_lock().

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v3 5/6] of: dynamic: Move dead property list check into property add/update functions
  2023-08-21 13:05   ` Geert Uytterhoeven
@ 2023-08-21 14:23     ` Rob Herring
  0 siblings, 0 replies; 19+ messages in thread
From: Rob Herring @ 2023-08-21 14:23 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Frank Rowand, Enrico Weigelt, metux IT consult, Rafael J. Wysocki,
	Sakari Ailus, Petr Mladek, Andy Shevchenko, Geert Uytterhoeven,
	devicetree, linux-kernel

On Mon, Aug 21, 2023 at 8:05 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Rob,
>
> Thanks for your patch!
>
>
> On Fri, Aug 18, 2023 at 10:41 PM Rob Herring <robh@kernel.org> wrote:
> > 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.
> >
> > With this there are 3 cases of removing a property from either deadprops
> > or properties lists, so add a helper to find and remove a matching
> > property.
> >
> > Signed-off-by: Rob Herring <robh@kernel.org>
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks!

>
> Perhaps this needs a Fixes tag?

I didn't simply because in the decades that these functions existed,
no one cared. It would require a specific sequence of calls which we
could pretty much determine doesn't happen just looking at the callers
in the kernel.

Rob

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v3 6/6] of: Refactor node and property manipulation function locking
  2023-08-21 13:18   ` Geert Uytterhoeven
@ 2023-08-21 19:00     ` Rob Herring
  0 siblings, 0 replies; 19+ messages in thread
From: Rob Herring @ 2023-08-21 19:00 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Frank Rowand, Enrico Weigelt, metux IT consult, Rafael J. Wysocki,
	Sakari Ailus, Petr Mladek, Andy Shevchenko, Geert Uytterhoeven,
	devicetree, linux-kernel

On Mon, Aug 21, 2023 at 8:19 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Rob,
>
> On Fri, Aug 18, 2023 at 10:41 PM Rob Herring <robh@kernel.org> wrote:
> > 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.
> >
> > Signed-off-by: Rob Herring <robh@kernel.org>
> > ---
> > v3:
> >  - Rebase due to changes in prior patch
>
> Thanks for your patch!
>
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
>
> > @@ -1576,37 +1587,36 @@ 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);
>
> The notify block should be kept.

Yes, good catch.

>
> The rest LGTM, although I have some second thoughts about more
> functions with a double underscore now taking devtree_lock().

We still take the of_mutex. :) I looked at it some and we're already
not consistent in usage of '__'. Not a great argument I know, but if
we're going to make things consistent I think we should do that
separately for the whole subsys.

Rob

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2023-08-21 19:01 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-18 20:40 [PATCH v3 0/6] dt: changeset fixes and cleanups Rob Herring
2023-08-18 20:40 ` [PATCH v3 1/6] of: unittest: Fix EXPECT for parse_phandle_with_args_map() test Rob Herring
2023-08-18 20:40 ` [PATCH v3 2/6] of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock Rob Herring
2023-08-21 11:52   ` Geert Uytterhoeven
2023-08-18 20:40 ` [PATCH v3 3/6] of: dynamic: Refactor changeset action printing to common helpers Rob Herring
2023-08-21 12:09   ` Geert Uytterhoeven
2023-08-18 20:40 ` [PATCH v3 4/6] of: dynamic: Fix race in getting old property when updating property Rob Herring
2023-08-21 12:53   ` Geert Uytterhoeven
2023-08-18 20:41 ` [PATCH v3 5/6] of: dynamic: Move dead property list check into property add/update functions Rob Herring
2023-08-21 10:49   ` Andy Shevchenko
2023-08-21 10:52     ` Andy Shevchenko
2023-08-21 12:24     ` Rob Herring
2023-08-21 12:35       ` Andy Shevchenko
2023-08-21 13:05   ` Geert Uytterhoeven
2023-08-21 14:23     ` Rob Herring
2023-08-18 20:41 ` [PATCH v3 6/6] of: Refactor node and property manipulation function locking Rob Herring
2023-08-21 10:51   ` Andy Shevchenko
2023-08-21 13:18   ` Geert Uytterhoeven
2023-08-21 19:00     ` Rob Herring

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).