[PATCH v2 0/6] dt: changeset fixes and cleanups

[PATCH v2 0/6] 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 
dynamic API" and the changeset API implementations.

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

Signed-off-by: Rob Herring <robh@kernel.org>
---
Changes in v2:
- Rework debug printing to fix issues with pr_debug() not having a 
  return value with dynamic debug
- Split action print refactoring into separate patch from fix
- Make removing property from deadprops a helper function
- Rework __of_add_property()/__of_update_property() exit code
- Link to v1: https://lore.kernel.org/r/20230801-dt-changeset-fixes-v1-0-b5203e3fc22f@kernel.org

---
Rob Herring (6):
      of: unittest: Fix EXPECT for parse_phandle_with_args_map() test
      of: dynamic: Refactor action prints to not use "%pOF" inside devtree_lock
      of: dynamic: Refactor changeset action printing to common helpers
      of: dynamic: Fix race in getting old property when updating property
      of: dynamic: Move dead property list check into property add/update functions
      of: Refactor node and property manipulation function locking

 drivers/of/base.c     |  87 ++++++++++++++++++-----------
 drivers/of/dynamic.c  | 149 ++++++++++----------------------------------------
 drivers/of/unittest.c |   4 +-
 3 files changed, 86 insertions(+), 154 deletions(-)
---
base-commit: 66a4210bc82e024e6de0f94298ad9230984a5924
change-id: 20230801-dt-changeset-fixes-b76b88fecc43

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

[PATCH v2 1/6] 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

Re: [PATCH v2 1/6] of: unittest: Fix EXPECT for parse_phandle_with_args_map() test

[Geert Uytterhoeven]

On Sat, Aug 5, 2023 at 12:42 AM Rob Herring <robh@kernel.org> wrote:
> Commit 12e17243d8a1 ("of: base: improve error msg in
> of_phandle_iterator_next()") added printing of the phandle value on
> error, but failed to update the unittest.
>
> Fixes: 12e17243d8a1 ("of: base: improve error msg in of_phandle_iterator_next()")
> Signed-off-by: Rob Herring <robh@kernel.org>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v2 2/6] 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

Fix this by moving the printing in __of_changeset_entry_apply() outside
the lock. As the only difference in the the multiple prints is the
action name, use the existing "action_names" to refactor the prints into
a single print.

Fixes: a92eb7621b9fb2c2 ("lib/vsprintf: Make use of fwnode API to obtain node names and separators")
Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Rob Herring <robh@kernel.org>
---
v5 (v2 in this series):
 - Move majority of refactoring to separate patch and minimize the fix
   to just moving the print out of the locked section.
v4:
 - Add missing 'static' reported by 0-day
v3:
 - Re-implement Geert's fix to move the prints rather than move the
   spinlock

v1 and v2 from Geert simply moved the devtree_lock into each case
statement:

https://lore.kernel.org/all/c593d8389352c574b5be69d4ca4810da13326a50.1690533838.git.geert+renesas@glider.be/
---
 drivers/of/dynamic.c | 27 +++++----------------------
 1 file changed, 5 insertions(+), 22 deletions(-)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index e311d406b170..2f0eb0053773 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -63,15 +63,13 @@ int of_reconfig_notifier_unregister(struct notifier_block *nb)
 }
 EXPORT_SYMBOL_GPL(of_reconfig_notifier_unregister);
 
-#ifdef DEBUG
-const char *action_names[] = {
+static const char *action_names[] = {
 	[OF_RECONFIG_ATTACH_NODE] = "ATTACH_NODE",
 	[OF_RECONFIG_DETACH_NODE] = "DETACH_NODE",
 	[OF_RECONFIG_ADD_PROPERTY] = "ADD_PROPERTY",
 	[OF_RECONFIG_REMOVE_PROPERTY] = "REMOVE_PROPERTY",
 	[OF_RECONFIG_UPDATE_PROPERTY] = "UPDATE_PROPERTY",
 };
-#endif
 
 int of_reconfig_notify(unsigned long action, struct of_reconfig_data *p)
 {
@@ -620,21 +618,9 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 		}
 
 		ret = __of_add_property(ce->np, ce->prop);
-		if (ret) {
-			pr_err("changeset: add_property failed @%pOF/%s\n",
-				ce->np,
-				ce->prop->name);
-			break;
-		}
 		break;
 	case OF_RECONFIG_REMOVE_PROPERTY:
 		ret = __of_remove_property(ce->np, ce->prop);
-		if (ret) {
-			pr_err("changeset: remove_property failed @%pOF/%s\n",
-				ce->np,
-				ce->prop->name);
-			break;
-		}
 		break;
 
 	case OF_RECONFIG_UPDATE_PROPERTY:
@@ -648,20 +634,17 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 		}
 
 		ret = __of_update_property(ce->np, ce->prop, &old_prop);
