diff mbox series

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

Message ID 20230801-dt-changeset-fixes-v1-2-b5203e3fc22f@kernel.org
State New
Headers
Series dt: changeset fixes and cleanups |

Commit Message

Rob Herring Aug. 1, 2023, 9:54 p.m. UTC 
  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(-)

Comments

kernel test robot Aug. 2, 2023, 2:49 a.m. UTC | #1 
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
Andy Shevchenko Aug. 2, 2023, 3:28 a.m. UTC | #2 
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);
Andy Shevchenko Aug. 2, 2023, 3:35 a.m. UTC | #3 
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);
Rob Herring Aug. 2, 2023, 9:33 p.m. UTC | #4 
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
Rob Herring Aug. 3, 2023, 5:36 p.m. UTC | #5 
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
Petr Mladek Aug. 4, 2023, 6:55 p.m. UTC | #6 
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
Rob Herring Aug. 4, 2023, 8:31 p.m. UTC | #7 
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
diff mbox series

Patch

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: