[v3,0/6] dt: changeset fixes and cleanups

Geert's locking fix[1] prompted my closer look at 
__of_changeset_entry_apply() and related functions. The result is a 
couple of fixes I found and some refactoring that unifies the "old 
dynamic API" and the changeset API implementations.

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

Signed-off-by: Rob Herring <robh@kernel.org>
---
Changes in v3:
- Drop print changeset entry pointers
- Add bounds check for action value
- Further rework deadprops helper to remove a property from either list
- Keep existing style for deadprops loop
- Link to v2: https://lore.kernel.org/r/20230801-dt-changeset-fixes-v2-0-c2b701579dee@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     |  92 +++++++++++++++++-------------
 drivers/of/dynamic.c  | 153 +++++++++++---------------------------------------
 drivers/of/unittest.c |   4 +-
 3 files changed, 88 insertions(+), 161 deletions(-)
---
base-commit: 66a4210bc82e024e6de0f94298ad9230984a5924
change-id: 20230801-dt-changeset-fixes-b76b88fecc43

Best regards,

Hi Rob,

Thanks for the update!

On Fri, Aug 18, 2023 at 10:41 PM 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

scripts/checkpatch.pl says: WARNING: Possible repeated word: '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>
> ---
> v6 (v3 in this series):
>  - Add check on 'action' bounds. As action is only set in
>    of_changeset_action(), add the check there.
>  - Drop printing the changeset entry pointer

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

Gr{oetje,eeting}s,

                        Geert

On Mon, Aug 21, 2023 at 5:49 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Aug 18, 2023 at 03:41:00PM -0500, 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.
> >
> > With this there are 3 cases of removing a property from either deadprops
> > or properties lists, so add a helper to find and remove a matching
> > property.
>
> ...
>
> > v3:
> >  - Keep existing style in deadprops loop
>
> Not sure where exactly in the code that one, but...

That was your previous comment...

>
> ...
>
> >  int __of_remove_property(struct device_node *np, struct property *prop)
> >  {
> > -     struct property **next;
> > -
> > -     for (next = &np->properties; *next; next = &(*next)->next) {
> > -             if (*next == prop)
> > -                     break;
> > +     if (__of_remove_property_from_list(&np->properties, prop)) {
> > +             /* Found the property, add it to deadprops list */
> > +             prop->next = np->deadprops;
> > +             np->deadprops = prop;
> > +             return 0;
> >       }
> > -     if (*next == NULL)
> > -             return -ENODEV;
> > -
> > -     /* found the node */
> > -     *next = prop->next;
> > -     prop->next = np->deadprops;
> > -     np->deadprops = prop;
> >
> > -     return 0;
> > +     return -ENODEV;
> >  }
>
>
> ...if it's this one, I don't see how it's better than
>
>         if (!__of_remove_property_from_list(&np->properties, prop))
>                 return -ENODEV;

Because this way doesn't work well when we move the spinlock in here.
Maybe cleanup.h will help, but I'm not going to do that now. If we do,
then I'll do it for the whole subsystem/file.

Rob

Hi Rob,

Thanks for your patch!


On Fri, Aug 18, 2023 at 10:41 PM Rob Herring <robh@kernel.org> 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.
>
> With this there are 3 cases of removing a property from either deadprops
> or properties lists, so add a helper to find and remove a matching
> property.
>
> Signed-off-by: Rob Herring <robh@kernel.org>

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

Perhaps this needs a Fixes tag?

Gr{oetje,eeting}s,

                        Geert

On Mon, Aug 21, 2023 at 8:05 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Rob,
>
> Thanks for your patch!
>
>
> On Fri, Aug 18, 2023 at 10:41 PM Rob Herring <robh@kernel.org> 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.
> >
> > With this there are 3 cases of removing a property from either deadprops
> > or properties lists, so add a helper to find and remove a matching
> > property.
> >
> > Signed-off-by: Rob Herring <robh@kernel.org>
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks!

>
> Perhaps this needs a Fixes tag?

I didn't simply because in the decades that these functions existed,
no one cared. It would require a specific sequence of calls which we
could pretty much determine doesn't happen just looking at the callers
in the kernel.

Rob