-		if (ret) {
-			pr_err("changeset: update_property failed @%pOF/%s\n",
-				ce->np,
-				ce->prop->name);
-			break;
-		}
 		break;
 	default:
 		ret = -EINVAL;
 	}
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
-	if (ret)
+	if (ret) {
+		pr_err("changeset: apply failed: cset<%p> %-15s %pOF:%s\n",
+		       ce, action_names[ce->action], ce->np, ce->prop->name);
 		return ret;
+	}
 
 	switch (ce->action) {
 	case OF_RECONFIG_ATTACH_NODE:

-- 
2.40.1

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

[Andy Shevchenko]

On Fri, Aug 04, 2023 at 04:41:52PM -0600, Rob Herring wrote:

...

> v5 (v2 in this series):
> v4:
> v3:
> v1 and v2 from Geert simply moved the devtree_lock into each case
> statement:
> https://lore.kernel.org/all/c593d8389352c574b5be69d4ca4810da13326a50.1690533838.git.geert+renesas@glider.be/

Side note: More natural to use main versioning as of this series and put
the other to the parentheses.

-- 
With Best Regards,
Andy Shevchenko

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

[Geert Uytterhoeven]

Hi Rob,

On Sat, Aug 5, 2023 at 12:42 AM Rob Herring <robh@kernel.org> wrote:
> While originally it was fine to format strings using "%pOF" while
> holding devtree_lock, this now causes a deadlock.  Lockdep reports:
>
>     of_get_parent from of_fwnode_get_parent+0x18/0x24
>     ^^^^^^^^^^^^^
>     of_fwnode_get_parent from fwnode_count_parents+0xc/0x28
>     fwnode_count_parents from fwnode_full_name_string+0x18/0xac
>     fwnode_full_name_string from device_node_string+0x1a0/0x404
>     device_node_string from pointer+0x3c0/0x534
>     pointer from vsnprintf+0x248/0x36c
>     vsnprintf from vprintk_store+0x130/0x3b4
>
> Fix this by moving the printing in __of_changeset_entry_apply() outside
> the lock. As the only difference in the the multiple prints is the
> action name, use the existing "action_names" to refactor the prints into
> a single print.
>
> Fixes: a92eb7621b9fb2c2 ("lib/vsprintf: Make use of fwnode API to obtain node names and separators")
> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> v5 (v2 in this series):
>  - Move majority of refactoring to separate patch and minimize the fix
>    to just moving the print out of the locked section.

Thanks for your patch!

> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c

> @@ -648,20 +634,17 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
>                 }
>
>                 ret = __of_update_property(ce->np, ce->prop, &old_prop);
> -               if (ret) {
> -                       pr_err("changeset: update_property failed @%pOF/%s\n",
> -                               ce->np,
> -                               ce->prop->name);
> -                       break;
> -               }
>                 break;
>         default:
>                 ret = -EINVAL;
>         }
>         raw_spin_unlock_irqrestore(&devtree_lock, flags);
>
> -       if (ret)
> +       if (ret) {
> +               pr_err("changeset: apply failed: cset<%p> %-15s %pOF:%s\n",

Printing the cset pointer will (needlessly?) complicate the EXPECT_*()
handling in the unit test.

> +                      ce, action_names[ce->action], ce->np, ce->prop->name);

This should check ce->action to avoid an out-of-bounds access beyond
the end of action_names[].

>                 return ret;
> +       }
>
>         switch (ce->action) {
>         case OF_RECONFIG_ATTACH_NODE:

The rest LGTM to me.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Rob Herring]

