[PATCH 0/5] dt: changeset fixes and cleanups

[PATCH 0/5] dt: changeset fixes and cleanups

[Rob Herring]

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 API" 
and the changeset API.

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

Signed-off-by: Rob Herring <robh@kernel.org>
---
Rob Herring (5):
      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: 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     |  86 ++++++++++++++++------------
 drivers/of/dynamic.c  | 153 +++++++++++---------------------------------------
 drivers/of/unittest.c |   4 +-
 3 files changed, 87 insertions(+), 156 deletions(-)
---
base-commit: e251a4e28a27884e8bfb7fccbf53b24736f3ef87
change-id: 20230801-dt-changeset-fixes-b76b88fecc43

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

[PATCH 1/5] of: unittest: Fix EXPECT for parse_phandle_with_args_map() test

[Rob Herring]

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

Fixes: 12e17243d8a1 ("of: base: improve error msg in of_phandle_iterator_next()")
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

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

[Rob Herring]

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

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

To fix this, move the printing in __of_changeset_entry_apply() outside the
lock. As there's already similar printing of the same changeset actions,
refactor all of them to use a common action print function. This has the
side benefit of getting rid of some ifdefs.

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>
---
v3:
 - Add missing 'static' reported by 0-day

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 | 80 ++++++++++++----------------------------------------
 1 file changed, 18 insertions(+), 62 deletions(-)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index e311d406b170..aa3821836937 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -63,37 +63,31 @@ 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
+
+static void of_changeset_action_print(unsigned long action, struct device_node *np,
+				      const char *prop_name)
+{
+	if (prop_name)
+		pr_cont("%-15s %pOF:%s\n", action_names[action], np, prop_name);
+	else
+		pr_cont("%-15s %pOF\n", action_names[action], np);
+}
 
 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;
+	if (pr_debug("notify "))
+		of_changeset_action_print(action, pr->dn, pr->prop ? pr->prop->name : NULL);
 
-	}
-#endif
 	rc = blocking_notifier_call_chain(&of_reconfig_chain, action, p);
 	return notifier_to_errno(rc);
 }
