of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock

PATCH New Rob Herring 2 participants 4 messages Jul 28, 2023 – Jul 29, 2023 Device tree

Share & tools

Rob Herring

author maintainer

2023-07-28 23:20 UTC #0 lore raw

PATCH v3 1/1 of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock

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 <[email protected]>

Signed-off-by Rob Herring <[email protected]>

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

drivers/of/dynamic.c +17 ~~-62~~

index e311d406b170..6edb084e928a 100644
@@ -63,37 +63,30 @@ 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
+
+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 +497,6 @@ static void __of_changeset_entry_destroy(struct of_changeset_entry *ce)
 	kfree(ce);
 }
 
-#ifdef DEBUG
-static void __of_changeset_entry_dump(struct of_changeset_entry *ce)
-{
-	switch (ce->action) {
-	case OF_RECONFIG_ADD_PROPERTY:
-	case OF_RECONFIG_REMOVE_PROPERTY:
-	case OF_RECONFIG_UPDATE_PROPERTY:
-		pr_debug("cset<%p> %-15s %pOF/%s\n", ce, action_names[ce->action],
-			ce->np, ce->prop->name);
-		break;
-	case OF_RECONFIG_ATTACH_NODE:
-	case OF_RECONFIG_DETACH_NODE:
-		pr_debug("cset<%p> %-15s %pOF\n", ce, action_names[ce->action],
-			ce->np);
-		break;
-	}
-}
-#else
-static inline void __of_changeset_entry_dump(struct of_changeset_entry *ce)
-{
-	/* empty */
-}
-#endif
-
 static void __of_changeset_entry_invert(struct of_changeset_entry *ce,
 					  struct of_changeset_entry *rce)
 {

@@ -599,7 +568,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 +590,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 +606,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:

Signature · 1 line

2.40.1

kernel test robot

build bot

2023-07-29 03:03 UTC #1 lore raw

kernel test robot noticed the following build errors:

Hi Rob,

kernel test robot noticed the following build errors:

[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master v6.5-rc3 next-20230728] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Rob-Herring/of-dynamic-Refactor-action-prints-to-not-use-pOF-inside-devtree_lock/20230729-072155
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20230728231950.1619073-1-robh%40kernel.org
patch subject: [PATCH v3] of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock
config: loongarch-allyesconfig (https://download.01.org/0day-ci/archive/20230729/[email protected]/config)
compiler: loongarch64-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230729/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All error/warnings (new ones prefixed by >>):

>> drivers/of/dynamic.c:74:6: warning: no previous prototype for 'of_changeset_action_print' [-Wmissing-prototypes]
      74 | void of_changeset_action_print(unsigned long action, struct device_node *np, const char *prop_name)
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~
   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:87:13: note: in expansion of macro 'pr_debug'
      87 |         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:571:13: note: in expansion of macro 'pr_debug'
     571 |         if (pr_debug("changeset: applying: cset<%p> ", ce))
         |             ^~~~~~~~

vim +/of_changeset_action_print +74 drivers/of/dynamic.c

    73	
  > 74	void of_changeset_action_print(unsigned long action, struct device_node *np, const char *prop_name)
    75	{
    76		if (prop_name)
    77			pr_cont("%-15s %pOF:%s\n", action_names[action], np, prop_name);
    78		else
    79			pr_cont("%-15s %pOF\n", action_names[action], np);
    80	}
    81

Signature · 2 lines

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

kernel test robot

build bot

2023-07-29 06:20 UTC #2 lore raw

In reply to Rob Herring

kernel test robot noticed the following build warnings:

Hi Rob,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master v6.5-rc3 next-20230728] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Rob-Herring/of-dynamic-Refactor-action-prints-to-not-use-pOF-inside-devtree_lock/20230729-072155
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20230728231950.1619073-1-robh%40kernel.org
patch subject: [PATCH v3] of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock
config: powerpc-randconfig-r014-20230728 (https://download.01.org/0day-ci/archive/20230729/[email protected]/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce: (https://download.01.org/0day-ci/archive/20230729/[email protected]/reproduce)

All warnings (new ones prefixed by >>):

>> drivers/of/dynamic.c:74:6: warning: no previous prototype for function 'of_changeset_action_print' [-Wmissing-prototypes]
   void of_changeset_action_print(unsigned long action, struct device_node *np, const char *prop_name)
        ^

drivers/of/dynamic.c:74:1: note: declare 'static' if the function is not intended to be used outside of this translation unit void of_changeset_action_print(unsigned long action, struct device_node *np, const char *prop_name)
^
static
1 warning generated.

vim +/of_changeset_action_print +74 drivers/of/dynamic.c

    73	
  > 74	void of_changeset_action_print(unsigned long action, struct device_node *np, const char *prop_name)
    75	{
    76		if (prop_name)
    77			pr_cont("%-15s %pOF:%s\n", action_names[action], np, prop_name);
    78		else
    79			pr_cont("%-15s %pOF\n", action_names[action], np);
    80	}
    81

Signature · 2 lines

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

kernel test robot

build bot

2023-07-29 11:39 UTC #3 lore raw

In reply to Rob Herring

kernel test robot noticed the following build errors:

Hi Rob,

kernel test robot noticed the following build errors:

url:    https://github.com/intel-lab-lkp/linux/commits/Rob-Herring/of-dynamic-Refactor-action-prints-to-not-use-pOF-inside-devtree_lock/20230729-072155
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20230728231950.1619073-1-robh%40kernel.org
patch subject: [PATCH v3] of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock
config: i386-randconfig-i014-20230728 (https://download.01.org/0day-ci/archive/20230729/[email protected]/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce: (https://download.01.org/0day-ci/archive/20230729/[email protected]/reproduce)

All errors (new ones prefixed by >>):

drivers/of/dynamic.c:74:6: warning: no previous prototype for function 'of_changeset_action_print' [-Wmissing-prototypes] void of_changeset_action_print(unsigned long action, struct device_node *np, const char *prop_name)

        ^
   drivers/of/dynamic.c:74:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void of_changeset_action_print(unsigned long action, struct device_node *np, const char *prop_name)
   ^
   static 
>> drivers/of/dynamic.c:87:6: error: expected expression
           if (pr_debug("notify "))
               ^
   include/linux/printk.h:579:2: note: expanded from macro 'pr_debug'
           dynamic_pr_debug(fmt, ##__VA_ARGS__)
           ^
   include/linux/dynamic_debug.h:267:2: note: expanded from macro 'dynamic_pr_debug'
           _dynamic_func_call(fmt, __dynamic_pr_debug,             \
           ^
   include/linux/dynamic_debug.h:248:2: note: expanded from macro '_dynamic_func_call'
           _dynamic_func_call_cls(_DPRINTK_CLASS_DFLT, fmt, func, ##__VA_ARGS__)
           ^
   include/linux/dynamic_debug.h:246:2: note: expanded from macro '_dynamic_func_call_cls'
           __dynamic_func_call_cls(__UNIQUE_ID(ddebug), cls, fmt, func, ##__VA_ARGS__)
           ^
   include/linux/dynamic_debug.h:219:58: note: expanded from macro '__dynamic_func_call_cls'
   #define __dynamic_func_call_cls(id, cls, fmt, func, ...) do {   \
                                                            ^
   drivers/of/dynamic.c:571:6: error: expected expression
           if (pr_debug("changeset: applying: cset<%p> ", ce))
               ^
   include/linux/printk.h:579:2: note: expanded from macro 'pr_debug'
           dynamic_pr_debug(fmt, ##__VA_ARGS__)
           ^
   include/linux/dynamic_debug.h:267:2: note: expanded from macro 'dynamic_pr_debug'
           _dynamic_func_call(fmt, __dynamic_pr_debug,             \
           ^
   include/linux/dynamic_debug.h:248:2: note: expanded from macro '_dynamic_func_call'
           _dynamic_func_call_cls(_DPRINTK_CLASS_DFLT, fmt, func, ##__VA_ARGS__)
           ^
   include/linux/dynamic_debug.h:246:2: note: expanded from macro '_dynamic_func_call_cls'
           __dynamic_func_call_cls(__UNIQUE_ID(ddebug), cls, fmt, func, ##__VA_ARGS__)
           ^
   include/linux/dynamic_debug.h:219:58: note: expanded from macro '__dynamic_func_call_cls'
   #define __dynamic_func_call_cls(id, cls, fmt, func, ...) do {   \
                                                            ^

1 warning and 2 errors generated.

vim +87 drivers/of/dynamic.c

    81	
    82	int of_reconfig_notify(unsigned long action, struct of_reconfig_data *p)
    83	{
    84		int rc;
    85		struct of_reconfig_data *pr = p;
    86	
  > 87		if (pr_debug("notify "))
    88			of_changeset_action_print(action, pr->dn, pr->prop ? pr->prop->name : NULL);
    89	
    90		rc = blocking_notifier_call_chain(&of_reconfig_chain, action, p);
    91		return notifier_to_errno(rc);
    92	}
    93

Signature · 2 lines

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