On Fri, Aug 18, 2023 at 10:36 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Rob,
>
> On Sat, Aug 5, 2023 at 12:42 AM Rob Herring <robh@kernel.org> wrote:
> > While originally it was fine to format strings using "%pOF" while
> > holding devtree_lock, this now causes a deadlock.  Lockdep reports:
> >
> >     of_get_parent from of_fwnode_get_parent+0x18/0x24
> >     ^^^^^^^^^^^^^
> >     of_fwnode_get_parent from fwnode_count_parents+0xc/0x28
> >     fwnode_count_parents from fwnode_full_name_string+0x18/0xac
> >     fwnode_full_name_string from device_node_string+0x1a0/0x404
> >     device_node_string from pointer+0x3c0/0x534
> >     pointer from vsnprintf+0x248/0x36c
> >     vsnprintf from vprintk_store+0x130/0x3b4
> >
> > Fix this by moving the printing in __of_changeset_entry_apply() outside
> > the lock. As the only difference in the the multiple prints is the
> > action name, use the existing "action_names" to refactor the prints into
> > a single print.
> >
> > Fixes: a92eb7621b9fb2c2 ("lib/vsprintf: Make use of fwnode API to obtain node names and separators")
> > Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > Signed-off-by: Rob Herring <robh@kernel.org>
> > ---
> > v5 (v2 in this series):
> >  - Move majority of refactoring to separate patch and minimize the fix
> >    to just moving the print out of the locked section.
>
> Thanks for your patch!
>
> > --- a/drivers/of/dynamic.c
> > +++ b/drivers/of/dynamic.c
>
> > @@ -648,20 +634,17 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
> >                 }
> >
> >                 ret = __of_update_property(ce->np, ce->prop, &old_prop);
> > -               if (ret) {
> > -                       pr_err("changeset: update_property failed @%pOF/%s\n",
> > -                               ce->np,
> > -                               ce->prop->name);
> > -                       break;
> > -               }
> >                 break;
> >         default:
> >                 ret = -EINVAL;
> >         }
> >         raw_spin_unlock_irqrestore(&devtree_lock, flags);
> >
> > -       if (ret)
> > +       if (ret) {
> > +               pr_err("changeset: apply failed: cset<%p> %-15s %pOF:%s\n",
>
> Printing the cset pointer will (needlessly?) complicate the EXPECT_*()
> handling in the unit test.

That's added largely because the other prints which I rework later in
this series had them. Either printing the changeset ptr is useful or
it isn't. I think people running the unittest and the post-processor
can easily enough filter this out when looking at the results.
Honestly, even I probably run it less than once a cycle.

>
> > +                      ce, action_names[ce->action], ce->np, ce->prop->name);
>
> This should check ce->action to avoid an out-of-bounds access beyond
> the end of action_names[].

Indeed.

I think I'll add "invalid" to action_names names and then do something
like: "(ce->action < FOO) ? ce->action : 0".

Rob

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

[Geert Uytterhoeven]

Hi Rob,

On Fri, Aug 18, 2023 at 6:17 PM Rob Herring <robh@kernel.org> wrote:
> On Fri, Aug 18, 2023 at 10:36 AM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > On Sat, Aug 5, 2023 at 12:42 AM Rob Herring <robh@kernel.org> wrote:
> > > While originally it was fine to format strings using "%pOF" while
> > > holding devtree_lock, this now causes a deadlock.  Lockdep reports:
> > >
> > >     of_get_parent from of_fwnode_get_parent+0x18/0x24
> > >     ^^^^^^^^^^^^^
> > >     of_fwnode_get_parent from fwnode_count_parents+0xc/0x28
> > >     fwnode_count_parents from fwnode_full_name_string+0x18/0xac
> > >     fwnode_full_name_string from device_node_string+0x1a0/0x404
> > >     device_node_string from pointer+0x3c0/0x534
> > >     pointer from vsnprintf+0x248/0x36c
> > >     vsnprintf from vprintk_store+0x130/0x3b4
> > >
> > > Fix this by moving the printing in __of_changeset_entry_apply() outside
> > > the lock. As the only difference in the the multiple prints is the
> > > action name, use the existing "action_names" to refactor the prints into
> > > a single print.
> > >
> > > Fixes: a92eb7621b9fb2c2 ("lib/vsprintf: Make use of fwnode API to obtain node names and separators")
> > > Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > Signed-off-by: Rob Herring <robh@kernel.org>
> > > ---
> > > v5 (v2 in this series):
> > >  - Move majority of refactoring to separate patch and minimize the fix
> > >    to just moving the print out of the locked section.
> >
> > Thanks for your patch!
> >
> > > --- a/drivers/of/dynamic.c
> > > +++ b/drivers/of/dynamic.c
> >
> > > @@ -648,20 +634,17 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
> > >                 }
> > >
> > >                 ret = __of_update_property(ce->np, ce->prop, &old_prop);
> > > -               if (ret) {
> > > -                       pr_err("changeset: update_property failed @%pOF/%s\n",
> > > -                               ce->np,
> > > -                               ce->prop->name);
> > > -                       break;
> > > -               }
> > >                 break;
> > >         default:
> > >                 ret = -EINVAL;
> > >         }
> > >         raw_spin_unlock_irqrestore(&devtree_lock, flags);
> > >
> > > -       if (ret)
> > > +       if (ret) {
> > > +               pr_err("changeset: apply failed: cset<%p> %-15s %pOF:%s\n",
> >
> > Printing the cset pointer will (needlessly?) complicate the EXPECT_*()
> > handling in the unit test.
>
> That's added largely because the other prints which I rework later in
> this series had them. Either printing the changeset ptr is useful or
> it isn't. I think people running the unittest and the post-processor
> can easily enough filter this out when looking at the results.
> Honestly, even I probably run it less than once a cycle.

Do you have a use for printing the pointer value?
And by default, it will be an obfuscated cookie anyway.

> > > +                      ce, action_names[ce->action], ce->np, ce->prop->name);
> >
> > This should check ce->action to avoid an out-of-bounds access beyond
> > the end of action_names[].
>
> Indeed.
>
> I think I'll add "invalid" to action_names names and then do something
> like: "(ce->action < FOO) ? ce->action : 0".

OK, zero is invalid.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Rob Herring]