@@ -504,30 +498,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)
 {
@@ -599,7 +569,8 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 	unsigned long flags;
 	int ret = 0;
 
-	__of_changeset_entry_dump(ce);
+	if (pr_debug("changeset: applying: cset<%p> ", ce))
+		of_changeset_action_print(ce->action, ce->np, ce->prop ? ce->prop->name : NULL);
 
 	raw_spin_lock_irqsave(&devtree_lock, flags);
 	switch (ce->action) {
@@ -620,21 +591,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 +607,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> ", ce);
+		of_changeset_action_print(ce->action, ce->np, ce->prop ? ce->prop->name : NULL);
 		return ret;
+	}
 
 	switch (ce->action) {
 	case OF_RECONFIG_ATTACH_NODE:

-- 
2.40.1

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

[kernel test robot]

Hi Rob,

kernel test robot noticed the following build errors:

[auto build test ERROR on e251a4e28a27884e8bfb7fccbf53b24736f3ef87]

url:    https://github.com/intel-lab-lkp/linux/commits/Rob-Herring/of-unittest-Fix-EXPECT-for-parse_phandle_with_args_map-test/20230802-055739
base:   e251a4e28a27884e8bfb7fccbf53b24736f3ef87
patch link:    https://lore.kernel.org/r/20230801-dt-changeset-fixes-v1-2-b5203e3fc22f%40kernel.org
patch subject: [PATCH 2/5] of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock
config: loongarch-allyesconfig (https://download.01.org/0day-ci/archive/20230802/202308021009.r1Hzc7YD-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230802/202308021009.r1Hzc7YD-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308021009.r1Hzc7YD-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from include/linux/printk.h:564,
                    from include/asm-generic/bug.h:22,
                    from arch/loongarch/include/asm/bug.h:59,
                    from include/linux/bug.h:5,
                    from include/linux/thread_info.h:13,
                    from include/asm-generic/current.h:5,
                    from ./arch/loongarch/include/generated/asm/current.h:1,
                    from include/linux/mutex.h:14,
                    from include/linux/kernfs.h:11,
                    from include/linux/sysfs.h:16,
                    from include/linux/kobject.h:20,
                    from include/linux/of.h:17,
                    from drivers/of/dynamic.c:12:
   drivers/of/dynamic.c: In function 'of_reconfig_notify':
>> include/linux/dynamic_debug.h:219:58: error: expected expression before 'do'
     219 | #define __dynamic_func_call_cls(id, cls, fmt, func, ...) do {   \
         |                                                          ^~
   include/linux/dynamic_debug.h:246:9: note: in expansion of macro '__dynamic_func_call_cls'
     246 |         __dynamic_func_call_cls(__UNIQUE_ID(ddebug), cls, fmt, func, ##__VA_ARGS__)
         |         ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:248:9: note: in expansion of macro '_dynamic_func_call_cls'
     248 |         _dynamic_func_call_cls(_DPRINTK_CLASS_DFLT, fmt, func, ##__VA_ARGS__)
         |         ^~~~~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:267:9: note: in expansion of macro '_dynamic_func_call'
     267 |         _dynamic_func_call(fmt, __dynamic_pr_debug,             \
         |         ^~~~~~~~~~~~~~~~~~
   include/linux/printk.h:579:9: note: in expansion of macro 'dynamic_pr_debug'
     579 |         dynamic_pr_debug(fmt, ##__VA_ARGS__)
         |         ^~~~~~~~~~~~~~~~
   drivers/of/dynamic.c:88:13: note: in expansion of macro 'pr_debug'
      88 |         if (pr_debug("notify "))
         |             ^~~~~~~~
   drivers/of/dynamic.c: In function '__of_changeset_entry_apply':
>> include/linux/dynamic_debug.h:219:58: error: expected expression before 'do'
     219 | #define __dynamic_func_call_cls(id, cls, fmt, func, ...) do {   \
         |                                                          ^~
   include/linux/dynamic_debug.h:246:9: note: in expansion of macro '__dynamic_func_call_cls'
     246 |         __dynamic_func_call_cls(__UNIQUE_ID(ddebug), cls, fmt, func, ##__VA_ARGS__)
         |         ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:248:9: note: in expansion of macro '_dynamic_func_call_cls'
     248 |         _dynamic_func_call_cls(_DPRINTK_CLASS_DFLT, fmt, func, ##__VA_ARGS__)
         |         ^~~~~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:267:9: note: in expansion of macro '_dynamic_func_call'
     267 |         _dynamic_func_call(fmt, __dynamic_pr_debug,             \
         |         ^~~~~~~~~~~~~~~~~~
   include/linux/printk.h:579:9: note: in expansion of macro 'dynamic_pr_debug'
     579 |         dynamic_pr_debug(fmt, ##__VA_ARGS__)
         |         ^~~~~~~~~~~~~~~~
   drivers/of/dynamic.c:572:13: note: in expansion of macro 'pr_debug'
     572 |         if (pr_debug("changeset: applying: cset<%p> ", ce))
         |             ^~~~~~~~


vim +/do +219 include/linux/dynamic_debug.h

9049fc745300c5 Jason Baron      2016-08-03  207  
ca90fca7f7b518 Jim Cromie       2022-09-04  208  /*
ca90fca7f7b518 Jim Cromie       2022-09-04  209   * Factory macros: ($prefix)dynamic_func_call($suffix)
ca90fca7f7b518 Jim Cromie       2022-09-04  210   *
ca90fca7f7b518 Jim Cromie       2022-09-04  211   * Lower layer (with __ prefix) gets the callsite metadata, and wraps
ca90fca7f7b518 Jim Cromie       2022-09-04  212   * the func inside a debug-branch/static-key construct.  Upper layer
ca90fca7f7b518 Jim Cromie       2022-09-04  213   * (with _ prefix) does the UNIQUE_ID once, so that lower can ref the
ca90fca7f7b518 Jim Cromie       2022-09-04  214   * name/label multiple times, and tie the elements together.
ca90fca7f7b518 Jim Cromie       2022-09-04  215   * Multiple flavors:
ca90fca7f7b518 Jim Cromie       2022-09-04  216   * (|_cls):	adds in _DPRINT_CLASS_DFLT as needed
ca90fca7f7b518 Jim Cromie       2022-09-04  217   * (|_no_desc):	former gets callsite descriptor as 1st arg (for prdbgs)
ca90fca7f7b518 Jim Cromie       2022-09-04  218   */
ca90fca7f7b518 Jim Cromie       2022-09-04 @219  #define __dynamic_func_call_cls(id, cls, fmt, func, ...) do {	\
ca90fca7f7b518 Jim Cromie       2022-09-04  220  	DEFINE_DYNAMIC_DEBUG_METADATA_CLS(id, cls, fmt);	\
47cdd64be4832f Rasmus Villemoes 2019-03-07  221  	if (DYNAMIC_DEBUG_BRANCH(id))				\
47cdd64be4832f Rasmus Villemoes 2019-03-07  222  		func(&id, ##__VA_ARGS__);			\
e9d376f0fa66bd Jason Baron      2009-02-05  223  } while (0)
ca90fca7f7b518 Jim Cromie       2022-09-04  224  #define __dynamic_func_call(id, fmt, func, ...)				\
ca90fca7f7b518 Jim Cromie       2022-09-04  225  	__dynamic_func_call_cls(id, _DPRINTK_CLASS_DFLT, fmt,		\
ca90fca7f7b518 Jim Cromie       2022-09-04  226  				func, ##__VA_ARGS__)
e9d376f0fa66bd Jason Baron      2009-02-05  227  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Andy Shevchenko]

On Tue, Aug 01, 2023 at 03:54:45PM -0600, Rob Herring 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
> 
> To fix this, move the printing in __of_changeset_entry_apply() outside the
> lock. As there's already similar printing of the same changeset actions,
> refactor all of them to use a common action print function. This has the
> side benefit of getting rid of some ifdefs.

...

> v3:
>  - Add missing 'static' reported by 0-day

It reported two issues (at least what I see).

...

> +	if (pr_debug("notify "))

This is weird. How did you compile it?

> +		of_changeset_action_print(action, pr->dn, pr->prop ? pr->prop->name : NULL);

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, Aug 02, 2023 at 06:28:48AM +0300, Andy Shevchenko wrote:
> On Tue, Aug 01, 2023 at 03:54:45PM -0600, Rob Herring 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
> > 
> > To fix this, move the printing in __of_changeset_entry_apply() outside the
> > lock. As there's already similar printing of the same changeset actions,
> > refactor all of them to use a common action print function. This has the
> > side benefit of getting rid of some ifdefs.

...

> > v3:
> >  - Add missing 'static' reported by 0-day
> 
> It reported two issues (at least what I see).

...

> > +	if (pr_debug("notify "))
> 
> This is weird. How did you compile it?

Urgh, you need to fix dynamic debug macros to return an error code.

> > +		of_changeset_action_print(action, pr->dn, pr->prop ? pr->prop->name : NULL);

-- 
With Best Regards,
Andy Shevchenko

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

[Rob Herring]

On Tue, Aug 1, 2023 at 9:35 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Aug 02, 2023 at 06:28:48AM +0300, Andy Shevchenko wrote:
> > On Tue, Aug 01, 2023 at 03:54:45PM -0600, Rob Herring 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
> > >
> > > To fix this, move the printing in __of_changeset_entry_apply() outside the
> > > lock. As there's already similar printing of the same changeset actions,
> > > refactor all of them to use a common action print function. This has the
> > > side benefit of getting rid of some ifdefs.
>
> ...
>
> > > v3:
> > >  - Add missing 'static' reported by 0-day
> >
> > It reported two issues (at least what I see).

Indeed. I missed the 2nd one.

> ...
>
> > > +   if (pr_debug("notify "))
> >
> > This is weird. How did you compile it?

I agree it's a weird pattern...

> Urgh, you need to fix dynamic debug macros to return an error code.

Or adding a 'pr_debug_cont' macro would do it. I'm inclined to wrap it
in an "#ifdef DEBUG" and be done with it.

Rob

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

[Rob Herring]

On Wed, Aug 2, 2023 at 3:33 PM Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Aug 1, 2023 at 9:35 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Wed, Aug 02, 2023 at 06:28:48AM +0300, Andy Shevchenko wrote:
> > > On Tue, Aug 01, 2023 at 03:54:45PM -0600, Rob Herring 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
> > > >
> > > > To fix this, move the printing in __of_changeset_entry_apply() outside the
> > > > lock. As there's already similar printing of the same changeset actions,
> > > > refactor all of them to use a common action print function. This has the
> > > > side benefit of getting rid of some ifdefs.
> >
> > ...
> >
> > > > v3:
> > > >  - Add missing 'static' reported by 0-day
> > >
> > > It reported two issues (at least what I see).
>
> Indeed. I missed the 2nd one.
>
> > ...
> >
> > > > +   if (pr_debug("notify "))
> > >
> > > This is weird. How did you compile it?
>
> I agree it's a weird pattern...
>
> > Urgh, you need to fix dynamic debug macros to return an error code.
>
> Or adding a 'pr_debug_cont' macro would do it. I'm inclined to wrap it
> in an "#ifdef DEBUG" and be done with it.

Here's what I've come up with instead:

#define _do_print(func, prefix, action, node, prop, ...) ({ \
  if (prop) \
    func(prefix "%-15s %pOF:%s\n", ##__VA_ARGS__,
action_names[action], node, prop); \
  else \
    func(prefix "%-15s %pOF\n", ##__VA_ARGS__, action_names[action], node);\
})
#define of_changeset_action_err(...) _do_print(pr_err, __VA_ARGS__)
#define of_changeset_action_debug(...) _do_print(pr_debug, __VA_ARGS__)

Rob

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

[Petr Mladek]

On Tue 2023-08-01 15:54:45, Rob Herring 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
> 
> To fix this, move the printing in __of_changeset_entry_apply() outside the
> lock. As there's already similar printing of the same changeset actions,
> refactor all of them to use a common action print function. This has the
> side benefit of getting rid of some ifdefs.
> 
> 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>

> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -63,37 +63,31 @@ 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
> +
> +static void of_changeset_action_print(unsigned long action, struct device_node *np,
> +				      const char *prop_name)
> +{
> +	if (prop_name)
> +		pr_cont("%-15s %pOF:%s\n", action_names[action], np, prop_name);

Note that pr_cont() does not guarantee that the message will be appended to the
previous part. Any message printed from another CPU or interrupt
context might break the two pieces.

It is better to avoid pr_cont() when possible.

> +	else
> +		pr_cont("%-15s %pOF\n", action_names[action], np);
> +}
>  
>  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;
> +	if (pr_debug("notify "))
> +		of_changeset_action_print(action, pr->dn, pr->prop ? pr->prop->name : NULL);

If you really want to simplify this, then I would do:

	pr_debug("notify %-15s %pOF%s%s\n",
		  action_names[action], pr->dn,
		  pr->prop ? ":" : ",
		  pr->prop ? pr->prop->name : "");



> -	}
> -#endif
>  	rc = blocking_notifier_call_chain(&of_reconfig_chain, action, p);
>  	return notifier_to_errno(rc);
>  }
> @@ -599,7 +569,8 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
>  	unsigned long flags;
>  	int ret = 0;
>  
> -	__of_changeset_entry_dump(ce);
> +	if (pr_debug("changeset: applying: cset<%p> ", ce))
> +		of_changeset_action_print(ce->action, ce->np, ce->prop ? ce->prop->name : NULL);

One possibility would be to create a macro for this, something like:

#define of_ce_action_print(printk_level, prefix, ce)		\
	printk(printk_level "%s cset<%p> %-15s %pOF%s%s\n"	\
		prefix, ce, action_names[action], pr->dn,	\
		  pr->prop ? ":" : ",				\
		  pr->prop ? pr->prop->name : "");

And use it like:

	of_ce_action_print(KERN_DEBUG, "changeset: applying:", ce);

But I am not sure if it is worth it. Sometimes it is better to
opencode things so that it is clear what is going on.


>  
>  	raw_spin_lock_irqsave(&devtree_lock, flags);
>  	switch (ce->action) {
> @@ -620,21 +591,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 +607,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> ", ce);
> +		of_changeset_action_print(ce->action, ce->np, ce->prop ? ce->prop->name : NULL);
>  		return ret;
> +	}
>  
>  	switch (ce->action) {
>  	case OF_RECONFIG_ATTACH_NODE:

I would suggest to split the changes into two so that the fix is in a
separate patch. And the fix should be first so that it might be
easier for backporting.

Best Regards,
Petr

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

[Rob Herring]

On Fri, Aug 4, 2023 at 12:55 PM Petr Mladek <pmladek@suse.com> wrote:
>
> On Tue 2023-08-01 15:54:45, Rob Herring 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
> >
> > To fix this, move the printing in __of_changeset_entry_apply() outside the
> > lock. As there's already similar printing of the same changeset actions,
> > refactor all of them to use a common action print function. This has the
> > side benefit of getting rid of some ifdefs.
> >
> > 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>
>
> > --- a/drivers/of/dynamic.c
> > +++ b/drivers/of/dynamic.c
> > @@ -63,37 +63,31 @@ 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
> > +
> > +static void of_changeset_action_print(unsigned long action, struct device_node *np,
> > +                                   const char *prop_name)
> > +{
> > +     if (prop_name)
> > +             pr_cont("%-15s %pOF:%s\n", action_names[action], np, prop_name);
>
> Note that pr_cont() does not guarantee that the message will be appended to the
> previous part. Any message printed from another CPU or interrupt
> context might break the two pieces.
>
> It is better to avoid pr_cont() when possible.

Yeah, I got rid of it in the snippet I posted.

>
> > +     else
> > +             pr_cont("%-15s %pOF\n", action_names[action], np);
> > +}
> >
> >  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;
> > +     if (pr_debug("notify "))
> > +             of_changeset_action_print(action, pr->dn, pr->prop ? pr->prop->name : NULL);
>
> If you really want to simplify this, then I would do:
>
>         pr_debug("notify %-15s %pOF%s%s\n",
>                   action_names[action], pr->dn,
>                   pr->prop ? ":" : ",
>                   pr->prop ? pr->prop->name : "");

That's a good idea.

> > -     }
> > -#endif
> >       rc = blocking_notifier_call_chain(&of_reconfig_chain, action, p);
> >       return notifier_to_errno(rc);
> >  }
> > @@ -599,7 +569,8 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
> >       unsigned long flags;
> >       int ret = 0;
> >
> > -     __of_changeset_entry_dump(ce);
> > +     if (pr_debug("changeset: applying: cset<%p> ", ce))
> > +             of_changeset_action_print(ce->action, ce->np, ce->prop ? ce->prop->name : NULL);
>
> One possibility would be to create a macro for this, something like:
>
> #define of_ce_action_print(printk_level, prefix, ce)            \
>         printk(printk_level "%s cset<%p> %-15s %pOF%s%s\n"      \
>                 prefix, ce, action_names[action], pr->dn,       \
>                   pr->prop ? ":" : ",                           \
>                   pr->prop ? pr->prop->name : "");
>
> And use it like:
>
>         of_ce_action_print(KERN_DEBUG, "changeset: applying:", ce);

The problem there is the debug print is always enabled.

>
> But I am not sure if it is worth it. Sometimes it is better to
> opencode things so that it is clear what is going on.

Maybe so.

Rob

[PATCH 3/5] of: dynamic: Fix race in getting old property when updating property

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

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 aa3821836937..31603b5a4ffc 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -565,7 +565,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;
 
@@ -606,7 +606,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

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

[Rob Herring]

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

Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/of/base.c    | 17 +++++++++++++++++
 drivers/of/dynamic.c | 19 -------------------
 2 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 166fb7d75337..99c07f3cbf10 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1538,6 +1538,14 @@ int __of_add_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) {
+			*next = prop->next;
+			break;
+		}
+	}
+
 	prop->next = NULL;
 	next = &np->properties;
 	while (*next) {
@@ -1640,6 +1648,15 @@ int __of_update_property(struct device_node *np, struct property *newprop,
 {
 	struct property **next, *oldprop;
 
+	/* If the property is in deadprops then it must be removed */
+	for (next = &np->deadprops; *next; next = &(*next)->next) {
+		if (*next == newprop) {
+			*next = newprop->next;
+			newprop->next = NULL;
+			break;
+		}
+	}
+
 	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 31603b5a4ffc..4fd3699691b6 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -565,7 +565,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;
 
@@ -581,15 +580,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:
@@ -597,15 +587,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

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

[Andy Shevchenko]

On Tue, Aug 01, 2023 at 03:54:47PM -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.

...

Seems like this...

> +	/* If the property is in deadprops then it must be removed */
> +	for (next = &np->deadprops; *next; next = &(*next)->next) {
> +		if (*next == prop) {
> +			*next = prop->next;
> +			break;
> +		}
> +	}

>  	prop->next = NULL;

...

> +	for (next = &np->deadprops; *next; next = &(*next)->next) {
> +		if (*next == newprop) {
> +			*next = newprop->next;
> +			newprop->next = NULL;
> +			break;
> +		}
> +	}

...is a dup of this. Are you planing to have a helper or at least conditional
for_each_*() macro for them?

-- 
With Best Regards,
Andy Shevchenko

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

[Rob Herring]

On Wed, Aug 2, 2023 at 9:12 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Aug 01, 2023 at 03:54:47PM -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.
>
> ...
>
> Seems like this...
>
> > +     /* If the property is in deadprops then it must be removed */
> > +     for (next = &np->deadprops; *next; next = &(*next)->next) {
> > +             if (*next == prop) {
> > +                     *next = prop->next;
> > +                     break;
> > +             }
> > +     }
>
> >       prop->next = NULL;
>
> ...
>
> > +     for (next = &np->deadprops; *next; next = &(*next)->next) {
> > +             if (*next == newprop) {
> > +                     *next = newprop->next;
> > +                     newprop->next = NULL;
> > +                     break;
> > +             }
> > +     }
>
> ...is a dup of this. Are you planing to have a helper or at least conditional
> for_each_*() macro for them?

At least the latter would be more clean-up than I want to do here
because this will be backported to stable. We have an iterator for
properties list, but note how this iterates on the "next" ptr address.
So it's a bit different from the normal iterators.

I'll go ahead and add a helper.

Rob

[PATCH 5/5] of: Refactor node and property manipulation function locking

[Rob Herring]

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

Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/of/base.c    | 69 ++++++++++++++++++++++++++--------------------------
 drivers/of/dynamic.c | 51 ++++++++++++--------------------------
 2 files changed, 50 insertions(+), 70 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 99c07f3cbf10..4ee050ace11e 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1536,8 +1536,12 @@ EXPORT_SYMBOL(of_count_phandle_with_args);
  */
 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);
+
 	/* If the property is in deadprops then it must be removed */
 	for (next = &np->deadprops; *next; next = &(*next)->next) {
 		if (*next == prop) {
@@ -1549,15 +1553,21 @@ int __of_add_property(struct device_node *np, struct property *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;
+		}
 		next = &(*next)->next;
 	}
 	*next = prop;
 
-	return 0;
+out:
+	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+	if (!rc)
+		__of_add_property_sysfs(np, prop);
+
+	return rc;
 }
 
 /**
@@ -1567,23 +1577,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);
@@ -1591,20 +1590,29 @@ 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;
+	}
 	/* found the node */
 	*next = prop->next;
 	prop->next = np->deadprops;
 	np->deadprops = prop;
 
-	return 0;
+out:
+	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+	if (!rc)
+		__of_remove_property_sysfs(np, prop);
+	return rc;
 }
 
 /**
@@ -1619,21 +1627,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)
@@ -1647,6 +1647,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);
 
 	/* If the property is in deadprops then it must be removed */
 	for (next = &np->deadprops; *next; next = &(*next)->next) {
@@ -1675,6 +1678,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;
 }
 
@@ -1690,21 +1697,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 4fd3699691b6..c59f581792f1 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -199,6 +199,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);
@@ -221,6 +224,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);
 }
 
 /**
@@ -230,17 +237,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);
@@ -251,13 +253,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;
@@ -274,6 +278,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);
 }
 
 /**
@@ -283,17 +291,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);
@@ -565,13 +568,11 @@ 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;
 
 	if (pr_debug("changeset: applying: cset<%p> ", ce))
 		of_changeset_action_print(ce->action, ce->np, ce->prop ? ce->prop->name : NULL);
 
-	raw_spin_lock_irqsave(&devtree_lock, flags);
 	switch (ce->action) {
 	case OF_RECONFIG_ATTACH_NODE:
 		__of_attach_node(ce->np);
@@ -592,7 +593,6 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 	default:
 		ret = -EINVAL;
 	}
-	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
 	if (ret) {
 		pr_err("changeset: apply failed: cset<%p> ", ce);
@@ -600,25 +600,6 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *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

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

[Andy Shevchenko]

On Tue, Aug 01, 2023 at 03:54:48PM -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.

...

> +out:

out_unlock: ?

> +	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +	if (!rc)
> +		__of_add_property_sysfs(np, prop);
> +
> +	return rc;

Why not

	if (rc)
		return rc;

	__of_add_property_sysfs(np, prop);
	return 0;

?

...

> +out:
> +	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +	if (!rc)
> +		__of_remove_property_sysfs(np, prop);
> +	return rc;

As per above.

-- 
With Best Regards,
Andy Shevchenko