* [PATCH v2 0/6] dt: changeset fixes and cleanups
@ 2023-08-04 22:41 Rob Herring
2023-08-04 22:41 ` [PATCH v2 1/6] of: unittest: Fix EXPECT for parse_phandle_with_args_map() test Rob Herring
` (5 more replies)
0 siblings, 6 replies; 17+ messages in thread
From: Rob Herring @ 2023-08-04 22: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
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 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 | 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,
--
Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 1/6] of: unittest: Fix EXPECT for parse_phandle_with_args_map() test
2023-08-04 22:41 [PATCH v2 0/6] dt: changeset fixes and cleanups Rob Herring
@ 2023-08-04 22:41 ` Rob Herring
2023-08-18 15:27 ` Geert Uytterhoeven
2023-08-04 22:41 ` [PATCH v2 2/6] of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock Rob Herring
` (4 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Rob Herring @ 2023-08-04 22: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
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()")
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] 17+ messages in thread
* [PATCH v2 2/6] of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock
2023-08-04 22:41 [PATCH v2 0/6] dt: changeset fixes and cleanups Rob Herring
2023-08-04 22:41 ` [PATCH v2 1/6] of: unittest: Fix EXPECT for parse_phandle_with_args_map() test Rob Herring
@ 2023-08-04 22:41 ` Rob Herring
2023-08-07 15:30 ` Andy Shevchenko
2023-08-18 15:36 ` Geert Uytterhoeven
2023-08-04 22:41 ` [PATCH v2 3/6] of: dynamic: Refactor changeset action printing to common helpers Rob Herring
` (3 subsequent siblings)
5 siblings, 2 replies; 17+ messages in thread
From: Rob Herring @ 2023-08-04 22: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
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>
---
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(-)
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index e311d406b170..2f0eb0053773 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -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:
--
2.40.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 3/6] of: dynamic: Refactor changeset action printing to common helpers
2023-08-04 22:41 [PATCH v2 0/6] dt: changeset fixes and cleanups Rob Herring
2023-08-04 22:41 ` [PATCH v2 1/6] of: unittest: Fix EXPECT for parse_phandle_with_args_map() test Rob Herring
2023-08-04 22:41 ` [PATCH v2 2/6] of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock Rob Herring
@ 2023-08-04 22:41 ` Rob Herring
2023-08-04 22:41 ` [PATCH v2 4/6] of: dynamic: Fix race in getting old property when updating property Rob Herring
` (2 subsequent siblings)
5 siblings, 0 replies; 17+ messages in thread
From: Rob Herring @ 2023-08-04 22: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
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>
---
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 2f0eb0053773..6eaa66b11a02 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -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;
}
--
2.40.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 4/6] of: dynamic: Fix race in getting old property when updating property
2023-08-04 22:41 [PATCH v2 0/6] dt: changeset fixes and cleanups Rob Herring
` (2 preceding siblings ...)
2023-08-04 22:41 ` [PATCH v2 3/6] of: dynamic: Refactor changeset action printing to common helpers Rob Herring
@ 2023-08-04 22:41 ` Rob Herring
2023-08-04 22:41 ` [PATCH v2 5/6] of: dynamic: Move dead property list check into property add/update functions Rob Herring
2023-08-04 22:41 ` [PATCH v2 6/6] of: Refactor node and property manipulation function locking Rob Herring
5 siblings, 0 replies; 17+ messages in thread
From: Rob Herring @ 2023-08-04 22: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
__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 6eaa66b11a02..fbc7c29896a2 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -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;
--
2.40.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 5/6] of: dynamic: Move dead property list check into property add/update functions
2023-08-04 22:41 [PATCH v2 0/6] dt: changeset fixes and cleanups Rob Herring
` (3 preceding siblings ...)
2023-08-04 22:41 ` [PATCH v2 4/6] of: dynamic: Fix race in getting old property when updating property Rob Herring
@ 2023-08-04 22:41 ` Rob Herring
2023-08-07 15:37 ` Andy Shevchenko
2023-08-04 22:41 ` [PATCH v2 6/6] of: Refactor node and property manipulation function locking Rob Herring
5 siblings, 1 reply; 17+ messages in thread
From: Rob Herring @ 2023-08-04 22: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.
Signed-off-by: Rob Herring <robh@kernel.org>
---
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(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index e235f3a57ea8..c63a4cde281e 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -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;
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index fbc7c29896a2..769869e8b847 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -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:
--
2.40.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 6/6] of: Refactor node and property manipulation function locking
2023-08-04 22:41 [PATCH v2 0/6] dt: changeset fixes and cleanups Rob Herring
` (4 preceding siblings ...)
2023-08-04 22:41 ` [PATCH v2 5/6] of: dynamic: Move dead property list check into property add/update functions Rob Herring
@ 2023-08-04 22:41 ` Rob Herring
2023-08-07 15:40 ` Andy Shevchenko
5 siblings, 1 reply; 17+ messages in thread
From: Rob Herring @ 2023-08-04 22: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>
---
v2:
- Re-arrange exit handling
---
drivers/of/base.c | 68 +++++++++++++++++++++++++++-------------------------
drivers/of/dynamic.c | 51 +++++++++++++--------------------------
2 files changed, 51 insertions(+), 68 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index c63a4cde281e..3ec134429f11 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -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)
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 769869e8b847..715365fbb8ea 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -197,6 +197,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);
@@ -219,6 +222,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);
}
/**
@@ -228,17 +235,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);
@@ -249,13 +251,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;
@@ -272,6 +276,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);
}
/**
@@ -281,17 +289,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);
@@ -563,12 +566,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: cset<%p> ", ce->action, ce->np, ce->prop, ce);
- raw_spin_lock_irqsave(&devtree_lock, flags);
switch (ce->action) {
case OF_RECONFIG_ATTACH_NODE:
__of_attach_node(ce->np);
@@ -589,32 +590,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: cset<%p> ", ce->action, ce->np, ce->prop, ce);
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] 17+ messages in thread
* Re: [PATCH v2 2/6] of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock
2023-08-04 22:41 ` [PATCH v2 2/6] of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock Rob Herring
@ 2023-08-07 15:30 ` Andy Shevchenko
2023-08-18 15:36 ` Geert Uytterhoeven
1 sibling, 0 replies; 17+ messages in thread
From: Andy Shevchenko @ 2023-08-07 15:30 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 04, 2023 at 04:41:52PM -0600, Rob Herring wrote:
...
> 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.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 5/6] of: dynamic: Move dead property list check into property add/update functions
2023-08-04 22:41 ` [PATCH v2 5/6] of: dynamic: Move dead property list check into property add/update functions Rob Herring
@ 2023-08-07 15:37 ` Andy Shevchenko
2023-08-17 17:09 ` Rob Herring
0 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2023-08-07 15:37 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 04, 2023 at 04:41:55PM -0600, 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.
Suggested-by: ? :-)
...
> +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?
> + }
> +}
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 6/6] of: Refactor node and property manipulation function locking
2023-08-04 22:41 ` [PATCH v2 6/6] of: Refactor node and property manipulation function locking Rob Herring
@ 2023-08-07 15:40 ` Andy Shevchenko
0 siblings, 0 replies; 17+ messages in thread
From: Andy Shevchenko @ 2023-08-07 15:40 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 04, 2023 at 04:41:56PM -0600, 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.
...
> 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;
But this also looks like a dup for deadprop, maybe something like
static int __of_remove_property_dead_or_alive() // pun intended.
?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 5/6] of: dynamic: Move dead property list check into property add/update functions
2023-08-07 15:37 ` Andy Shevchenko
@ 2023-08-17 17:09 ` Rob Herring
2023-08-17 17:15 ` Andy Shevchenko
0 siblings, 1 reply; 17+ messages in thread
From: Rob Herring @ 2023-08-17 17:09 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 07, 2023 at 06:37:15PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 04, 2023 at 04:41:55PM -0600, 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.
>
> 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.
>
> ...
>
> > +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?
Sure. I like the style I wrote, but whichever way ends the discussion is
fine for me.
Rob
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 5/6] of: dynamic: Move dead property list check into property add/update functions
2023-08-17 17:09 ` Rob Herring
@ 2023-08-17 17:15 ` Andy Shevchenko
0 siblings, 0 replies; 17+ messages in thread
From: Andy Shevchenko @ 2023-08-17 17:15 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 Thu, Aug 17, 2023 at 12:09:34PM -0500, Rob Herring wrote:
> On Mon, Aug 07, 2023 at 06:37:15PM +0300, Andy Shevchenko wrote:
> > On Fri, Aug 04, 2023 at 04:41:55PM -0600, Rob Herring wrote:
...
> > 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!
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/6] of: unittest: Fix EXPECT for parse_phandle_with_args_map() test
2023-08-04 22:41 ` [PATCH v2 1/6] of: unittest: Fix EXPECT for parse_phandle_with_args_map() test Rob Herring
@ 2023-08-18 15:27 ` Geert Uytterhoeven
0 siblings, 0 replies; 17+ messages in thread
From: Geert Uytterhoeven @ 2023-08-18 15:27 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
On Sat, Aug 5, 2023 at 12:42 AM Rob Herring <robh@kernel.org> wrote:
> 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()")
> Signed-off-by: Rob Herring <robh@kernel.org>
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] 17+ messages in thread
* Re: [PATCH v2 2/6] of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock
2023-08-04 22:41 ` [PATCH v2 2/6] of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock Rob Herring
2023-08-07 15:30 ` Andy Shevchenko
@ 2023-08-18 15:36 ` Geert Uytterhoeven
2023-08-18 16:16 ` Rob Herring
1 sibling, 1 reply; 17+ messages in thread
From: Geert Uytterhoeven @ 2023-08-18 15:36 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 Sat, Aug 5, 2023 at 12:42 AM 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
> 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>
> ---
> 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!
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -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",
Printing the cset pointer will (needlessly?) complicate the EXPECT_*()
handling in the unit test.
> + 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[].
> return ret;
> + }
>
> switch (ce->action) {
> case OF_RECONFIG_ATTACH_NODE:
The rest LGTM to me.
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] 17+ messages in thread
* Re: [PATCH v2 2/6] of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock
2023-08-18 15:36 ` Geert Uytterhoeven
@ 2023-08-18 16:16 ` Rob Herring
2023-08-18 17:36 ` Geert Uytterhoeven
0 siblings, 1 reply; 17+ messages in thread
From: Rob Herring @ 2023-08-18 16:16 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 Fri, Aug 18, 2023 at 10:36 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Rob,
>
> On Sat, Aug 5, 2023 at 12:42 AM 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
> > 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>
> > ---
> > 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!
>
> > --- a/drivers/of/dynamic.c
> > +++ b/drivers/of/dynamic.c
>
> > @@ -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",
>
> 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.
>
> > + 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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/6] of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock
2023-08-18 16:16 ` Rob Herring
@ 2023-08-18 17:36 ` Geert Uytterhoeven
2023-08-18 18:13 ` Rob Herring
0 siblings, 1 reply; 17+ messages in thread
From: Geert Uytterhoeven @ 2023-08-18 17:36 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,
On Fri, Aug 18, 2023 at 6:17 PM Rob Herring <robh@kernel.org> wrote:
> On Fri, Aug 18, 2023 at 10:36 AM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > On Sat, Aug 5, 2023 at 12:42 AM 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
> > > 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>
> > > ---
> > > 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!
> >
> > > --- a/drivers/of/dynamic.c
> > > +++ b/drivers/of/dynamic.c
> >
> > > @@ -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",
> >
> > 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.
Do you have a use for printing the pointer value?
And by default, it will be an obfuscated cookie anyway.
> > > + 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
--
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] 17+ messages in thread
* Re: [PATCH v2 2/6] of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock
2023-08-18 17:36 ` Geert Uytterhoeven
@ 2023-08-18 18:13 ` Rob Herring
0 siblings, 0 replies; 17+ messages in thread
From: Rob Herring @ 2023-08-18 18:13 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Frank Rowand, Enrico Weigelt, metux IT consult,
Rafael J. Wysocki, Sakari Ailus, Petr Mladek, Andy Shevchenko,
devicetree, linux-kernel
On Fri, Aug 18, 2023 at 12:37 PM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Rob,
>
> On Fri, Aug 18, 2023 at 6:17 PM Rob Herring <robh@kernel.org> wrote:
> > On Fri, Aug 18, 2023 at 10:36 AM Geert Uytterhoeven
> > <geert@linux-m68k.org> wrote:
> > > On Sat, Aug 5, 2023 at 12:42 AM 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
> > > > 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>
> > > > ---
> > > > 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!
> > >
> > > > --- a/drivers/of/dynamic.c
> > > > +++ b/drivers/of/dynamic.c
> > >
> > > > @@ -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",
> > >
> > > 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.
>
> 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
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2023-08-18 18:15 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-04 22:41 [PATCH v2 0/6] dt: changeset fixes and cleanups Rob Herring
2023-08-04 22:41 ` [PATCH v2 1/6] of: unittest: Fix EXPECT for parse_phandle_with_args_map() test Rob Herring
2023-08-18 15:27 ` Geert Uytterhoeven
2023-08-04 22:41 ` [PATCH v2 2/6] of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock Rob Herring
2023-08-07 15:30 ` Andy Shevchenko
2023-08-18 15:36 ` Geert Uytterhoeven
2023-08-18 16:16 ` Rob Herring
2023-08-18 17:36 ` Geert Uytterhoeven
2023-08-18 18:13 ` Rob Herring
2023-08-04 22:41 ` [PATCH v2 3/6] of: dynamic: Refactor changeset action printing to common helpers Rob Herring
2023-08-04 22:41 ` [PATCH v2 4/6] of: dynamic: Fix race in getting old property when updating property Rob Herring
2023-08-04 22:41 ` [PATCH v2 5/6] of: dynamic: Move dead property list check into property add/update functions Rob Herring
2023-08-07 15:37 ` Andy Shevchenko
2023-08-17 17:09 ` Rob Herring
2023-08-17 17:15 ` Andy Shevchenko
2023-08-04 22:41 ` [PATCH v2 6/6] of: Refactor node and property manipulation function locking Rob Herring
2023-08-07 15:40 ` Andy Shevchenko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.