On Fri, Aug 18, 2023 at 12:37 PM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Rob,
>
> On Fri, Aug 18, 2023 at 6:17 PM Rob Herring <robh@kernel.org> wrote:
> > On Fri, Aug 18, 2023 at 10:36 AM Geert Uytterhoeven
> > <geert@linux-m68k.org> wrote:
> > > On Sat, Aug 5, 2023 at 12:42 AM Rob Herring <robh@kernel.org> wrote:
> > > > While originally it was fine to format strings using "%pOF" while
> > > > holding devtree_lock, this now causes a deadlock.  Lockdep reports:
> > > >
> > > >     of_get_parent from of_fwnode_get_parent+0x18/0x24
> > > >     ^^^^^^^^^^^^^
> > > >     of_fwnode_get_parent from fwnode_count_parents+0xc/0x28
> > > >     fwnode_count_parents from fwnode_full_name_string+0x18/0xac
> > > >     fwnode_full_name_string from device_node_string+0x1a0/0x404
> > > >     device_node_string from pointer+0x3c0/0x534
> > > >     pointer from vsnprintf+0x248/0x36c
> > > >     vsnprintf from vprintk_store+0x130/0x3b4
> > > >
> > > > Fix this by moving the printing in __of_changeset_entry_apply() outside
> > > > the lock. As the only difference in the the multiple prints is the
> > > > action name, use the existing "action_names" to refactor the prints into
> > > > a single print.
> > > >
> > > > Fixes: a92eb7621b9fb2c2 ("lib/vsprintf: Make use of fwnode API to obtain node names and separators")
> > > > Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > Signed-off-by: Rob Herring <robh@kernel.org>
> > > > ---
> > > > v5 (v2 in this series):
> > > >  - Move majority of refactoring to separate patch and minimize the fix
> > > >    to just moving the print out of the locked section.
> > >
> > > Thanks for your patch!
> > >
> > > > --- a/drivers/of/dynamic.c
> > > > +++ b/drivers/of/dynamic.c
> > >
> > > > @@ -648,20 +634,17 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
> > > >                 }
> > > >
> > > >                 ret = __of_update_property(ce->np, ce->prop, &old_prop);
> > > > -               if (ret) {
> > > > -                       pr_err("changeset: update_property failed @%pOF/%s\n",
> > > > -                               ce->np,
> > > > -                               ce->prop->name);
> > > > -                       break;
> > > > -               }
> > > >                 break;
> > > >         default:
> > > >                 ret = -EINVAL;
> > > >         }
> > > >         raw_spin_unlock_irqrestore(&devtree_lock, flags);
> > > >
> > > > -       if (ret)
> > > > +       if (ret) {
> > > > +               pr_err("changeset: apply failed: cset<%p> %-15s %pOF:%s\n",
> > >
> > > Printing the cset pointer will (needlessly?) complicate the EXPECT_*()
> > > handling in the unit test.
> >
> > That's added largely because the other prints which I rework later in
> > this series had them. Either printing the changeset ptr is useful or
> > it isn't. I think people running the unittest and the post-processor
> > can easily enough filter this out when looking at the results.
> > Honestly, even I probably run it less than once a cycle.
>
> Do you have a use for printing the pointer value?

I have no use for overlays in general, so no. :)

I'd assumed it was there to provide a changeset ID to tell which
actions belong to the same changeset. But it's printing the changeset
entry rather than the changeset, so I agree it is not really useful.

Rob

[PATCH v2 3/6] of: dynamic: Refactor changeset action printing to common helpers

[Rob Herring]

Several places print the changeset action with node and property
details. Refactor these into a common printing helper. The complicating
factor is some prints are debug and some are errors. Solve this with a
bit of preprocessor magic.

Signed-off-by: Rob Herring <robh@kernel.org>
---
v2:
 - Split out refactoring from fix in prior patch
 - Avoid pr_cont and relying on pr_debug return value which dynamic
   debug doesn't have
---
 drivers/of/dynamic.c | 53 +++++++++++-----------------------------------------
 1 file changed, 11 insertions(+), 42 deletions(-)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 2f0eb0053773..6eaa66b11a02 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -71,27 +71,21 @@ static const char *action_names[] = {
 	[OF_RECONFIG_UPDATE_PROPERTY] = "UPDATE_PROPERTY",
 };
 
+#define _do_print(func, prefix, action, node, prop, ...) ({	\
+	func("changeset: " prefix "%-15s %pOF%s%s\n",		\
+	     ##__VA_ARGS__, action_names[action], node,		\
+	     prop ? ":" : "", prop ? prop->name : "");		\
+})
+#define of_changeset_action_err(...) _do_print(pr_err, __VA_ARGS__)
+#define of_changeset_action_debug(...) _do_print(pr_debug, __VA_ARGS__)
+
 int of_reconfig_notify(unsigned long action, struct of_reconfig_data *p)
 {
 	int rc;
-#ifdef DEBUG
 	struct of_reconfig_data *pr = p;
 
-	switch (action) {
-	case OF_RECONFIG_ATTACH_NODE:
-	case OF_RECONFIG_DETACH_NODE:
-		pr_debug("notify %-15s %pOF\n", action_names[action],
-			pr->dn);
-		break;
-	case OF_RECONFIG_ADD_PROPERTY:
-	case OF_RECONFIG_REMOVE_PROPERTY:
-	case OF_RECONFIG_UPDATE_PROPERTY:
-		pr_debug("notify %-15s %pOF:%s\n", action_names[action],
-			pr->dn, pr->prop->name);
-		break;
+	of_changeset_action_debug("notify: ", action, pr->dn, pr->prop);
 
-	}
-#endif
 	rc = blocking_notifier_call_chain(&of_reconfig_chain, action, p);
 	return notifier_to_errno(rc);
 }
