dt: changeset fixes and cleanups
Rob Herring
maintainerv2 cover dt: changeset fixes and cleanups
Geert's locking fix[1] prompted my closer look at __of_changeset_entry_apply() and related functions. The result is a couple of fixes I found and some refactoring that unifies the "old dynamic API" and the changeset API implementations.
---
Changes in v2:
- Rework debug printing to fix issues with pr_debug() not having a
return value with dynamic debug
- Split action print refactoring into separate patch from fix
- Make removing property from deadprops a helper function
- Rework __of_add_property()/__of_update_property() exit code
- Link to v1: https://lore.kernel.org/r/[email protected]
---
Rob Herring (6):
of: unittest: Fix EXPECT for parse_phandle_with_args_map() test
of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock
of: dynamic: Refactor changeset action printing to common helpers
of: dynamic: Fix race in getting old property when updating property
of: dynamic: Move dead property list check into property add/update functions
of: Refactor node and property manipulation function locking
drivers/of/base.c | 87 ++++++++++++++++++----------- drivers/of/dynamic.c | 149 ++++++++++---------------------------------------- drivers/of/unittest.c | 4 +- 3 files changed, 86 insertions(+), 154 deletions(-)
---
base-commit: 66a4210bc82e024e6de0f94298ad9230984a5924
change-id: 20230801-dt-changeset-fixes-b76b88fecc43
Best regards,
Signature · 1 line
Rob Herring <[email protected]>
Rob Herring
maintainerPATCH v2 1/6 of: unittest: Fix EXPECT for parse_phandle_with_args_map() test
Commit 12e17243d8a1 ("of: base: improve error msg in of_phandle_iterator_next()") added printing of the phandle value on error, but failed to update the unittest.
--- drivers/of/unittest.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/of/unittest.c +2 -2
@@ -664,12 +664,12 @@ static void __init of_unittest_parse_phandle_with_args_map(void)memset (&args,0 ,sizeof (args));EXPECT_BEGIN (KERN_INFO , -"OF: /testcase-data/phandle-tests/consumer-b: could not find phandle" ); +"OF: /testcase-data/phandle-tests/consumer-b: could not find phandle 12345678" ); rc =of_parse_phandle_with_args_map (np,"phandle-list-bad-phandle" ,"phandle" ,0 , &args);EXPECT_END (KERN_INFO , -"OF: /testcase-data/phandle-tests/consumer-b: could not find phandle" ); +"OF: /testcase-data/phandle-tests/consumer-b: could not find phandle 12345678" );unittest (rc == -EINVAL ,"expected:%i got:%i\n" , -EINVAL , rc);
Signature · 1 line
2.40.1
Re: [PATCH v2 1/6] of: unittest: Fix EXPECT for parse_phandle_with_args_map() test
Quoted 6 lines · Rob Herring
Commit
12e17243d8a1("of: base: improve error msg inof_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 <[email protected]>
Gr{oetje,eeting}s,
Geert
Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected] In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Rob Herring
maintainerPATCH v2 2/6 of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock
In reply to Rob HerringWhile 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/0x3b4Fix 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.
---
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:
--- drivers/of/dynamic.c | 27 +++++---------------------- 1 file changed, 5 insertions(+), 22 deletions(-)
drivers/of/dynamic.c +5 -22
@@ -63,15 +63,13 @@ int of_reconfig_notifier_unregister(struct notifier_block *nb) }EXPORT_SYMBOL_GPL (of_reconfig_notifier_unregister); -#ifdef DEBUG -const char *action_names[] = { +static const char *action_names[] = { [OF_RECONFIG_ATTACH_NODE ] ="ATTACH_NODE" , [OF_RECONFIG_DETACH_NODE ] ="DETACH_NODE" , [OF_RECONFIG_ADD_PROPERTY ] ="ADD_PROPERTY" , [OF_RECONFIG_REMOVE_PROPERTY ] ="REMOVE_PROPERTY" , [OF_RECONFIG_UPDATE_PROPERTY ] ="UPDATE_PROPERTY" , }; -#endif int of_reconfig_notify (unsigned long action,struct of_reconfig_data *p) {
@@ -620,21 +618,9 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce) } ret =__of_add_property (ce->np, ce->prop); -if (ret) { -pr_err ("changeset: add_property failed @%pOF/%s\n" , - ce->np, - ce->prop->name); -break ; - }break ;case OF_RECONFIG_REMOVE_PROPERTY : ret =__of_remove_property (ce->np, ce->prop); -if (ret) { -pr_err ("changeset: remove_property failed @%pOF/%s\n" , - ce->np, - ce->prop->name); -break ; - }break ;case OF_RECONFIG_UPDATE_PROPERTY :
@@ -648,20 +634,17 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce) } ret =__of_update_property (ce->np, ce->prop, &old_prop); -if (ret) { -pr_err ("changeset: update_property failed @%pOF/%s\n" , - ce->np, - ce->prop->name); -break ; - }break ;default : ret = -EINVAL ; }raw_spin_unlock_irqrestore (&devtree_lock, flags); -if (ret) +if (ret) { +pr_err ("changeset: apply failed: cset<%p> %-15s %pOF:%s\n" , + ce, action_names[ce->action], ce->np, ce->prop->name);return ret; + }switch (ce->action) {case OF_RECONFIG_ATTACH_NODE :
Signature · 1 line
2.40.1
Re: [PATCH v2 2/6] of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock
On Fri, Aug 04, 2023 at 04:41:52PM -0600, Rob Herring wrote:
...
Quoted 6 lines
v5 (v2 in this series):
v4:
v3:
v1 and v2 from Geert simply moved thedevtree_lockinto each case statement:
https://lore.kernel.org/all/c593d8389352c574b5be69d4ca4810da13326a50.1690533838.git.geert+renesas@glider.be/
Side note: More natural to use main versioning as of this series and put the other to the parentheses.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v2 2/6] of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock
In reply to Rob HerringHi Rob,
Quoted 17 lines · Rob Herring
While originally it was fine to format strings using "%pOF" while holding
devtree_lock, this now causes a deadlock. Lockdep reports:of_get_parent from of_fwnode_get_parent+0x18/0x24 ^^^^^^^^^^^^^ of_fwnode_get_parent from fwnode_count_parents+0xc/0x28 fwnode_count_parents from fwnode_full_name_string+0x18/0xac fwnode_full_name_string from device_node_string+0x1a0/0x404 device_node_string from pointer+0x3c0/0x534 pointer from vsnprintf+0x248/0x36c vsnprintf from vprintk_store+0x130/0x3b4Fix this by moving the printing in
__of_changeset_entry_apply()outside the lock. As the only difference in the the multiple prints is the action name, use the existing "action_names" to refactor the prints into a single print.
Quoted 7 lines
Fixes a92eb7621b9fb2c2 ("lib/vsprintf: Make use of fwnode API to obtain node names and separators")Reported-by Geert Uytterhoeven <[email protected]>Signed-off-by Rob Herring <[email protected]>---
v5 (v2 in this series):
- Move majority of refactoring to separate patch and minimize the fix to just moving the print out of the locked section.
Thanks for your patch!
Quoted 12 lines
drivers/of/dynamic.c +0
-5@@ -648,20 +634,17 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce) } ret =__of_update_property (ce->np, ce->prop, &old_prop); -if (ret) { -pr_err ("changeset: update_property failed @%pOF/%s\n" , - ce->np, - ce->prop->name); -break ;
drivers/of/dynamic.c static int __of_changeset_entry_apply(struct of_changeset_entry *ce) · line 644
Quoted 10 lines
drivers/of/dynamic.c +2
-2- }break ;default : ret = -EINVAL ; }raw_spin_unlock_irqrestore (&devtree_lock, flags); -if (ret) +if (ret) { + pr_err("changeset: apply failed: cset<%p> %-15s %pOF:%s\n",
Printing the cset pointer will (needlessly?) complicate the EXPECT_*() handling in the unit test.
drivers/of/dynamic.c static int __of_changeset_entry_apply(struct of_changeset_entry *ce) · line 645
Quoted 1 line
drivers/of/dynamic.c +1
-0+ ce, action_names[ce->action], ce->np, ce->prop->name);
This should check ce->action to avoid an out-of-bounds access beyond the end of action_names[].
drivers/of/dynamic.c static int __of_changeset_entry_apply(struct of_changeset_entry *ce) · line 650
Quoted 5 lines
drivers/of/dynamic.c +1
-0return ret; + }switch (ce->action) {case OF_RECONFIG_ATTACH_NODE :
The rest LGTM to me.
Gr{oetje,eeting}s,
Geert
Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected] In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Rob Herring
maintainerRe: [PATCH v2 2/6] of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock
Quoted 50 lines · Geert Uytterhoeven
Hi Rob,
Quoted 17 lines · Rob Herring
While originally it was fine to format strings using "%pOF" while holding
devtree_lock, this now causes a deadlock. Lockdep reports:of_get_parent from of_fwnode_get_parent+0x18/0x24 ^^^^^^^^^^^^^ of_fwnode_get_parent from fwnode_count_parents+0xc/0x28 fwnode_count_parents from fwnode_full_name_string+0x18/0xac fwnode_full_name_string from device_node_string+0x1a0/0x404 device_node_string from pointer+0x3c0/0x534 pointer from vsnprintf+0x248/0x36c vsnprintf from vprintk_store+0x130/0x3b4Fix this by moving the printing in
__of_changeset_entry_apply()outside the lock. As the only difference in the the multiple prints is the action name, use the existing "action_names" to refactor the prints into a single print.Quoted 7 lines
Fixes a92eb7621b9fb2c2 ("lib/vsprintf: Make use of fwnode API to obtain node names and separators")Reported-by Geert Uytterhoeven <[email protected]>Signed-off-by Rob Herring <[email protected]>---
v5 (v2 in this series):
- Move majority of refactoring to separate patch and minimize the fix to just moving the print out of the locked section.Thanks for your patch!
Quoted 19 lines
drivers/of/dynamic.c +0
-6@@ -648,20 +634,17 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce) } ret =__of_update_property (ce->np, ce->prop, &old_prop); -if (ret) { -pr_err ("changeset: update_property failed @%pOF/%s\n" , - ce->np, - ce->prop->name); -break ; - }break ;default : ret = -EINVAL ; }raw_spin_unlock_irqrestore (&devtree_lock, flags);
Quoted 6 lines
drivers/of/dynamic.c static int __of_changeset_entry_apply(struct of_changeset_entry *ce) · line 644
Quoted 3 lines
drivers/of/dynamic.c +2
-1-if (ret) +if (ret) { + pr_err("changeset: apply failed: cset<%p> %-15s %pOF:%s\n",Printing the cset pointer will (needlessly?) complicate the EXPECT_*() handling in the unit test.
That's added largely because the other prints which I rework later in this series had them. Either printing the changeset ptr is useful or it isn't. I think people running the unittest and the post-processor can easily enough filter this out when looking at the results. Honestly, even I probably run it less than once a cycle.
Quoted 5 lines
drivers/of/dynamic.c static int __of_changeset_entry_apply(struct of_changeset_entry *ce) · line 645
Quoted 2 lines
drivers/of/dynamic.c +1
-0+ ce, action_names[ce->action], ce->np, ce->prop->name);This should check
ce->actionto 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
Re: [PATCH v2 2/6] of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock
Hi Rob,
Quoted 56 lines · Rob Herring
Quoted 47 lines · Geert Uytterhoeven
Quoted 17 lines · Rob Herring
While originally it was fine to format strings using "%pOF" while holding
devtree_lock, this now causes a deadlock. Lockdep reports:of_get_parent from of_fwnode_get_parent+0x18/0x24 ^^^^^^^^^^^^^ of_fwnode_get_parent from fwnode_count_parents+0xc/0x28 fwnode_count_parents from fwnode_full_name_string+0x18/0xac fwnode_full_name_string from device_node_string+0x1a0/0x404 device_node_string from pointer+0x3c0/0x534 pointer from vsnprintf+0x248/0x36c vsnprintf from vprintk_store+0x130/0x3b4Fix this by moving the printing in
__of_changeset_entry_apply()outside the lock. As the only difference in the the multiple prints is the action name, use the existing "action_names" to refactor the prints into a single print.Quoted 7 lines
Fixes a92eb7621b9fb2c2 ("lib/vsprintf: Make use of fwnode API to obtain node names and separators")Reported-by Geert Uytterhoeven <[email protected]>Signed-off-by Rob Herring <[email protected]>---
v5 (v2 in this series):
- Move majority of refactoring to separate patch and minimize the fix to just moving the print out of the locked section.Thanks for your patch!
Quoted 19 lines
drivers/of/dynamic.c +0
-6@@ -648,20 +634,17 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce) } ret =__of_update_property (ce->np, ce->prop, &old_prop); -if (ret) { -pr_err ("changeset: update_property failed @%pOF/%s\n" , - ce->np, - ce->prop->name); -break ; - }break ;default : ret = -EINVAL ; }raw_spin_unlock_irqrestore (&devtree_lock, flags);Quoted 6 lines
drivers/of/dynamic.c static int __of_changeset_entry_apply(struct of_changeset_entry *ce) · line 644
Quoted 3 lines
drivers/of/dynamic.c +2
-1-if (ret) +if (ret) { + pr_err("changeset: apply failed: cset<%p> %-15s %pOF:%s\n",Printing the cset pointer will (needlessly?) complicate the EXPECT_*() handling in the unit test.
Quoted 5 lines
That's added largely because the other prints which I rework later in this series had them. Either printing the changeset ptr is useful or it isn't. I think people running the unittest and the post-processor can easily enough filter this out when looking at the results. Honestly, even I probably run it less than once a cycle.
Do you have a use for printing the pointer value? And by default, it will be an obfuscated cookie anyway.
Quoted 9 lines
Quoted 4 lines
drivers/of/dynamic.c static int __of_changeset_entry_apply(struct of_changeset_entry *ce) · line 645
Quoted 1 line
drivers/of/dynamic.c +1
-0+ ce, action_names[ce->action], ce->np, ce->prop->name);This should check
ce->actionto avoid an out-of-bounds access beyond the end of action_names[].Indeed.
I think I'll add "invalid" to
action_namesnames and then do something
like: "(ce->action< FOO) ?ce->action: 0".
OK, zero is invalid.
Gr{oetje,eeting}s,
Geert
Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected] In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Rob Herring
maintainerRe: [PATCH v2 2/6] of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock
Quoted 60 lines · Geert Uytterhoeven
Hi Rob,
Quoted 56 lines · Rob Herring
Quoted 53 lines · Geert Uytterhoeven
Quoted 17 lines · Rob Herring
While originally it was fine to format strings using "%pOF" while holding
devtree_lock, this now causes a deadlock. Lockdep reports:of_get_parent from of_fwnode_get_parent+0x18/0x24 ^^^^^^^^^^^^^ of_fwnode_get_parent from fwnode_count_parents+0xc/0x28 fwnode_count_parents from fwnode_full_name_string+0x18/0xac fwnode_full_name_string from device_node_string+0x1a0/0x404 device_node_string from pointer+0x3c0/0x534 pointer from vsnprintf+0x248/0x36c vsnprintf from vprintk_store+0x130/0x3b4Fix this by moving the printing in
__of_changeset_entry_apply()outside the lock. As the only difference in the the multiple prints is the action name, use the existing "action_names" to refactor the prints into a single print.Quoted 7 lines
Fixes a92eb7621b9fb2c2 ("lib/vsprintf: Make use of fwnode API to obtain node names and separators")Reported-by Geert Uytterhoeven <[email protected]>Signed-off-by Rob Herring <[email protected]>---
v5 (v2 in this series):
- Move majority of refactoring to separate patch and minimize the fix to just moving the print out of the locked section.Thanks for your patch!
Quoted 12 lines
drivers/of/dynamic.c +0
-5@@ -648,20 +634,17 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce) } ret =__of_update_property (ce->np, ce->prop, &old_prop); -if (ret) { -pr_err ("changeset: update_property failed @%pOF/%s\n" , - ce->np, - ce->prop->name); -break ;drivers/of/dynamic.c static int __of_changeset_entry_apply(struct of_changeset_entry *ce) · line 644
Quoted 10 lines
drivers/of/dynamic.c +2
-2- }break ;default : ret = -EINVAL ; }raw_spin_unlock_irqrestore (&devtree_lock, flags); -if (ret) +if (ret) { + pr_err("changeset: apply failed: cset<%p> %-15s %pOF:%s\n",Printing the cset pointer will (needlessly?) complicate the EXPECT_*() handling in the unit test.
Quoted 7 lines
Quoted 5 lines
That's added largely because the other prints which I rework later in this series had them. Either printing the changeset ptr is useful or it isn't. I think people running the unittest and the post-processor can easily enough filter this out when looking at the results. Honestly, even I probably run it less than once a cycle.
Do you have a use for printing the pointer value?
I have no use for overlays in general, so no. :)
I'd assumed it was there to provide a changeset ID to tell which actions belong to the same changeset. But it's printing the changeset entry rather than the changeset, so I agree it is not really useful.
Rob
Rob Herring
maintainerPATCH v2 3/6 of: dynamic: Refactor changeset action printing to common helpers
In reply to Rob HerringSeveral places print the changeset action with node and property details. Refactor these into a common printing helper. The complicating factor is some prints are debug and some are errors. Solve this with a bit of preprocessor magic.
---
v2:
- Split out refactoring from fix in prior patch
- Avoid pr_cont and relying on pr_debug return value which dynamic debug doesn't have
--- drivers/of/dynamic.c | 53 +++++++++++----------------------------------------- 1 file changed, 11 insertions(+), 42 deletions(-)
drivers/of/dynamic.c +11 -42
@@ -71,27 +71,21 @@ static const char *action_names[] = { [OF_RECONFIG_UPDATE_PROPERTY] = "UPDATE_PROPERTY", }; +#define _do_print (func, prefix, action, node, prop, ...) ({ \ + func("changeset: " prefix "%-15s %pOF%s%s\n", \ + ##__VA_ARGS__, action_names[action], node, \ + prop ? ":" : "", prop ? prop->name : ""); \ +}) +#define of_changeset_action_err (...) _do_print(pr_err, __VA_ARGS__) +#define of_changeset_action_debug (...) _do_print(pr_debug, __VA_ARGS__) +int of_reconfig_notify (unsigned long action,struct of_reconfig_data *p) {int rc; -#ifdef DEBUG struct of_reconfig_data *pr = p; -switch (action) { -case OF_RECONFIG_ATTACH_NODE : -case OF_RECONFIG_DETACH_NODE : -pr_debug ("notify %-15s %pOF\n" , action_names[action], - pr->dn); -break ; -case OF_RECONFIG_ADD_PROPERTY : -case OF_RECONFIG_REMOVE_PROPERTY : -case OF_RECONFIG_UPDATE_PROPERTY : -pr_debug ("notify %-15s %pOF:%s\n" , action_names[action], - pr->dn, pr->prop->name); -break ; +of_changeset_action_debug ("notify: " , action, pr->dn, pr->prop); - } -#endif rc =blocking_notifier_call_chain (&of_reconfig_chain, action, p);return notifier_to_errno (rc); }
@@ -502,30 +496,6 @@ static void __of_changeset_entry_destroy(struct of_changeset_entry *ce)kfree (ce); } -#ifdef DEBUG -static void __of_changeset_entry_dump (struct of_changeset_entry *ce) -{ -switch (ce->action) { -case OF_RECONFIG_ADD_PROPERTY : -case OF_RECONFIG_REMOVE_PROPERTY : -case OF_RECONFIG_UPDATE_PROPERTY : -pr_debug ("cset<%p> %-15s %pOF/%s\n" , ce, action_names[ce->action], - ce->np, ce->prop->name); -break ; -case OF_RECONFIG_ATTACH_NODE : -case OF_RECONFIG_DETACH_NODE : -pr_debug ("cset<%p> %-15s %pOF\n" , ce, action_names[ce->action], - ce->np); -break ; - } -} -#else -static inline void __of_changeset_entry_dump (struct of_changeset_entry *ce) -{ -/* empty */ -} -#endif -static void __of_changeset_entry_invert (struct of_changeset_entry *ce,struct of_changeset_entry *rce) {
@@ -597,7 +567,7 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)unsigned long flags;int ret =0 ; -__of_changeset_entry_dump (ce); +of_changeset_action_debug ("applying: cset<%p> " , ce->action, ce->np, ce->prop, ce);raw_spin_lock_irqsave (&devtree_lock, flags);switch (ce->action) {
@@ -641,8 +611,7 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)raw_spin_unlock_irqrestore (&devtree_lock, flags);if (ret) { -pr_err ("changeset: apply failed: cset<%p> %-15s %pOF:%s\n" , - ce, action_names[ce->action], ce->np, ce->prop->name); +of_changeset_action_err ("apply failed: cset<%p> " , ce->action, ce->np, ce->prop, ce);return ret; }
Signature · 1 line
2.40.1
Rob Herring
maintainerPATCH v2 4/6 of: dynamic: Fix race in getting old property when updating property
In reply to Rob Herring__of_update_property() returns the existing property if there is one, but that value is never added to the changeset. Updates work because the existing property is also retrieved in of_changeset_action(), but that is racy as of_changeset_action() doesn't hold any locks. The property could be changed before the changeset is applied.
--- drivers/of/dynamic.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
drivers/of/dynamic.c +2 -5
@@ -563,7 +563,7 @@ static int __of_changeset_entry_notify(struct of_changeset_entry *ce,static int __of_changeset_entry_apply (struct of_changeset_entry *ce) { -struct property *old_prop, **propp; +struct property **propp;unsigned long flags;int ret =0 ;
@@ -603,7 +603,7 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce) } } - ret =__of_update_property (ce->np, ce->prop, &old_prop); + ret =__of_update_property (ce->np, ce->prop, &ce->old_prop);break ;default : ret = -EINVAL ;
@@ -904,9 +904,6 @@ int of_changeset_action(struct of_changeset *ocs, unsigned long action, ce->np =of_node_get (np); ce->prop = prop; -if (action ==OF_RECONFIG_UPDATE_PROPERTY && prop) - ce->old_prop =of_find_property (np, prop->name,NULL ); -/* add it to the list */ list_add_tail (&ce->node, &ocs->entries);return 0 ;
Signature · 1 line
2.40.1
Rob Herring
maintainerPATCH v2 5/6 of: dynamic: Move dead property list check into property add/update functions
In reply to Rob HerringThe changeset code checks for a property in the deadprops list when adding/updating a property, but of_add_property() and of_update_property() do not. As the users of these functions are pretty simple, they have not hit this scenario or else the property lists would get corrupted.
---
v2:
- Add helper to remove property from deadprops list
--- drivers/of/base.c | 19 +++++++++++++++++++ drivers/of/dynamic.c | 19 ------------------- 2 files changed, 19 insertions(+), 19 deletions(-)
drivers/of/base.c +19 -0
@@ -1530,6 +1530,21 @@ int of_count_phandle_with_args(const struct device_node *np, const char *list_na }EXPORT_SYMBOL (of_count_phandle_with_args); +static void __of_remove_dead_property (struct device_node *np,struct property *prop) +{ +struct property **next; + +/* If the property is in deadprops then it must be removed */ +for (next = &np->deadprops; *next; next = &(*next)->next) { +if (*next != prop) +continue ; + + *next = prop->next; + prop->next =NULL ; +break ; + } +} +/** * __of_add_property - Add a property to a node without lock operations * @np: Caller's Device Node
@@ -1539,6 +1554,8 @@ int __of_add_property(struct device_node *np, struct property *prop) {struct property **next; +__of_remove_dead_property (np, prop); + prop->next =NULL ; next = &np->properties;while (*next) {
@@ -1641,6 +1658,8 @@ int __of_update_property(struct device_node *np, struct property *newprop, {struct property **next, *oldprop; +__of_remove_dead_property (np, newprop); +for (next = &np->properties; *next; next = &(*next)->next) {if (of_prop_cmp ((*next)->name, newprop->name) ==0 )break ;
drivers/of/dynamic.c +0 -19
@@ -563,7 +563,6 @@ static int __of_changeset_entry_notify(struct of_changeset_entry *ce,static int __of_changeset_entry_apply (struct of_changeset_entry *ce) { -struct property **propp;unsigned long flags;int ret =0 ;
@@ -578,15 +577,6 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)__of_detach_node (ce->np);break ;case OF_RECONFIG_ADD_PROPERTY : -/* If the property is in deadprops then it must be removed */ -for (propp = &ce->np->deadprops; *propp; propp = &(*propp)->next) { -if (*propp == ce->prop) { - *propp = ce->prop->next; - ce->prop->next =NULL ; -break ; - } - } - ret =__of_add_property (ce->np, ce->prop);break ;case OF_RECONFIG_REMOVE_PROPERTY :
@@ -594,15 +584,6 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)break ;case OF_RECONFIG_UPDATE_PROPERTY : -/* If the property is in deadprops then it must be removed */ -for (propp = &ce->np->deadprops; *propp; propp = &(*propp)->next) { -if (*propp == ce->prop) { - *propp = ce->prop->next; - ce->prop->next =NULL ; -break ; - } - } - ret =__of_update_property (ce->np, ce->prop, &ce->old_prop);break ;default :
Signature · 1 line
2.40.1
Re: [PATCH v2 5/6] of: dynamic: Move dead property list check into property add/update functions
Quoted 5 lines · Rob Herring
The changeset code checks for a property in the deadprops list when adding/updating a property, but
of_add_property()andof_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.
...
drivers/of/base.c int of_count_phandle_with_args(const struct device_node *np, const char *list_na · line 1544
Quoted 12 lines
drivers/of/base.c +12
-0+static void __of_remove_dead_property (struct device_node *np,struct property *prop) +{ +struct property **next; + +/* If the property is in deadprops then it must be removed */ +for (next = &np->deadprops; *next; next = &(*next)->next) { +if (*next != prop) +continue ; + + *next = prop->next; + prop->next =NULL ; +break ;
Why not
if (*next == prop) {
*next = prop->next;
prop->next = NULL;
break;
}which seems to me clearer?
Quoted 2 lines
drivers/of/base.c +2
-0+ } +}
Signature · 2 lines
With Best Regards, Andy Shevchenko
Rob Herring
maintainerRe: [PATCH v2 5/6] of: dynamic: Move dead property list check into property add/update functions
Quoted 8 lines · Andy Shevchenko
Quoted 5 lines · Rob Herring
The changeset code checks for a property in the deadprops list when adding/updating a property, but
of_add_property()andof_update_property()do not. As the users of these functions are pretty simple, they have not hit this scenario or else the property lists would get corrupted.Suggested-by ? :-)
Humm, by me? The change in behavior and point of this patch comes from me. You've provided review comments which will get covered by a Reviewed-by I presume.
Quoted 18 lines
...
drivers/of/base.c int of_count_phandle_with_args(const struct device_node *np, const char *list_na · line 1544
Quoted 12 lines
drivers/of/base.c +12
-0+static void __of_remove_dead_property (struct device_node *np,struct property *prop) +{ +struct property **next; + +/* If the property is in deadprops then it must be removed */ +for (next = &np->deadprops; *next; next = &(*next)->next) { +if (*next != prop) +continue ; + + *next = prop->next; + prop->next =NULL ; +break ;Why not
Quoted 7 lines
if (*next == prop) { *next = prop->next; prop->next = NULL; break; }which seems to me clearer?
Sure. I like the style I wrote, but whichever way ends the discussion is fine for me.
Rob
Re: [PATCH v2 5/6] of: dynamic: Move dead property list check into property add/update functions
Quoted 2 lines · Rob Herring
Quoted 1 line · Andy Shevchenko
On Fri, Aug 04, 2023 at 04:41:55PM -0600, Rob Herring wrote:
...
Quoted 5 lines
Quoted 1 line
Suggested-by ? :-)Humm, by me? The change in behavior and point of this patch comes from me. You've provided review comments which will get covered by a Reviewed-by I presume.
OK!
Signature · 2 lines
With Best Regards, Andy Shevchenko
Rob Herring
maintainerPATCH v2 6/6 of: Refactor node and property manipulation function locking
In reply to Rob HerringAll callers of __of_{add,remove,update}_property() and __of_{attach,detach}_node() wrap the call with the devtree_lock spinlock. Let's move the spinlock into the functions. This allows moving the sysfs update functions into those functions as well.
---
v2:
- Re-arrange exit handling
--- drivers/of/base.c | 68 +++++++++++++++++++++++++++------------------------- drivers/of/dynamic.c | 51 +++++++++++++-------------------------- 2 files changed, 51 insertions(+), 68 deletions(-)
drivers/of/base.c +35 -33
@@ -1552,21 +1552,32 @@ static void __of_remove_dead_property(struct device_node *np, struct property *p*/ int __of_add_property (struct device_node *np,struct property *prop) { +int rc =0 ; +unsigned long flags;struct property **next; +raw_spin_lock_irqsave (&devtree_lock, flags); +__of_remove_dead_property (np, prop); prop->next =NULL ; next = &np->properties;while (*next) { -if (strcmp (prop->name, (*next)->name) ==0 ) +if (strcmp (prop->name, (*next)->name) ==0 ) {/* duplicate ! don't insert it */ -return -EEXIST ; - + rc = -EEXIST ; + goto out_unlock; + } next = &(*next)->next; } *next = prop; +out_unlock: +raw_spin_unlock_irqrestore (&devtree_lock, flags); +if (rc) +return rc; + +__of_add_property_sysfs (np, prop);return 0 ; }
@@ -1577,23 +1588,12 @@ int __of_add_property(struct device_node *np, struct property *prop)*/ int of_add_property (struct device_node *np,struct property *prop) { -unsigned long flags;int rc;mutex_lock (&of_mutex); - -raw_spin_lock_irqsave (&devtree_lock, flags); rc =__of_add_property (np, prop); -raw_spin_unlock_irqrestore (&devtree_lock, flags); - -if (!rc) -__of_add_property_sysfs (np, prop); -mutex_unlock (&of_mutex); -if (!rc) -of_property_notify (OF_RECONFIG_ADD_PROPERTY , np, prop,NULL ); -return rc; }EXPORT_SYMBOL_GPL (of_add_property);
@@ -1601,19 +1601,30 @@ EXPORT_SYMBOL_GPL(of_add_property);int __of_remove_property (struct device_node *np,struct property *prop) {struct property **next; +unsigned long flags; +int rc =0 ; + +raw_spin_lock_irqsave (&devtree_lock, flags);for (next = &np->properties; *next; next = &(*next)->next) {if (*next == prop)break ; } -if (*next ==NULL ) -return -ENODEV ; - +if (*next ==NULL ) { + rc = -ENODEV ; + goto out_unlock; + }/* found the node */ *next = prop->next; prop->next = np->deadprops; np->deadprops = prop; +out_unlock: +raw_spin_unlock_irqrestore (&devtree_lock, flags); +if (rc) +return rc; + +__of_remove_property_sysfs (np, prop);return 0 ; }
@@ -1629,21 +1640,13 @@ int __of_remove_property(struct device_node *np, struct property *prop)*/ int of_remove_property (struct device_node *np,struct property *prop) { -unsigned long flags;int rc;if (!prop)return -ENODEV ;mutex_lock (&of_mutex); - -raw_spin_lock_irqsave (&devtree_lock, flags); rc =__of_remove_property (np, prop); -raw_spin_unlock_irqrestore (&devtree_lock, flags); - -if (!rc) -__of_remove_property_sysfs (np, prop); -mutex_unlock (&of_mutex);if (!rc)
@@ -1657,6 +1660,9 @@ int __of_update_property(struct device_node *np, struct property *newprop, struct property **oldpropp) {struct property **next, *oldprop; +unsigned long flags; + +raw_spin_lock_irqsave (&devtree_lock, flags);__of_remove_dead_property (np, newprop);
@@ -1678,6 +1684,10 @@ int __of_update_property(struct device_node *np, struct property *newprop, *next = newprop; } +raw_spin_unlock_irqrestore (&devtree_lock, flags); + +__of_update_property_sysfs (np, newprop, oldprop); +return 0 ; }
@@ -1693,21 +1703,13 @@ int __of_update_property(struct device_node *np, struct property *newprop,int of_update_property (struct device_node *np,struct property *newprop) {struct property *oldprop; -unsigned long flags;int rc;if (!newprop->name)return -EINVAL ;mutex_lock (&of_mutex); - -raw_spin_lock_irqsave (&devtree_lock, flags); rc =__of_update_property (np, newprop, &oldprop); -raw_spin_unlock_irqrestore (&devtree_lock, flags); - -if (!rc) -__of_update_property_sysfs (np, newprop, oldprop); -mutex_unlock (&of_mutex);if (!rc)
drivers/of/dynamic.c +16 -35
125 lines not on this page · raw message
Signature · 1 line
2.40.1
Re: [PATCH v2 6/6] of: Refactor node and property manipulation function locking
Quoted 4 lines · Rob Herring
All callers of __of_{add,remove,update}_property() and __of_{attach,detach}_node() wrap the call with the
devtree_lockspinlock. Let's move the spinlock into the functions. This allows moving the sysfs update functions into those functions as well.
...
Quoted 4 lines
drivers/of/base.c +0
-0for (next = &np->properties; *next; next = &(*next)->next) {if (*next == prop)break ; }
drivers/of/base.c EXPORT_SYMBOL_GPL(of_add_property); · line 1619
Quoted 10 lines
drivers/of/base.c +4
-3-if (*next ==NULL ) -return -ENODEV ; - +if (*next ==NULL ) { + rc = -ENODEV ; + goto out_unlock; + }/* found the node */ *next = prop->next; prop->next = np->deadprops;
But this also looks like a dup for deadprop, maybe something like
static int __of_remove_property_dead_or_alive() // pun intended.
?
Signature · 2 lines
With Best Regards, Andy Shevchenko