| Message ID | 20230801-dt-changeset-fixes-v3-0-5f0410e007dd@kernel.org |
|---|---|
| Headers | show |
| Series | dt: changeset fixes and cleanups | expand |
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
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,