@@ -502,30 +496,6 @@ static void __of_changeset_entry_destroy(struct of_changeset_entry *ce)
 	kfree(ce);
 }
 
-#ifdef DEBUG
-static void __of_changeset_entry_dump(struct of_changeset_entry *ce)
-{
-	switch (ce->action) {
-	case OF_RECONFIG_ADD_PROPERTY:
-	case OF_RECONFIG_REMOVE_PROPERTY:
-	case OF_RECONFIG_UPDATE_PROPERTY:
-		pr_debug("cset<%p> %-15s %pOF/%s\n", ce, action_names[ce->action],
-			ce->np, ce->prop->name);
-		break;
-	case OF_RECONFIG_ATTACH_NODE:
-	case OF_RECONFIG_DETACH_NODE:
-		pr_debug("cset<%p> %-15s %pOF\n", ce, action_names[ce->action],
-			ce->np);
-		break;
-	}
-}
-#else
-static inline void __of_changeset_entry_dump(struct of_changeset_entry *ce)
-{
-	/* empty */
-}
-#endif
-
 static void __of_changeset_entry_invert(struct of_changeset_entry *ce,
 					  struct of_changeset_entry *rce)
 {
@@ -597,7 +567,7 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 	unsigned long flags;
 	int ret = 0;
 
-	__of_changeset_entry_dump(ce);
+	of_changeset_action_debug("applying: cset<%p> ", ce->action, ce->np, ce->prop, ce);
 
 	raw_spin_lock_irqsave(&devtree_lock, flags);
 	switch (ce->action) {
@@ -641,8 +611,7 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
 	if (ret) {
-		pr_err("changeset: apply failed: cset<%p> %-15s %pOF:%s\n",
-		       ce, action_names[ce->action], ce->np, ce->prop->name);
+		of_changeset_action_err("apply failed: cset<%p> ", ce->action, ce->np, ce->prop, ce);
 		return ret;
 	}
 

-- 
2.40.1

[PATCH v2 4/6] 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 6eaa66b11a02..fbc7c29896a2 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -563,7 +563,7 @@ static int __of_changeset_entry_notify(struct of_changeset_entry *ce,
 
 static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 {
-	struct property *old_prop, **propp;
+	struct property **propp;
 	unsigned long flags;
 	int ret = 0;
 
@@ -603,7 +603,7 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 			}
 		}
 
-		ret = __of_update_property(ce->np, ce->prop, &old_prop);
+		ret = __of_update_property(ce->np, ce->prop, &ce->old_prop);
 		break;
 	default:
 		ret = -EINVAL;
@@ -904,9 +904,6 @@ int of_changeset_action(struct of_changeset *ocs, unsigned long action,
 	ce->np = of_node_get(np);
 	ce->prop = prop;
 
-	if (action == OF_RECONFIG_UPDATE_PROPERTY && prop)
-		ce->old_prop = of_find_property(np, prop->name, NULL);
-
 	/* add it to the list */
 	list_add_tail(&ce->node, &ocs->entries);
 	return 0;

-- 
2.40.1

[PATCH v2 5/6] 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>
---
v2:
 - Add helper to remove property from deadprops list
---
 drivers/of/base.c    | 19 +++++++++++++++++++
 drivers/of/dynamic.c | 19 -------------------
 2 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index e235f3a57ea8..c63a4cde281e 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1530,6 +1530,21 @@ int of_count_phandle_with_args(const struct device_node *np, const char *list_na
 }
 EXPORT_SYMBOL(of_count_phandle_with_args);
 
+static void __of_remove_dead_property(struct device_node *np, struct property *prop)
+{
+	struct property **next;
+
+	/* If the property is in deadprops then it must be removed */
+	for (next = &np->deadprops; *next; next = &(*next)->next) {
+		if (*next != prop)
+			continue;
+
+		*next = prop->next;
+		prop->next = NULL;
+		break;
+	}
+}
+
 /**
  * __of_add_property - Add a property to a node without lock operations
  * @np:		Caller's Device Node
@@ -1539,6 +1554,8 @@ int __of_add_property(struct device_node *np, struct property *prop)
 {
 	struct property **next;
 
+	__of_remove_dead_property(np, prop);
+
 	prop->next = NULL;
 	next = &np->properties;
 	while (*next) {
@@ -1641,6 +1658,8 @@ int __of_update_property(struct device_node *np, struct property *newprop,
 {
 	struct property **next, *oldprop;
 
+	__of_remove_dead_property(np, newprop);
+
 	for (next = &np->properties; *next; next = &(*next)->next) {
 		if (of_prop_cmp((*next)->name, newprop->name) == 0)
 			break;
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index fbc7c29896a2..769869e8b847 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -563,7 +563,6 @@ static int __of_changeset_entry_notify(struct of_changeset_entry *ce,
 
 static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 {
-	struct property **propp;
 	unsigned long flags;
 	int ret = 0;
 
@@ -578,15 +577,6 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 		__of_detach_node(ce->np);
 		break;
 	case OF_RECONFIG_ADD_PROPERTY:
-		/* If the property is in deadprops then it must be removed */
-		for (propp = &ce->np->deadprops; *propp; propp = &(*propp)->next) {
-			if (*propp == ce->prop) {
-				*propp = ce->prop->next;
-				ce->prop->next = NULL;
-				break;
-			}
-		}
-
 		ret = __of_add_property(ce->np, ce->prop);
 		break;
 	case OF_RECONFIG_REMOVE_PROPERTY:
@@ -594,15 +584,6 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 		break;
 
 	case OF_RECONFIG_UPDATE_PROPERTY:
-		/* If the property is in deadprops then it must be removed */
-		for (propp = &ce->np->deadprops; *propp; propp = &(*propp)->next) {
-			if (*propp == ce->prop) {
-				*propp = ce->prop->next;
-				ce->prop->next = NULL;
-				break;
-			}
-		}
-
 		ret = __of_update_property(ce->np, ce->prop, &ce->old_prop);
 		break;
 	default:

-- 
2.40.1

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

[Andy Shevchenko]

On Fri, Aug 04, 2023 at 04:41:55PM -0600, Rob Herring wrote:
> The changeset code checks for a property in the deadprops list when
> adding/updating a property, but of_add_property() and
> of_update_property() do not. As the users of these functions are pretty
> simple, they have not hit this scenario or else the property lists
> would get corrupted.

Suggested-by: ? :-)

...

> +static void __of_remove_dead_property(struct device_node *np, struct property *prop)
> +{
> +	struct property **next;
> +
> +	/* If the property is in deadprops then it must be removed */
> +	for (next = &np->deadprops; *next; next = &(*next)->next) {
> +		if (*next != prop)
> +			continue;
> +
> +		*next = prop->next;
> +		prop->next = NULL;
> +		break;

Why not

		if (*next == prop) {
			*next = prop->next;
			prop->next = NULL;
			break;
		}

which seems to me clearer?

> +	}
> +}

-- 
With Best Regards,
Andy Shevchenko

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

[Rob Herring]

On Mon, Aug 07, 2023 at 06:37:15PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 04, 2023 at 04:41:55PM -0600, Rob Herring wrote:
> > The changeset code checks for a property in the deadprops list when
> > adding/updating a property, but of_add_property() and
> > of_update_property() do not. As the users of these functions are pretty
> > simple, they have not hit this scenario or else the property lists
> > would get corrupted.
> 
> Suggested-by: ? :-)

Humm, by me? The change in behavior and point of this patch comes from 
me. You've provided review comments which will get covered by a
Reviewed-by I presume.

> 
> ...
> 
> > +static void __of_remove_dead_property(struct device_node *np, struct property *prop)
> > +{
> > +	struct property **next;
> > +
> > +	/* If the property is in deadprops then it must be removed */
> > +	for (next = &np->deadprops; *next; next = &(*next)->next) {
> > +		if (*next != prop)
> > +			continue;
> > +
> > +		*next = prop->next;
> > +		prop->next = NULL;
> > +		break;
> 
> Why not
> 
> 		if (*next == prop) {
> 			*next = prop->next;
> 			prop->next = NULL;
> 			break;
> 		}
> 
> which seems to me clearer?

Sure. I like the style I wrote, but whichever way ends the discussion is 
fine for me.

Rob

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

[Andy Shevchenko]

On Thu, Aug 17, 2023 at 12:09:34PM -0500, Rob Herring wrote:
> On Mon, Aug 07, 2023 at 06:37:15PM +0300, Andy Shevchenko wrote:
> > On Fri, Aug 04, 2023 at 04:41:55PM -0600, Rob Herring wrote:

...

> > Suggested-by: ? :-)
> 
> Humm, by me? The change in behavior and point of this patch comes from 
> me. You've provided review comments which will get covered by a
> Reviewed-by I presume.

OK!

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 6/6] 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>
---
v2:
 - Re-arrange exit handling
---
 drivers/of/base.c    | 68 +++++++++++++++++++++++++++-------------------------
 drivers/of/dynamic.c | 51 +++++++++++++--------------------------
 2 files changed, 51 insertions(+), 68 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index c63a4cde281e..3ec134429f11 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1552,21 +1552,32 @@ static void __of_remove_dead_property(struct device_node *np, struct property *p
  */
 int __of_add_property(struct device_node *np, struct property *prop)
 {
+	int rc = 0;
+	unsigned long flags;
 	struct property **next;
 
+	raw_spin_lock_irqsave(&devtree_lock, flags);
+
 	__of_remove_dead_property(np, prop);
 
 	prop->next = NULL;
 	next = &np->properties;
 	while (*next) {
-		if (strcmp(prop->name, (*next)->name) == 0)
+		if (strcmp(prop->name, (*next)->name) == 0) {
 			/* duplicate ! don't insert it */
-			return -EEXIST;
-
+			rc = -EEXIST;
+			goto out_unlock;
+		}
 		next = &(*next)->next;
 	}
 	*next = prop;
 
+out_unlock:
+	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+	if (rc)
+		return rc;
+
+	__of_add_property_sysfs(np, prop);
 	return 0;
 }
 
@@ -1577,23 +1588,12 @@ int __of_add_property(struct device_node *np, struct property *prop)
  */
 int of_add_property(struct device_node *np, struct property *prop)
 {
-	unsigned long flags;
 	int rc;
 
 	mutex_lock(&of_mutex);
-
-	raw_spin_lock_irqsave(&devtree_lock, flags);
 	rc = __of_add_property(np, prop);
-	raw_spin_unlock_irqrestore(&devtree_lock, flags);
-
-	if (!rc)
-		__of_add_property_sysfs(np, prop);
-
 	mutex_unlock(&of_mutex);
 
-	if (!rc)
-		of_property_notify(OF_RECONFIG_ADD_PROPERTY, np, prop, NULL);
-
 	return rc;
 }
 EXPORT_SYMBOL_GPL(of_add_property);
@@ -1601,19 +1601,30 @@ EXPORT_SYMBOL_GPL(of_add_property);
 int __of_remove_property(struct device_node *np, struct property *prop)
 {
 	struct property **next;
+	unsigned long flags;
+	int rc = 0;
+
+	raw_spin_lock_irqsave(&devtree_lock, flags);
 
 	for (next = &np->properties; *next; next = &(*next)->next) {
 		if (*next == prop)
 			break;
 	}
-	if (*next == NULL)
-		return -ENODEV;
-
+	if (*next == NULL) {
+		rc = -ENODEV;
+		goto out_unlock;
+	}
 	/* found the node */
 	*next = prop->next;
 	prop->next = np->deadprops;
 	np->deadprops = prop;
 
+out_unlock:
+	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+	if (rc)
+		return rc;
+
+	__of_remove_property_sysfs(np, prop);
 	return 0;
 }
 
@@ -1629,21 +1640,13 @@ int __of_remove_property(struct device_node *np, struct property *prop)
  */
 int of_remove_property(struct device_node *np, struct property *prop)
 {
-	unsigned long flags;
 	int rc;
 
 	if (!prop)
 		return -ENODEV;
 
 	mutex_lock(&of_mutex);
-
-	raw_spin_lock_irqsave(&devtree_lock, flags);
 	rc = __of_remove_property(np, prop);
-	raw_spin_unlock_irqrestore(&devtree_lock, flags);
-
-	if (!rc)
-		__of_remove_property_sysfs(np, prop);
-
 	mutex_unlock(&of_mutex);
 
 	if (!rc)
@@ -1657,6 +1660,9 @@ int __of_update_property(struct device_node *np, struct property *newprop,
 		struct property **oldpropp)
 {
 	struct property **next, *oldprop;
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&devtree_lock, flags);
 
 	__of_remove_dead_property(np, newprop);
 
@@ -1678,6 +1684,10 @@ int __of_update_property(struct device_node *np, struct property *newprop,
 		*next = newprop;
 	}
 
+	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+
+	__of_update_property_sysfs(np, newprop, oldprop);
+
 	return 0;
 }
 
@@ -1693,21 +1703,13 @@ int __of_update_property(struct device_node *np, struct property *newprop,
 int of_update_property(struct device_node *np, struct property *newprop)
 {
 	struct property *oldprop;
-	unsigned long flags;
 	int rc;
 
 	if (!newprop->name)
 		return -EINVAL;
 
 	mutex_lock(&of_mutex);
-
-	raw_spin_lock_irqsave(&devtree_lock, flags);
 	rc = __of_update_property(np, newprop, &oldprop);
-	raw_spin_unlock_irqrestore(&devtree_lock, flags);
-
-	if (!rc)
-		__of_update_property_sysfs(np, newprop, oldprop);
-
 	mutex_unlock(&of_mutex);
 
 	if (!rc)
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 769869e8b847..715365fbb8ea 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -197,6 +197,9 @@ static void __of_attach_node(struct device_node *np)
 {
 	const __be32 *phandle;
 	int sz;
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&devtree_lock, flags);
 
 	if (!of_node_check_flag(np, OF_OVERLAY)) {
 		np->name = __of_get_property(np, "name", NULL);
@@ -219,6 +222,10 @@ static void __of_attach_node(struct device_node *np)
 	np->parent->child = np;
 	of_node_clear_flag(np, OF_DETACHED);
 	np->fwnode.flags |= FWNODE_FLAG_NOT_DEVICE;
+
+	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+
+	__of_attach_node_sysfs(np);
 }
 
 /**
@@ -228,17 +235,12 @@ static void __of_attach_node(struct device_node *np)
 int of_attach_node(struct device_node *np)
 {
 	struct of_reconfig_data rd;
-	unsigned long flags;
 
 	memset(&rd, 0, sizeof(rd));
 	rd.dn = np;
 
 	mutex_lock(&of_mutex);
-	raw_spin_lock_irqsave(&devtree_lock, flags);
 	__of_attach_node(np);
-	raw_spin_unlock_irqrestore(&devtree_lock, flags);
-
-	__of_attach_node_sysfs(np);
 	mutex_unlock(&of_mutex);
 
 	of_reconfig_notify(OF_RECONFIG_ATTACH_NODE, &rd);
@@ -249,13 +251,15 @@ int of_attach_node(struct device_node *np)
 void __of_detach_node(struct device_node *np)
 {
 	struct device_node *parent;
+	unsigned long flags;
 
-	if (WARN_ON(of_node_check_flag(np, OF_DETACHED)))
-		return;
+	raw_spin_lock_irqsave(&devtree_lock, flags);
 
 	parent = np->parent;
-	if (WARN_ON(!parent))
+	if (WARN_ON(of_node_check_flag(np, OF_DETACHED) || !parent)) {
+		raw_spin_unlock_irqrestore(&devtree_lock, flags);
 		return;
+	}
 
 	if (parent->child == np)
 		parent->child = np->sibling;
@@ -272,6 +276,10 @@ void __of_detach_node(struct device_node *np)
 
 	/* race with of_find_node_by_phandle() prevented by devtree_lock */
 	__of_phandle_cache_inv_entry(np->phandle);
+
+	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+
+	__of_detach_node_sysfs(np);
 }
 
 /**
@@ -281,17 +289,12 @@ void __of_detach_node(struct device_node *np)
 int of_detach_node(struct device_node *np)
 {
 	struct of_reconfig_data rd;
-	unsigned long flags;
 
 	memset(&rd, 0, sizeof(rd));
 	rd.dn = np;
 
 	mutex_lock(&of_mutex);
-	raw_spin_lock_irqsave(&devtree_lock, flags);
 	__of_detach_node(np);
-	raw_spin_unlock_irqrestore(&devtree_lock, flags);
-
-	__of_detach_node_sysfs(np);
 	mutex_unlock(&of_mutex);
 
 	of_reconfig_notify(OF_RECONFIG_DETACH_NODE, &rd);
@@ -563,12 +566,10 @@ static int __of_changeset_entry_notify(struct of_changeset_entry *ce,
 
 static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 {
-	unsigned long flags;
 	int ret = 0;
 
 	of_changeset_action_debug("applying: cset<%p> ", ce->action, ce->np, ce->prop, ce);
 
-	raw_spin_lock_irqsave(&devtree_lock, flags);
 	switch (ce->action) {
 	case OF_RECONFIG_ATTACH_NODE:
 		__of_attach_node(ce->np);
@@ -589,32 +590,12 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)
 	default:
 		ret = -EINVAL;
 	}
-	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
 	if (ret) {
 		of_changeset_action_err("apply failed: cset<%p> ", ce->action, ce->np, ce->prop, ce);
 		return ret;
 	}
 
-	switch (ce->action) {
-	case OF_RECONFIG_ATTACH_NODE:
-		__of_attach_node_sysfs(ce->np);
-		break;
-	case OF_RECONFIG_DETACH_NODE:
-		__of_detach_node_sysfs(ce->np);
-		break;
-	case OF_RECONFIG_ADD_PROPERTY:
-		/* ignore duplicate names */
-		__of_add_property_sysfs(ce->np, ce->prop);
-		break;
-	case OF_RECONFIG_REMOVE_PROPERTY:
-		__of_remove_property_sysfs(ce->np, ce->prop);
-		break;
-	case OF_RECONFIG_UPDATE_PROPERTY:
-		__of_update_property_sysfs(ce->np, ce->prop, ce->old_prop);
-		break;
-	}
-
 	return 0;
 }
 

-- 
2.40.1

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

[Andy Shevchenko]

On Fri, Aug 04, 2023 at 04:41:56PM -0600, Rob Herring wrote:
> All callers of __of_{add,remove,update}_property() and
> __of_{attach,detach}_node() wrap the call with the devtree_lock
> spinlock. Let's move the spinlock into the functions. This allows moving
> the sysfs update functions into those functions as well.

...

>  	for (next = &np->properties; *next; next = &(*next)->next) {
>  		if (*next == prop)
>  			break;
>  	}
> -	if (*next == NULL)
> -		return -ENODEV;
> -
> +	if (*next == NULL) {
> +		rc = -ENODEV;
> +		goto out_unlock;
> +	}
>  	/* found the node */
>  	*next = prop->next;
>  	prop->next = np->deadprops;

But this also looks like a dup for deadprop, maybe something like

static int __of_remove_property_dead_or_alive() // pun intended.

?

-- 
With Best Regards,
Andy Shevchenko