| Message ID | 20230801-dt-changeset-fixes-v2-2-c2b701579dee@kernel.org |
|---|---|
| State | Accepted |
| Commit | 914d9d831e6126a6e7a92e27fcfaa250671be42c |
| Headers | show |
| Series | dt: changeset fixes and cleanups | expand |
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.
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
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
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
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:
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(-)